From ac7c44c1d1465bd125ef9a2c6da7e3b42a52569c Mon Sep 17 00:00:00 2001 From: Tobias Naumann Date: Fri, 21 Mar 2025 13:27:29 +0100 Subject: [PATCH 1/4] Use write closure to clear idle flag, only read rdr on usart versions which need it --- embassy-stm32/src/usart/ringbuffered.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/embassy-stm32/src/usart/ringbuffered.rs b/embassy-stm32/src/usart/ringbuffered.rs index f1161aa79..d76a52d1e 100644 --- a/embassy-stm32/src/usart/ringbuffered.rs +++ b/embassy-stm32/src/usart/ringbuffered.rs @@ -261,14 +261,13 @@ fn clear_idle_flag(r: Regs) -> Sr { 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.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)); From 402b78138b76ae125bb9be0a336e5d0bc17e7833 Mon Sep 17 00:00:00 2001 From: Tobias Naumann Date: Fri, 21 Mar 2025 14:04:33 +0100 Subject: [PATCH 2/4] Check and clear idle and error flags together --- embassy-stm32/src/usart/ringbuffered.rs | 79 ++++++++++++------------- 1 file 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 { + // 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; } From 9e31a45134ec8ab114108825989c4e623c7ccfe8 Mon Sep 17 00:00:00 2001 From: Tobias Naumann Date: Wed, 26 Mar 2025 18:04:59 +0100 Subject: [PATCH 3/4] Fix unused imports --- embassy-stm32/src/usart/mod.rs | 5 ----- embassy-stm32/src/usart/ringbuffered.rs | 8 ++------ 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/embassy-stm32/src/usart/mod.rs b/embassy-stm32/src/usart/mod.rs index 568067360..e52647795 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))] diff --git a/embassy-stm32/src/usart/ringbuffered.rs b/embassy-stm32/src/usart/ringbuffered.rs index 3ab9f95e0..69423c940 100644 --- a/embassy-stm32/src/usart/ringbuffered.rs +++ b/embassy-stm32/src/usart/ringbuffered.rs @@ -8,16 +8,12 @@ use embassy_hal_internal::PeripheralRef; 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; /// Rx-only Ring-buffered UART Driver /// From 14d079ae84ac28ce860015ad6ca8d040edf3f065 Mon Sep 17 00:00:00 2001 From: Tobias Naumann Date: Thu, 27 Mar 2025 12:03:52 +0100 Subject: [PATCH 4/4] Cleanup stm32_metapac register usage in usart module --- embassy-stm32/src/usart/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/embassy-stm32/src/usart/mod.rs b/embassy-stm32/src/usart/mod.rs index e52647795..803a5d579 100644 --- a/embassy-stm32/src/usart/mod.rs +++ b/embassy-stm32/src/usart/mod.rs @@ -398,7 +398,7 @@ pub struct UartRx<'d, M: Mode> { rx_dma: Option>, detect_previous_overrun: bool, #[cfg(any(usart_v1, usart_v2))] - buffered_sr: stm32_metapac::usart::regs::Sr, + buffered_sr: regs::Sr, _phantom: PhantomData, } @@ -945,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) @@ -1444,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)?;