io::{Read,Write} traits

[Nemo157]

I mentioned on rust-embedded/embedded-hal#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.

[therealprof]

&mut &mut?

[Nemo157]

Removed one, it does become an &mut &mut during the method calls, but it doesn't need to be one to pass into write_all.

[austinglaser]

I feel like I'm missing something, but shouldn't the return type here (and elsewhere) be nb::Result<usize, Self::Error>?

Is there a type alias somewhere that I'm missing?

[Nemo157]

This is referring to nb::Result, it's just since this is defined in the nb crate the crate root is just ::. This was a leftover from initially implementing in embedded-hal then doing minimal changes to get it compiling here, I'll switch to an actual use of Result here.

[austinglaser]

Oh good lord that was silly of me. You linked from the embedded-hal crate, and I didn't notice that this was a pull request for nb

[Nemo157]

rust-lang-nursery/portability-wg#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 rust-lang-nursery/portability-wg#1 (comment)

[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).