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

@@ -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

esp-hal/src/dma/gdma.rs

@@ -420,14 +420,25 @@ impl<const N: u8> RegisterAccess for Channel<N> {
 #[doc(hidden)]
 pub struct ChannelTxImpl<const N: u8> {}
 
+#[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<const N: u8> crate::private::Sealed for ChannelTxImpl<N> {}
 
 impl<const N: u8> TxChannel<Channel<N>> for ChannelTxImpl<N> {
     #[cfg(feature = "async")]
-    fn waker() -> &'static embassy_sync::waitqueue::AtomicWaker {
+    fn waker() -> &'static AtomicWaker {
-        static WAKER: embassy_sync::waitqueue::AtomicWaker =
+        &TX_WAKERS[N as usize]
-            embassy_sync::waitqueue::AtomicWaker::new();
-        &WAKER
     }
 }
 
@@ -439,10 +450,8 @@ impl<const N: u8> crate::private::Sealed for ChannelRxImpl<N> {}
 
 impl<const N: u8> RxChannel<Channel<N>> for ChannelRxImpl<N> {
     #[cfg(feature = "async")]
-    fn waker() -> &'static embassy_sync::waitqueue::AtomicWaker {
+    fn waker() -> &'static AtomicWaker {
-        static WAKER: embassy_sync::waitqueue::AtomicWaker =
+        &RX_WAKERS[N as usize]
-            embassy_sync::waitqueue::AtomicWaker::new();
-        &WAKER
     }
 }
 
@@ -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);

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)]

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

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;
+            }
+        }
+    }
+}