Fix interrupt executors waking up thread-mode executors on the first core (#2377)

* Clean up

* Add failing multi-core test

* Reset interrupts before main

* Changelog

* Add test case

esp-hal-embassy/CHANGELOG.md

@@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- Reduce memory footprint by 4 bytes on multi-core MCUs.
+
 ### Fixed
 
 ### Removed

esp-hal-embassy/src/executor/interrupt.rs

@@ -9,12 +9,8 @@ use esp_hal::{
 };
 use portable_atomic::{AtomicUsize, Ordering};
 
-static mut EXECUTORS: [CallbackContext; 4] = [
-    CallbackContext::new(),
-    CallbackContext::new(),
-    CallbackContext::new(),
-    CallbackContext::new(),
-];
+const COUNT: usize = 3 + cfg!(not(multi_core)) as usize;
+static mut EXECUTORS: [CallbackContext; COUNT] = [const { CallbackContext::new() }; COUNT];
 
 /// Interrupt mode executor.
 ///
@@ -46,13 +42,11 @@ impl CallbackContext {
     }
 
     fn set(&self, executor: *mut raw::Executor) {
-        unsafe {
-            self.raw_executor.get().write(executor);
-        }
+        unsafe { self.raw_executor.get().write(executor) };
     }
 }
 
-fn handle_interrupt<const NUM: u8>() {
+extern "C" fn handle_interrupt<const NUM: u8>() {
     let swi = unsafe { SoftwareInterrupt::<NUM>::steal() };
     swi.reset();
 
@@ -62,22 +56,6 @@ fn handle_interrupt<const NUM: u8>() {
     }
 }
 
-extern "C" fn swi_handler0() {
-    handle_interrupt::<0>();
-}
-
-extern "C" fn swi_handler1() {
-    handle_interrupt::<1>();
-}
-
-extern "C" fn swi_handler2() {
-    handle_interrupt::<2>();
-}
-
-extern "C" fn swi_handler3() {
-    handle_interrupt::<3>();
-}
-
 impl<const SWI: u8> InterruptExecutor<SWI> {
     /// Create a new `InterruptExecutor`.
     /// This takes the software interrupt to be used internally.
@@ -127,10 +105,11 @@ impl<const SWI: u8> InterruptExecutor<SWI> {
         }
 
         let swi_handler = match SWI {
-            0 => swi_handler0,
-            1 => swi_handler1,
-            2 => swi_handler2,
-            3 => swi_handler3,
+            0 => handle_interrupt::<0>,
+            1 => handle_interrupt::<1>,
+            2 => handle_interrupt::<2>,
+            #[cfg(not(multi_core))]
+            3 => handle_interrupt::<3>,
             _ => unreachable!(),
         };
 

esp-hal-embassy/src/executor/thread.rs

@@ -5,9 +5,7 @@ use core::marker::PhantomData;
 use embassy_executor::{raw, Spawner};
 use esp_hal::get_core;
 #[cfg(multi_core)]
-use esp_hal::macros::handler;
-#[cfg(multi_core)]
-use esp_hal::peripherals::SYSTEM;
+use esp_hal::{interrupt::software::SoftwareInterrupt, macros::handler};
 use portable_atomic::{AtomicBool, Ordering};
 
 pub(crate) const THREAD_MODE_CONTEXT: u8 = 16;
@@ -26,10 +24,7 @@ fn software3_interrupt() {
     // woken. It doesn't matter which core handles this interrupt first, the
     // point is just to wake up the core that is currently executing
     // `waiti`.
-    let system = unsafe { &*SYSTEM::PTR };
-    system
-        .cpu_intr_from_cpu_3()
-        .write(|w| w.cpu_intr_from_cpu_3().bit(false));
+    unsafe { SoftwareInterrupt::<3>::steal().reset() };
 }
 
 pub(crate) fn pend_thread_mode(core: usize) {
@@ -43,11 +38,7 @@ pub(crate) fn pend_thread_mode(core: usize) {
         // We need to clear the interrupt from software. We don't actually
         // need it to trigger and run the interrupt handler, we just need to
         // kick waiti to return.
-
-        let system = unsafe { &*SYSTEM::PTR };
-        system
-            .cpu_intr_from_cpu_3()
-            .write(|w| w.cpu_intr_from_cpu_3().bit(true));
+        unsafe { SoftwareInterrupt::<3>::steal().raise() };
     }
 }
 
@@ -77,8 +68,7 @@ This will use software-interrupt 3 which isn't available for anything else to wa
     pub fn new() -> Self {
         #[cfg(multi_core)]
         unsafe {
-            esp_hal::interrupt::software::SoftwareInterrupt::<3>::steal()
-                .set_interrupt_handler(software3_interrupt)
+            SoftwareInterrupt::<3>::steal().set_interrupt_handler(software3_interrupt);
         }
 
         Self {
@@ -149,12 +139,11 @@ This will use software-interrupt 3 which isn't available for anything else to wa
                 );
             }
         } else {
-            // waiti sets the PS.INTLEVEL when slipping into sleep
-            // because critical sections in Xtensa are implemented via increasing
-            // PS.INTLEVEL the critical section ends here
-            // take care not add code after `waiti` if it needs to be inside the CS
-            unsafe { core::arch::asm!("waiti 0") }; // critical section ends
-                                                    // here
+            // `waiti` sets the `PS.INTLEVEL` when slipping into sleep because critical
+            // sections in Xtensa are implemented via increasing `PS.INTLEVEL`.
+            // The critical section ends here. Take care not add code after
+            // `waiti` if it needs to be inside the CS.
+            unsafe { core::arch::asm!("waiti 0") };
         }
     }
 

