Check and clear idle and error flags together

1 files changed, 39 insertions, 40 deletions

diff --git a/embassy-stm32/src/usart/ringbuffered.rs b/embassy-stm32/src/usart/ringbuffered.rs
index d76a52d1e..3ab9f95e0 100644
--- a/embassy-stm32/src/usart/ringbuffered.rs
+++ b/embassy-stm32/src/usart/ringbuffered.rs
@@ -99,8 +99,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 +132,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 +187,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,40 +230,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();
-
-    #[cfg(any(usart_v3, usart_v4))]
-    r.icr().write(|w| w.set_idle(true));
-    #[cfg(not(any(usart_v3, usart_v4)))]
-    unsafe {
-        // This read also clears the error and idle interrupt flags on v1.
-        rdr(r).read_volatile()
-    };
-
-    r.cr1().modify(|w| w.set_idleie(true));
-
-    sr
-}
-
 impl embedded_io_async::ErrorType for RingBufferedUartRx<'_> {
     type Error = Error;
 }