Merge pull request #3989 from embedded-rust-iml/fix/ringbuffered-error-handling

Rework status handling (idle and errors) in ringbuffered uart

2 files changed, 44 insertions, 55 deletions

diff --git a/embassy-stm32/src/usart/mod.rs b/embassy-stm32/src/usart/mod.rs
index 675e90c7f..5b7f8dc26 100644
--- a/embassy-stm32/src/usart/mod.rs
+++ b/embassy-stm32/src/usart/mod.rs
@@ -18,11 +18,6 @@ use crate::gpio::{self, AfType, AnyPin, OutputType, Pull, SealedPin as _, Speed}
 use crate::interrupt::typelevel::Interrupt as _;
 use crate::interrupt::{self, Interrupt, InterruptExt};
 use crate::mode::{Async, Blocking, Mode};
-#[allow(unused_imports)]
-#[cfg(not(any(usart_v1, usart_v2)))]
-use crate::pac::usart::regs::Isr as Sr;
-#[cfg(any(usart_v1, usart_v2))]
-use crate::pac::usart::regs::Sr;
 #[cfg(not(any(usart_v1, usart_v2)))]
 use crate::pac::usart::Lpuart as Regs;
 #[cfg(any(usart_v1, usart_v2))]
@@ -403,7 +398,7 @@ pub struct UartRx<'d, M: Mode> {
     rx_dma: Option<ChannelAndRequest<'d>>,
     detect_previous_overrun: bool,
     #[cfg(any(usart_v1, usart_v2))]
-    buffered_sr: stm32_metapac::usart::regs::Sr,
+    buffered_sr: regs::Sr,
     _phantom: PhantomData<M>,
 }
 
@@ -950,7 +945,7 @@ impl<'d, M: Mode> UartRx<'d, M> {
             rx_dma,
             detect_previous_overrun: config.detect_previous_overrun,
             #[cfg(any(usart_v1, usart_v2))]
-            buffered_sr: stm32_metapac::usart::regs::Sr(0),
+            buffered_sr: regs::Sr(0),
         };
         this.enable_and_configure(&config)?;
         Ok(this)
@@ -1449,7 +1444,7 @@ impl<'d, M: Mode> Uart<'d, M> {
                 rx_dma,
                 detect_previous_overrun: config.detect_previous_overrun,
                 #[cfg(any(usart_v1, usart_v2))]
-                buffered_sr: stm32_metapac::usart::regs::Sr(0),
+                buffered_sr: regs::Sr(0),
             },
         };
         this.enable_and_configure(&config)?;
diff --git a/embassy-stm32/src/usart/ringbuffered.rs b/embassy-stm32/src/usart/ringbuffered.rs
index 3a7c3adf4..8e42d5917 100644
--- a/embassy-stm32/src/usart/ringbuffered.rs
+++ b/embassy-stm32/src/usart/ringbuffered.rs
@@ -7,16 +7,12 @@ use embassy_embedded_hal::SetConfig;
 use embedded_io_async::ReadReady;
 use futures_util::future::{select, Either};
 
-use super::{
-    clear_interrupt_flags, rdr, reconfigure, set_baudrate, sr, Config, ConfigError, Error, Info, State, UartRx,
-};
+use super::{rdr, reconfigure, set_baudrate, sr, Config, ConfigError, Error, Info, State, UartRx};
 use crate::dma::ReadableRingBuffer;
 use crate::gpio::{AnyPin, SealedPin as _};
 use crate::mode::Async;
-#[cfg(any(usart_v3, usart_v4))]
-use crate::pac::usart::regs;
 use crate::time::Hertz;
-use crate::usart::{Regs, Sr};
+use crate::usart::Regs;
 use crate::Peri;
 
 /// Rx-only Ring-buffered UART Driver
@@ -99,8 +95,6 @@ impl<'d> RingBufferedUartRx<'d> {
             // enable idle line interrupt
             w.set_idleie(true);
         });
