UartDriver as embedded_io::Read blocks until buffer is full

[DaneSlattery]

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

[ivmarkov]

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

[ivmarkov]

@DaneSlattery ^^^

[DaneSlattery]

That fixes it, thank you! Happy with the solution you came to.