From f0d2ebdc7ead41307155b083790b8450ca2b7eac Mon Sep 17 00:00:00 2001 From: Alexandros Liarokapis Date: Tue, 1 Oct 2024 17:59:04 +0300 Subject: [PATCH] stm32: fix ringbugger overrun errors due to bad dma wrap-around behavior --- embassy-stm32/src/dma/dma_bdma.rs | 30 ++++--- embassy-stm32/src/dma/ringbuffer/mod.rs | 65 +++++++++------ embassy-stm32/src/dma/ringbuffer/tests/mod.rs | 83 +------------------ embassy-stm32/src/sai/mod.rs | 10 ++- embassy-stm32/src/usart/ringbuffered.rs | 1 - 5 files changed, 72 insertions(+), 117 deletions(-) diff --git a/embassy-stm32/src/dma/dma_bdma.rs b/embassy-stm32/src/dma/dma_bdma.rs index 59ad4d988..3f047a9a4 100644 --- a/embassy-stm32/src/dma/dma_bdma.rs +++ b/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. - pub fn read(&mut self, buf: &mut [W]) -> Result<(usize, usize), OverrunError> { + /// 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), 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 { + pub async fn read_exact(&mut self, buffer: &mut [W]) -> Result { self.ringbuf .read_exact(&mut DmaCtrlImpl(self.channel.reborrow()), buffer) .await } + /// The current length of the ringbuffer + pub fn len(&mut self) -> Result { + 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 { + pub async fn write_exact(&mut self, buffer: &[W]) -> Result { self.ringbuf .write_exact(&mut DmaCtrlImpl(self.channel.reborrow()), buffer) .await } + /// The current length of the ringbuffer + pub fn len(&mut self) -> Result { + Ok(self.ringbuf.len(&mut DmaCtrlImpl(self.channel.reborrow()))?) + } + /// The capacity of the ringbuffer pub const fn capacity(&self) -> usize { self.ringbuf.cap() diff --git a/embassy-stm32/src/dma/ringbuffer/mod.rs b/embassy-stm32/src/dma/ringbuffer/mod.rs index eb64ad016..a257faa5b 100644 --- a/embassy-stm32/src/dma/ringbuffer/mod.rs +++ b/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(); - self.complete_count += dma.reset_complete_count(); - self.pos = cap - dma.get_remaining_transfers(); + // Important! + // The ordering of the first two lines matters! + // 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 - // 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(); - } + self.complete_count += count_diff; } 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 { + pub fn len(&mut self, dma: &mut impl DmaCtrl) -> Result { 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 { - Err(OverrunError) + if diff < 0 { + 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 { + pub async fn read_exact(&mut self, dma: &mut impl DmaCtrl, buffer: &mut [W]) -> Result { 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 { + pub fn len(&mut self, dma: &mut impl DmaCtrl) -> Result { 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 { - Err(OverrunError) + if diff > self.cap() as isize { + 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 { + pub async fn write_exact(&mut self, dma: &mut impl DmaCtrl, buffer: &[W]) -> Result { 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]); diff --git a/embassy-stm32/src/dma/ringbuffer/tests/mod.rs b/embassy-stm32/src/dma/ringbuffer/tests/mod.rs index 1800eae69..6fabedb83 100644 --- a/embassy-stm32/src/dma/ringbuffer/tests/mod.rs +++ b/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>, @@ -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) { 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(); diff --git a/embassy-stm32/src/sai/mod.rs b/embassy-stm32/src/sai/mod.rs index 63f48ace0..7d2f071de 100644 --- a/embassy-stm32/src/sai/mod.rs +++ b/embassy-stm32/src/sai/mod.rs @@ -27,8 +27,14 @@ pub enum Error { } #[cfg(not(gpdma))] -impl From for Error { - fn from(_: ringbuffer::OverrunError) -> Self { +impl From for Error { + fn from(#[allow(unused)] err: ringbuffer::Error) -> Self { + #[cfg(feature = "defmt")] + { + if err == ringbuffer::Error::DmaUnsynced { + defmt::error!("Ringbuffer broken invariants detected!"); + } + } Self::Overrun } } diff --git a/embassy-stm32/src/usart/ringbuffered.rs b/embassy-stm32/src/usart/ringbuffered.rs index 75834bf37..eb2399d9c 100644 --- a/embassy-stm32/src/usart/ringbuffered.rs +++ b/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