io::{Read,Write} traits

[Nemo157]

I mentioned on https://github.com/japaric/embedded-hal/pull/56 that it seems like a good idea for embedded_hal to provide base io traits similar to the ones in std. After a bit more thought I decided that they should probably be somewhere a little more fundamental than embedded_hal so even non-HAL using code could be based off them.

I'm not certain that nb itself should be the place to include these, or whether it would make sense to have a separate nb-io crate for them.

I'm relatively unhappy about adding a third set of these traits to the ecosystem (on top of the std ones and the futures-io ones, and maybe there are others I'm unaware of). It's also disappointing that these can't be compatible with the futures-utils::io adaptors, so like the example read_exact and write_all adaptors I've added they'll have to be rewritten for nb::io. But without something like the futures-io crate switching to an associated error type like these to avoid the std::io::Error dependency, I don't see any other way to do this; and I really don't think the futures ecosystem would like the ergonomic hit introducing that associated type would bring.

[Nemo157]

https://github.com/rust-lang-nursery/portability-wg/issues/12 may allow using the futures-io traits instead of these, that would be much nicer because then you get all the adaptors for free from futures-util.

[japaric]

Thanks for the PR, @Nemo157

The Read and Write traits look good to me but I'm unsure about having the ReadExact and WriteAll traits return an nb::Result -- those traits look like should be returning a Future (the method is even called poll :-); in which case they probably (?) shouldn't be in this crate.

In my mind operations that return nb::Result signal that they can't be started and completed right now via WouldBlock but once you get an Ok you know that the operation started and completed in one step, without blocking. This also means that operations that return nb::Result carry no program state -- there may be some state in the hardware registers or in the kernel but it's not on the program stack / heap.

Once you have an operation that requires keeping program state then you should pick one of: blocking behavior, generators or futures. I see the appeal in having, say, ReadExact return a nb::Result: then you can lift it into a blocking operation, generators or futures using one of the block!, try_nb! or await! macros. But I think that's a bit backwards, at least in the case of generators; there instead you should use nb::Read plus await! to avoid explicitly writing the state machine, which is what ReadExact requires.

Now that sounds like you would have to repeat the logic when writing a futures based version of ReadExact, but that one can be written on top of the generator version -- generators are more fundamental than futures; generators are just suspension points whereas futures assume an event loop and require you to specify a wakeup mechanism. Same thing with the blocking version: it can be written on top of the generator version.

All this may sound like we should write everything using generators instead of using nb::Result but there are a few reasons I didn't go that path: (a) generators weren't in the compiler when I created the nb crate, (b) you can't return a generator (impl Generator) from a trait method and (c) nb::Result is more natural when dealing with reactive / callback-y code.

To elaborate on the last point in reactive code you usually write something like this:

#[interrupt]
fn callback() {
   let byte = SERIAL.read().unwrap();
   // do stuff with byte
}

The hardware will call the callback function when there's new data in the RX buffer. If SERIAL.read() returns Error::WouldBlock then that would indicate some sort of bug / programmer error (e.g. you bound the callback to the wrong interrupt source), and it makes sense to panic (unwrap) on programmer errors. OTOH, the error wouldn't be so clear if Serial.read() returned a generator -- what are you suppose to do with the generator? busy wait? That would lead to an infinite loop if there's a bug.

That being said I think there's a place for generators in reactive code: you could store the generator in a static variable and have the interrupt handler resume the generator each time is called -- you could even share a generator between more two or more interrupt handlers. I think that way you could write your code in a "straight line" fashion where today you need to write state machines. Experimentation on that is blocked on static mut FOO: impl Generator = /* .. */ not being allowed today, though.

TL; DR IMO, nb::Result is for operations that require no program state and are completed in one step once they can start. Read and Write make sense to add but ReadExact and WriteAll should return a generator or a future; in which case they can't be traits because traits can't return an impl Trait so they would have to be functions but that would probably limit their usefulness in generic drivers.

[austinglaser]

I feel like it would be good to put the distinction between nb::Result and generators/futures (with respect to program state) in the documentation for this crate

[japaric]

@austinglaser I agree

[Nemo157]

@japaric that all sounds reasonable to me. I was able to find an actual potential integration point between nb and futures-io that would allow having just the simple Read/Write traits here then utilise the futures-utils::io adaptors for ReadExact/WriteAll etc., I've opened an issue on the futures library to discuss whether it's something they could support.

I'll update this PR tomorrow with just the simple traits.

[Ericson2314]

FYI I proposed adding core::io::{Read,Write} to the portability working group in https://github.com/rust-lang-nursery/portability-wg/issues/1#issuecomment-373790342

[Nemo157]

I finally got round to updating this to just the io::{Read, Write} traits. I've confirmed that this is forward compatible with at least one design of futures-io becoming no_std compatible (although, I have doubts whether that will ever happen).