From 8a2af086efb46e94fe26d616244474d028093186 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Fri, 30 Aug 2024 14:41:47 +0200 Subject: [PATCH 1/5] Add failing test --- hil-test/Cargo.toml | 5 + hil-test/tests/embassy_interrupt_spi_dma.rs | 130 ++++++++++++++++++++ 2 files changed, 135 insertions(+) create mode 100644 hil-test/tests/embassy_interrupt_spi_dma.rs diff --git a/hil-test/Cargo.toml b/hil-test/Cargo.toml index e048755d346..e9427591d45 100644 --- a/hil-test/Cargo.toml +++ b/hil-test/Cargo.toml @@ -158,6 +158,11 @@ name = "embassy_interrupt_executor" harness = false required-features = ["async", "embassy"] +[[test]] +name = "embassy_interrupt_spi_dma" +harness = false +required-features = ["async", "embassy"] + [[test]] name = "twai" harness = false diff --git a/hil-test/tests/embassy_interrupt_spi_dma.rs b/hil-test/tests/embassy_interrupt_spi_dma.rs new file mode 100644 index 00000000000..5e6ce64c3ec --- /dev/null +++ b/hil-test/tests/embassy_interrupt_spi_dma.rs @@ -0,0 +1,130 @@ +//! Reproduction and regression test for a sneaky issue. + +//% CHIPS: esp32s3 +//% FEATURES: integrated-timers +//% FEATURES: generic-queue + +#![no_std] +#![no_main] + +use embassy_time::{Duration, Instant, Ticker}; +use esp_hal::{ + dma::{Dma, DmaPriority, DmaRxBuf, DmaTxBuf}, + dma_buffers, + interrupt::{software::SoftwareInterruptControl, Priority}, + peripherals::SPI3, + prelude::*, + spi::{ + master::{Spi, SpiDma}, + FullDuplexMode, + SpiMode, + }, + timer::{timg::TimerGroup, ErasedTimer}, + Async, +}; +use esp_hal_embassy::InterruptExecutor; +use hil_test as _; + +cfg_if::cfg_if! { + if #[cfg(any( + feature = "esp32", + feature = "esp32s2", + ))] { + use esp_hal::dma::Spi3DmaChannel as DmaChannel1; + } else { + use esp_hal::dma::DmaChannel1; + } +} + +macro_rules! mk_static { + ($t:ty,$val:expr) => {{ + static STATIC_CELL: static_cell::StaticCell<$t> = static_cell::StaticCell::new(); + #[deny(unused_attributes)] + let x = STATIC_CELL.uninit().write(($val)); + x + }}; +} + +#[embassy_executor::task] +async fn interrupt_driven_task(spi: SpiDma<'static, SPI3, DmaChannel1, FullDuplexMode, Async>) { + let mut ticker = Ticker::every(Duration::from_millis(1)); + + let (tx_buffer, tx_descriptors, rx_buffer, rx_descriptors) = dma_buffers!(128); + let dma_tx_buf = DmaTxBuf::new(tx_descriptors, tx_buffer).unwrap(); + let dma_rx_buf = DmaRxBuf::new(rx_descriptors, rx_buffer).unwrap(); + + let mut spi = spi.with_buffers(dma_tx_buf, dma_rx_buf); + + loop { + let mut buffer: [u8; 8] = [0; 8]; + + spi.transfer_in_place_async(&mut buffer).await.unwrap(); + + ticker.next().await; + } +} + +#[cfg(test)] +#[embedded_test::tests(executor = esp_hal_embassy::Executor::new())] +mod test { + use super::*; + + #[test] + #[timeout(3)] + async fn run_interrupt_executor_test() { + let (peripherals, clocks) = esp_hal::init(esp_hal::Config::default()); + + let timg0 = TimerGroup::new(peripherals.TIMG0, &clocks); + esp_hal_embassy::init( + &clocks, + [ + ErasedTimer::from(timg0.timer0), + ErasedTimer::from(timg0.timer1), + ], + ); + + let dma = Dma::new(peripherals.DMA); + + cfg_if::cfg_if! { + if #[cfg(any(feature = "esp32", feature = "esp32s2"))] { + let dma_channel1 = dma.spi2channel; + let dma_channel2 = dma.spi3channel; + } else { + let dma_channel1 = dma.channel0; + let dma_channel2 = dma.channel1; + } + } + + let (tx_buffer, tx_descriptors, rx_buffer, rx_descriptors) = dma_buffers!(1024); + let dma_tx_buf = DmaTxBuf::new(tx_descriptors, tx_buffer).unwrap(); + let dma_rx_buf = DmaRxBuf::new(rx_descriptors, rx_buffer).unwrap(); + + let mut spi = Spi::new(peripherals.SPI2, 100.kHz(), SpiMode::Mode0, &clocks) + .with_dma(dma_channel1.configure_for_async(false, DmaPriority::Priority0)) + .with_buffers(dma_tx_buf, dma_rx_buf); + + let spi2 = Spi::new(peripherals.SPI3, 100.kHz(), SpiMode::Mode0, &clocks) + .with_dma(dma_channel2.configure_for_async(false, DmaPriority::Priority1)); + + let sw_ints = SoftwareInterruptControl::new(peripherals.SW_INTERRUPT); + + let interrupt_executor = mk_static!( + InterruptExecutor<1>, + InterruptExecutor::new(sw_ints.software_interrupt1) + ); + + let spawner = interrupt_executor.start(Priority::Priority3); + + spawner.spawn(interrupt_driven_task(spi2)).unwrap(); + + let start = Instant::now(); + let mut buffer: [u8; 1024] = [0; 1024]; + loop { + spi.transfer_in_place_async(&mut buffer).await.unwrap(); + + if start.elapsed() > Duration::from_secs(1) { + break; + } + } + } +} From 12c1baa30d51e89558bba60cfe83d93cbcf74934 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Mon, 2 Sep 2024 20:11:41 +0200 Subject: [PATCH 2/5] Fix enabled interrupt --- esp-hal/src/spi/master.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index ebd086f79c7..4f81c6910ee 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -3661,7 +3661,7 @@ impl Instance for crate::peripherals::SPI3 { #[inline(always)] fn set_interrupt_handler(&mut self, handler: InterruptHandler) { self.bind_spi3_interrupt(handler.handler()); - crate::interrupt::enable(crate::peripherals::Interrupt::SPI2, handler.priority()).unwrap(); + crate::interrupt::enable(crate::peripherals::Interrupt::SPI3, handler.priority()).unwrap(); } #[inline(always)] @@ -3792,7 +3792,7 @@ impl Instance for crate::peripherals::SPI3 { #[inline(always)] fn set_interrupt_handler(&mut self, handler: InterruptHandler) { self.bind_spi3_interrupt(handler.handler()); - crate::interrupt::enable(crate::peripherals::Interrupt::SPI2, handler.priority()).unwrap(); + crate::interrupt::enable(crate::peripherals::Interrupt::SPI3, handler.priority()).unwrap(); } #[inline(always)] From 4c2b975c8782a24da08688f1f19b40850b8109cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Mon, 2 Sep 2024 20:22:48 +0200 Subject: [PATCH 3/5] Fix using the correct waker --- esp-hal/src/dma/gdma.rs | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/esp-hal/src/dma/gdma.rs b/esp-hal/src/dma/gdma.rs index 39f24a5862c..6176d73704b 100644 --- a/esp-hal/src/dma/gdma.rs +++ b/esp-hal/src/dma/gdma.rs @@ -420,14 +420,24 @@ impl RegisterAccess for Channel { #[doc(hidden)] pub struct ChannelTxImpl {} +#[cfg(feature = "async")] +use embassy_sync::waitqueue::AtomicWaker; + +#[cfg(feature = "async")] +const NEW_WAKER: AtomicWaker = AtomicWaker::new(); + +#[cfg(feature = "async")] +static TX_WAKER: [AtomicWaker; CHANNEL_COUNT] = [NEW_WAKER; CHANNEL_COUNT]; + +#[cfg(feature = "async")] +static RX_WAKER: [AtomicWaker; CHANNEL_COUNT] = [NEW_WAKER; CHANNEL_COUNT]; + impl crate::private::Sealed for ChannelTxImpl {} impl TxChannel> for ChannelTxImpl { #[cfg(feature = "async")] - fn waker() -> &'static embassy_sync::waitqueue::AtomicWaker { - static WAKER: embassy_sync::waitqueue::AtomicWaker = - embassy_sync::waitqueue::AtomicWaker::new(); - &WAKER + fn waker() -> &'static AtomicWaker { + &TX_WAKER[N as usize] } } @@ -439,10 +449,8 @@ impl crate::private::Sealed for ChannelRxImpl {} impl RxChannel> for ChannelRxImpl { #[cfg(feature = "async")] - fn waker() -> &'static embassy_sync::waitqueue::AtomicWaker { - static WAKER: embassy_sync::waitqueue::AtomicWaker = - embassy_sync::waitqueue::AtomicWaker::new(); - &WAKER + fn waker() -> &'static AtomicWaker { + &RX_WAKER[N as usize] } } @@ -569,16 +577,24 @@ macro_rules! impl_channel { cfg_if::cfg_if! { if #[cfg(esp32c2)] { + #[cfg(feature = "async")] + const CHANNEL_COUNT: usize = 1; impl_channel!(0, super::asynch::interrupt::interrupt_handler_ch0, DMA_CH0); } else if #[cfg(esp32c3)] { + #[cfg(feature = "async")] + const CHANNEL_COUNT: usize = 3; impl_channel!(0, super::asynch::interrupt::interrupt_handler_ch0, DMA_CH0); impl_channel!(1, super::asynch::interrupt::interrupt_handler_ch1, DMA_CH1); impl_channel!(2, super::asynch::interrupt::interrupt_handler_ch2, DMA_CH2); } else if #[cfg(any(esp32c6, esp32h2))] { + #[cfg(feature = "async")] + const CHANNEL_COUNT: usize = 3; impl_channel!(0, super::asynch::interrupt::interrupt_handler_ch0, DMA_IN_CH0, DMA_OUT_CH0); impl_channel!(1, super::asynch::interrupt::interrupt_handler_ch1, DMA_IN_CH1, DMA_OUT_CH1); impl_channel!(2, super::asynch::interrupt::interrupt_handler_ch2, DMA_IN_CH2, DMA_OUT_CH2); - } else if #[cfg(esp32s3)] { + } else if #[cfg(esp32s3)] { + #[cfg(feature = "async")] + const CHANNEL_COUNT: usize = 5; impl_channel!(0, super::asynch::interrupt::interrupt_handler_ch0, DMA_IN_CH0, DMA_OUT_CH0); impl_channel!(1, super::asynch::interrupt::interrupt_handler_ch1, DMA_IN_CH1, DMA_OUT_CH1); impl_channel!(2, super::asynch::interrupt::interrupt_handler_ch2, DMA_IN_CH2, DMA_OUT_CH2); From 0fa2cd75c9f05ee0e2c9a3fbd30a8f7c20e7bda7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Mon, 2 Sep 2024 20:23:24 +0200 Subject: [PATCH 4/5] Changelog --- esp-hal/CHANGELOG.md | 2 ++ esp-hal/src/dma/gdma.rs | 11 ++++++----- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index 1e91f7d9440..d0e2ba73101 100644 --- a/esp-hal/CHANGELOG.md +++ b/esp-hal/CHANGELOG.md @@ -16,6 +16,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Fixed an issue with DMA transfers potentially not waking up the correct async task (#2065) + ### Removed ## [0.20.1] - 2024-08-30 diff --git a/esp-hal/src/dma/gdma.rs b/esp-hal/src/dma/gdma.rs index 6176d73704b..1c7ed6f6064 100644 --- a/esp-hal/src/dma/gdma.rs +++ b/esp-hal/src/dma/gdma.rs @@ -424,20 +424,21 @@ pub struct ChannelTxImpl {} use embassy_sync::waitqueue::AtomicWaker; #[cfg(feature = "async")] -const NEW_WAKER: AtomicWaker = AtomicWaker::new(); +#[allow(clippy::declare_interior_mutable_const)] +const INIT: AtomicWaker = AtomicWaker::new(); #[cfg(feature = "async")] -static TX_WAKER: [AtomicWaker; CHANNEL_COUNT] = [NEW_WAKER; CHANNEL_COUNT]; +static TX_WAKERS: [AtomicWaker; CHANNEL_COUNT] = [INIT; CHANNEL_COUNT]; #[cfg(feature = "async")] -static RX_WAKER: [AtomicWaker; CHANNEL_COUNT] = [NEW_WAKER; CHANNEL_COUNT]; +static RX_WAKERS: [AtomicWaker; CHANNEL_COUNT] = [INIT; CHANNEL_COUNT]; impl crate::private::Sealed for ChannelTxImpl {} impl TxChannel> for ChannelTxImpl { #[cfg(feature = "async")] fn waker() -> &'static AtomicWaker { - &TX_WAKER[N as usize] + &TX_WAKERS[N as usize] } } @@ -450,7 +451,7 @@ impl crate::private::Sealed for ChannelRxImpl {} impl RxChannel> for ChannelRxImpl { #[cfg(feature = "async")] fn waker() -> &'static AtomicWaker { - &RX_WAKER[N as usize] + &RX_WAKERS[N as usize] } } From e39e0d2a94179f4dff3a53e21763e85ed9f579ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Tue, 3 Sep 2024 10:12:23 +0200 Subject: [PATCH 5/5] Enable test on more devices that have SPI3 --- hil-test/tests/embassy_interrupt_spi_dma.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hil-test/tests/embassy_interrupt_spi_dma.rs b/hil-test/tests/embassy_interrupt_spi_dma.rs index 5e6ce64c3ec..13412f5768a 100644 --- a/hil-test/tests/embassy_interrupt_spi_dma.rs +++ b/hil-test/tests/embassy_interrupt_spi_dma.rs @@ -1,6 +1,6 @@ //! Reproduction and regression test for a sneaky issue. -//% CHIPS: esp32s3 +//% CHIPS: esp32 esp32s2 esp32s3 //% FEATURES: integrated-timers //% FEATURES: generic-queue @@ -104,7 +104,7 @@ mod test { .with_buffers(dma_tx_buf, dma_rx_buf); let spi2 = Spi::new(peripherals.SPI3, 100.kHz(), SpiMode::Mode0, &clocks) - .with_dma(dma_channel2.configure_for_async(false, DmaPriority::Priority1)); + .with_dma(dma_channel2.configure_for_async(false, DmaPriority::Priority0)); let sw_ints = SoftwareInterruptControl::new(peripherals.SW_INTERRUPT);