From 9465244e0d5b4a526963c772c1fcf5affd897ecc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Tue, 3 Sep 2024 12:02:21 +0200 Subject: [PATCH] Fix various SPI/DMA issues (#2065) * Add failing test * Fix enabled interrupt * Fix using the correct waker * Changelog * Enable test on more devices that have SPI3 --- esp-hal/CHANGELOG.md | 2 + esp-hal/src/dma/gdma.rs | 35 ++++-- esp-hal/src/spi/master.rs | 4 +- hil-test/Cargo.toml | 5 + hil-test/tests/embassy_interrupt_spi_dma.rs | 130 ++++++++++++++++++++ 5 files changed, 165 insertions(+), 11 deletions(-) create mode 100644 hil-test/tests/embassy_interrupt_spi_dma.rs 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 39f24a5862c..1c7ed6f6064 100644 --- a/esp-hal/src/dma/gdma.rs +++ b/esp-hal/src/dma/gdma.rs @@ -420,14 +420,25 @@ impl RegisterAccess for Channel { #[doc(hidden)] pub struct ChannelTxImpl {} +#[cfg(feature = "async")] +use embassy_sync::waitqueue::AtomicWaker; + +#[cfg(feature = "async")] +#[allow(clippy::declare_interior_mutable_const)] +const INIT: AtomicWaker = AtomicWaker::new(); + +#[cfg(feature = "async")] +static TX_WAKERS: [AtomicWaker; CHANNEL_COUNT] = [INIT; CHANNEL_COUNT]; + +#[cfg(feature = "async")] +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 embassy_sync::waitqueue::AtomicWaker { - static WAKER: embassy_sync::waitqueue::AtomicWaker = - embassy_sync::waitqueue::AtomicWaker::new(); - &WAKER + fn waker() -> &'static AtomicWaker { + &TX_WAKERS[N as usize] } } @@ -439,10 +450,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_WAKERS[N as usize] } } @@ -569,16 +578,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); 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)] 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..13412f5768a --- /dev/null +++ b/hil-test/tests/embassy_interrupt_spi_dma.rs @@ -0,0 +1,130 @@ +//! Reproduction and regression test for a sneaky issue. + +//% CHIPS: esp32 esp32s2 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::Priority0)); + + 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; + } + } + } +}