Merge pull request #3356 from peterkrull/ringbuffered-uartrx-deadlock

stm32: Fix RingBufferedUartRx hard-resetting DMA after initial error

2 files changed, 72 insertions, 33 deletions

diff --git a/embassy-stm32/src/dma/dma_bdma.rs b/embassy-stm32/src/dma/dma_bdma.rs
index df041c4e9..d10b5554f 100644
--- a/embassy-stm32/src/dma/dma_bdma.rs
+++ b/embassy-stm32/src/dma/dma_bdma.rs
@@ -493,6 +493,26 @@ impl AnyChannel {
         }
     }
 
+    fn request_pause(&self) {
+        let info = self.info();
+        match self.info().dma {
+            #[cfg(dma)]
+            DmaInfo::Dma(r) => {
+                // Disable the channel without overwriting the existing configuration
+                r.st(info.num).cr().modify(|w| {
+                    w.set_en(false);
+                });
+            }
+            #[cfg(bdma)]
+            DmaInfo::Bdma(r) => {
+                // Disable the channel without overwriting the existing configuration
+                r.ch(info.num).cr().modify(|w| {
+                    w.set_en(false);
+                });
+            }
+        }
+    }
+
     fn is_running(&self) -> bool {
         let info = self.info();
         match self.info().dma {
@@ -667,12 +687,22 @@ impl<'a> Transfer<'a> {
     }
 
     /// Request the transfer to stop.
+    /// The configuration for this channel will **not be preserved**. If you need to restart the transfer
+    /// at a later point with the same configuration, see [`request_pause`](Self::request_pause) instead.
     ///
     /// This doesn't immediately stop the transfer, you have to wait until [`is_running`](Self::is_running) returns false.
     pub fn request_stop(&mut self) {
         self.channel.request_stop()
     }
 
+    /// Request the transfer to pause, keeping the existing configuration for this channel.
+    /// To restart the transfer, call [`start`](Self::start) again.
+    ///
+    /// This doesn't immediately stop the transfer, you have to wait until [`is_running`](Self::is_running) returns false.
+    pub fn request_pause(&mut self) {
+        self.channel.request_pause()
+    }
+
     /// Return whether this transfer is still running.
     ///
     /// If this returns `false`, it can be because either the transfer finished, or
@@ -846,13 +876,23 @@ impl<'a, W: Word> ReadableRingBuffer<'a, W> {
         DmaCtrlImpl(self.channel.reborrow()).set_waker(waker);
     }
 
-    /// Request DMA to stop.
+    /// Request the DMA to stop.
+    /// The configuration for this channel will **not be preserved**. If you need to restart the transfer
+    /// at a later point with the same configuration, see [`request_pause`](Self::request_pause) instead.
     ///
     /// This doesn't immediately stop the transfer, you have to wait until [`is_running`](Self::is_running) returns false.
     pub fn request_stop(&mut self) {
         self.channel.request_stop()
     }
 
+    /// Request the transfer to pause, keeping the existing configuration for this channel.
+    /// To restart the transfer, call [`start`](Self::start) again.
+    ///
+    /// This doesn't immediately stop the transfer, you have to wait until [`is_running`](Self::is_running) returns false.
+    pub fn request_pause(&mut self) {
+        self.channel.request_pause()
+    }
+
     /// Return whether DMA is still running.
     ///
     /// If this returns `false`, it can be because either the transfer finished, or
@@ -977,13 +1017,23 @@ impl<'a, W: Word> WritableRingBuffer<'a, W> {
         DmaCtrlImpl(self.channel.reborrow()).set_waker(waker);
     }
 
-    /// Request DMA to stop.
+    /// Request the DMA to stop.
+    /// The configuration for this channel will **not be preserved**. If you need to restart the transfer
+    /// at a later point with the same configuration, see [`request_pause`](Self::request_pause) instead.
     ///
     /// This doesn't immediately stop the transfer, you have to wait until [`is_running`](Self::is_running) returns false.
     pub fn request_stop(&mut self) {
         self.channel.request_stop()
     }
 
+    /// Request the transfer to pause, keeping the existing configuration for this channel.
+    /// To restart the transfer, call [`start`](Self::start) again.
+    ///
+    /// This doesn't immediately stop the transfer, you have to wait until [`is_running`](Self::is_running) returns false.
+    pub fn request_pause(&mut self) {
+        self.channel.request_pause()
+    }
+
     /// Return whether DMA is still running.
     ///
     /// If this returns `false`, it can be because either the transfer finished, or
diff --git a/embassy-stm32/src/usart/ringbuffered.rs b/embassy-stm32/src/usart/ringbuffered.rs
index b0652046c..75834bf37 100644
--- a/embassy-stm32/src/usart/ringbuffered.rs
+++ b/embassy-stm32/src/usart/ringbuffered.rs
@@ -71,34 +71,19 @@ impl<'d> UartRx<'d, Async> {
 }
 
 impl<'d> RingBufferedUartRx<'d> {
-    /// Clear the ring buffer and start receiving in the background
-    pub fn start(&mut self) -> Result<(), Error> {
-        // Clear the ring buffer so that it is ready to receive data
-        self.ring_buf.clear();
-
-        self.setup_uart();
-
-        Ok(())
-    }
-
-    fn stop(&mut self, err: Error) -> Result<usize, Error> {
-        self.teardown_uart();
-
-        Err(err)
-    }
-
     /// Reconfigure the driver
     pub fn set_config(&mut self, config: &Config) -> Result<(), ConfigError> {
         reconfigure(self.info, self.kernel_clock, config)
     }
 
-    /// Start uart background receive
-    fn setup_uart(&mut self) {
-        // fence before starting DMA.
+    /// Configure and start the DMA backed UART receiver
+    ///
+    /// Note: This is also done automatically by [`read()`] if required.
+    pub fn start_uart(&mut self) {
+        // Clear the buffer so that it is ready to receive data
         compiler_fence(Ordering::SeqCst);
-
-        // start the dma controller
         self.ring_buf.start();
+        self.ring_buf.clear();
 
         let r = self.info.regs;
         // clear all interrupts and DMA Rx Request
@@ -118,9 +103,9 @@ impl<'d> RingBufferedUartRx<'d> {
         });
     }
 
-    /// Stop uart background receive
-    fn teardown_uart(&mut self) {
-        self.ring_buf.request_stop();
+    /// Stop DMA backed UART receiver
+    fn stop_uart(&mut self) {
+        self.ring_buf.request_pause();
 
         let r = self.info.regs;
         // clear all interrupts and DMA Rx Request
@@ -153,13 +138,15 @@ impl<'d> RingBufferedUartRx<'d> {
     pub async fn read(&mut self, buf: &mut [u8]) -> Result<usize, Error> {
         let r = self.info.regs;
 
-        // Start background receive if it was not already started
+        // Start DMA and Uart if it was not already started,
+        // otherwise check for errors in status register.
+        let sr = clear_idle_flag(r);
         if !r.cr3().read().dmar() {
-            self.start()?;
+            self.start_uart();
+        } else {
+            check_for_errors(sr)?;
         }
 
-        check_for_errors(clear_idle_flag(r))?;
-
         loop {
             match self.ring_buf.read(buf) {
                 Ok((0, _)) => {}
@@ -167,14 +154,16 @@ impl<'d> RingBufferedUartRx<'d> {
                     return Ok(len);
                 }
                 Err(_) => {
-                    return self.stop(Error::Overrun);
+                    self.stop_uart();
+                    return Err(Error::Overrun);
                 }
             }
 
             match self.wait_for_data_or_idle().await {
                 Ok(_) => {}
                 Err(err) => {
-                    return self.stop(err);
+                    self.stop_uart();
+                    return Err(err);
                 }
             }
         }
@@ -228,7 +217,7 @@ impl<'d> RingBufferedUartRx<'d> {
 
 impl Drop for RingBufferedUartRx<'_> {
     fn drop(&mut self) {
-        self.teardown_uart();
+        self.stop_uart();
         self.rx.as_ref().map(|x| x.set_as_disconnected());
         self.rts.as_ref().map(|x| x.set_as_disconnected());
         super::drop_tx_rx(self.info, self.state);