serial: add ReadExact, ReadUntilIdle

[Dirbaio]

REQUIRES #442

Write mirrors the blocking trait. Read is new.

Unresolved issue: how does Read work. See previous thread #334 (comment)

[rust-highfive]

r? @ryankurte

(rust-highfive has picked a reviewer for you, use r? to override)

[Dirbaio]

okay, so:

There's 2 ways of writing a UART HAL drivers:

• Buffered: The driver has a reasonably-sized buffer in RAM. Hardware places Incoming bytes are placed into the buffer automatically without user code having to do anything (via DMA, or an ISR). User code pops bytes out of the buffer. Bytes are only lost if received while buffer is full.
• Unbuffered: The driver has no buffer (or a pathetically small buffer: stm32 and nrf have a 4 byte buffer in hardware). User code does "read" operations that pop bytes directly from the hardware into the user's buffer. Bytes received while the user code is not actively waiting on a read are lost.

For example:

• Linux's /dev/ttyX is buffered.
• Embassy has both: nrf buffered, unbuffered, stm32 buffered unbuffered
• Almost all other Rust HALs uarts are unbuffered.

In both buffered and unbuffered, if you're using RTS/CTS data never gets lost. If you're not using RTS/CTS, it's super easy to lose data with unbuffered UARTs. People do use UART without RTS/CTS in practice, because out of pins in the MCU, or in a cable, or they simply forgot, or their protocol doesn't allow it (RS-485). IMO the traits have to be usable without RTS/CTS.

So, the question is: are the traits for buffered or unbuffered, or both?

For buffered, an "io::Read-like" trait that allows short reads works fine: fn read(&mut self, read: &mut [Word]) -> Result<usize, Error>. The impl returns when there's at least 1 byte received. If user code gets fewer bytes than it wants, it calls read() again. No bytes are lost thanks to the buffer.

For unbuffered, such "io::Read-like" trait does not work in practice. Some data arrives, the impl returns early, user code calls read() to get more bytes, but if it takes too long between read() calls, bytes will already be lost. Drivers need much more precise control in this case. either "read exactly N bytes, never ever early return", or read until line idle or buffer full if they don't know the size of incoming packets.

I don't think "allowing but not require early return" is a good solution. It'd make writing drivers harder, behavior should be consistent between all impls.

So I can think of these solutions:

• Mandate buffered impls, make the traits io-like (must do early return).
• Allow unbuffered impls, make the traits lower level (read exactly N bytes, read until idle. ..?)
• Do both.

In my experience, buffered UART has been MUCH nicer to work with. For "AT-command-like" stuff, unbuffered is incredibly fiddly to get right. You often have to do "read until newline", where you don't know how many bytes it'll be. If you use RTS/CTS you can get away with reading bytes one by one, but that's slow: there's big pauses between bytes, since RTS gets toggled for each one. If you don't have RTS/CTS, there's no way to do it reliably at all. Unbuffered read is only usable for "packet-like" comms where you either know the length upfront or use "read until idle", and ideally only with CRCs/acks/retries.

The downside for buffered is it requires more RAM (for the buffer), and it's much harder to impl for HALs (only sane way is to use DMA, really).

So... I don't know what's best! 😅

Speaking of "io-like" traits, if we pick that option maybe we should have a set of "general" IO traits instead, and have Serial use the io traits, instead of having Serial-specific traits. This'd allow running the same code (an interactive shell maybe?) over serial and, say, a TCP connection, for example! I've been working on a set of IO traits here (because I need them for Embassy) . I think this might be starting to get out of scope for embedded-hal though.

Also, this applies to both blocking and async UART read. IMO we should decide on some solution, then add it in both blocking and async forms. Not having blocking UART read is not very nice.

cc @lulf

[ryankurte]

good summary!

In my experience, buffered UART has been MUCH nicer to work with.

agreed, and i think this is the right level of abstraction for the majority of drivers (as well as being implied by the existing Write::flush method).

