implement `embedded-io` v0.6 `Read` & `Write` #484

rursprung · 2023-12-28T19:07:23Z

with embedded-hal v1 the USART traits have been removed in favour of the new embedded-io crate.

this adds a (very basic) implementation for Read and Write. other traits (such as the *Ready or BufRead traits) have not (yet) been implemented and some (like Seek) probably can't be implemented for this HAL.

a better implementation might use a buffer in the background to receive more than one byte at once.

see also #249 for a related PR.

this is part of #468

i've tested this with an Arduino Uno and the newly provided example (though running at 9600 baud, as outlined in #478).

rursprung · 2023-12-28T19:20:10Z

this seems to have the same issue (at least on my device) as the existing implementation: for larger inputs it drops bytes when reading.

output from uno-usart (unchanged):

Hello from Arduino!
test
Got 116!
Got 101!
Got 115!
Got 10!
foobar
Got 102!
Got 111!
Got 111!
Got 10!
x
Got 120!
Got 13!
Got 10!

output from uno-usart-embedded-io:

Hello from Arduino!
x
Got [120] (which is 1 bytes long)
Got [13, 10] (which is 2 bytes long)
test
Got [116] (which is 1 bytes long)
Got [101, 115, 10] (which is 3 bytes long)
q
Got [113] (which is 1 bytes long)
Got [13, 10] (which is 2 bytes long)

