UART: clear RX FIFO on overflow (#3190)

* Add failing test case

* Use pointer difference as the true FIFO count

* Take FIFO pointers into account when calculating FIFO length

* Fix grammar

* Clear RX FIFO on overflow

* Update esp-hal/src/uart.rs

Co-authored-by: Dominic Fischer <14130965+Dominaezzz@users.noreply.github.com>

* Handle overflow caught by RxFuture

* Reset the fifo after clearing the interrupt

---------

Co-authored-by: Dominic Fischer <14130965+Dominaezzz@users.noreply.github.com>

esp-hal/CHANGELOG.md

@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Full-duplex SPI works when mixed with half-duplex SPI (#3176)
 - `Uart::flush_async` should no longer return prematurely (#3186)
+- Detecting a UART overflow now clears the RX FIFO. (#3190)
 - ESP32-S2: Fixed PSRAM initialization (#3196)
 
 ### Removed

esp-hal/src/uart.rs

@@ -73,9 +73,10 @@ use crate::{
 #[cfg_attr(feature = "defmt", derive(defmt::Format))]
 #[non_exhaustive]
 pub enum RxError {
-    /// The RX FIFO overflow happened.
+    /// An RX FIFO overflow happened.
     ///
-    /// This error occurs when RX FIFO is full and a new byte is received.
+    /// This error occurs when RX FIFO is full and a new byte is received. The
+    /// RX FIFO is then automatically reset by the driver.
     FifoOverflowed,
 
     /// A glitch was detected on the RX line.
@@ -832,6 +833,8 @@ where
     }
 
     /// Reads and clears errors set by received data.
+    ///
+    /// If a FIFO overflow is detected, the RX FIFO is reset.
     #[instability::unstable]
     pub fn check_for_errors(&mut self) -> Result<(), RxError> {
         let errors = RxEvent::FifoOvf
@@ -843,6 +846,9 @@ where
         let result = rx_event_check_for_error(events);
         if result.is_err() {
             self.uart.info().clear_rx_events(errors);
+            if events.contains(RxEvent::FifoOvf) {
+                self.uart.info().rxfifo_reset();
+            }
         }
         result
     }
@@ -910,31 +916,31 @@ where
         Ok(to_read)
     }
 
-    #[allow(clippy::useless_conversion)]
+    #[cfg(not(esp32))]
+    #[allow(clippy::unnecessary_cast)]
     fn rx_fifo_count(&self) -> u16 {
-        let fifo_cnt: u16 = self.regs().status().read().rxfifo_cnt().bits().into();
+        self.regs().status().read().rxfifo_cnt().bits() as u16
+    }
+
+    #[cfg(esp32)]
+    fn rx_fifo_count(&self) -> u16 {
+        let fifo_cnt = self.regs().status().read().rxfifo_cnt().bits();
 
         // Calculate the real count based on the FIFO read and write offset address:
         // https://docs.espressif.com/projects/esp-chip-errata/en/latest/esp32/03-errata-description/esp32/uart-fifo-cnt-indicates-data-length-incorrectly.html
-        #[cfg(esp32)]
+        let status = self.regs().mem_rx_status().read();
-        {
+        let rd_addr: u16 = status.mem_rx_rd_addr().bits();
-            let status = self.regs().mem_rx_status().read();
+        let wr_addr: u16 = status.mem_rx_wr_addr().bits();
-            let rd_addr = status.mem_rx_rd_addr().bits();
-            let wr_addr = status.mem_rx_wr_addr().bits();
 
-            if wr_addr > rd_addr {
+        if wr_addr > rd_addr {
-                wr_addr - rd_addr
+            wr_addr - rd_addr
-            } else if wr_addr < rd_addr {
+        } else if wr_addr < rd_addr {
-                (wr_addr + Info::UART_FIFO_SIZE) - rd_addr
+            (wr_addr + Info::UART_FIFO_SIZE) - rd_addr
-            } else if fifo_cnt > 0 {
+        } else if fifo_cnt > 0 {
-                Info::UART_FIFO_SIZE
+            Info::UART_FIFO_SIZE
-            } else {
+        } else {
-                0
+            0
-            }
         }
-
-        #[cfg(not(esp32))]
-        fifo_cnt
     }
 
     /// Disables all RX-related interrupts for this UART instance.
@@ -2002,8 +2008,15 @@ impl UartRx<'_, Async> {
                 events |= RxEvent::FifoTout;
             }
 
-            let event = UartRxFuture::new(self.uart.reborrow(), events).await;
+            let events = UartRxFuture::new(self.uart.reborrow(), events).await;
-            rx_event_check_for_error(event)?;
+
+            let result = rx_event_check_for_error(events);
+            if let Err(error) = result {
+                if error == RxError::FifoOverflowed {
+                    self.uart.info().rxfifo_reset();
+                }
+                return Err(error);
+            }
         }
 
         Ok(())

hil-test/Cargo.toml

@@ -221,6 +221,7 @@ embassy-sync       = "0.6.0"
 embassy-time       = "0.4.0"
 embedded-hal       = "1.0.0"
 embedded-io        = "0.6.1"
+embedded-io-async  = "0.6.1"
 embedded-can       = "0.4.1"
 embedded-hal-async = "1.0.0"
 embedded-hal-nb    = "1.0.0"

hil-test/tests/uart_tx_rx_async.rs

@@ -25,6 +25,7 @@ mod tests {
         select::{select, Either},
     };
     use embassy_time::{Duration, Timer};
+    use embedded_io_async::Write;
     use esp_hal::{
         timer::timg::TimerGroup,
         uart::{RxConfig, RxError},
@@ -129,6 +130,46 @@ mod tests {
         assert!(matches!(res, Err(RxError::FifoOverflowed)), "{:?}", res);
     }
 
+    #[test]
+    async fn flushing_after_overflow_remains_consistent(mut ctx: Context) {
+        let mut read = [0u8; 1];
+
+        ctx.tx.write_all(&[0; 350]).await.unwrap();
+        ctx.tx.flush_async().await.unwrap();
+
+        // The read is supposed to return an error because the FIFO is just 128 bytes
+        // long.
+        let res = ctx.rx.read_async(&mut read).await;
+        assert!(matches!(res, Err(RxError::FifoOverflowed)), "{:?}", res);
+
+        // Now we should be able to write and read back the same data.
+        for _ in 0..5 {
+            ctx.tx.write_all(&[1, 2, 3, 4]).await.unwrap();
+
+            let mut read = [0u8; 4];
+            ctx.rx.read_exact_async(&mut read).await.unwrap();
+
+            assert_eq!(&read, &[1, 2, 3, 4]);
+        }
+
+        // Can we still handle a full FIFO?
+        // Join two futures to ensure that `read_async` is called first, which should
+        // set up waiting for the FIFO full interrupt.
+        let mut read = [0u8; 128];
+        let read = async {
+            // This read should return as many bytes as the FIFO threshold, which is 120
+            // bytes by default.
+            let read_count = ctx.rx.read_async(&mut read).await.unwrap();
+            assert_eq!(read_count, 120);
+
+            ctx.rx.read_exact_async(&mut read[120..]).await.unwrap();
+            assert_eq!(&read, &[1; 128]);
+        };
+        let write = async { ctx.tx.write_all(&[1; 128]).await.unwrap() };
+
+        embassy_futures::join::join(read, write).await;
+    }
+
     #[test]
     async fn read_returns_with_partial_data_if_read_times_out(mut ctx: Context) {
         let mut data = [0u8; 16];