rp/uart: report errors from buffered uart

this reports errors at the same location the blocking uart would, which
works out to being mostly exact (except in the case of overruns, where
one extra character is dropped). this is actually easier than going
nuclear in the case of errors and nuking both the buffer contents and
the rx fifo, both of which are things we'd have to do in addition to
what's added here, and neither are needed for correctness.

embassy-rp/src/uart/buffered.rs

@@ -2,6 +2,7 @@ use core::future::{poll_fn, Future};
 use core::slice;
 use core::task::Poll;
 
+use atomic_polyfill::{AtomicU8, Ordering};
 use embassy_cortex_m::interrupt::{Interrupt, InterruptExt};
 use embassy_hal_common::atomic_ring_buffer::RingBuffer;
 use embassy_sync::waitqueue::AtomicWaker;
@@ -16,8 +17,15 @@ pub struct State {
     tx_buf: RingBuffer,
     rx_waker: AtomicWaker,
     rx_buf: RingBuffer,
+    rx_error: AtomicU8,
 }
 
+// these must match bits 8..11 in UARTDR
+const RXE_OVERRUN: u8 = 8;
+const RXE_BREAK: u8 = 4;
+const RXE_PARITY: u8 = 2;
+const RXE_FRAMING: u8 = 1;
+
 impl State {
     pub const fn new() -> Self {
         Self {
@@ -25,6 +33,7 @@ impl State {
             tx_buf: RingBuffer::new(),
             rx_waker: AtomicWaker::new(),
             tx_waker: AtomicWaker::new(),
+            rx_error: AtomicU8::new(0),
         }
     }
 }
@@ -196,7 +205,25 @@ impl<'d, T: Instance> BufferedUartRx<'d, T> {
         })
     }
 
