From 9c7b296432af40ac44e5e65a3742c691f1414444 Mon Sep 17 00:00:00 2001 From: Alexandros Liarokapis Date: Tue, 24 Sep 2024 00:59:34 +0300 Subject: [PATCH] overrun at invalid diffs, rename clear to reset, simplify dma_sync method --- embassy-stm32/src/adc/ringbuffered_v2.rs | 3 +- embassy-stm32/src/dma/dma_bdma.rs | 4 +- embassy-stm32/src/dma/ringbuffer/mod.rs | 53 +++++++++---------- embassy-stm32/src/dma/ringbuffer/tests/mod.rs | 22 ++++---- .../dma/ringbuffer/tests/prop_test/reader.rs | 10 ++-- .../dma/ringbuffer/tests/prop_test/writer.rs | 10 ++-- 6 files changed, 49 insertions(+), 53 deletions(-) diff --git a/embassy-stm32/src/adc/ringbuffered_v2.rs b/embassy-stm32/src/adc/ringbuffered_v2.rs index 3b064044e..9fc233222 100644 --- a/embassy-stm32/src/adc/ringbuffered_v2.rs +++ b/embassy-stm32/src/adc/ringbuffered_v2.rs @@ -226,9 +226,8 @@ impl<'d, T: Instance> RingBufferedAdc<'d, T> { /// Turns on ADC if it is not already turned on and starts continuous DMA transfer. pub fn start(&mut self) -> Result<(), OverrunError> { - self.ring_buf.clear(); - self.setup_adc(); + self.ring_buf.clear(); Ok(()) } diff --git a/embassy-stm32/src/dma/dma_bdma.rs b/embassy-stm32/src/dma/dma_bdma.rs index a43137c6e..59ad4d988 100644 --- a/embassy-stm32/src/dma/dma_bdma.rs +++ b/embassy-stm32/src/dma/dma_bdma.rs @@ -833,7 +833,7 @@ impl<'a, W: Word> ReadableRingBuffer<'a, W> { /// Clear all data in the ring buffer. pub fn clear(&mut self) { - self.ringbuf.clear(&mut DmaCtrlImpl(self.channel.reborrow())); + self.ringbuf.reset(&mut DmaCtrlImpl(self.channel.reborrow())); } /// Read elements from the ring buffer @@ -980,7 +980,7 @@ impl<'a, W: Word> WritableRingBuffer<'a, W> { /// Clear all data in the ring buffer. pub fn clear(&mut self) { - self.ringbuf.clear(&mut DmaCtrlImpl(self.channel.reborrow())); + self.ringbuf.reset(&mut DmaCtrlImpl(self.channel.reborrow())); } /// Write elements directly to the raw buffer. diff --git a/embassy-stm32/src/dma/ringbuffer/mod.rs b/embassy-stm32/src/dma/ringbuffer/mod.rs index 76f7f36ec..eb64ad016 100644 --- a/embassy-stm32/src/dma/ringbuffer/mod.rs +++ b/embassy-stm32/src/dma/ringbuffer/mod.rs @@ -23,14 +23,14 @@ pub struct OverrunError; #[derive(Debug, Clone, Copy, Default)] struct DmaIndex { - completion_count: usize, + complete_count: usize, pos: usize, } impl DmaIndex { fn reset(&mut self) { self.pos = 0; - self.completion_count = 0; + self.complete_count = 0; } fn as_index(&self, cap: usize, offset: usize) -> usize { @@ -38,34 +38,31 @@ impl DmaIndex { } fn dma_sync(&mut self, cap: usize, dma: &mut impl DmaCtrl) { - let fst_pos = cap - dma.get_remaining_transfers(); - let fst_count = dma.reset_complete_count(); - let pos = cap - dma.get_remaining_transfers(); + let first_pos = cap - dma.get_remaining_transfers(); + self.complete_count += dma.reset_complete_count(); + self.pos = cap - dma.get_remaining_transfers(); - let wrap_count = if pos >= fst_pos { - fst_count - } else { - fst_count + dma.reset_complete_count() - }; - - self.pos = pos; - self.completion_count += wrap_count; + // 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(); + } } fn advance(&mut self, cap: usize, steps: usize) { let next = self.pos + steps; - self.completion_count += next / cap; + self.complete_count += next / cap; self.pos = next % cap; } fn normalize(lhs: &mut DmaIndex, rhs: &mut DmaIndex) { - let min_count = lhs.completion_count.min(rhs.completion_count); - lhs.completion_count -= min_count; - rhs.completion_count -= min_count; + let min_count = lhs.complete_count.min(rhs.complete_count); + lhs.complete_count -= min_count; + rhs.complete_count -= min_count; } fn diff(&self, cap: usize, rhs: &DmaIndex) -> isize { - (self.completion_count * cap + self.pos) as isize - (rhs.completion_count * cap + rhs.pos) as isize + (self.complete_count * cap + self.pos) as isize - (rhs.complete_count * cap + rhs.pos) as isize } } @@ -86,7 +83,7 @@ impl<'a, W: Word> ReadableDmaRingBuffer<'a, W> { } /// Reset the ring buffer to its initial state. - pub fn clear(&mut self, dma: &mut impl DmaCtrl) { + pub fn reset(&mut self, dma: &mut impl DmaCtrl) { dma.reset_complete_count(); self.write_index.reset(); self.write_index.dma_sync(self.cap(), dma); @@ -103,12 +100,12 @@ impl<'a, W: Word> ReadableDmaRingBuffer<'a, W> { self.write_index.dma_sync(self.cap(), dma); DmaIndex::normalize(&mut self.write_index, &mut self.read_index); - let diff: usize = self.write_index.diff(self.cap(), &self.read_index).try_into().unwrap(); + let diff = self.write_index.diff(self.cap(), &self.read_index); - if diff > self.cap() { + if diff < 0 || diff > self.cap() as isize { Err(OverrunError) } else { - Ok(diff) + Ok(diff as usize) } } @@ -117,10 +114,10 @@ impl<'a, W: Word> ReadableDmaRingBuffer<'a, W> { /// 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, - /// in which case the rinbuffer will automatically clear itself. + /// 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> { self.read_raw(dma, buf).inspect_err(|_e| { - self.clear(dma); + self.reset(dma); }) } @@ -192,14 +189,14 @@ impl<'a, W: Word> WritableDmaRingBuffer<'a, W> { dma_buf, read_index: Default::default(), write_index: DmaIndex { - completion_count: 0, + complete_count: 0, pos: len, }, } } /// Reset the ring buffer to its initial state. The buffer after the reset will be full. - pub fn clear(&mut self, dma: &mut impl DmaCtrl) { + pub fn reset(&mut self, dma: &mut impl DmaCtrl) { dma.reset_complete_count(); self.read_index.reset(); self.read_index.dma_sync(self.cap(), dma); @@ -214,7 +211,7 @@ impl<'a, W: Word> WritableDmaRingBuffer<'a, W> { let diff = self.write_index.diff(self.cap(), &self.read_index); - if diff < 0 { + if diff < 0 || diff > self.cap() as isize { Err(OverrunError) } else { Ok(self.cap().saturating_sub(diff as usize)) @@ -233,7 +230,7 @@ impl<'a, W: Word> WritableDmaRingBuffer<'a, W> { /// leeway between the write index and the DMA. pub fn write(&mut self, dma: &mut impl DmaCtrl, buf: &[W]) -> Result<(usize, usize), OverrunError> { self.write_raw(dma, buf).inspect_err(|_e| { - self.clear(dma); + self.reset(dma); }) } diff --git a/embassy-stm32/src/dma/ringbuffer/tests/mod.rs b/embassy-stm32/src/dma/ringbuffer/tests/mod.rs index 9768e1df8..1800eae69 100644 --- a/embassy-stm32/src/dma/ringbuffer/tests/mod.rs +++ b/embassy-stm32/src/dma/ringbuffer/tests/mod.rs @@ -64,12 +64,12 @@ fn dma_index_dma_sync_syncs_position_to_last_read_if_sync_takes_place_on_same_dm ]); let mut index = DmaIndex::default(); index.dma_sync(CAP, &mut dma); - assert_eq!(index.completion_count, 0); + assert_eq!(index.complete_count, 0); assert_eq!(index.pos, 7); } #[test] -fn dma_index_dma_sync_updates_completion_count_properly_if_sync_takes_place_on_same_dma_cycle() { +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), @@ -77,9 +77,9 @@ fn dma_index_dma_sync_updates_completion_count_properly_if_sync_takes_place_on_s TestCircularTransferRequest::PositionRequest(7), ]); let mut index = DmaIndex::default(); - index.completion_count = 1; + index.complete_count = 1; index.dma_sync(CAP, &mut dma); - assert_eq!(index.completion_count, 3); + assert_eq!(index.complete_count, 3); assert_eq!(index.pos, 7); } @@ -94,12 +94,12 @@ fn dma_index_dma_sync_syncs_to_last_position_if_reads_occur_on_different_dma_cyc ]); let mut index = DmaIndex::default(); index.dma_sync(CAP, &mut dma); - assert_eq!(index.completion_count, 1); + 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_completion_count_occurs_on_first_cycle( +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![ @@ -109,14 +109,14 @@ fn dma_index_dma_sync_detects_new_cycle_if_later_position_is_less_than_first_and TestCircularTransferRequest::ResetCompleteCount(1), ]); let mut index = DmaIndex::default(); - index.completion_count = 1; + index.complete_count = 1; index.dma_sync(CAP, &mut dma); - assert_eq!(index.completion_count, 3); + 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_completion_count_occurs_on_later_cycle( +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![ @@ -126,9 +126,9 @@ fn dma_index_dma_sync_detects_new_cycle_if_later_position_is_less_than_first_and TestCircularTransferRequest::ResetCompleteCount(0), ]); let mut index = DmaIndex::default(); - index.completion_count = 1; + index.complete_count = 1; index.dma_sync(CAP, &mut dma); - assert_eq!(index.completion_count, 3); + assert_eq!(index.complete_count, 3); assert_eq!(index.pos, 5); } diff --git a/embassy-stm32/src/dma/ringbuffer/tests/prop_test/reader.rs b/embassy-stm32/src/dma/ringbuffer/tests/prop_test/reader.rs index 6e640a813..4f3957a68 100644 --- a/embassy-stm32/src/dma/ringbuffer/tests/prop_test/reader.rs +++ b/embassy-stm32/src/dma/ringbuffer/tests/prop_test/reader.rs @@ -5,7 +5,7 @@ use super::*; #[derive(Debug, Clone)] enum ReaderTransition { Write(usize), - Clear, + Reset, ReadUpTo(usize), } @@ -23,14 +23,14 @@ impl ReferenceStateMachine for ReaderSM { prop_oneof![ (1..50_usize).prop_map(ReaderTransition::Write), (1..50_usize).prop_map(ReaderTransition::ReadUpTo), - strategy::Just(ReaderTransition::Clear), + strategy::Just(ReaderTransition::Reset), ] .boxed() } fn apply(status: Self::State, transition: &Self::Transition) -> Self::State { match (status, transition) { - (_, ReaderTransition::Clear) => Status::Available(0), + (_, ReaderTransition::Reset) => Status::Available(0), (Status::Available(x), ReaderTransition::Write(y)) => { if x + y > CAP { Status::Failed @@ -87,8 +87,8 @@ impl StateMachineTest for ReaderTest { ) -> Self::SystemUnderTest { match transition { ReaderTransition::Write(x) => sut.producer.advance(x), - ReaderTransition::Clear => { - sut.consumer.clear(&mut sut.producer); + ReaderTransition::Reset => { + sut.consumer.reset(&mut sut.producer); } ReaderTransition::ReadUpTo(x) => { let status = sut.status; diff --git a/embassy-stm32/src/dma/ringbuffer/tests/prop_test/writer.rs b/embassy-stm32/src/dma/ringbuffer/tests/prop_test/writer.rs index c1b3859b2..15433c0ee 100644 --- a/embassy-stm32/src/dma/ringbuffer/tests/prop_test/writer.rs +++ b/embassy-stm32/src/dma/ringbuffer/tests/prop_test/writer.rs @@ -6,7 +6,7 @@ use super::*; enum WriterTransition { Read(usize), WriteUpTo(usize), - Clear, + Reset, } struct WriterSM; @@ -23,14 +23,14 @@ impl ReferenceStateMachine for WriterSM { prop_oneof![ (1..50_usize).prop_map(WriterTransition::Read), (1..50_usize).prop_map(WriterTransition::WriteUpTo), - strategy::Just(WriterTransition::Clear), + strategy::Just(WriterTransition::Reset), ] .boxed() } fn apply(status: Self::State, transition: &Self::Transition) -> Self::State { match (status, transition) { - (_, WriterTransition::Clear) => Status::Available(CAP), + (_, WriterTransition::Reset) => Status::Available(CAP), (Status::Available(x), WriterTransition::Read(y)) => { if x < *y { Status::Failed @@ -87,8 +87,8 @@ impl StateMachineTest for WriterTest { ) -> Self::SystemUnderTest { match transition { WriterTransition::Read(x) => sut.consumer.advance(x), - WriterTransition::Clear => { - sut.producer.clear(&mut sut.consumer); + WriterTransition::Reset => { + sut.producer.reset(&mut sut.consumer); } WriterTransition::WriteUpTo(x) => { let status = sut.status;