-        // Clear all potential error interrupt flags
-        clear_interrupt_flags(r, sr(r).read());
         r.cr3().modify(|w| {
             // enable Error Interrupt: (Frame error, Noise error, Overrun error)
             w.set_eie(true);
@@ -134,17 +128,15 @@ impl<'d> RingBufferedUartRx<'d> {
     }
 
     /// (Re-)start DMA and Uart if it is not running (has not been started yet or has failed), and
-    /// check for errors in status register. Error flags are cleared in `start_uart()` so they need
-    /// to be read first without returning yet.
+    /// check for errors in status register. Error flags are checked/cleared first.
     fn start_dma_or_check_errors(&mut self) -> Result<(), Error> {
         let r = self.info.regs;
 
-        let sr = clear_idle_flag(r);
-        let res = check_for_errors(sr);
+        check_idle_and_errors(r)?;
         if !r.cr3().read().dmar() {
             self.start_uart();
         }
-        res
+        Ok(())
     }
 
     /// Read bytes that are readily available in the ring buffer.
@@ -191,13 +183,7 @@ impl<'d> RingBufferedUartRx<'d> {
 
             compiler_fence(Ordering::SeqCst);
 
-            // Critical section is needed so that IDLE isn't set after
-            // our read but before we clear it.
-            let sr = critical_section::with(|_| clear_idle_flag(self.info.regs));
-
-            check_for_errors(sr)?;
-
-            if sr.idle() {
+            if check_idle_and_errors(self.info.regs)? {
                 // Idle line is detected
                 Poll::Ready(Ok(()))
             } else {
@@ -240,41 +226,49 @@ impl Drop for RingBufferedUartRx<'_> {
     }
 }
 
-/// Return an error result if the Sr register has errors
-fn check_for_errors(s: Sr) -> Result<(), Error> {
-    if s.pe() {
+/// Check and clear idle and error interrupts, return true if idle, Err(e) on error
+///
+/// All flags are read and cleared in a single step, respectively. When more than one flag is set
+/// at the same time, all flags will be cleared but only one flag will be reported. So the other
+/// flag(s) will gone missing unnoticed. The error flags are checked first, the idle flag last.
+///
+/// For usart_v1 and usart_v2, all status flags must be handled together anyway because all flags
+/// are cleared by a single read to the RDR register.
+fn check_idle_and_errors(r: Regs) -> Result<bool, Error> {
+    // Critical section is required so that the flags aren't set after read and before clear
+    let sr = critical_section::with(|_| {
+        // SAFETY: read only and we only use Rx related flags
+        let sr = sr(r).read();
+
+        #[cfg(any(usart_v3, usart_v4))]
+        r.icr().write(|w| {
+            w.set_idle(true);
+            w.set_pe(true);
+            w.set_fe(true);
+            w.set_ne(true);
+            w.set_ore(true);
+        });
+        #[cfg(not(any(usart_v3, usart_v4)))]
+        unsafe {
+            // This read also clears the error and idle interrupt flags on v1 (TODO and v2?)
+            rdr(r).read_volatile()
+        };
+        sr
+    });
+    if sr.pe() {
         Err(Error::Parity)
-    } else if s.fe() {
+    } else if sr.fe() {
         Err(Error::Framing)
-    } else if s.ne() {
+    } else if sr.ne() {
         Err(Error::Noise)
-    } else if s.ore() {
+    } else if sr.ore() {
         Err(Error::Overrun)
     } else {
-        Ok(())
+        r.cr1().modify(|w| w.set_idleie(true));
+        Ok(sr.idle())
     }
 }
 
-/// Clear IDLE and return the Sr register
-fn clear_idle_flag(r: Regs) -> Sr {
-    // SAFETY: read only and we only use Rx related flags
-
-    let sr = sr(r).read();
-
-    // This read also clears the error and idle interrupt flags on v1.
-    unsafe { rdr(r).read_volatile() };
-    #[cfg(any(usart_v3, usart_v4))]
-    {
-        let mut clear_idle = regs::Icr(0);
-        clear_idle.set_idle(true);
-        r.icr().write_value(clear_idle);
-    }
-
-    r.cr1().modify(|w| w.set_idleie(true));
-
-    sr
-}
-
 impl embedded_io_async::ErrorType for RingBufferedUartRx<'_> {
     type Error = Error;
 }