UartDriver as `embedded_io::Read` blocks until buffer is full
DaneSlattery opened this issue · 3 comments
In the below code snippet, modified from the uart loopback.
We know that the uart TX has written 1 byte.
The receive buffer is [0;10] , which is typical when we don't know how large the receive buffer is.
In this case, the read blocks forever, instead of popping 1 byte from the UART buffer (into buf) and returning 1. If the buffer is [0;1], the read returns instantly.
This means that the embedded_io::Read contract is not being upheld:
/// Read some bytes from this source into the specified buffer, returning how many bytes were read.
///
/// If no bytes are currently available to read, this function blocks until at least one byte is available.
///
/// If bytes are available, a non-zero amount of bytes is read to the beginning of `buf`, and the amount
/// is returned. It is not guaranteed that *all* available bytes are returned, it is possible for the
/// implementation to read an amount of bytes less than `buf.len()` while there are more bytes immediately
/// available.
///
/// If the reader is at end-of-file (EOF), `Ok(0)` is returned. There is no guarantee that a reader at EOF
/// will always be so in the future, for example a reader can stop being at EOF if another process appends
/// more bytes to the underlying file.
///
/// 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.
//! UART loopback test
//!
//! Folowing pins are used:
//! TX GPIO12
//! RX GPIO13
//!
//! Depending on your target and the board you are using you have to change the pins.
//!
//! This example transfers data via UART.
//! Connect TX and RX pins to see the outgoing data is read as incoming data.
use embedded_io::Read;
use esp_idf_hal::delay::BLOCK;
use esp_idf_hal::gpio;
use esp_idf_hal::peripherals::Peripherals;
use esp_idf_hal::prelude::*;
use esp_idf_hal::uart::*;
use esp_idf_sys::EspError;
fn main() -> anyhow::Result<()> {
esp_idf_hal::sys::link_patches();
let peripherals = Peripherals::take()?;
let tx = peripherals.pins.gpio12;
let rx = peripherals.pins.gpio13;
println!("Starting UART loopback test");
let config = config::Config::new().baudrate(Hertz(115_200));
let mut uart = UartDriver::new(
peripherals.uart1,
tx,
rx,
Option::<gpio::Gpio0>::None,
Option::<gpio::Gpio1>::None,
&config,
)?;
loop {
uart.write(&[0xaa])?;
read(&mut uart)?;
}
}
fn read<R: embedded_io::Read>(r: &mut R) -> Result<(), anyhow::Error> {
let mut buf = [0u8; 10];
match r.read(&mut buf) {
Err(x) => println!("Error reading"),
Ok(x) => println!("Read :{:?} ", &buf[..x]),
};
Ok(())
}The approach in UartRxDriver is to call uart_read_bytes with length=buffer.len().
I propose the following:
/// Read multiple bytes into a slice; block until specified timeout
pub fn read(&self, buf: &mut [u8], delay: TickType_t) -> Result<usize, EspError> {
// uart_read_bytes() returns error (-1) or how many bytes were read out
// 0 means timeout and nothing is yet read out
let available = self.count()?;
if (available > buf.len()) {
// should we throw? or only read as many as the buffer has space for
}
let len = unsafe {
uart_read_bytes(
self.port(),
buf.as_mut_ptr().cast(),
available as u32,
delay,
)
};
if len >= 0 {
Ok(len as usize)
} else {
Err(EspError::from_infallible::<ESP_ERR_INVALID_STATE>())
}
}UPDATED:
You might be right to request the semantics you request as I polled the "Embedded Rust" Matrix channel, and it seems the consensus is it should operate as you intuitively expect. Though - I must say and the other folks agreed - this is not explicitly documented on the embedded_io::Read trait, nor on the std::io::Read trait. The POSIX read and recv syscalls seem a bit more explicit but IMO still leave room for interpretation!
(
I'll probably PR an update of the docu of embedded_io::Read and embedded_io_async::Read to be more explicit in this regard. And will probably PR or at least open an issue for std::io::Read as it has similar vagueness that leaves room for interpretation.
)
With that said, I just pushed a potential fix to master, so if you could try it out?
The reason why I did not accept your proposal from above ^^^ verbatim is because it has several problems. For completeness, let me comment on these (look for the comments marked with "(Ivan)"):
/// Read multiple bytes into a slice; block until specified timeout
pub fn read(&self, buf: &mut [u8], delay: TickType_t) -> Result<usize, EspError> {
// uart_read_bytes() returns error (-1) or how many bytes were read out
// 0 means timeout and nothing is yet read out
// (Ivan) -1 also might mean a timeout. -1 is returned when `uart_read_bytes` cannot lock the internal semaphore, which
// might happen if multiple threads try to call this `read` method
let available = self.count()?;
if (available > buf.len()) {
// should we throw? or only read as many as the buffer has space for
// (Ivan) We should only read as many bytes as the buffer has space for
}
// (Ivan) The problem with this code is when available = 0
// In this case, the call below will return immediately (as that's how `uart_read_bytes` is implemented)
// - yet - you want it to block until at least one byte becomes available, or until the `delay` expires
let len = unsafe {
uart_read_bytes(
self.port(),
buf.as_mut_ptr().cast(),
available as u32,
delay,
)
};
if len >= 0 {
Ok(len as usize)
} else {
Err(EspError::from_infallible::<ESP_ERR_INVALID_STATE>())
}
}@DaneSlattery ^^^
That fixes it, thank you! Happy with the solution you came to.