From 5992933ca7dd44a8d3f4fa42c914997458d55919 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Fri, 30 May 2025 13:35:59 +0200 Subject: [PATCH] Simplify esp-wifi timer code (#3576) * Simplify esp-wifi timer code * Reserve and use FROM_CPU_INTR2 on RISC-V --- esp-hal/CHANGELOG.md | 1 + esp-hal/Cargo.toml | 3 + esp-hal/src/interrupt/software.rs | 9 ++- esp-wifi/Cargo.toml | 2 +- esp-wifi/src/preempt_builtin/mod.rs | 7 +-- esp-wifi/src/preempt_builtin/timer/mod.rs | 54 +++++++++++++++++- esp-wifi/src/preempt_builtin/timer/riscv.rs | 59 ++++---------------- esp-wifi/src/preempt_builtin/timer/xtensa.rs | 54 +----------------- 8 files changed, 81 insertions(+), 108 deletions(-) diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index 1650ba0bc..85cd6e09f 100644 --- a/esp-hal/CHANGELOG.md +++ b/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 diff --git a/esp-hal/Cargo.toml b/esp-hal/Cargo.toml index 00920cf78..f749344eb 100644 --- a/esp-hal/Cargo.toml +++ b/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", diff --git a/esp-hal/src/interrupt/software.rs b/esp-hal/src/interrupt/software.rs index 02dc92606..dd7a0723b 100644 --- a/esp-hal/src/interrupt/software.rs +++ b/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, }, diff --git a/esp-wifi/Cargo.toml b/esp-wifi/Cargo.toml index dbeed985f..ded543f79 100644 --- a/esp-wifi/Cargo.toml +++ b/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 diff --git a/esp-wifi/src/preempt_builtin/mod.rs b/esp-wifi/src/preempt_builtin/mod.rs index 8db2f1b9c..558800a24 100644 --- a/esp-wifi/src/preempt_builtin/mod.rs +++ b/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) { diff --git a/esp-wifi/src/preempt_builtin/timer/mod.rs b/esp-wifi/src/preempt_builtin/timer/mod.rs index 52003467c..52bcc0fc7 100644 --- a/esp-wifi/src/preempt_builtin/timer/mod.rs +++ b/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> = 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); + } + } +} diff --git a/esp-wifi/src/preempt_builtin/timer/riscv.rs b/esp-wifi/src/preempt_builtin/timer/riscv.rs index fc792e557..cb58ce112 100644 --- a/esp-wifi/src/preempt_builtin/timer/riscv.rs +++ b/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()); } diff --git a/esp-wifi/src/preempt_builtin/timer/xtensa.rs b/esp-wifi/src/preempt_builtin/timer/xtensa.rs index 29f308086..3260983e5 100644 --- a/esp-wifi/src/preempt_builtin/timer/xtensa.rs +++ b/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"))]