From ec47e931acbc4a4b1a9f0781e9317f1e680427eb Mon Sep 17 00:00:00 2001 From: Andres Vahter Date: Mon, 8 Jan 2024 17:33:11 +0200 Subject: [PATCH 1/5] stm32: fix buffered uart flush usart_v4 uses internal FIFO and therefore actually all bytes are not yet sent out although state.tx_buf.is_empty() --- embassy-stm32/src/usart/buffered.rs | 30 +++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/embassy-stm32/src/usart/buffered.rs b/embassy-stm32/src/usart/buffered.rs index f8e0bfda5..e050f8e0f 100644 --- a/embassy-stm32/src/usart/buffered.rs +++ b/embassy-stm32/src/usart/buffered.rs @@ -1,5 +1,6 @@ use core::future::poll_fn; use core::slice; +use core::sync::atomic::{AtomicBool, Ordering}; use core::task::Poll; use embassy_hal_internal::atomic_ring_buffer::RingBuffer; @@ -61,6 +62,18 @@ impl interrupt::typelevel::Handler for Interrupt state.rx_waker.wake(); } + // With `usart_v4` hardware FIFO is enabled, making `state.tx_buf` + // insufficient to determine if all bytes are sent out. + // Transmission complete (TC) interrupt here indicates that all bytes are pushed out from the FIFO. + #[cfg(usart_v4)] + if sr_val.tc() { + r.cr1().modify(|w| { + w.set_tcie(false); + }); + state.tx_done.store(true, Ordering::Release); + state.rx_waker.wake(); + } + // TX if sr(r).read().txe() { let mut tx_reader = state.tx_buf.reader(); @@ -69,6 +82,12 @@ impl interrupt::typelevel::Handler for Interrupt r.cr1().modify(|w| { w.set_txeie(true); }); + + #[cfg(usart_v4)] + r.cr1().modify(|w| { + w.set_tcie(true); + }); + tdr(r).write_volatile(buf[0].into()); tx_reader.pop_done(1); state.tx_waker.wake(); @@ -90,6 +109,7 @@ pub(crate) mod sealed { pub(crate) rx_buf: RingBuffer, pub(crate) tx_waker: AtomicWaker, pub(crate) tx_buf: RingBuffer, + pub(crate) tx_done: AtomicBool, } impl State { @@ -100,6 +120,7 @@ pub(crate) mod sealed { tx_buf: RingBuffer::new(), rx_waker: AtomicWaker::new(), tx_waker: AtomicWaker::new(), + tx_done: AtomicBool::new(true), } } } @@ -366,6 +387,8 @@ impl<'d, T: BasicInstance> BufferedUartTx<'d, T> { async fn write(&self, buf: &[u8]) -> Result { poll_fn(move |cx| { let state = T::buffered_state(); + state.tx_done.store(false, Ordering::Release); + let empty = state.tx_buf.is_empty(); let mut tx_writer = unsafe { state.tx_buf.writer() }; @@ -391,6 +414,13 @@ impl<'d, T: BasicInstance> BufferedUartTx<'d, T> { async fn flush(&self) -> Result<(), Error> { poll_fn(move |cx| { let state = T::buffered_state(); + + #[cfg(usart_v4)] + if !state.tx_done.load(Ordering::Acquire) { + state.tx_waker.register(cx.waker()); + return Poll::Pending; + } + #[cfg(not(usart_v4))] if !state.tx_buf.is_empty() { state.tx_waker.register(cx.waker()); return Poll::Pending; From 17d6e4eefeb9bdc9d7c3776afb815298be5b468c Mon Sep 17 00:00:00 2001 From: Andres Vahter Date: Tue, 9 Jan 2024 09:25:10 +0200 Subject: [PATCH 2/5] stm32 uart: do not wake after sending each byte usart_v4 uses TC interrupt to see if all bytes are sent out from the FIFO and waker is called from this interrupt. This minimises unnecessary wakeups during sending. --- embassy-stm32/src/usart/buffered.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/embassy-stm32/src/usart/buffered.rs b/embassy-stm32/src/usart/buffered.rs index e050f8e0f..211091c38 100644 --- a/embassy-stm32/src/usart/buffered.rs +++ b/embassy-stm32/src/usart/buffered.rs @@ -62,8 +62,8 @@ impl interrupt::typelevel::Handler for Interrupt state.rx_waker.wake(); } - // With `usart_v4` hardware FIFO is enabled, making `state.tx_buf` - // insufficient to determine if all bytes are sent out. + // With `usart_v4` hardware FIFO is enabled, making `state.tx_buf` insufficient + // to determine if all bytes are sent out. // Transmission complete (TC) interrupt here indicates that all bytes are pushed out from the FIFO. #[cfg(usart_v4)] if sr_val.tc() { @@ -90,9 +90,12 @@ impl interrupt::typelevel::Handler for Interrupt tdr(r).write_volatile(buf[0].into()); tx_reader.pop_done(1); + + // Notice that in case of `usart_v4` waker is called when TC interrupt happens. + #[cfg(not(usart_v4))] state.tx_waker.wake(); } else { - // Disable interrupt until we have something to transmit again + // Disable interrupt until we have something to transmit again. r.cr1().modify(|w| { w.set_txeie(false); }); From c936d66934225db8ddd283d7da2d7a158686df71 Mon Sep 17 00:00:00 2001 From: Andres Vahter Date: Tue, 9 Jan 2024 14:47:30 +0200 Subject: [PATCH 3/5] stm32 uart: fix `flush` for non usart_v4 variants Byte was written to TDR and right after that waker was called. This means `flush` would see that `tx_buf` is empty and can return Ready although actually hardware was still writing this last byte to the wire. With this change non `usart_v4 ` variants would also use TC interrupt to check when last byte was sent out. --- embassy-stm32/src/usart/buffered.rs | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/embassy-stm32/src/usart/buffered.rs b/embassy-stm32/src/usart/buffered.rs index 211091c38..52740c13e 100644 --- a/embassy-stm32/src/usart/buffered.rs +++ b/embassy-stm32/src/usart/buffered.rs @@ -62,10 +62,9 @@ impl interrupt::typelevel::Handler for Interrupt state.rx_waker.wake(); } - // With `usart_v4` hardware FIFO is enabled, making `state.tx_buf` insufficient - // to determine if all bytes are sent out. - // Transmission complete (TC) interrupt here indicates that all bytes are pushed out from the FIFO. - #[cfg(usart_v4)] + // With `usart_v4` hardware FIFO is enabled and Transmission complete (TC) + // indicates that all bytes are pushed out from the FIFO. + // For other usart variants it shows that last byte from the buffer was just sent. if sr_val.tc() { r.cr1().modify(|w| { w.set_tcie(false); @@ -83,17 +82,15 @@ impl interrupt::typelevel::Handler for Interrupt w.set_txeie(true); }); - #[cfg(usart_v4)] - r.cr1().modify(|w| { - w.set_tcie(true); - }); + // Enable transmission complete interrupt when last byte is going to be sent out. + if buf.len() == 1 { + r.cr1().modify(|w| { + w.set_tcie(true); + }); + } tdr(r).write_volatile(buf[0].into()); tx_reader.pop_done(1); - - // Notice that in case of `usart_v4` waker is called when TC interrupt happens. - #[cfg(not(usart_v4))] - state.tx_waker.wake(); } else { // Disable interrupt until we have something to transmit again. r.cr1().modify(|w| { @@ -418,16 +415,10 @@ impl<'d, T: BasicInstance> BufferedUartTx<'d, T> { poll_fn(move |cx| { let state = T::buffered_state(); - #[cfg(usart_v4)] if !state.tx_done.load(Ordering::Acquire) { state.tx_waker.register(cx.waker()); return Poll::Pending; } - #[cfg(not(usart_v4))] - if !state.tx_buf.is_empty() { - state.tx_waker.register(cx.waker()); - return Poll::Pending; - } Poll::Ready(Ok(())) }) From 534c53c901345f8af517b8991a4bcd99d290c11c Mon Sep 17 00:00:00 2001 From: Andres Vahter Date: Fri, 19 Jan 2024 19:58:00 +0200 Subject: [PATCH 4/5] stm32 uart: remove unwrap unwraps take more space because of panics --- embassy-stm32/src/usart/buffered.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/embassy-stm32/src/usart/buffered.rs b/embassy-stm32/src/usart/buffered.rs index 52740c13e..abc766bc7 100644 --- a/embassy-stm32/src/usart/buffered.rs +++ b/embassy-stm32/src/usart/buffered.rs @@ -47,8 +47,10 @@ impl interrupt::typelevel::Handler for Interrupt let mut rx_writer = state.rx_buf.writer(); let buf = rx_writer.push_slice(); if !buf.is_empty() { - buf[0] = dr.unwrap(); - rx_writer.push_done(1); + if let Some(byte) = dr { + buf[0] = byte; + rx_writer.push_done(1); + } } else { // FIXME: Should we disable any further RX interrupts when the buffer becomes full. } From ec2e3de0f493cd0cc116f5f67a814ae7c457d9b1 Mon Sep 17 00:00:00 2001 From: Andres Vahter Date: Fri, 19 Jan 2024 20:28:29 +0200 Subject: [PATCH 5/5] stm32 uart: fix buffered flush for usart_v1, usart_v2 There is one caveat. For some reason with first send using usart_v1/usart_v2 TC flag appears right after first byte from buffer is written to DR. Consecutive transfers work as expected - TC flag appears when last byte is fully transferred to wire. --- embassy-stm32/src/usart/buffered.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/embassy-stm32/src/usart/buffered.rs b/embassy-stm32/src/usart/buffered.rs index abc766bc7..c78752883 100644 --- a/embassy-stm32/src/usart/buffered.rs +++ b/embassy-stm32/src/usart/buffered.rs @@ -68,11 +68,16 @@ impl interrupt::typelevel::Handler for Interrupt // indicates that all bytes are pushed out from the FIFO. // For other usart variants it shows that last byte from the buffer was just sent. if sr_val.tc() { + // For others it is cleared above with `clear_interrupt_flags`. + #[cfg(any(usart_v1, usart_v2))] + sr(r).modify(|w| w.set_tc(false)); + r.cr1().modify(|w| { w.set_tcie(false); }); + state.tx_done.store(true, Ordering::Release); - state.rx_waker.wake(); + state.tx_waker.wake(); } // TX