stm32: fix ringbugger overrun errors due to bad dma wrap-around behavior

embassy-stm32/src/dma/dma_bdma.rs

@@ -6,7 +6,7 @@ use core::task::{Context, Poll, Waker};
 use embassy_hal_internal::{into_ref, Peripheral, PeripheralRef};
 use embassy_sync::waitqueue::AtomicWaker;
 
-use super::ringbuffer::{DmaCtrl, OverrunError, ReadableDmaRingBuffer, WritableDmaRingBuffer};
+use super::ringbuffer::{DmaCtrl, Error, ReadableDmaRingBuffer, WritableDmaRingBuffer};
 use super::word::{Word, WordSize};
 use super::{AnyChannel, Channel, Dir, Request, STATE};
 use crate::interrupt::typelevel::Interrupt;
@@ -299,7 +299,6 @@ impl AnyChannel {
                 } else {
                     return;
                 }
-
                 state.waker.wake();
             }
             #[cfg(bdma)]
@@ -828,7 +827,8 @@ impl<'a, W: Word> ReadableRingBuffer<'a, W> {
     ///
     /// You must call this after creating it for it to work.
     pub fn start(&mut self) {
-        self.channel.start()
+        self.channel.start();
+        self.clear();
     }
 
     /// Clear all data in the ring buffer.
@@ -840,15 +840,15 @@ impl<'a, W: Word> ReadableRingBuffer<'a, W> {
     /// Return a tuple of the length read and the length remaining in the buffer
     /// If not all of the elements were read, then there will be some elements in the buffer remaining
     /// The length remaining is the capacity, ring_buf.len(), less the elements remaining after the read
-    /// OverrunError is returned if the portion to be read was overwritten by the DMA controller.
+    /// Error is returned if the portion to be read was overwritten by the DMA controller.
-    pub fn read(&mut self, buf: &mut [W]) -> Result<(usize, usize), OverrunError> {
+    pub fn read(&mut self, buf: &mut [W]) -> Result<(usize, usize), Error> {
         self.ringbuf.read(&mut DmaCtrlImpl(self.channel.reborrow()), buf)
     }
 
     /// Read an exact number of elements from the ringbuffer.
     ///
     /// Returns the remaining number of elements available for immediate reading.
-    /// OverrunError is returned if the portion to be read was overwritten by the DMA controller.
+    /// Error is returned if the portion to be read was overwritten by the DMA controller.
     ///
     /// Async/Wake Behavior:
     /// The underlying DMA peripheral only can wake us when its buffer pointer has reached the halfway point,
@@ -856,12 +856,17 @@ impl<'a, W: Word> ReadableRingBuffer<'a, W> {
     /// ring buffer was created with a buffer of size 'N':
     /// - If M equals N/2 or N/2 divides evenly into M, this function will return every N/2 elements read on the DMA source.
     /// - Otherwise, this function may need up to N/2 extra elements to arrive before returning.
-    pub async fn read_exact(&mut self, buffer: &mut [W]) -> Result<usize, OverrunError> {
+    pub async fn read_exact(&mut self, buffer: &mut [W]) -> Result<usize, Error> {
         self.ringbuf
             .read_exact(&mut DmaCtrlImpl(self.channel.reborrow()), buffer)
             .await
     }
 
+    /// The current length of the ringbuffer
+    pub fn len(&mut self) -> Result<usize, Error> {
+        Ok(self.ringbuf.len(&mut DmaCtrlImpl(self.channel.reborrow()))?)
+    }
+
     /// The capacity of the ringbuffer
     pub const fn capacity(&self) -> usize {
         self.ringbuf.cap()
@@ -986,23 +991,28 @@ impl<'a, W: Word> WritableRingBuffer<'a, W> {
     /// Write elements directly to the raw buffer.
     /// This can be used to fill the buffer before starting the DMA transfer.
     #[allow(dead_code)]
-    pub fn write_immediate(&mut self, buf: &[W]) -> Result<(usize, usize), OverrunError> {
+    pub fn write_immediate(&mut self, buf: &[W]) -> Result<(usize, usize), Error> {
         self.ringbuf.write_immediate(buf)
     }
 
     /// Write elements from the ring buffer
     /// Return a tuple of the length written and the length remaining in the buffer
-    pub fn write(&mut self, buf: &[W]) -> Result<(usize, usize), OverrunError> {
+    pub fn write(&mut self, buf: &[W]) -> Result<(usize, usize), Error> {
         self.ringbuf.write(&mut DmaCtrlImpl(self.channel.reborrow()), buf)
     }
 
     /// Write an exact number of elements to the ringbuffer.
-    pub async fn write_exact(&mut self, buffer: &[W]) -> Result<usize, OverrunError> {
+    pub async fn write_exact(&mut self, buffer: &[W]) -> Result<usize, Error> {
         self.ringbuf
             .write_exact(&mut DmaCtrlImpl(self.channel.reborrow()), buffer)
             .await
     }
 
+    /// The current length of the ringbuffer
+    pub fn len(&mut self) -> Result<usize, Error> {
+        Ok(self.ringbuf.len(&mut DmaCtrlImpl(self.channel.reborrow()))?)
+    }
+
     /// The capacity of the ringbuffer
     pub const fn capacity(&self) -> usize {
         self.ringbuf.cap()

embassy-stm32/src/dma/ringbuffer/mod.rs

@@ -19,9 +19,13 @@ pub trait DmaCtrl {
 
 #[derive(Debug, PartialEq)]
 #[cfg_attr(feature = "defmt", derive(defmt::Format))]
-pub struct OverrunError;
+pub enum Error {
+    Overrun,
+    DmaUnsynced,
+}
 
 #[derive(Debug, Clone, Copy, Default)]
+#[cfg_attr(feature = "defmt", derive(defmt::Format))]
 struct DmaIndex {
     complete_count: usize,
     pos: usize,
@@ -38,15 +42,19 @@ impl DmaIndex {
     }
 
     fn dma_sync(&mut self, cap: usize, dma: &mut impl DmaCtrl) {
-        let first_pos = cap - dma.get_remaining_transfers();
+        // Important!
-        self.complete_count += dma.reset_complete_count();
+        // The ordering of the first two lines matters!
-        self.pos = cap - dma.get_remaining_transfers();
+        // If changed, the code will detect a wrong +capacity
+        // jump at wrap-around.
+        let count_diff = dma.reset_complete_count();
+        let pos = cap - dma.get_remaining_transfers();
+        self.pos = if pos < self.pos && count_diff == 0 {
+            cap - 1
+        } else {
+            pos
+        };
 
-        // If the latter call to get_remaining_transfers() returned a smaller value than the first, the dma
+        self.complete_count += count_diff;
-        // has wrapped around between calls and we must check if the complete count also incremented.
-        if self.pos < first_pos {
-            self.complete_count += dma.reset_complete_count();
-        }
     }
 
     fn advance(&mut self, cap: usize, steps: usize) {
@@ -96,14 +104,18 @@ impl<'a, W: Word> ReadableDmaRingBuffer<'a, W> {
     }
 
     /// Get the available readable dma samples.
-    pub fn len(&mut self, dma: &mut impl DmaCtrl) -> Result<usize, OverrunError> {
+    pub fn len(&mut self, dma: &mut impl DmaCtrl) -> Result<usize, Error> {
         self.write_index.dma_sync(self.cap(), dma);
         DmaIndex::normalize(&mut self.write_index, &mut self.read_index);
 
         let diff = self.write_index.diff(self.cap(), &self.read_index);
 
-        if diff < 0 || diff > self.cap() as isize {
+        if diff < 0 {
-            Err(OverrunError)
+            return Err(Error::DmaUnsynced);
+        }
+
+        if diff > self.cap() as isize {
+            Err(Error::Overrun)
         } else {
             Ok(diff as usize)
         }
@@ -113,9 +125,9 @@ impl<'a, W: Word> ReadableDmaRingBuffer<'a, W> {
     /// Return a tuple of the length read and the length remaining in the buffer
     /// If not all of the elements were read, then there will be some elements in the buffer remaining
     /// The length remaining is the capacity, ring_buf.len(), less the elements remaining after the read
-    /// OverrunError is returned if the portion to be read was overwritten by the DMA controller,
+    /// Error is returned if the portion to be read was overwritten by the DMA controller,
     /// in which case the rinbuffer will automatically reset itself.
-    pub fn read(&mut self, dma: &mut impl DmaCtrl, buf: &mut [W]) -> Result<(usize, usize), OverrunError> {
+    pub fn read(&mut self, dma: &mut impl DmaCtrl, buf: &mut [W]) -> Result<(usize, usize), Error> {
         self.read_raw(dma, buf).inspect_err(|_e| {
             self.reset(dma);
         })
@@ -124,7 +136,7 @@ impl<'a, W: Word> ReadableDmaRingBuffer<'a, W> {
     /// Read an exact number of elements from the ringbuffer.
     ///
     /// Returns the remaining number of elements available for immediate reading.
-    /// OverrunError is returned if the portion to be read was overwritten by the DMA controller.
+    /// Error is returned if the portion to be read was overwritten by the DMA controller.
     ///
     /// Async/Wake Behavior:
     /// The underlying DMA peripheral only can wake us when its buffer pointer has reached the halfway point,
@@ -132,7 +144,7 @@ impl<'a, W: Word> ReadableDmaRingBuffer<'a, W> {
     /// ring buffer was created with a buffer of size 'N':
     /// - If M equals N/2 or N/2 divides evenly into M, this function will return every N/2 elements read on the DMA source.
     /// - Otherwise, this function may need up to N/2 extra elements to arrive before returning.
-    pub async fn read_exact(&mut self, dma: &mut impl DmaCtrl, buffer: &mut [W]) -> Result<usize, OverrunError> {
+    pub async fn read_exact(&mut self, dma: &mut impl DmaCtrl, buffer: &mut [W]) -> Result<usize, Error> {
         let mut read_data = 0;
         let buffer_len = buffer.len();
 
@@ -154,7 +166,7 @@ impl<'a, W: Word> ReadableDmaRingBuffer<'a, W> {
         .await
     }
 
-    fn read_raw(&mut self, dma: &mut impl DmaCtrl, buf: &mut [W]) -> Result<(usize, usize), OverrunError> {
+    fn read_raw(&mut self, dma: &mut impl DmaCtrl, buf: &mut [W]) -> Result<(usize, usize), Error> {
         let readable = self.len(dma)?.min(buf.len());
         for i in 0..readable {
             buf[i] = self.read_buf(i);
@@ -205,14 +217,17 @@ impl<'a, W: Word> WritableDmaRingBuffer<'a, W> {
     }
 
     /// Get the remaining writable dma samples.
-    pub fn len(&mut self, dma: &mut impl DmaCtrl) -> Result<usize, OverrunError> {
+    pub fn len(&mut self, dma: &mut impl DmaCtrl) -> Result<usize, Error> {
         self.read_index.dma_sync(self.cap(), dma);
         DmaIndex::normalize(&mut self.read_index, &mut self.write_index);
 
         let diff = self.write_index.diff(self.cap(), &self.read_index);
 
-        if diff < 0 || diff > self.cap() as isize {
+        if diff > self.cap() as isize {
-            Err(OverrunError)
+            return Err(Error::DmaUnsynced);
+        }
+        if diff < 0 {
+            Err(Error::Overrun)
         } else {
             Ok(self.cap().saturating_sub(diff as usize))
         }
@@ -225,17 +240,17 @@ impl<'a, W: Word> WritableDmaRingBuffer<'a, W> {
 
     /// Append data to the ring buffer.
     /// Returns a tuple of the data written and the remaining write capacity in the buffer.
-    /// OverrunError is returned if the portion to be written was previously read by the DMA controller.
+    /// Error is returned if the portion to be written was previously read by the DMA controller.
     /// In this case, the ringbuffer will automatically reset itself, giving a full buffer worth of
     /// leeway between the write index and the DMA.
-    pub fn write(&mut self, dma: &mut impl DmaCtrl, buf: &[W]) -> Result<(usize, usize), OverrunError> {
+    pub fn write(&mut self, dma: &mut impl DmaCtrl, buf: &[W]) -> Result<(usize, usize), Error> {
         self.write_raw(dma, buf).inspect_err(|_e| {
             self.reset(dma);
         })
     }
 
     /// Write elements directly to the buffer.
-    pub fn write_immediate(&mut self, buf: &[W]) -> Result<(usize, usize), OverrunError> {
+    pub fn write_immediate(&mut self, buf: &[W]) -> Result<(usize, usize), Error> {
         for (i, data) in buf.iter().enumerate() {
             self.write_buf(i, *data)
         }
@@ -244,7 +259,7 @@ impl<'a, W: Word> WritableDmaRingBuffer<'a, W> {
     }
 
     /// Write an exact number of elements to the ringbuffer.
-    pub async fn write_exact(&mut self, dma: &mut impl DmaCtrl, buffer: &[W]) -> Result<usize, OverrunError> {
+    pub async fn write_exact(&mut self, dma: &mut impl DmaCtrl, buffer: &[W]) -> Result<usize, Error> {
         let mut written_data = 0;
         let buffer_len = buffer.len();
 
@@ -266,7 +281,7 @@ impl<'a, W: Word> WritableDmaRingBuffer<'a, W> {
         .await
     }
 
-    pub fn write_raw(&mut self, dma: &mut impl DmaCtrl, buf: &[W]) -> Result<(usize, usize), OverrunError> {
+    fn write_raw(&mut self, dma: &mut impl DmaCtrl, buf: &[W]) -> Result<(usize, usize), Error> {
         let writable = self.len(dma)?.min(buf.len());
         for i in 0..writable {
             self.write_buf(i, buf[i]);

embassy-stm32/src/dma/ringbuffer/tests/mod.rs

@@ -2,13 +2,14 @@ use std::{cell, vec};
 
 use super::*;
 
-#[allow(dead_code)]
+#[allow(unused)]
 #[derive(PartialEq, Debug)]
 enum TestCircularTransferRequest {
     ResetCompleteCount(usize),
     PositionRequest(usize),
 }
 
+#[allow(unused)]
 struct TestCircularTransfer {
     len: usize,
     requests: cell::RefCell<vec::Vec<TestCircularTransferRequest>>,
@@ -39,6 +40,7 @@ impl DmaCtrl for TestCircularTransfer {
 }
 
 impl TestCircularTransfer {
+    #[allow(unused)]
     pub fn new(len: usize) -> Self {
         Self {
             requests: cell::RefCell::new(vec![]),
@@ -46,6 +48,7 @@ impl TestCircularTransfer {
         }
     }
 
+    #[allow(unused)]
     pub fn setup(&self, mut requests: vec::Vec<TestCircularTransferRequest>) {
         requests.reverse();
         self.requests.replace(requests);
@@ -54,84 +57,6 @@ impl TestCircularTransfer {
 
 const CAP: usize = 16;
 
-#[test]
-fn dma_index_dma_sync_syncs_position_to_last_read_if_sync_takes_place_on_same_dma_cycle() {
-    let mut dma = TestCircularTransfer::new(CAP);
-    dma.setup(vec![
-        TestCircularTransferRequest::PositionRequest(4),
-        TestCircularTransferRequest::ResetCompleteCount(0),
-        TestCircularTransferRequest::PositionRequest(7),
-    ]);
-    let mut index = DmaIndex::default();
-    index.dma_sync(CAP, &mut dma);
-    assert_eq!(index.complete_count, 0);
-    assert_eq!(index.pos, 7);
-}
-
-#[test]
-fn dma_index_dma_sync_updates_complete_count_properly_if_sync_takes_place_on_same_dma_cycle() {
-    let mut dma = TestCircularTransfer::new(CAP);
-    dma.setup(vec![
-        TestCircularTransferRequest::PositionRequest(4),
-        TestCircularTransferRequest::ResetCompleteCount(2),
-        TestCircularTransferRequest::PositionRequest(7),
-    ]);
-    let mut index = DmaIndex::default();
-    index.complete_count = 1;
-    index.dma_sync(CAP, &mut dma);
-    assert_eq!(index.complete_count, 3);
-    assert_eq!(index.pos, 7);
-}
-
-#[test]
-fn dma_index_dma_sync_syncs_to_last_position_if_reads_occur_on_different_dma_cycles() {
-    let mut dma = TestCircularTransfer::new(CAP);
-    dma.setup(vec![
-        TestCircularTransferRequest::PositionRequest(10),
-        TestCircularTransferRequest::ResetCompleteCount(1),
-        TestCircularTransferRequest::PositionRequest(5),
-        TestCircularTransferRequest::ResetCompleteCount(0),
-    ]);
-    let mut index = DmaIndex::default();
-    index.dma_sync(CAP, &mut dma);
-    assert_eq!(index.complete_count, 1);
-    assert_eq!(index.pos, 5);
-}
-
-#[test]
-fn dma_index_dma_sync_detects_new_cycle_if_later_position_is_less_than_first_and_first_complete_count_occurs_on_first_cycle(
-) {
-    let mut dma = TestCircularTransfer::new(CAP);
-    dma.setup(vec![
-        TestCircularTransferRequest::PositionRequest(10),
-        TestCircularTransferRequest::ResetCompleteCount(1),
-        TestCircularTransferRequest::PositionRequest(5),
-        TestCircularTransferRequest::ResetCompleteCount(1),
-    ]);
-    let mut index = DmaIndex::default();
-    index.complete_count = 1;
-    index.dma_sync(CAP, &mut dma);
-    assert_eq!(index.complete_count, 3);
-    assert_eq!(index.pos, 5);
-}
-
-#[test]
-fn dma_index_dma_sync_detects_new_cycle_if_later_position_is_less_than_first_and_first_complete_count_occurs_on_later_cycle(
-) {
-    let mut dma = TestCircularTransfer::new(CAP);
-    dma.setup(vec![
-        TestCircularTransferRequest::PositionRequest(10),
-        TestCircularTransferRequest::ResetCompleteCount(2),
-        TestCircularTransferRequest::PositionRequest(5),
-        TestCircularTransferRequest::ResetCompleteCount(0),
-    ]);
-    let mut index = DmaIndex::default();
-    index.complete_count = 1;
-    index.dma_sync(CAP, &mut dma);
-    assert_eq!(index.complete_count, 3);
-    assert_eq!(index.pos, 5);
-}
-
 #[test]
 fn dma_index_as_index_returns_index_mod_cap_by_default() {
     let index = DmaIndex::default();

embassy-stm32/src/sai/mod.rs

@@ -27,8 +27,14 @@ pub enum Error {
 }
 
 #[cfg(not(gpdma))]
-impl From<ringbuffer::OverrunError> for Error {
+impl From<ringbuffer::Error> for Error {
-    fn from(_: ringbuffer::OverrunError) -> Self {
+    fn from(#[allow(unused)] err: ringbuffer::Error) -> Self {
+        #[cfg(feature = "defmt")]
+        {
+            if err == ringbuffer::Error::DmaUnsynced {
+                defmt::error!("Ringbuffer broken invariants detected!");
+            }
+        }
         Self::Overrun
     }
 }

embassy-stm32/src/usart/ringbuffered.rs

@@ -83,7 +83,6 @@ impl<'d> RingBufferedUartRx<'d> {
         // Clear the buffer so that it is ready to receive data
         compiler_fence(Ordering::SeqCst);
         self.ring_buf.start();
-        self.ring_buf.clear();
 
         let r = self.info.regs;
         // clear all interrupts and DMA Rx Request