Unbuffered read is only usable for "packet-like" comms where you either know the length upfront or use "read until idle", and ideally only with CRCs/acks/retries.

i think if you need this you're going to have to implement it for your hardware anyway, so while we could provide some kind of a ReadByte trait it'd be okay to elide this (especially for now).

[guillaume-michel]

Hi,
@Dirbaio What would it take to settle to a conclusion for this PR? It would be great to be able to write async drivers with serial interface.

My personal opinion would be to allow as much use case as possible and let the user choose what is best for him.

I guess we need:

• an io::Read-like which can return early and return the number of read bytes,
• a read exact kind of trait for unbuffered which does not return until the provided buffer is filled,
• a read_until_idle trait for hardware capable of detecting it which returns the number of read bytes

I would be happy to help if needed.

[Dirbaio]

I also think we need the 3, yes. Note that io-like traits already exist at embedded-io.

@ryankurte @eldruin My proposal to unblock this:

• Keep read to be "read until buffer full", with no early return.
• Add read_until_idle.
• Add docs explaining "buffered" vs "unbuffered" uarts and their tradeoffs. Explain the traits in embedded_hal::uart are for unbuffered UART, point people to embedded-io for traits for buffered uart.

[eldruin]

That sounds fine to me. I have not been deeply involved in the discussion, though.
You would need to document what "idle" means as well. i.e. does silence for X time also count as idle?

[guillaume-michel]

Hi,
As far as I know idle means silence for 1 period. This is actually how the nrf embassy implementation detects idle.

[Dirbaio]

Okay, following the previous discussions:

• I've split async write into async/serial: add Write. #442 since that should be uncontroversial.
• Renamed Read into ReadExact. Matches the std::io name and makes it more obvious there's no "early return".
• Added ReadUntilIdle
• Added docs explaining the difference between buffered and unbuffered reads, and clarify the traits here are for unbuffered serial port interfaces, and their caveats.

Should be ready for review.

[Dirbaio]

Discussed in today's WG meeting https://matrix.to/#/!BHcierreUuwCMxVqOf:matrix.org/$36Ghk5FUs3y2c-2eoOZKugKx4sS_xJ6VE8uS_78EyrE?via=matrix.org&via=psion.agg.io&via=tchncs.de

Discussed what exactly are the differences between buffered and unbuffered, and whether we need both. No conclusions reached.

[ryankurte]

we've needed this for a long time and it seems broadly right to me so, let's see how it shakes out / lgtm ^_^
(though again it would be great to have example impls in a couple hals / a driver)

[Dirbaio]

though again it would be great to have example impls in a couple hals / a driver

embassy-rp, embassy-stm32, embassy-nrf have implementations of these methods (not with the trait though, because it doesn't exist yet)

[Dirbaio]

Rebased to fix conflicts. Could you take a look @ryankurte @eldruin @therealprof ?

[eldruin]

This seems fine to me.
Let's continue the discussion about whether buffered/unbuffered is the right distinction and wording, though.

[Sympatron]

In my opinion buffered and unbuffered describe exactly what is happening under the hood and are therefore a perfect fit.
This naming scheme will probably result in the least amount of confusion.

[Dirbaio]

After discussions in the WG meetings, we decided it's better to not have traits for unbuffered UART.

• The vast majority of drivers need buffered to work correctly, and the few who could use unbuffered can also use buffered (at the cost of some extra RAM).
• There's a high complexity cost in having two traits for UART. The distinction between them is very subtle, it WILL cause confusion and mistakes.

Next steps:

• Should the buffered traits be embedded-io, or something UART-specific inside embedded-hal?
• If it's embedded-io, shall we move into the WG?
• Should we remove the serial::Write trait, in favor of embedded-io's Write?
• Should embedded-io get a nonblocking flavor, aside from the current blocking and asynch? Or is the embedded_hal_nb::serial::Read trait enough?