Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rursprung
Copy link
Contributor

@rursprung rursprung commented Dec 28, 2023

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
Copy link
Contributor Author

rursprung commented Dec 28, 2023

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
Copy link
Contributor Author

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

Copy link
Owner

@Rahix Rahix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +373 to +390
// 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`
}
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Comment on lines +414 to +434
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`
}
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?

Comment on lines +411 to +413
fn read(&mut self, buf: &mut [u8]) -> Result<usize, Self::Error> {
// block for first byte
buf[0] = self.read_byte();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +482 to +488
/// 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()
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +15 to +18
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();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants