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.

Sign up for GitHub

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

Jump to bottom

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

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions avr-hal-generic/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ paste = "1.0.0"
avr-device = "0.5.3"
embedded-storage = "0.2"
embedded-hal = "1.0"
embedded-io = "0.6.1"
unwrap-infallible = "0.1.5"

[dependencies.embedded-hal-v0]
Expand Down
152 changes: 143 additions & 9 deletions avr-hal-generic/src/usart.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,12 @@
//! Check the documentation of [`Usart`] for details.

use core::cmp::Ordering;
use core::convert::Infallible;
use core::marker;
use crate::prelude::*;

use embedded_io::ErrorType;

use crate::port;

/// Representation of a USART baudrate
Expand Down Expand Up @@ -184,21 +187,21 @@ pub trait UsartOps<H, RX, TX> {
/// was flushed yet.
///
/// **Warning**: This is a low-level method and should not be called directly from user code.
fn raw_flush(&mut self) -> nb::Result<(), core::convert::Infallible>;
fn raw_flush(&mut self) -> nb::Result<(), Infallible>;
/// Write a byte to the TX buffer.
///
/// This operation must be non-blocking and return [`nb::Error::WouldBlock`] until the byte is
/// enqueued. The operation should not wait for the byte to have actually been sent.
///
/// **Warning**: This is a low-level method and should not be called directly from user code.
fn raw_write(&mut self, byte: u8) -> nb::Result<(), core::convert::Infallible>;
fn raw_write(&mut self, byte: u8) -> nb::Result<(), Infallible>;
/// Read a byte from the RX buffer.
///
/// This operation must be non-blocking and return [`nb::Error::WouldBlock`] if no incoming
/// byte is available.
///
/// **Warning**: This is a low-level method and should not be called directly from user code.
fn raw_read(&mut self) -> nb::Result<u8, core::convert::Infallible>;
fn raw_read(&mut self) -> nb::Result<u8, Infallible>;

/// Enable/Disable a certain interrupt.
///
Expand Down Expand Up @@ -336,7 +339,7 @@ impl<H, USART: UsartOps<H, RX, TX>, RX, TX, CLOCK> Usart<H, USART, RX, TX, CLOCK
}

impl<H, USART: UsartOps<H, RX, TX>, RX, TX, CLOCK> ufmt::uWrite for Usart<H, USART, RX, TX, CLOCK> {
type Error = core::convert::Infallible;
type Error = Infallible;

fn write_str(&mut self, s: &str) -> Result<(), Self::Error> {
for b in s.as_bytes().iter() {
Expand All @@ -349,7 +352,7 @@ impl<H, USART: UsartOps<H, RX, TX>, RX, TX, CLOCK> ufmt::uWrite for Usart<H, USA
impl<H, USART: UsartOps<H, RX, TX>, RX, TX, CLOCK> embedded_hal_v0::serial::Write<u8>
for Usart<H, USART, RX, TX, CLOCK>
{
type Error = core::convert::Infallible;
type Error = Infallible;

fn write(&mut self, byte: u8) -> nb::Result<(), Self::Error> {
self.p.raw_write(byte)
Expand All @@ -360,16 +363,78 @@ impl<H, USART: UsartOps<H, RX, TX>, RX, TX, CLOCK> embedded_hal_v0::serial::Writ
}
}

impl<H, USART: UsartOps<H, RX, TX>, RX, TX, CLOCK> ErrorType for Usart<H, USART, RX, TX, CLOCK> { type Error = Infallible; }

impl<H, USART: UsartOps<H, RX, TX>, RX, TX, CLOCK> embedded_io::Write for Usart<H, USART, RX, TX, CLOCK> {
fn write(&mut self, buf: &[u8]) -> Result<usize, Self::Error> {
if buf.is_empty() {
return Ok(0);
}
// 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`
}
}
}
Comment on lines +373 to +390
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...

Ok(i)
}

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

Ok(())
}
}

impl<H, USART: UsartOps<H, RX, TX>, RX, TX, CLOCK> embedded_hal_v0::serial::Read<u8>
for Usart<H, USART, RX, TX, CLOCK>
{
type Error = core::convert::Infallible;
type Error = Infallible;

fn read(&mut self) -> nb::Result<u8, Self::Error> {
self.p.raw_read()
}
}

impl<H, USART: UsartOps<H, RX, TX>, RX, TX, CLOCK> embedded_io::Read for Usart<H, USART, RX, TX, CLOCK> {
fn read(&mut self, buf: &mut [u8]) -> Result<usize, Self::Error> {
// block for first byte
buf[0] = self.read_byte();
Comment on lines +411 to +413
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.

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`
}
}
}
Comment on lines +414 to +434
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()?

}
}

/// Writer half of a [`Usart`] peripheral.
///
/// Created by calling [`Usart::split`]. Splitting a peripheral into reader and writer allows
Expand Down Expand Up @@ -413,6 +478,14 @@ impl<H, USART: UsartOps<H, RX, TX>, RX, TX, CLOCK> UsartWriter<H, USART, RX, TX,
_h: marker::PhantomData,
}
}

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

}

impl<H, USART: UsartOps<H, RX, TX>, RX, TX, CLOCK> UsartReader<H, USART, RX, TX, CLOCK> {
Expand All @@ -434,7 +507,7 @@ impl<H, USART: UsartOps<H, RX, TX>, RX, TX, CLOCK> UsartReader<H, USART, RX, TX,
impl<H, USART: UsartOps<H, RX, TX>, RX, TX, CLOCK> ufmt::uWrite
for UsartWriter<H, USART, RX, TX, CLOCK>
{
type Error = core::convert::Infallible;
type Error = Infallible;

fn write_str(&mut self, s: &str) -> Result<(), Self::Error> {
for b in s.as_bytes().iter() {
Expand All @@ -447,7 +520,7 @@ impl<H, USART: UsartOps<H, RX, TX>, RX, TX, CLOCK> ufmt::uWrite
impl<H, USART: UsartOps<H, RX, TX>, RX, TX, CLOCK> embedded_hal_v0::serial::Write<u8>
for UsartWriter<H, USART, RX, TX, CLOCK>
{
type Error = core::convert::Infallible;
type Error = Infallible;

fn write(&mut self, byte: u8) -> nb::Result<(), Self::Error> {
self.p.raw_write(byte)
Expand All @@ -458,16 +531,77 @@ impl<H, USART: UsartOps<H, RX, TX>, RX, TX, CLOCK> embedded_hal_v0::serial::Writ
}
}

impl<H, USART: UsartOps<H, RX, TX>, RX, TX, CLOCK> ErrorType for UsartWriter<H, USART, RX, TX, CLOCK> { type Error = Infallible; }

impl<H, USART: UsartOps<H, RX, TX>, RX, TX, CLOCK> embedded_io::Write for UsartWriter<H, USART, RX, TX, CLOCK> {
fn write(&mut self, buf: &[u8]) -> Result<usize, Self::Error> {
if buf.is_empty() {
return Ok(0);
}
// 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`
}
}
}
Ok(i)
}

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

impl<H, USART: UsartOps<H, RX, TX>, RX, TX, CLOCK> embedded_hal_v0::serial::Read<u8>
for UsartReader<H, USART, RX, TX, CLOCK>
{
type Error = core::convert::Infallible;
type Error = Infallible;

fn read(&mut self) -> nb::Result<u8, Self::Error> {
self.p.raw_read()
}
}


impl<H, USART: UsartOps<H, RX, TX>, RX, TX, CLOCK> ErrorType for UsartReader<H, USART, RX, TX, CLOCK> { type Error = Infallible; }

impl<H, USART: UsartOps<H, RX, TX>, RX, TX, CLOCK> embedded_io::Read for UsartReader<H, USART, RX, TX, CLOCK> {
fn read(&mut self, buf: &mut [u8]) -> Result<usize, Self::Error> {
let mut i = 0;
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`
}
}
}
}
}

#[macro_export]
macro_rules! impl_usart_traditional {
(
Expand Down
1 change: 1 addition & 0 deletions examples/arduino-uno/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ embedded-hal = "1.0"
pwm-pca9685 = "0.3.1"
infrared = "0.14.1"
embedded-storage = "0.2"
embedded-io = "0.6.1"

[dependencies.embedded-hal-v0]
version = "0.2.3"
Expand Down
29 changes: 29 additions & 0 deletions examples/arduino-uno/src/bin/uno-usart-embedded-io.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*!
* Demonstration of writing to and reading from the serial console.
*/
#![no_std]
#![no_main]

use panic_halt as _;

use embedded_io::{Read, Write};

fn usart_handler(serial: &mut (impl Read + Write)) -> ! {
serial.write_all("Hello from Arduino!\r\n".as_bytes()).unwrap();

loop {
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();
Comment on lines +15 to +18
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...

}
}

#[arduino_hal::entry]
fn main() -> ! {
let dp = arduino_hal::Peripherals::take().unwrap();
let pins = arduino_hal::pins!(dp);
let mut serial = arduino_hal::default_serial!(dp, pins, 57600);

usart_handler(&mut serial);
}