From bc68c87a52a022262976e70015e93d16e3ba415e Mon Sep 17 00:00:00 2001 From: liebman Date: Fri, 5 Jul 2024 09:13:10 -0700 Subject: [PATCH] review changes: - `.div_ceil()` - return errors for bad chunk size and buffer sizes in Mem2Mem constructors - correct 0 chunk size check in descripter macros --- esp-hal/src/dma/gdma.rs | 55 ++++++++------- esp-hal/src/dma/mod.rs | 8 ++- examples/src/bin/dma_mem2mem.rs | 3 +- hil-test/tests/dma_macros.rs | 4 +- hil-test/tests/dma_mem2mem.rs | 117 +++++++++++++++++++++++++++++++- 5 files changed, 155 insertions(+), 32 deletions(-) diff --git a/esp-hal/src/dma/gdma.rs b/esp-hal/src/dma/gdma.rs index e471e1b1884..dc065d5ffa8 100644 --- a/esp-hal/src/dma/gdma.rs +++ b/esp-hal/src/dma/gdma.rs @@ -655,36 +655,38 @@ mod m2m { { /// Create a new Mem2Mem instance. pub fn new( - mut channel: Channel<'d, C, MODE>, + channel: Channel<'d, C, MODE>, peripheral: impl DmaEligible, tx_descriptors: &'static mut [DmaDescriptor], rx_descriptors: &'static mut [DmaDescriptor], - ) -> Self { - channel.tx.init_channel(); - channel.rx.init_channel(); - Mem2Mem { - channel, - peripheral: peripheral.dma_peripheral(), - tx_chain: DescriptorChain::new(tx_descriptors), - rx_chain: DescriptorChain::new(rx_descriptors), + ) -> Result { + unsafe { + Self::new_unsafe( + channel, + peripheral.dma_peripheral(), + tx_descriptors, + rx_descriptors, + crate::dma::CHUNK_SIZE, + ) } } /// Create a new Mem2Mem instance with specific chunk size. pub fn new_with_chunk_size( - mut channel: Channel<'d, C, MODE>, + channel: Channel<'d, C, MODE>, peripheral: impl DmaEligible, tx_descriptors: &'static mut [DmaDescriptor], rx_descriptors: &'static mut [DmaDescriptor], chunk_size: usize, - ) -> Self { - channel.tx.init_channel(); - channel.rx.init_channel(); - Mem2Mem { - channel, - peripheral: peripheral.dma_peripheral(), - tx_chain: DescriptorChain::new_with_chunk_size(tx_descriptors, chunk_size), - rx_chain: DescriptorChain::new_with_chunk_size(rx_descriptors, chunk_size), + ) -> Result { + unsafe { + Self::new_unsafe( + channel, + peripheral.dma_peripheral(), + tx_descriptors, + rx_descriptors, + chunk_size, + ) } } @@ -699,15 +701,22 @@ mod m2m { peripheral: DmaPeripheral, tx_descriptors: &'static mut [DmaDescriptor], rx_descriptors: &'static mut [DmaDescriptor], - ) -> Self { + chunk_size: usize, + ) -> Result { + if !(1..=4092).contains(&chunk_size) { + return Err(DmaError::InvalidChunkSize); + } + if tx_descriptors.is_empty() || rx_descriptors.is_empty() { + return Err(DmaError::OutOfDescriptors); + } channel.tx.init_channel(); channel.rx.init_channel(); - Mem2Mem { + Ok(Mem2Mem { channel, peripheral, - tx_chain: DescriptorChain::new(tx_descriptors), - rx_chain: DescriptorChain::new(rx_descriptors), - } + tx_chain: DescriptorChain::new_with_chunk_size(tx_descriptors, chunk_size), + rx_chain: DescriptorChain::new_with_chunk_size(rx_descriptors, chunk_size), + }) } /// Start a memory to memory transfer. diff --git a/esp-hal/src/dma/mod.rs b/esp-hal/src/dma/mod.rs index 96f232c2fb8..4ee368200b5 100644 --- a/esp-hal/src/dma/mod.rs +++ b/esp-hal/src/dma/mod.rs @@ -194,11 +194,11 @@ macro_rules! dma_circular_buffers { #[macro_export] macro_rules! dma_descriptors { ($tx_size:expr, $rx_size:expr) => { - $crate::dma_descriptors_chunk_size!($tx_size, $rx_size, CHUNK_SIZE) + $crate::dma_descriptors_chunk_size!($tx_size, $rx_size, $crate::dma::CHUNK_SIZE) }; ($size:expr) => { - $crate::dma_descriptors_chunk_size!($size, $size, CHUNK_SIZE) + $crate::dma_descriptors_chunk_size!($size, $size, $crate::dma::CHUNK_SIZE) }; } @@ -322,7 +322,7 @@ macro_rules! dma_circular_descriptors_chunk_size { ($tx_size:expr, $rx_size:expr, $chunk_size:expr) => {{ // these will check for size at compile time const _: () = assert!($chunk_size <= 4092, "chunk size must be <= 4092"); - const _: () = assert!($chunk_size > 9, "chunk size must be > 0"); + const _: () = assert!($chunk_size > 0, "chunk size must be > 0"); const tx_descriptor_len: usize = if $tx_size > $chunk_size * 2 { ($tx_size + $chunk_size - 1) / $chunk_size @@ -367,6 +367,8 @@ pub enum DmaError { BufferTooSmall, /// Descriptors or buffers are not located in a supported memory region UnsupportedMemoryRegion, + /// Invalid DMA chunk size + InvalidChunkSize, } /// DMA Priorities diff --git a/examples/src/bin/dma_mem2mem.rs b/examples/src/bin/dma_mem2mem.rs index a345cceec00..6374d03c7bb 100644 --- a/examples/src/bin/dma_mem2mem.rs +++ b/examples/src/bin/dma_mem2mem.rs @@ -38,7 +38,8 @@ fn main() -> ! { #[cfg(not(any(feature = "esp32c2", feature = "esp32c3", feature = "esp32s3")))] let dma_peripheral = peripherals.MEM2MEM1; - let mut mem2mem = Mem2Mem::new(channel, dma_peripheral, tx_descriptors, rx_descriptors); + let mut mem2mem = + Mem2Mem::new(channel, dma_peripheral, tx_descriptors, rx_descriptors).unwrap(); for i in 0..core::mem::size_of_val(tx_buffer) { tx_buffer[i] = (i % 256) as u8; diff --git a/hil-test/tests/dma_macros.rs b/hil-test/tests/dma_macros.rs index 12989806b62..d5193e40672 100644 --- a/hil-test/tests/dma_macros.rs +++ b/hil-test/tests/dma_macros.rs @@ -11,12 +11,12 @@ use esp_backtrace as _; const DATA_SIZE: usize = 1024 * 10; pub(crate) const fn compute_size(size: usize, chunk_size: usize) -> usize { - (size + chunk_size - 1) / chunk_size + size.div_ceil(chunk_size) } pub(crate) const fn compute_circular_size(size: usize, chunk_size: usize) -> usize { if size > chunk_size * 2 { - (size + chunk_size - 1) / chunk_size + size.div_ceil(chunk_size) } else { 3 } diff --git a/hil-test/tests/dma_mem2mem.rs b/hil-test/tests/dma_mem2mem.rs index 8702e947760..4e31916b31a 100644 --- a/hil-test/tests/dma_mem2mem.rs +++ b/hil-test/tests/dma_mem2mem.rs @@ -9,9 +9,10 @@ use defmt_rtt as _; use esp_backtrace as _; use esp_hal::{ clock::ClockControl, - dma::{Dma, DmaPriority, Mem2Mem}, + dma::{Dma, DmaError, DmaPriority, Mem2Mem}, dma_buffers, dma_buffers_chunk_size, + dma_descriptors, peripherals::Peripherals, system::SystemControl, }; @@ -43,7 +44,8 @@ mod tests { #[cfg(not(any(feature = "esp32c2", feature = "esp32c3", feature = "esp32s3")))] let dma_peripheral = peripherals.MEM2MEM1; - let mut mem2mem = Mem2Mem::new(channel, dma_peripheral, tx_descriptors, rx_descriptors); + let mut mem2mem = + Mem2Mem::new(channel, dma_peripheral, tx_descriptors, rx_descriptors).unwrap(); for i in 0..core::mem::size_of_val(tx_buffer) { tx_buffer[i] = (i % 256) as u8; @@ -81,7 +83,8 @@ mod tests { tx_descriptors, rx_descriptors, CHUNK_SIZE, - ); + ) + .unwrap(); for i in 0..core::mem::size_of_val(tx_buffer) { tx_buffer[i] = (i % 256) as u8; @@ -95,4 +98,112 @@ mod tests { assert_eq!(rx_buffer[i], tx_buffer[i]); } } + + #[test] + fn test_mem2mem_errors_zero_tx() { + use esp_hal::dma::CHUNK_SIZE; + + let peripherals = Peripherals::take(); + let system = SystemControl::new(peripherals.SYSTEM); + let _clocks = ClockControl::boot_defaults(system.clock_control).freeze(); + + let dma = Dma::new(peripherals.DMA); + let channel = dma.channel0.configure(false, DmaPriority::Priority0); + #[cfg(any(feature = "esp32c2", feature = "esp32c3", feature = "esp32s3"))] + let dma_peripheral = peripherals.SPI2; + #[cfg(not(any(feature = "esp32c2", feature = "esp32c3", feature = "esp32s3")))] + let dma_peripheral = peripherals.MEM2MEM1; + + let (tx_descriptors, rx_descriptors) = dma_descriptors!(0, 1024); + match Mem2Mem::new_with_chunk_size( + channel, + dma_peripheral, + tx_descriptors, + rx_descriptors, + CHUNK_SIZE, + ) { + Err(DmaError::OutOfDescriptors) => (), + _ => panic!("Expected OutOfDescriptors"), + } + } + + #[test] + fn test_mem2mem_errors_zero_rx() { + use esp_hal::dma::CHUNK_SIZE; + + let peripherals = Peripherals::take(); + let system = SystemControl::new(peripherals.SYSTEM); + let _clocks = ClockControl::boot_defaults(system.clock_control).freeze(); + + let dma = Dma::new(peripherals.DMA); + let channel = dma.channel0.configure(false, DmaPriority::Priority0); + #[cfg(any(feature = "esp32c2", feature = "esp32c3", feature = "esp32s3"))] + let dma_peripheral = peripherals.SPI2; + #[cfg(not(any(feature = "esp32c2", feature = "esp32c3", feature = "esp32s3")))] + let dma_peripheral = peripherals.MEM2MEM1; + + let (tx_descriptors, rx_descriptors) = dma_descriptors!(1024, 0); + match Mem2Mem::new_with_chunk_size( + channel, + dma_peripheral, + tx_descriptors, + rx_descriptors, + CHUNK_SIZE, + ) { + Err(DmaError::OutOfDescriptors) => (), + _ => panic!("Expected OutOfDescriptors"), + } + } + + #[test] + fn test_mem2mem_errors_chunk_size_too_small() { + let peripherals = Peripherals::take(); + let system = SystemControl::new(peripherals.SYSTEM); + let _clocks = ClockControl::boot_defaults(system.clock_control).freeze(); + + let dma = Dma::new(peripherals.DMA); + let channel = dma.channel0.configure(false, DmaPriority::Priority0); + #[cfg(any(feature = "esp32c2", feature = "esp32c3", feature = "esp32s3"))] + let dma_peripheral = peripherals.SPI2; + #[cfg(not(any(feature = "esp32c2", feature = "esp32c3", feature = "esp32s3")))] + let dma_peripheral = peripherals.MEM2MEM1; + + let (tx_descriptors, rx_descriptors) = dma_descriptors!(1024, 1024); + match Mem2Mem::new_with_chunk_size( + channel, + dma_peripheral, + tx_descriptors, + rx_descriptors, + 0, + ) { + Err(DmaError::InvalidChunkSize) => (), + _ => panic!("Expected InvalidChunkSize"), + } + } + + #[test] + fn test_mem2mem_errors_chunk_size_too_big() { + let peripherals = Peripherals::take(); + let system = SystemControl::new(peripherals.SYSTEM); + let _clocks = ClockControl::boot_defaults(system.clock_control).freeze(); + + let dma = Dma::new(peripherals.DMA); + let channel = dma.channel0.configure(false, DmaPriority::Priority0); + #[cfg(any(feature = "esp32c2", feature = "esp32c3", feature = "esp32s3"))] + let dma_peripheral = peripherals.SPI2; + #[cfg(not(any(feature = "esp32c2", feature = "esp32c3", feature = "esp32s3")))] + let dma_peripheral = peripherals.MEM2MEM1; + + let (tx_descriptors, rx_descriptors) = dma_descriptors!(1024, 1024); + match Mem2Mem::new_with_chunk_size( + channel, + dma_peripheral, + tx_descriptors, + rx_descriptors, + 4093, + ) { + Err(DmaError::InvalidChunkSize) => (), + _ => panic!("Expected InvalidChunkSize"), + } + } }