-    fn try_read(buf: &mut [u8]) -> Poll<Result<usize, Error>> {
+    fn get_rx_error() -> Option<Error> {
+        let errs = T::buffered_state().rx_error.swap(0, Ordering::Relaxed);
+        if errs & RXE_OVERRUN != 0 {
+            Some(Error::Overrun)
+        } else if errs & RXE_BREAK != 0 {
+            Some(Error::Break)
+        } else if errs & RXE_PARITY != 0 {
+            Some(Error::Parity)
+        } else if errs & RXE_FRAMING != 0 {
+            Some(Error::Framing)
+        } else {
+            None
+        }
+    }
+
+    fn try_read(buf: &mut [u8]) -> Poll<Result<usize, Error>>
+    where
+        T: 'd,
+    {
         if buf.is_empty() {
             return Poll::Ready(Ok(0));
         }
@@ -210,13 +237,16 @@ impl<'d, T: Instance> BufferedUartRx<'d, T> {
         });
 
         let result = if n == 0 {
-            return Poll::Pending;
+            match Self::get_rx_error() {
+                None => return Poll::Pending,
+                Some(e) => Err(e),
+            }
         } else {
             Ok(n)
         };
 
         // (Re-)Enable the interrupt to receive more data in case it was
-        // disabled because the buffer was full.
+        // disabled because the buffer was full or errors were detected.
         let regs = T::regs();
         unsafe {
             regs.uartimsc().write_set(|w| {
@@ -237,18 +267,28 @@ impl<'d, T: Instance> BufferedUartRx<'d, T> {
         }
     }
 
-    fn fill_buf<'a>() -> impl Future<Output = Result<&'a [u8], Error>> {
+    fn fill_buf<'a>() -> impl Future<Output = Result<&'a [u8], Error>>
+    where
+        T: 'd,
+    {
         poll_fn(move |cx| {
             let state = T::buffered_state();
             let mut rx_reader = unsafe { state.rx_buf.reader() };
             let (p, n) = rx_reader.pop_buf();
-            if n == 0 {
+            let result = if n == 0 {
-                state.rx_waker.register(cx.waker());
+                match Self::get_rx_error() {
-                return Poll::Pending;
+                    None => {
-            }
+                        state.rx_waker.register(cx.waker());
+                        return Poll::Pending;
+                    }
+                    Some(e) => Err(e),
+                }
+            } else {
+                let buf = unsafe { slice::from_raw_parts(p, n) };
+                Ok(buf)
+            };
 
-            let buf = unsafe { slice::from_raw_parts(p, n) };
+            Poll::Ready(result)
-            Poll::Ready(Ok(buf))
         })
     }
 
@@ -258,7 +298,7 @@ impl<'d, T: Instance> BufferedUartRx<'d, T> {
         rx_reader.pop_done(amt);
 
         // (Re-)Enable the interrupt to receive more data in case it was
-        // disabled because the buffer was full.
+        // disabled because the buffer was full or errors were detected.
         let regs = T::regs();
         unsafe {
             regs.uartimsc().write_set(|w| {
@@ -478,19 +518,37 @@ pub(crate) unsafe fn on_interrupt<T: Instance>(_: *mut ()) {
         let mut rx_writer = s.rx_buf.writer();
         let rx_buf = rx_writer.push_slice();
         let mut n_read = 0;
+        let mut error = false;
         for rx_byte in rx_buf {
             if r.uartfr().read().rxfe() {
                 break;
             }
-            *rx_byte = r.uartdr().read().data();
+            let dr = r.uartdr().read();
+            if (dr.0 >> 8) != 0 {
+                s.rx_error.fetch_or((dr.0 >> 8) as u8, Ordering::Relaxed);
+                error = true;
+                // only fill the buffer with valid characters. the current character is fine
+                // if the error is an overrun, but if we add it to the buffer we'll report
+                // the overrun one character too late. drop it instead and pretend we were
+                // a bit slower at draining the rx fifo than we actually were.
+                // this is consistent with blocking uart error reporting.
+                break;
+            }
+            *rx_byte = dr.data();
             n_read += 1;
         }
         if n_read > 0 {
             rx_writer.push_done(n_read);
             s.rx_waker.wake();
+        } else if error {
+            s.rx_waker.wake();
         }
-        // Disable any further RX interrupts when the buffer becomes full.
+        // Disable any further RX interrupts when the buffer becomes full or
-        if s.rx_buf.is_full() {
+        // errors have occured. this lets us buffer additional errors in the
+        // fifo without needing more error storage locations, and most applications
+        // will want to do a full reset of their uart state anyway once an error
+        // has happened.
+        if s.rx_buf.is_full() || error {
             r.uartimsc().write_clear(|w| {
                 w.set_rxim(true);
                 w.set_rtim(true);

tests/rp/src/bin/uart_buffered.rs

@@ -2,39 +2,248 @@
 #![no_main]
 #![feature(type_alias_impl_trait)]
 
-use defmt::{assert_eq, *};
+use defmt::{assert_eq, panic, *};
 use embassy_executor::Spawner;
+use embassy_rp::gpio::{Level, Output};
 use embassy_rp::interrupt;
-use embassy_rp::uart::{BufferedUart, Config};
+use embassy_rp::uart::{BufferedUart, BufferedUartRx, Config, Error, Instance, Parity};
-use embedded_io::asynch::{Read, Write};
+use embassy_time::{Duration, Timer};
+use embedded_io::asynch::{Read, ReadExactError, Write};
 use {defmt_rtt as _, panic_probe as _};
 
+async fn read<const N: usize>(uart: &mut BufferedUart<'_, impl Instance>) -> Result<[u8; N], Error> {
+    let mut buf = [255; N];
+    match uart.read_exact(&mut buf).await {
+        Ok(()) => Ok(buf),
+        // we should not ever produce an Eof condition
+        Err(ReadExactError::UnexpectedEof) => panic!(),
+        Err(ReadExactError::Other(e)) => Err(e),
+    }
+}
+
+async fn read1<const N: usize>(uart: &mut BufferedUartRx<'_, impl Instance>) -> Result<[u8; N], Error> {
+    let mut buf = [255; N];
+    match uart.read_exact(&mut buf).await {
+        Ok(()) => Ok(buf),
+        // we should not ever produce an Eof condition
+        Err(ReadExactError::UnexpectedEof) => panic!(),
+        Err(ReadExactError::Other(e)) => Err(e),
+    }
+}
+
+async fn send(pin: &mut Output<'_, impl embassy_rp::gpio::Pin>, v: u8, parity: Option<bool>) {
+    pin.set_low();
+    Timer::after(Duration::from_millis(1)).await;
+    for i in 0..8 {
+        if v & (1 << i) == 0 {
+            pin.set_low();
+        } else {
+            pin.set_high();
+        }
+        Timer::after(Duration::from_millis(1)).await;
+    }
+    if let Some(b) = parity {
+        if b {
+            pin.set_high();
+        } else {
+            pin.set_low();
+        }
+        Timer::after(Duration::from_millis(1)).await;
+    }
+    pin.set_high();
+    Timer::after(Duration::from_millis(1)).await;
+}
+
 #[embassy_executor::main]
 async fn main(_spawner: Spawner) {
     let p = embassy_rp::init(Default::default());
     info!("Hello World!");
 
-    let (tx, rx, uart) = (p.PIN_0, p.PIN_1, p.UART0);
+    let (mut tx, mut rx, mut uart) = (p.PIN_0, p.PIN_1, p.UART0);
+    let mut irq = interrupt::take!(UART0_IRQ);
 
-    let config = Config::default();
+    {
-    let irq = interrupt::take!(UART0_IRQ);
+        let config = Config::default();
-    let tx_buf = &mut [0u8; 16];
+        let tx_buf = &mut [0u8; 16];
-    let rx_buf = &mut [0u8; 16];
+        let rx_buf = &mut [0u8; 16];
-    let mut uart = BufferedUart::new(uart, irq, tx, rx, tx_buf, rx_buf, config);
+        let mut uart = BufferedUart::new(&mut uart, &mut irq, &mut tx, &mut rx, tx_buf, rx_buf, config);
 
-    // Make sure we send more bytes than fits in the FIFO, to test the actual
+        // Make sure we send more bytes than fits in the FIFO, to test the actual
-    // bufferedUart.
+        // bufferedUart.
 
-    let data = [
+        let data = [
-        1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30,
+            1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29,
-        31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48,
+            30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48,
-    ];
+        ];
-    uart.write_all(&data).await.unwrap();
+        uart.write_all(&data).await.unwrap();
-    info!("Done writing");
+        info!("Done writing");
 
-    let mut buf = [0; 48];
+        assert_eq!(read(&mut uart).await.unwrap(), data);
-    uart.read_exact(&mut buf).await.unwrap();
+    }
-    assert_eq!(buf, data);
+
+    info!("test overflow detection");
+    {
+        let config = Config::default();
+        let tx_buf = &mut [0u8; 16];
+        let rx_buf = &mut [0u8; 16];
+        let mut uart = BufferedUart::new(&mut uart, &mut irq, &mut tx, &mut rx, tx_buf, rx_buf, config);
+
+        // Make sure we send more bytes than fits in the FIFO, to test the actual
+        // bufferedUart.
+
+        let data = [
+            1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29,
+            30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48,
+        ];
+        let overflow = [
+            101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116,
+        ];
+        // give each block time to settle into the fifo. we want the overrun to occur at a well-defined point.
+        uart.write_all(&data).await.unwrap();
+        uart.blocking_flush().unwrap();
+        while uart.busy() {}
+        uart.write_all(&overflow).await.unwrap();
+        uart.blocking_flush().unwrap();
+        while uart.busy() {}
+
+        // already buffered/fifod prefix is valid
+        assert_eq!(read(&mut uart).await.unwrap(), data);
+        // next received character causes overrun error and is discarded
+        uart.write_all(&[1, 2, 3]).await.unwrap();
+        uart.blocking_flush().unwrap();
+        assert_eq!(read::<1>(&mut uart).await.unwrap_err(), Error::Overrun);
+        assert_eq!(read(&mut uart).await.unwrap(), [2, 3]);
+    }
+
+    info!("test break detection");
+    {
+        let mut config = Config::default();
+        config.baudrate = 1000;
+        let tx_buf = &mut [0u8; 16];
+        let rx_buf = &mut [0u8; 16];
+        let mut uart = BufferedUart::new(&mut uart, &mut irq, &mut tx, &mut rx, tx_buf, rx_buf, config);
+
+        // break on empty buffer
+        uart.send_break(20).await;
+        assert_eq!(read::<1>(&mut uart).await.unwrap_err(), Error::Break);
+        uart.write_all(&[64]).await.unwrap();
+        assert_eq!(read(&mut uart).await.unwrap(), [64]);
+
+        // break on partially filled buffer
+        uart.write_all(&[65; 2]).await.unwrap();
+        uart.send_break(20).await;
+        uart.write_all(&[66]).await.unwrap();
+        assert_eq!(read(&mut uart).await.unwrap(), [65; 2]);
+        assert_eq!(read::<1>(&mut uart).await.unwrap_err(), Error::Break);
+        assert_eq!(read(&mut uart).await.unwrap(), [66]);
+
+        // break on full buffer
+        uart.write_all(&[64; 16]).await.unwrap();
+        uart.send_break(20).await;
+        uart.write_all(&[65]).await.unwrap();
+        assert_eq!(read(&mut uart).await.unwrap(), [64; 16]);
+        assert_eq!(read::<1>(&mut uart).await.unwrap_err(), Error::Break);
+        assert_eq!(read(&mut uart).await.unwrap(), [65]);
+    }
+
+    // parity detection. here we bitbang to not require two uarts.
+    info!("test parity error detection");
+    {
+        let mut pin = Output::new(&mut tx, Level::High);
+        // choose a very slow baud rate to make tests reliable even with O0
+        let mut config = Config::default();
+        config.baudrate = 1000;
+        config.parity = Parity::ParityEven;
+        let rx_buf = &mut [0u8; 16];
+        let mut uart = BufferedUartRx::new(&mut uart, &mut irq, &mut rx, rx_buf, config);
+
+        async fn chr(pin: &mut Output<'_, impl embassy_rp::gpio::Pin>, v: u8, parity: u32) {
+            send(pin, v, Some(parity != 0)).await;
+        }
+
+        // first check that we can send correctly
+        chr(&mut pin, 64, 1).await;
+        assert_eq!(read1(&mut uart).await.unwrap(), [64]);
+
+        // parity on empty buffer
+        chr(&mut pin, 64, 0).await;
+        chr(&mut pin, 4, 1).await;
+        assert_eq!(read1::<1>(&mut uart).await.unwrap_err(), Error::Parity);
+        assert_eq!(read1(&mut uart).await.unwrap(), [4]);
+
+        // parity on partially filled buffer
+        chr(&mut pin, 64, 1).await;
+        chr(&mut pin, 32, 1).await;
+        chr(&mut pin, 64, 0).await;
+        chr(&mut pin, 65, 0).await;
+        assert_eq!(read1(&mut uart).await.unwrap(), [64, 32]);
+        assert_eq!(read1::<1>(&mut uart).await.unwrap_err(), Error::Parity);
+        assert_eq!(read1(&mut uart).await.unwrap(), [65]);
+
+        // parity on full buffer
+        for i in 0..16 {
+            chr(&mut pin, i, i.count_ones() % 2).await;
+        }
+        chr(&mut pin, 64, 0).await;
+        chr(&mut pin, 65, 0).await;
+        assert_eq!(
+            read1(&mut uart).await.unwrap(),
+            [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15]
+        );
+        assert_eq!(read1::<1>(&mut uart).await.unwrap_err(), Error::Parity);
+        assert_eq!(read1(&mut uart).await.unwrap(), [65]);
+    }
+
+    // framing error detection. here we bitbang because there's no other way.
+    info!("test framing error detection");
+    {
+        let mut pin = Output::new(&mut tx, Level::High);
+        // choose a very slow baud rate to make tests reliable even with O0
+        let mut config = Config::default();
+        config.baudrate = 1000;
+        let rx_buf = &mut [0u8; 16];
+        let mut uart = BufferedUartRx::new(&mut uart, &mut irq, &mut rx, rx_buf, config);
+
+        async fn chr(pin: &mut Output<'_, impl embassy_rp::gpio::Pin>, v: u8, good: bool) {
+            if good {
+                send(pin, v, None).await;
+            } else {
+                send(pin, v, Some(false)).await;
+            }
+        }
+
+        // first check that we can send correctly
+        chr(&mut pin, 64, true).await;
+        assert_eq!(read1(&mut uart).await.unwrap(), [64]);
+
+        // framing on empty buffer
+        chr(&mut pin, 64, false).await;
+        assert_eq!(read1::<1>(&mut uart).await.unwrap_err(), Error::Framing);
+        chr(&mut pin, 65, true).await;
+        assert_eq!(read1(&mut uart).await.unwrap(), [65]);
+
+        // framing on partially filled buffer
+        chr(&mut pin, 64, true).await;
+        chr(&mut pin, 32, true).await;
+        chr(&mut pin, 64, false).await;
+        chr(&mut pin, 65, true).await;
+        assert_eq!(read1(&mut uart).await.unwrap(), [64, 32]);
+        assert_eq!(read1::<1>(&mut uart).await.unwrap_err(), Error::Framing);
+        assert_eq!(read1(&mut uart).await.unwrap(), [65]);
+
+        // framing on full buffer
+        for i in 0..16 {
+            chr(&mut pin, i, true).await;
+        }
+        chr(&mut pin, 64, false).await;
+        chr(&mut pin, 65, true).await;
+        assert_eq!(
+            read1(&mut uart).await.unwrap(),
+            [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15]
+        );
+        assert_eq!(read1::<1>(&mut uart).await.unwrap_err(), Error::Framing);
+        assert_eq!(read1(&mut uart).await.unwrap(), [65]);
+    }
 
     info!("Test OK");
     cortex_m::asm::bkpt();