rust-embedded / embedded-hal

A Hardware Abstraction Layer (HAL) for embedded systems
Apache License 2.0
2.01k stars 202 forks source link

Add `discard` to `BufRead`. #564

Closed andresv closed 10 months ago

andresv commented 10 months ago

Let's say I have an akward protocol that uses time gaps between fixed length messages. Here it starts reading from wrong place and continues to do so.

XXXXXXXX    XXXXXXXXX
     |          |                
     |          got 10 bytes
    start reading 10 bytes

Bunch of wrong bytes are feed into parser. In this situation I would like to discard all data that is in internal buffer and start over.

loop {
    match timeout(10.millis(), uart.fill_buf()).await {
        Ok(bytes) => {
            for (cnt, byte) in bytes.iter().enumerate() {
                match parser.put(byte) {
                    Ok(msg) => {
                        uart.consume(cnt + 1);
                        return Some(msg);
                    }
                    Err(parser_error) => {
                        // Parser error indicates that something is wrong with the data.
                        // We do not have start delimiter so we just have to wait for the next full message.
                        // If there is something in the buffer it should be discarded, otherwise half from the previous
                        // message is feed in to the parser again.
                        uart.discard();
                    }
                }
            }
        }
        Err(timeout) => {
            // Get rid of everything that is in internal buffer.
            uart.discard();
            parser.reset();
        }
    }
}

Currently there isn't any way to discard data so I propose addind discard method for BufRead.

/// Async buffered reader.
///
/// This trait is the `embedded-io-async` equivalent of [`std::io::BufRead`].
pub trait BufRead: ErrorType {
    /// Return the contents of the internal buffer, filling it with more data from the inner reader if it is empty.
    ///
    /// If no bytes are currently available to read, this function waits until at least one byte is available.
    ///
    /// If the reader is at end-of-file (EOF), an empty slice 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.
    async fn fill_buf(&mut self) -> Result<&[u8], Self::Error>;

    /// Tell this buffer that `amt` bytes have been consumed from the buffer, so they should no longer be returned in calls to `fill_buf`.
    fn consume(&mut self, amt: usize);

    /// Discard all data from internal buffer.
    fn discard(&mut self);
}
Dirbaio commented 10 months ago

I don't think we should add it, you can already do it with the available API.

while uart.read_ready()? {
   let n = uart.fill_buf()?.len();
   uart.consume(n);
}
andresv commented 10 months ago

Ahaa then there is just a bug in embassy https://github.com/embassy-rs/embassy/blob/main/embassy-stm32/src/usart/buffered.rs#L340 which does not allow to just call consume with max buffer value.

Notice that it only modifies start and not the end: https://github.com/embassy-rs/embassy/blob/main/embassy-hal-internal/src/atomic_ring_buffer.rs#L338.

I'll move this discussion to embassy.

Dirbaio commented 10 months ago

does not allow to just call consume with max buffer value

you're only allowed to call consume with at most fill_buf().len(). You can't consume bytes you haven't received yet.

andresv commented 10 months ago

Then it is not that easy to do with async. Because in order to figure out how many bytes there are in internal buffer I have to call fill_buf().await again. I cannot get the len otherwise.

loop {
    match uart.fill_buf.await {
        Ok(bytes) => {
            match parser.parse(bytes) {
                Ok(msg) => {
                    uart.consume(bytes.len());
                    return Some(msg);
                }
                Err(e) => {
                    // Let's just discard all possible data in the incoming buffer.
                    // This here returns immediately if there is some data in buffers,
                    // but if there isn't then it waits until something is received
                    // but then it already starts to receive data that I would like to handle in main match.
                    // Here I just wanted to be sure that everything that we already have is discarded
                    // and no new data is received.
                    if let Ok(bytes) = uart.fill_buf.await {
                        uart.consume(bytes.len());
                    }
                }
            }
        }
        Err(_) => {}

    }
}
Dirbaio commented 10 months ago

this is why I wrapped it in a while uart.read_ready()? loop. If there's no data ready to be retrieved from the rx buffer, then don't do any fill_buf(). If there's data, fill_buf() will return just that, not waiting for more. So the loop is guaranteed to empty the buffer, and never block.