esp-hal/CHANGELOG.md

@@ -25,6 +25,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - Fix conflict between `RtcClock::get_xtal_freq` and `Rtc::disable_rom_message_printing` (#2360)
+- Fixed an issue where interrupts enabled before `esp_hal::init` were disabled. This issue caused the executor created by `#[esp_hal_embassy::main]` to behave incorrectly in multi-core applications. (#2377)
 
 ### Removed
 

esp-hal/src/lib.rs

@@ -525,8 +525,6 @@ pub fn init(config: Config) -> Peripherals {
 
     Clocks::init(config.cpu_clock);
 
-    #[cfg(xtensa)]
-    crate::interrupt::setup_interrupts();
     #[cfg(esp32)]
     crate::time::time_init();
 

esp-hal/src/soc/esp32/mod.rs

@@ -107,8 +107,10 @@ pub unsafe extern "C" fn ESP32Reset() -> ! {
         stack_chk_guard.write_volatile(0xdeadbabe);
     }
 
+    crate::interrupt::setup_interrupts();
+
     // continue with default reset handler
-    xtensa_lx_rt::Reset();
+    xtensa_lx_rt::Reset()
 }
 
 /// The ESP32 has a first stage bootloader that handles loading program data

esp-hal/src/soc/esp32s2/mod.rs

@@ -112,6 +112,8 @@ pub unsafe extern "C" fn ESP32Reset() -> ! {
         stack_chk_guard.write_volatile(0xdeadbabe);
     }
 
+    crate::interrupt::setup_interrupts();
+
     // continue with default reset handler
     xtensa_lx_rt::Reset()
 }

esp-hal/src/soc/esp32s3/mod.rs

@@ -151,8 +151,10 @@ pub unsafe extern "C" fn ESP32Reset() -> ! {
         stack_chk_guard.write_volatile(0xdeadbabe);
     }
 
+    crate::interrupt::setup_interrupts();
+
     // continue with default reset handler
-    xtensa_lx_rt::Reset();
+    xtensa_lx_rt::Reset()
 }
 
 /// The ESP32 has a first stage bootloader that handles loading program data

hil-test/tests/embassy_interrupt_executor.rs

@@ -9,6 +9,8 @@
 #![no_main]
 
 use embassy_sync::{blocking_mutex::raw::CriticalSectionRawMutex, signal::Signal};
