Skip to content

Commit 9465244

Browse files
authored
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
1 parent 08d406e commit 9465244

File tree

5 files changed

+165
-11
lines changed

5 files changed

+165
-11
lines changed

esp-hal/CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1616

1717
### Fixed
1818

19+
- Fixed an issue with DMA transfers potentially not waking up the correct async task (#2065)
20+
1921
### Removed
2022

2123
## [0.20.1] - 2024-08-30

esp-hal/src/dma/gdma.rs

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -420,14 +420,25 @@ impl<const N: u8> RegisterAccess for Channel<N> {
420420
#[doc(hidden)]
421421
pub struct ChannelTxImpl<const N: u8> {}
422422

423+
#[cfg(feature = "async")]
424+
use embassy_sync::waitqueue::AtomicWaker;
425+
426+
#[cfg(feature = "async")]
427+
#[allow(clippy::declare_interior_mutable_const)]
428+
const INIT: AtomicWaker = AtomicWaker::new();
429+
430+
#[cfg(feature = "async")]
431+
static TX_WAKERS: [AtomicWaker; CHANNEL_COUNT] = [INIT; CHANNEL_COUNT];
432+
433+
#[cfg(feature = "async")]
434+
static RX_WAKERS: [AtomicWaker; CHANNEL_COUNT] = [INIT; CHANNEL_COUNT];
435+
423436
impl<const N: u8> crate::private::Sealed for ChannelTxImpl<N> {}
424437

425438
impl<const N: u8> TxChannel<Channel<N>> for ChannelTxImpl<N> {
426439
#[cfg(feature = "async")]
427-
fn waker() -> &'static embassy_sync::waitqueue::AtomicWaker {
428-
static WAKER: embassy_sync::waitqueue::AtomicWaker =
429-
embassy_sync::waitqueue::AtomicWaker::new();
430-
&WAKER
440+
fn waker() -> &'static AtomicWaker {
441+
&TX_WAKERS[N as usize]
431442
}
432443
}
433444

@@ -439,10 +450,8 @@ impl<const N: u8> crate::private::Sealed for ChannelRxImpl<N> {}
439450

440451
impl<const N: u8> RxChannel<Channel<N>> for ChannelRxImpl<N> {
441452
#[cfg(feature = "async")]
442-
fn waker() -> &'static embassy_sync::waitqueue::AtomicWaker {
443-
static WAKER: embassy_sync::waitqueue::AtomicWaker =
444-
embassy_sync::waitqueue::AtomicWaker::new();
445-
&WAKER
453+
fn waker() -> &'static AtomicWaker {
454+
&RX_WAKERS[N as usize]
446455
}
447456
}
448457

@@ -569,16 +578,24 @@ macro_rules! impl_channel {
569578

570579
cfg_if::cfg_if! {
571580
if #[cfg(esp32c2)] {
581+
#[cfg(feature = "async")]
582+
const CHANNEL_COUNT: usize = 1;
572583
impl_channel!(0, super::asynch::interrupt::interrupt_handler_ch0, DMA_CH0);
573584
} else if #[cfg(esp32c3)] {
585+
#[cfg(feature = "async")]
586+
const CHANNEL_COUNT: usize = 3;
574587
impl_channel!(0, super::asynch::interrupt::interrupt_handler_ch0, DMA_CH0);
575588
impl_channel!(1, super::asynch::interrupt::interrupt_handler_ch1, DMA_CH1);
576589
impl_channel!(2, super::asynch::interrupt::interrupt_handler_ch2, DMA_CH2);
577590
} else if #[cfg(any(esp32c6, esp32h2))] {
591+
#[cfg(feature = "async")]
592+
const CHANNEL_COUNT: usize = 3;
578593
impl_channel!(0, super::asynch::interrupt::interrupt_handler_ch0, DMA_IN_CH0, DMA_OUT_CH0);
579594
impl_channel!(1, super::asynch::interrupt::interrupt_handler_ch1, DMA_IN_CH1, DMA_OUT_CH1);
580595
impl_channel!(2, super::asynch::interrupt::interrupt_handler_ch2, DMA_IN_CH2, DMA_OUT_CH2);
581-
} else if #[cfg(esp32s3)] {
596+
} else if #[cfg(esp32s3)] {
597+
#[cfg(feature = "async")]
598+
const CHANNEL_COUNT: usize = 5;
582599
impl_channel!(0, super::asynch::interrupt::interrupt_handler_ch0, DMA_IN_CH0, DMA_OUT_CH0);
583600
impl_channel!(1, super::asynch::interrupt::interrupt_handler_ch1, DMA_IN_CH1, DMA_OUT_CH1);
584601
impl_channel!(2, super::asynch::interrupt::interrupt_handler_ch2, DMA_IN_CH2, DMA_OUT_CH2);