this also shows that after the first character it seems to have a small lag (where it'd block) which is why it then reads the rest in a subsequent step (where it manages to read multiple characters without blocking).

with `embedded-hal` v1 the USART traits have been removed in favour of the new `embedded-io` crate. this adds a (very basic) implementation for `Read` and `Write`. other traits (such as the `*Ready` or `BufRead` traits) have not (yet) been implemented and some (like `Seek`) probably can't be implemented for this HAL. a better implementation might use a buffer in the background to receive more than one byte at once. see also Rahix#249 for a related PR. this is part of Rahix#468

rursprung · 2024-02-21T12:36:53Z

@Rahix: is there anything specific from my side which you need for the review?

Rahix

is there anything specific from my side which you need for the review?

No, just my horrible response latency at work again :(

In any case, thanks a lot for the contribution! Please check my comments below.

Rahix · 2024-04-01T01:48:19Z

avr-hal-generic/src/usart.rs

+        // block for first byte
+        self.write_byte(buf[0]);
+        let mut i = 1;
+
+        // write more bytes if it's possible
+        for byte in buf[1..].iter() {
+            match self.p.raw_write(*byte) {
+                Ok(_) => {
+                    i += 1;
+                }
+                Err(nb::Error::WouldBlock) => {
+                    return Ok(i);
+                }
+                Err(_) => {
+                    unreachable!(); // `raw_write` is `Infallible`
+                }
+            }
+        }


I think you are making this more complex than it needs to be. embedded_io::Write really only wants this:

At least one byte is enqueued

The first self.write_byte() call does just that. So the for loop afterwards isn't needed to fulfil the trait contract.

Now on technical terms, I assume you will also find that the for loop will most likely never actually cause any additional bytes to get written anyway - the transmitter won't be done with the first byte by the time you attempt to send the next one unless you somehow manage to combine very low CPU clock speeds with very high baudrates...

Rahix · 2024-04-01T01:49:30Z

avr-hal-generic/src/usart.rs

+    }
+
+    fn flush(&mut self) -> Result<(), Self::Error> {
+        self.p.raw_flush().unwrap(); // `raw_write` is `Infallible`


The comment here is wrong - raw_flush is fallible, as it can return WouldBlock. So really this needs to be

Suggested change

self.p.raw_flush().unwrap(); // `raw_write` is `Infallible`

nb::block!(self.p.raw_flush()).unwrap_infallible();

Rahix · 2024-04-01T01:56:00Z

avr-hal-generic/src/usart.rs

+        let mut i = 1;
+
+        // grab more bytes if available
+        loop {
+            match self.p.raw_read() {
+                Ok(byte) => {
+                    buf[i] = byte;
+                    i += 1;
+
+                    if i == buf.len() {
+                        return Ok(i);
+                    }
+                }
+                Err(nb::Error::WouldBlock) => {
+                    return Ok(i);
+                }
+                Err(_) => {
+                    unreachable!(); // `raw_read` is `Infallible`
+                }
+            }
+        }


Similar to my comment about write() above, I think you are trying to do more than necessary here. But the situation is a bit more complicated as the receive FIFO is two characters deep on AVR microcontrollers.

However, as it is not a violation of the trait contract, I'd err on the side of only reading one byte regardless. In most situations, the second byte will still be in transit when the first one is read, so any attempt to read it will WouldBlock anyway...

TL;DR: Only the first read_byte() is enough IMO.

What does surprise me a bit is that you are able to read 3 bytes in your example. A character at 9600 baud takes 1.1ms to receive, that's roughly 18k CPU cycles. How does this much time pass between taking the first character out of the receiver (read_byte()) and then reading the followup characters using raw_read()?

Rahix · 2024-04-01T02:18:53Z

avr-hal-generic/src/usart.rs

+    fn read(&mut self, buf: &mut [u8]) -> Result<usize, Self::Error> {
+        // block for first byte
+        buf[0] = self.read_byte();


Missing the treatment of buf.len() == 0. From the trait docs:

If buf.len() == 0, read returns without blocking, with either Ok(0) or an error. The Ok(0) doesn’t indicate EOF, unlike when called with a non-empty buffer.

Rahix · 2024-04-01T02:20:43Z

avr-hal-generic/src/usart.rs

+    /// Transmit a byte.
+    ///
+    /// This method will block until the byte has been enqueued for transmission but **not** until
+    /// it was entirely sent.
+    fn write_byte(&mut self, byte: u8) {
+        nb::block!(self.p.raw_write(byte)).unwrap()
+    }


Unsure whether it wouldn't be more readable to simply call nb::block!(self.p.raw_write(byte)) in the write() implementation directly to remove the additonal layer.

Rahix · 2024-04-01T02:27:05Z

examples/arduino-uno/src/bin/uno-usart-embedded-io.rs

+        let mut rx_buf: [u8; 16] = [0; 16];
+        let len = serial.read(&mut rx_buf).unwrap();
+
+        writeln!(serial, "Got {:?} (which is {} bytes long)", &rx_buf[..len], len).unwrap();


The problem with this code is that read() won't every return more than a few bytes. Immediately after receiving them, the writeln!() will block for a long time to spell out the response. During this time, we probably receive more data but can't handle it as we're still busy and the hardware doesn't have a (large enough) buffer.

I'm trying to think what a better example could look like. Maybe waiting for a newline? Or waiting for a large enough time of silence? I'd choose whatever variant is easier to implement, so probably the newline...

rursprung force-pushed the implement-embedded-io branch from f99adad to 3679a64 Compare December 28, 2023 19:20

rursprung mentioned this pull request Dec 29, 2023

implement embedded-io traits stm32-rs/stm32f4xx-hal#721

Open

rursprung force-pushed the implement-embedded-io branch from 3679a64 to 252e5ad Compare January 17, 2024 16:01

Rahix requested changes Apr 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement `embedded-io` v0.6 `Read` & `Write` #484

implement `embedded-io` v0.6 `Read` & `Write` #484

rursprung commented Dec 28, 2023 •

edited

rursprung commented Dec 28, 2023 •

edited

rursprung commented Feb 21, 2024

Rahix left a comment

Rahix Apr 1, 2024

Rahix Apr 1, 2024

Rahix Apr 1, 2024

Rahix Apr 1, 2024

Rahix Apr 1, 2024

Rahix Apr 1, 2024

	self.p.raw_flush().unwrap(); // `raw_write` is `Infallible`
	nb::block!(self.p.raw_flush()).unwrap_infallible();

implement embedded-io v0.6 Read & Write #484

Are you sure you want to change the base?

implement embedded-io v0.6 Read & Write #484

Conversation

rursprung commented Dec 28, 2023 • edited

rursprung commented Dec 28, 2023 • edited

rursprung commented Feb 21, 2024

Rahix left a comment

Choose a reason for hiding this comment

Rahix Apr 1, 2024

Choose a reason for hiding this comment

Rahix Apr 1, 2024

Choose a reason for hiding this comment

Rahix Apr 1, 2024

Choose a reason for hiding this comment

Rahix Apr 1, 2024

Choose a reason for hiding this comment

Rahix Apr 1, 2024

Choose a reason for hiding this comment

Rahix Apr 1, 2024

Choose a reason for hiding this comment

implement `embedded-io` v0.6 `Read` & `Write` #484

implement `embedded-io` v0.6 `Read` & `Write` #484

rursprung commented Dec 28, 2023 •

edited

rursprung commented Dec 28, 2023 •

edited