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 { + // 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; }