Simplify esp-wifi timer code (#3576)

* Simplify esp-wifi timer code

* Reserve and use FROM_CPU_INTR2 on RISC-V

esp-hal/CHANGELOG.md

@@ -67,6 +67,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - The `debug` feature has been removed. (#3425)
 - The `usb_otg` and `bluetooth` features are now considered private and have been renamed accordingly. (#3425)
 - Include `.uninit` in the `noinit` section (#3558)
+- `SoftwareInterruptControl::software_interrupt2` is no longer available when using `esp-wifi/builtin-scheduler` (#3576)
 
 ### Fixed
 

esp-hal/Cargo.toml

@@ -107,7 +107,10 @@ default = []
 # These features are considered private and unstable. They are not covered by
 # semver guarantees and may change or be removed without notice.
 __bluetooth = []
+# Reserves FROM_CPU_INTR3 for multi-core MCUs.
 __esp_hal_embassy = []
+# Reserves FROM_CPU_INTR2 for RISC-V MCUs.
+__esp_wifi_builtin_scheduler = []
 __usb_otg = [
     "dep:embassy-usb-driver",
     "dep:embassy-usb-synopsys-otg",

esp-hal/src/interrupt/software.rs

@@ -180,11 +180,13 @@ pub struct SoftwareInterruptControl<'d> {
     pub software_interrupt0: SoftwareInterrupt<'d, 0>,
     /// Software interrupt 1.
     pub software_interrupt1: SoftwareInterrupt<'d, 1>,
-    /// Software interrupt 2.
+    /// Software interrupt 2. Not available when using esp-wifi's builtin
+    /// scheduler on RISC-V architectures.
+    #[cfg(not(all(feature = "__esp_wifi_builtin_scheduler", riscv)))]
     pub software_interrupt2: SoftwareInterrupt<'d, 2>,
     #[cfg(not(all(feature = "__esp_hal_embassy", multi_core)))]
-    /// Software interrupt 3. Only available when not using `esp-hal-embassy`,
-    /// or on single-core systems.
+    /// Software interrupt 3. Not available when using `esp-hal-embassy`,
+    /// on multi-core systems.
     pub software_interrupt3: SoftwareInterrupt<'d, 3>,
 }
 
@@ -198,6 +200,7 @@ impl<'d> SoftwareInterruptControl<'d> {
             software_interrupt1: SoftwareInterrupt {
                 _lifetime: PhantomData,
             },
+            #[cfg(not(all(feature = "__esp_wifi_builtin_scheduler", riscv)))]
             software_interrupt2: SoftwareInterrupt {
                 _lifetime: PhantomData,
             },

esp-wifi/Cargo.toml

@@ -110,7 +110,7 @@ esp-alloc = ["dep:esp-alloc"]
 sys-logs = ["esp-wifi-sys/sys-logs"]
 
 ## Use builtin scheduler
-builtin-scheduler = []
+builtin-scheduler = ["esp-hal/__esp_wifi_builtin_scheduler"]
 
 #! ### Wireless Feature Flags
 

esp-wifi/src/preempt_builtin/mod.rs

@@ -8,12 +8,13 @@ use core::{ffi::c_void, mem::MaybeUninit};
 use allocator_api2::boxed::Box;
 use arch_specific::*;
 pub(crate) use timer::setup_timer;
-use timer::{disable_multitasking, disable_timer, setup_multitasking};
+use timer::{disable_multitasking, setup_multitasking};
 
 use crate::{
     compat::malloc::InternalMemory,
     hal::{sync::Locked, trapframe::TrapFrame},
     preempt::Scheduler,
+    preempt_builtin::timer::disable_timebase,
 };
 
 struct Context {
@@ -128,11 +129,9 @@ impl Scheduler for BuiltinScheduler {
     }
 
     fn disable(&self) {
-        disable_timer();
+        disable_timebase();
         disable_multitasking();
         delete_all_tasks();
-
-        timer::TIMER.with(|timer| timer.take());
     }
 
     fn yield_task(&self) {

esp-wifi/src/preempt_builtin/timer/mod.rs

@@ -1,4 +1,9 @@
-use esp_hal::sync::Locked;
+use esp_hal::{
+    interrupt::{InterruptHandler, Priority},
+    sync::Locked,
+    time::Rate,
+    trapframe::TrapFrame,
+};
 
 #[cfg_attr(xtensa, path = "xtensa.rs")]
 #[cfg_attr(riscv, path = "riscv.rs")]
@@ -8,4 +13,51 @@ pub(crate) use arch_specific::*;
 
 use crate::TimeBase;
 
+/// The timer responsible for time slicing.
+const TIMESLICE_FREQUENCY: Rate = Rate::from_hz(crate::CONFIG.tick_rate_hz);
+
 pub(crate) static TIMER: Locked<Option<TimeBase>> = Locked::new(None);
+
+pub(crate) fn setup_timebase(mut timer: TimeBase) {
+    // The timer needs to tick at Priority 1 to prevent accidentally interrupting
+    // priority 1 limited locks.
+    let cb: extern "C" fn() = unsafe { core::mem::transmute(timer_tick_handler as *const ()) };
+    let handler = InterruptHandler::new(cb, Priority::Priority1);
+
+    timer.set_interrupt_handler(handler);
+    unwrap!(timer.start(TIMESLICE_FREQUENCY.as_duration()));
+    TIMER.with(|t| {
+        timer.enable_interrupt(true);
+        t.replace(timer);
+    });
+}
+
+pub(crate) fn clear_timer_interrupt() {
+    TIMER.with(|timer| {
+        unwrap!(timer.as_mut()).clear_interrupt();
+    });
+}
+
+pub(crate) fn disable_timebase() {
+    TIMER.with(|timer| {
+        let mut timer = unwrap!(timer.take());
+        timer.enable_interrupt(false);
+        unwrap!(timer.cancel());
+    });
+}
+
+extern "C" fn timer_tick_handler(_context: &mut TrapFrame) {
+    clear_timer_interrupt();
+
+    // `task_switch` must be called on a single interrupt priority level only.
+    // Because on ESP32 the software interrupt is triggered at priority 3 but
+    // the timer interrupt is triggered at priority 1, we need to trigger the
+    // software interrupt manually.
+    cfg_if::cfg_if! {
+        if #[cfg(esp32)] {
+            yield_task();
+        } else {
+            super::task_switch(_context);
+        }
+    }
+}

esp-wifi/src/preempt_builtin/timer/riscv.rs

@@ -1,7 +1,6 @@
-use esp_hal::interrupt::InterruptHandler;
-#[cfg(any(feature = "esp32c6", feature = "esp32h2"))]
+#[cfg(any(esp32c6, esp32h2))]
 use peripherals::INTPRI as SystemPeripheral;
-#[cfg(not(any(feature = "esp32c6", feature = "esp32h2")))]
+#[cfg(not(any(esp32c6, esp32h2)))]
 use peripherals::SYSTEM as SystemPeripheral;
 
 use crate::{
@@ -10,40 +9,20 @@ use crate::{
         interrupt::{self, TrapFrame},
         peripherals::{self, Interrupt},
         riscv,
-        time::Rate,
     },
-    preempt_builtin::task_switch,
+    preempt_builtin::{task_switch, timer::setup_timebase},
 };
 
-/// The timer responsible for time slicing.
-const TIMESLICE_FREQUENCY: Rate = Rate::from_hz(crate::CONFIG.tick_rate_hz);
-
-use super::TIMER;
-
-pub(crate) fn setup_timer(mut alarm0: TimeBase) {
+pub(crate) fn setup_timer(timer: TimeBase) {
     // make sure the scheduling won't start before everything is setup
     riscv::interrupt::disable();
 
-    let cb: extern "C" fn() = unsafe { core::mem::transmute(handler as *const ()) };
-    alarm0.set_interrupt_handler(InterruptHandler::new(cb, interrupt::Priority::Priority1));
-    unwrap!(alarm0.start(TIMESLICE_FREQUENCY.as_duration()));
-    TIMER.with(|timer| {
-        alarm0.enable_interrupt(true);
-        timer.replace(alarm0);
-    });
-}
-
-pub(crate) fn disable_timer() {
-    TIMER.with(|timer| {
-        let timer = unwrap!(timer.as_mut());
-        timer.enable_interrupt(false);
-        unwrap!(timer.cancel());
-    });
+    setup_timebase(timer);
 }
 
 pub(crate) fn setup_multitasking() {
     unwrap!(interrupt::enable(
-        Interrupt::FROM_CPU_INTR3,
+        Interrupt::FROM_CPU_INTR2,
         interrupt::Priority::Priority1,
     ));
 
@@ -53,35 +32,21 @@ pub(crate) fn setup_multitasking() {
 }
 
 pub(crate) fn disable_multitasking() {
-    interrupt::disable(crate::hal::system::Cpu::ProCpu, Interrupt::FROM_CPU_INTR3);
-}
-
-extern "C" fn handler(trap_frame: &mut TrapFrame) {
-    // clear the systimer intr
-    TIMER.with(|timer| {
-        unwrap!(timer.as_mut()).clear_interrupt();
-    });
-
-    task_switch(trap_frame);
+    interrupt::disable(crate::hal::system::Cpu::ProCpu, Interrupt::FROM_CPU_INTR2);
 }
 
 #[unsafe(no_mangle)]
-extern "C" fn FROM_CPU_INTR3(trap_frame: &mut TrapFrame) {
+extern "C" fn FROM_CPU_INTR2(trap_frame: &mut TrapFrame) {
     // clear FROM_CPU_INTR3
     SystemPeripheral::regs()
-        .cpu_intr_from_cpu_3()
-        .modify(|_, w| w.cpu_intr_from_cpu_3().clear_bit());
-
-    TIMER.with(|alarm0| {
-        let alarm0 = unwrap!(alarm0.as_mut());
-        alarm0.clear_interrupt();
-    });
+        .cpu_intr_from_cpu_2()
+        .modify(|_, w| w.cpu_intr_from_cpu_2().clear_bit());
 
     task_switch(trap_frame);
 }
 
 pub(crate) fn yield_task() {
     SystemPeripheral::regs()
-        .cpu_intr_from_cpu_3()
-        .modify(|_, w| w.cpu_intr_from_cpu_3().set_bit());
+        .cpu_intr_from_cpu_2()
+        .modify(|_, w| w.cpu_intr_from_cpu_2().set_bit());
 }

esp-wifi/src/preempt_builtin/timer/xtensa.rs

@@ -1,44 +1,13 @@
-use esp_hal::interrupt::InterruptHandler;
-
+pub(crate) use crate::preempt_builtin::timer::setup_timebase as setup_timer;
 use crate::{
-    TimeBase,
-    hal::{interrupt, time::Rate, trapframe::TrapFrame, xtensa_lx, xtensa_lx_rt},
+    hal::{trapframe::TrapFrame, xtensa_lx, xtensa_lx_rt},
     preempt_builtin::task_switch,
 };
 
-/// The timer responsible for time slicing.
-const TIMESLICE_FREQUENCY: Rate = Rate::from_hz(crate::CONFIG.tick_rate_hz);
-
-use super::TIMER;
-
 // ESP32 uses Software1 (priority 3) for task switching, because it reserves
 // Software0 for the Bluetooth stack.
 const SW_INTERRUPT: u32 = if cfg!(esp32) { 1 << 29 } else { 1 << 7 };
 
-pub(crate) fn setup_timer(mut timer1: TimeBase) {
-    // The timer needs to tick at Priority 1 to prevent accidentally interrupting
-    // priority 1 limited locks.
-    timer1.set_interrupt_handler(InterruptHandler::new(
-        unsafe {
-            core::mem::transmute::<*const (), extern "C" fn()>(timer_tick_handler as *const ())
-        },
-        interrupt::Priority::Priority1,
-    ));
-    unwrap!(timer1.start(TIMESLICE_FREQUENCY.as_duration()));
-    TIMER.with(|timer| {
-        timer1.enable_interrupt(true);
-        timer.replace(timer1);
-    });
-}
-
-pub(crate) fn disable_timer() {
-    TIMER.with(|timer| {
-        let timer = unwrap!(timer.as_mut());
-        timer.enable_interrupt(false);
-        unwrap!(timer.cancel());
-    });
-}
-
 pub(crate) fn setup_multitasking() {
     unsafe {
         let enabled = xtensa_lx::interrupt::disable();
@@ -55,25 +24,6 @@ pub(crate) fn disable_multitasking() {
     xtensa_lx::interrupt::disable_mask(SW_INTERRUPT);
 }
 
-extern "C" fn timer_tick_handler(_context: &mut TrapFrame) {
-    TIMER.with(|timer| {
-        let timer = unwrap!(timer.as_mut());
-        timer.clear_interrupt();
-    });
-
-    // `task_switch` must be called on a single interrupt priority level only.
-    // Because on ESP32 the software interrupt is triggered at priority 3 but
-    // the timer interrupt is triggered at priority 1, we need to trigger the
-    // software interrupt manually.
-    cfg_if::cfg_if! {
-        if #[cfg(esp32)] {
-            yield_task();
-        } else {
-            task_switch(_context);
-        }
-    }
-}
-
 #[allow(non_snake_case)]
 #[cfg_attr(not(esp32), unsafe(export_name = "Software0"))]
 #[cfg_attr(esp32, unsafe(export_name = "Software1"))]