esp-hal/src/spi/master.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3661,7 +3661,7 @@ impl Instance for crate::peripherals::SPI3 {
36613661
#[inline(always)]
36623662
fn set_interrupt_handler(&mut self, handler: InterruptHandler) {
36633663
self.bind_spi3_interrupt(handler.handler());
3664-
crate::interrupt::enable(crate::peripherals::Interrupt::SPI2, handler.priority()).unwrap();
3664+
crate::interrupt::enable(crate::peripherals::Interrupt::SPI3, handler.priority()).unwrap();
36653665
}
36663666

36673667
#[inline(always)]
@@ -3792,7 +3792,7 @@ impl Instance for crate::peripherals::SPI3 {
37923792
#[inline(always)]
37933793
fn set_interrupt_handler(&mut self, handler: InterruptHandler) {
37943794
self.bind_spi3_interrupt(handler.handler());
3795-
crate::interrupt::enable(crate::peripherals::Interrupt::SPI2, handler.priority()).unwrap();
3795+
crate::interrupt::enable(crate::peripherals::Interrupt::SPI3, handler.priority()).unwrap();
37963796
}
37973797

37983798
#[inline(always)]

hil-test/Cargo.toml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,11 @@ name = "embassy_interrupt_executor"
158158
harness = false
159159
required-features = ["async", "embassy"]
160160

161+
[[test]]
162+
name = "embassy_interrupt_spi_dma"
163+
harness = false
164+
required-features = ["async", "embassy"]
165+
161166
[[test]]
162167
name = "twai"
163168
harness = false
Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
//! Reproduction and regression test for a sneaky issue.
2+
3+
//% CHIPS: esp32 esp32s2 esp32s3
4+
//% FEATURES: integrated-timers
5+
//% FEATURES: generic-queue
6+
7+
#![no_std]
8+
#![no_main]
9+
10+
use embassy_time::{Duration, Instant, Ticker};
11+
use esp_hal::{
12+
dma::{Dma, DmaPriority, DmaRxBuf, DmaTxBuf},
13+
dma_buffers,
14+
interrupt::{software::SoftwareInterruptControl, Priority},
15+
peripherals::SPI3,
16+
prelude::*,
17+
spi::{
18+
master::{Spi, SpiDma},
19+
FullDuplexMode,
20+
SpiMode,
21+
},
22+
timer::{timg::TimerGroup, ErasedTimer},
23+
Async,
24+
};
25+
use esp_hal_embassy::InterruptExecutor;
26+
use hil_test as _;
27+
28+
cfg_if::cfg_if! {
29+
if #[cfg(any(
30+
feature = "esp32",
31+
feature = "esp32s2",
32+
))] {
33+
use esp_hal::dma::Spi3DmaChannel as DmaChannel1;
34+
} else {
35+
use esp_hal::dma::DmaChannel1;
36+
}
37+
}
38+
39+
macro_rules! mk_static {
40+
($t:ty,$val:expr) => {{
41+
static STATIC_CELL: static_cell::StaticCell<$t> = static_cell::StaticCell::new();
42+
#[deny(unused_attributes)]
43+
let x = STATIC_CELL.uninit().write(($val));
44+
x
45+
}};
46+
}
47+
48+
#[embassy_executor::task]
49+
async fn interrupt_driven_task(spi: SpiDma<'static, SPI3, DmaChannel1, FullDuplexMode, Async>) {
50+
let mut ticker = Ticker::every(Duration::from_millis(1));
51+
52+
let (tx_buffer, tx_descriptors, rx_buffer, rx_descriptors) = dma_buffers!(128);
53+
let dma_tx_buf = DmaTxBuf::new(tx_descriptors, tx_buffer).unwrap();
54+
let dma_rx_buf = DmaRxBuf::new(rx_descriptors, rx_buffer).unwrap();
55+
56+
let mut spi = spi.with_buffers(dma_tx_buf, dma_rx_buf);
57+
58+
loop {
59+
let mut buffer: [u8; 8] = [0; 8];
60+
61+
spi.transfer_in_place_async(&mut buffer).await.unwrap();
62+
63+
ticker.next().await;
64+
}
65+
}
66+
67+
#[cfg(test)]
68+
#[embedded_test::tests(executor = esp_hal_embassy::Executor::new())]
69+
mod test {
70+
use super::*;
71+
72+
#[test]
73+
#[timeout(3)]
74+
async fn run_interrupt_executor_test() {
75+
let (peripherals, clocks) = esp_hal::init(esp_hal::Config::default());
76+
77+
let timg0 = TimerGroup::new(peripherals.TIMG0, &clocks);
78+
esp_hal_embassy::init(
79+
&clocks,
80+
[
81+
ErasedTimer::from(timg0.timer0),
82+
ErasedTimer::from(timg0.timer1),
83+
],
84+
);
85+
86+
let dma = Dma::new(peripherals.DMA);
87+
88+
cfg_if::cfg_if! {
89+
if #[cfg(any(feature = "esp32", feature = "esp32s2"))] {
90+
let dma_channel1 = dma.spi2channel;
91+
let dma_channel2 = dma.spi3channel;
92+
} else {
93+
let dma_channel1 = dma.channel0;
94+
let dma_channel2 = dma.channel1;
95+
}
96+
}
97+
98+
let (tx_buffer, tx_descriptors, rx_buffer, rx_descriptors) = dma_buffers!(1024);
99+
let dma_tx_buf = DmaTxBuf::new(tx_descriptors, tx_buffer).unwrap();
100+
let dma_rx_buf = DmaRxBuf::new(rx_descriptors, rx_buffer).unwrap();
101+
102+
let mut spi = Spi::new(peripherals.SPI2, 100.kHz(), SpiMode::Mode0, &clocks)
103+
.with_dma(dma_channel1.configure_for_async(false, DmaPriority::Priority0))
104+
.with_buffers(dma_tx_buf, dma_rx_buf);
105+
106+
let spi2 = Spi::new(peripherals.SPI3, 100.kHz(), SpiMode::Mode0, &clocks)
107+
.with_dma(dma_channel2.configure_for_async(false, DmaPriority::Priority0));
108+
109+
let sw_ints = SoftwareInterruptControl::new(peripherals.SW_INTERRUPT);
110+
111+
let interrupt_executor = mk_static!(
112+
InterruptExecutor<1>,
113+
InterruptExecutor::new(sw_ints.software_interrupt1)
114+
);
115+
116+
let spawner = interrupt_executor.start(Priority::Priority3);
117+
118+
spawner.spawn(interrupt_driven_task(spi2)).unwrap();
119+
120+
let start = Instant::now();
121+
let mut buffer: [u8; 1024] = [0; 1024];
122+
loop {
123+
spi.transfer_in_place_async(&mut buffer).await.unwrap();
124+
125+
if start.elapsed() > Duration::from_secs(1) {
126+
break;
127+
}
128+
}
129+
}
130+
}

0 commit comments

Comments
 (0)