+#[cfg(multi_core)]
+use esp_hal::cpu_control::{CpuControl, Stack};
 use esp_hal::{
     interrupt::{
         software::{SoftwareInterrupt, SoftwareInterruptControl},
@@ -29,41 +31,94 @@ macro_rules! mk_static {
 }
 
 #[embassy_executor::task]
-async fn interrupt_driven_task(signal: &'static Signal<CriticalSectionRawMutex, ()>) {
+async fn interrupt_driven_task(
+    signal: &'static Signal<CriticalSectionRawMutex, ()>,
+    response: &'static Signal<CriticalSectionRawMutex, ()>,
+) {
     loop {
         signal.wait().await;
+        response.signal(());
     }
 }
 
+struct Context {
+    interrupt: SoftwareInterrupt<1>,
+    #[cfg(multi_core)]
+    cpu_control: CpuControl<'static>,
+}
+
 #[cfg(test)]
 #[embedded_test::tests(executor = esp_hal_embassy::Executor::new())]
 mod test {
-    use hil_test as _;
-
     use super::*;
 
     #[init]
-    fn init() -> SoftwareInterrupt<1> {
+    fn init() -> Context {
         let peripherals = esp_hal::init(esp_hal::Config::default());
 
         let timg0 = TimerGroup::new(peripherals.TIMG0);
         esp_hal_embassy::init(timg0.timer0);
 
         let sw_ints = SoftwareInterruptControl::new(peripherals.SW_INTERRUPT);
-        sw_ints.software_interrupt1
+        Context {
+            interrupt: sw_ints.software_interrupt1,
+            #[cfg(multi_core)]
+            cpu_control: CpuControl::new(peripherals.CPU_CTRL),
+        }
     }
 
     #[test]
     #[timeout(3)]
-    fn run_interrupt_executor_test(interrupt: SoftwareInterrupt<1>) {
+    async fn run_interrupt_executor_test(ctx: Context) {
         let interrupt_executor =
-            mk_static!(InterruptExecutor<1>, InterruptExecutor::new(interrupt));
+            mk_static!(InterruptExecutor<1>, InterruptExecutor::new(ctx.interrupt));
         let signal = mk_static!(Signal<CriticalSectionRawMutex, ()>, Signal::new());
+        let response = mk_static!(Signal<CriticalSectionRawMutex, ()>, Signal::new());
 
         let spawner = interrupt_executor.start(Priority::Priority3);
 
-        spawner.spawn(interrupt_driven_task(signal)).unwrap();
+        spawner
+            .spawn(interrupt_driven_task(signal, response))
+            .unwrap();
 
-        signal.signal(());
+        for _ in 0..3 {
+            signal.signal(());
+            response.wait().await;
+        }
+    }
+
+    #[test]
+    #[cfg(multi_core)]
+    #[timeout(3)]
+    async fn run_interrupt_executor_test_on_core_1(mut ctx: Context) {
+        let app_core_stack = mk_static!(Stack<8192>, Stack::new());
+        let response = &*mk_static!(Signal<CriticalSectionRawMutex, ()>, Signal::new());
+        let signal = &*mk_static!(Signal<CriticalSectionRawMutex, ()>, Signal::new());
+
+        let cpu1_fnctn = {
+            move || {
+                let interrupt_executor =
+                    mk_static!(InterruptExecutor<1>, InterruptExecutor::new(ctx.interrupt));
+
+                let spawner = interrupt_executor.start(Priority::Priority3);
+
+                spawner
+                    .spawn(interrupt_driven_task(signal, response))
+                    .unwrap();
+
+                loop {}
+            }
+        };
+
+        #[allow(static_mut_refs)]
+        let _guard = ctx
+            .cpu_control
+            .start_app_core(app_core_stack, cpu1_fnctn)
+            .unwrap();
+
+        for _ in 0..3 {
+            signal.signal(());
+            response.wait().await;
+        }
     }
 }

hil-test/tests/embassy_interrupt_spi_dma.rs

@@ -59,7 +59,7 @@ mod test {
 
     #[test]
     #[timeout(3)]
-    async fn run_interrupt_executor_test() {
+    async fn dma_does_not_lock_up_when_used_in_different_executors() {
         let peripherals = esp_hal::init(esp_hal::Config::default());
 
         let timg0 = TimerGroup::new(peripherals.TIMG0);
@@ -68,7 +68,7 @@ mod test {
         let dma = Dma::new(peripherals.DMA);
 
         cfg_if::cfg_if! {
-            if #[cfg(any(feature = "esp32", feature = "esp32s2"))] {
+            if #[cfg(pdma)] {
                 let dma_channel1 = dma.spi2channel;
                 let dma_channel2 = dma.spi3channel;
             } else {
@@ -109,4 +109,116 @@ mod test {
             }
         }
     }
+
+    // Reproducer of https://github.com/esp-rs/esp-hal/issues/2369
+    #[cfg(multi_core)]
+    #[test]
+    #[timeout(3)]
+    async fn dma_does_not_lock_up_on_core_1() {
+        use embassy_time::Timer;
+        use esp_hal::peripherals::SPI2;
+        use portable_atomic::{AtomicU32, Ordering};
+
+        cfg_if::cfg_if! {
+            if #[cfg(pdma)] {
+                use esp_hal::dma::Spi2DmaChannelCreator as DmaChannelCreator;
+            } else {
+                type DmaChannelCreator = esp_hal::dma::ChannelCreator<0>;
+            }
+        }
+
+        const BUFFER_SIZE: usize = 256;
+        static LOOP_COUNT: AtomicU32 = AtomicU32::new(0);
+
+        pub struct SpiPeripherals {
+            pub spi: SPI2,
+            pub dma_channel: DmaChannelCreator,
+        }
+
+        #[embassy_executor::task]
+        async fn run_spi(peripherals: SpiPeripherals) {
+            let (rx_buffer, rx_descriptors, tx_buffer, tx_descriptors) = dma_buffers!(3200);
+            let dma_rx_buf = DmaRxBuf::new(rx_descriptors, rx_buffer).unwrap();
+            let dma_tx_buf = DmaTxBuf::new(tx_descriptors, tx_buffer).unwrap();
+
+            let mut spi = Spi::new(peripherals.spi, 100.kHz(), SpiMode::Mode0)
+                .with_dma(
+                    peripherals
+                        .dma_channel
+                        .configure_for_async(false, DmaPriority::Priority0),
+                )
+                .with_buffers(dma_rx_buf, dma_tx_buf);
+
+            let send_buffer = mk_static!([u8; BUFFER_SIZE], [0u8; BUFFER_SIZE]);
+            loop {
+                let mut buffer = [0; 8];
+                embedded_hal_async::spi::SpiBus::transfer(&mut spi, &mut buffer, send_buffer)
+                    .await
+                    .unwrap();
+                LOOP_COUNT.fetch_add(1, Ordering::Relaxed);
+            }
+        }
+
+        let peripherals = esp_hal::init(esp_hal::Config::default());
+        let dma = Dma::new(peripherals.DMA);
+
+        let timg0 = TimerGroup::new(peripherals.TIMG0);
+        esp_hal_embassy::init(timg0.timer0);
+
+        cfg_if::cfg_if! {
+            if #[cfg(pdma)] {
+                let dma_channel = dma.spi2channel;
+            } else {
+                let dma_channel = dma.channel0;
+            }
+        }
+
+        let spi_peripherals = SpiPeripherals {
+            spi: peripherals.SPI2,
+            dma_channel,
+        };
+
+        let cpu1_fnctn = {
+            move || {
+                use esp_hal::interrupt::Priority;
+                use esp_hal_embassy::InterruptExecutor;
+                let sw_ints = esp_hal::interrupt::software::SoftwareInterruptControl::new(
+                    peripherals.SW_INTERRUPT,
+                );
+                let software_interrupt = sw_ints.software_interrupt2;
+                let hp_executor = mk_static!(
+                    InterruptExecutor<2>,
+                    InterruptExecutor::new(software_interrupt)
+                );
+                let high_pri_spawner = hp_executor.start(Priority::Priority2);
+
+                // hub75 runs as high priority task
+                high_pri_spawner.spawn(run_spi(spi_peripherals)).ok();
+
+                // This loop is necessary to avoid parking the core after creating the interrupt
+                // executor.
+                loop {}
+            }
+        };
+
+        use esp_hal::cpu_control::{CpuControl, Stack};
+        const DISPLAY_STACK_SIZE: usize = 8192;
+        let app_core_stack = mk_static!(Stack<DISPLAY_STACK_SIZE>, Stack::new());
+        let cpu_control = CpuControl::new(peripherals.CPU_CTRL);
+        let mut _cpu_control = cpu_control;
+
+        #[allow(static_mut_refs)]
+        let _guard = _cpu_control
+            .start_app_core(app_core_stack, cpu1_fnctn)
+            .unwrap();
+
+        let mut last = 0u32;
+        for _ in 0..5 {
+            Timer::after(Duration::from_millis(200)).await;
+
+            let next = LOOP_COUNT.load(Ordering::Relaxed);
+            assert_ne!(next, last, "stuck");
+            last = next;
+        }
+    }
 }