General feedback

[japaric]

You can leave general feedback about the HAL here.

[japaric]

One thing I was wondering if it makes sense to further splits traits like Serial into two: one for Serial.read and another for Serial.write. My original thought was that such split would let you pick a different Error enum for each implementation; after all a read may run into different errors than a write. But I'm questioning how helpful is that in practice: on one hand higher level APIs will just try / ? several read and write calls so having a single error type makes unification simpler (there's only one Error enum); on the other hand even though read and write may cause different errors a write operation may encounter an error produced by a previous read call: should the write call not report that error? With two error types it may be impossible to report the error in that scenario.

[japaric]

My other question is where generic higher level abstractions like these should live? I would prefer to keep them in a different repository to keep the dependency count of this repo low. (I certainly don't want to make this crate depend on the futures crate.)

[adamgreig]

One quick thought, looking at how I'm currently doing serial/SPI and how I'd do it with this crate, is that in my applications almost all peripheral IO happens through DMA to large buffers, and almost none of it is word-at-a-time. How could the current traits be extended to support that sort of thing?

DMA fits very nicely into a variety of async patterns (since you start it, and then get an interrupt or can check a flag to see if it's done yet), and is generally faster and lower overhead. If I'm writing a high-level abstract library, say something to communicate packets over a UART with framing (so COBS or PPP style escapes), I'll want to tell the underlying hardware, via the HAL, to send/receive a whole &[Word] (or u8 for Serial but that should probably be Word too to handle 9-bit serial), and to tell me when it's done. Does that live as some new traits? Should the current traits have read_block and write_block style things, which could always fall back to a loop over read/write if there's no DMA support?

Edit: Ok, I found and read the section on higher level abstractions that touches on this -- that's an option, but I think "transfer a whole block" is both so commonly used by higher levels, and so commonly doable better with DMA, that it makes sense for the device HAL implementations to provide for it natively, rather than it being done generically as a word-by-word loop, even with async.

[Kixunil]

I agree with @adamgreig. Writing more bytes is a common thing. Maybe use something like genio would help. I think splitting Serial trait is good idea. Not just because of possibly different errors, but also possible consumers only using one half. Similar decision was made in case of tokio-io.

[japaric]

@adamgreig Yes, that would be in a different trait: SerialAsync with, for example, an #[async] write_all(&self, buf: &[u8]) -> Result<(), E> method where the default implementation (which you get for free if your Serial type implements the hal::Serial trait) makes progress word by word using Serial.write and await!. But you can specialize your write_all implementation to use the DMA under the hood. Wait, I take that back: having written a memory safe DMA transfer abstraction before I don't think both can have the same signature; with that signature you could (a) allocate a buffer on the stack, (b) start the async write_all operation that uses the DMA, (c) not complete the async operation and drop the generator, (d) free the buffer that was allocated on the stack (yes, the compiler won't complain in this case) and (e) have a Bad Day (the DMA transfer is still ongoing).

So, yeah I think stuff that uses the DMA will require different signatures / traits to actually be memory safe.

@Kixunil

but also possible consumers only using one half

That makes sense.

Maybe use something like genio would help

That seems to be a blocking API. We probably will want two APIs: a blocking way and a non blocking one.

[Kixunil]

That seems to be a blocking API.

As far as I understand, the only difference is Error type. You could easily bound by Read<ReadError=nb::Error<E>>

Maybe add some convenience traits or just duplicate those with slightly modified return types.

[japaric]

As far as I understand, the only difference is Error type. You could easily bound by Read<ReadError=nb::Error>

No, that wouldn't work for read_all / write_all. nb::Error and its associated macros are for retry-able operations who's retry operation doesn't require keeping / changing state. For operations that require a change of state you have to pick between futures, #[async] or blocking; you can't abstract over async-ness by changing the error type in that case because each model has a different function signature (-> impl Future vs #[async] -> Result vs -> Result).

[vitiral]

I've opened PR #13, as I don't think we should use th nb crate -- futures will serve us just fine.

However, I think @adamgreig touches on a very important issue which is that we need ways to indicate that a whole buffer of data is ready. In std rust this would just be futures::Async<Vec<u8>> -- but obviously we can't use Vec!

In REQ-std I state that one core data type must be "buffering: creating global buffers and sharing them with functions and between threads". We must have core data types that enable sharing and returning of owned global buffers, as these kind of objects are extremely core to the operation of most embedded systems.

Then most of these IO features could be built on top of these buffers.

[Kixunil]

@japaric well, read_exact and write_all would both fail with nb::Error::WouldBlock, throwing away the state. Although it would be possible to use it without using such broken functions, I'm in favour of creating a new trait now.

That being said, I believe such trait should look like this:

trait AsyncRead {
    type ReadError;

                // Maybe some kind of buffer with possibly uninitialized memoy instead of [u8]?
    fn read(&mut [u8]) -> Result<usize, nb::Error<Self::ReadError>>;
}

Similar in case of Write.

[etrombly]

Another thing to consider is where you would want to host libraries that are implemented using this hal. If I write say a stepper driver using the gpio hal, it would be nice if other people could easily find and use it. It could just be an index that someone maintains like the awesome rust page, or maybe something more involved like platformio

[nicholastmosher]

How will pin selection be handled? If there is overlap between, say, a GPIO line and a PWM channel line, will the HAL provide an error mechanism if one attempts to use both at the same time? Or is it possible that the svd2rust output already accounts for this (I haven't read into svd2rust thoroughly)? I can imagine one way to do this would be to make some sort of Pin struct that acts as a lock that a peripheral must acquire in order to work.

My apologies if this is a non-issue because of some implementation details I'm unaware of :)

[etrombly]

Also a few other API's you may want to consider adding are RTC, Clock speed selection (I know you've mentioned in the past leaving this up to the board support crates, but I think it's pretty common), power saving (setting sleep/stop/standby), attaching an interrupt to a pin (may be out of scope), and DAC.

[adamgreig]

I think we're mixing up what's wanted from a HAL. embedded-hal provides a bunch of generic traits which is really useful to build device-portable libraries on top of (sensor drivers, communication stacks, etc), but things like pin selection, clock selection, etc are rightfully out of scope for this crate.

Where they would be really nice is microcontroller-specific HALs that make configuring that microcontroller easier, and there you could totally imagine having something that checked that your pin selections made sense, etc. But I think that's a lot of work and by its nature can't be very cross-device without losing a lot of utility, just because so many features are so device-specific.

[etrombly]

@adamgreig you're right, I was looking at this more as a platform (the hal being more generic than just for writing libraries). It does make sense for device specific settings to be in separate crates, I was just looking at this crate to be where the API for those interactions would be defined. It does make sense if that's out of scope for this crate, but I do think it should be considered in the early stages of design.

[japaric]

@nicholastmosher @etrombly

From the documentation

Out of scope

• Initialization and configuration stuff like "ensure this serial interface and that SPI interface are not using the same pins". The HAL will focus on doing I/O.

Now I think that that ^ is a useful thing to have but it doesn't belong to this crate, to this set of traits. This set of traits has constraits around zero cost-ness and async model agnostic-ness that don't make sense for a configuration layer.

I have written my thoughts on configuration at the end of this comment. I'll eventually open an issue about configuration on the rust-embedded/rfcs so let's keep this thread on topic please.

[etrombly]

No problem, I believe having a DAC API would still be in scope then, and is available on several of the stm32 chips.

[oli-obk]

We (https://github.com/embed-rs/) cannot use the APIs as suggested here, since the APIs take &self instead of &mut self, even if the method obviously needs to modify the hardware-state. We pass around owned handles to different parts of the hardware to prevent accidentally using the same registers twice from different drivers (On purpose is still possible with Rc). To make this really useful, all methods modifying the hardware need &mut references to the register.

I think changing the appropriate methods to &mut will also work for you, since the traits can be implemented on &SomeType instead of SomeType. What do you think?

cc @f-bro @phil-opp

[Kixunil]

@oli-obk good observation. &mut self is also useful in cases one wants to write "virtual" devices for tests. I agree the traits should take &mut self

[japaric]

@oli-obk Sounds good to me.

[gsnoff]

@oli-obk For the record, there might be some downsides in using unique &mut references for hardware instances, particularly related to the performance of single-threaded event-driven code, as well as general auditability. See this post for details. I guess this doesn't apply for accessing the same hardware from multiple threads, in which case there would unlikely be any other alternative to &mut references for safety guarantees.

cc @f-bro @phil-opp

[oli-obk]

@Protagores I don't think your and our method are in conflict. Inside the map function you have a mutable reference to the object and can therefor call the &mut methods.

[getreu]

@japaric @etrombly

My other question is where generic higher level abstractions like these should live?

It is mostly a matter how we want different people to cooperate.
The advantage of having separate repositories is, that there can be different maintainers and work-groups. As a side effect those API's are usually better documented and more stable.
My suggestion:

repository type a: RTFM core developers
repository type b: Driver developers for chips
repository type c: Driver developers for board X (who combine and configure a. and b.)
repository type d: End user ("user land developers" in traditional design)

It is worth do have a look in TockOS. They differentiate between "chips" and "boards" and have a "capsule" API for drivers.

[japaric]

I think this issue has served its purpose. The signature of the methods has changed from &self to &mut self and the higher level traits are also living in this crate / repo. If someone wants to raise a concern please open a more targeted issue.