Fix S2 systimers

esp-hal/CHANGELOG.md

@@ -41,6 +41,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - ESP32C6: Make ADC usable after TRNG deinicialization (#1945)
 - We should no longer generate 1GB .elf files for ESP32C2 and ESP32C3 (#1962)
 - Reset peripherals in driver constructors where missing (#1893, #1961)
+- Fixed ESP32-S2 systimer interrupts (#1979)
 
 ### Removed
 

esp-hal/src/interrupt/mod.rs

@@ -225,10 +225,6 @@ impl Iterator for InterruptStatusIterator {
     type Item = u8;
 
     fn next(&mut self) -> Option<Self::Item> {
-        if self.idx == usize::MAX {
-            return None;
-        }
-
         for i in self.idx..STATUS_WORDS {
             if self.status.status[i] != 0 {
                 let bit = self.status.status[i].trailing_zeros();

esp-hal/src/interrupt/xtensa.rs

@@ -1,6 +1,6 @@
 //! Interrupt handling
 
-use xtensa_lx::interrupt::{self, InterruptNumber};
+use xtensa_lx::interrupt;
 use xtensa_lx_rt::exception::Context;
 
 pub use self::vectored::*;
@@ -502,7 +502,7 @@ mod vectored {
             fn EspDefaultHandler(level: u32, interrupt: Interrupt);
         }
 
-        let handler = peripherals::__INTERRUPTS[interrupt.number() as usize]._handler;
+        let handler = peripherals::__INTERRUPTS[interrupt as usize]._handler;
         if core::ptr::eq(
             handler as *const _,
             EspDefaultHandler as *const unsafe extern "C" fn(),
@@ -520,9 +520,9 @@ mod vectored {
     mod chip_specific {
         use super::*;
         pub const INTERRUPT_EDGE: InterruptStatus = InterruptStatus::from(
-            0b0000_0000_0000_0000_0000_0000_0000_0011,
-            0b1111_1100_0000_0000_0000_0000_0000_0000,
             0b0000_0000_0000_0000_0000_0000_0000_0000,
+            0b1111_1100_0000_0000_0000_0000_0000_0000,
+            0b0000_0000_0000_0000_0000_0000_0000_0011,
         );
         #[inline]
         pub fn interrupt_is_edge(interrupt: Interrupt) -> bool {
@@ -541,14 +541,13 @@ mod vectored {
         }
     }
 
-    #[allow(clippy::unusual_byte_groupings)]
     #[cfg(esp32s2)]
     mod chip_specific {
         use super::*;
         pub const INTERRUPT_EDGE: InterruptStatus = InterruptStatus::from(
-            0b0000_0000_0000_0000_0000_0011_1011_1111,
-            0b1100_0000_0000_0000_0000_0000_0000_0000,
             0b0000_0000_0000_0000_0000_0000_0000_0000,
+            0b1100_0000_0000_0000_0000_0000_0000_0000,
+            0b0000_0000_0000_0000_0000_0011_1011_1111,
         );
         #[inline]
         pub fn interrupt_is_edge(interrupt: Interrupt) -> bool {

esp-hal/src/timer/systimer.rs

@@ -396,20 +396,20 @@ pub trait Comparator {
     fn set_enable(&self, enable: bool) {
         let systimer = unsafe { &*SYSTIMER::ptr() };
 
-        #[cfg(not(esp32s2))]
         critical_section::with(|_| {
+            #[cfg(not(esp32s2))]
             systimer.conf().modify(|_, w| match self.channel() {
                 0 => w.target0_work_en().bit(enable),
                 1 => w.target1_work_en().bit(enable),
                 2 => w.target2_work_en().bit(enable),
                 _ => unreachable!(),
             });
-        });
 
-        #[cfg(esp32s2)]
-        systimer
-            .target_conf(self.channel() as usize)
-            .modify(|_r, w| w.work_en().bit(enable));
+            #[cfg(esp32s2)]
+            systimer
+                .target_conf(self.channel() as usize)
+                .modify(|_r, w| w.work_en().bit(enable));
+        });
     }
 
     /// Returns true if the comparator has been enabled. This means
@@ -524,9 +524,48 @@ pub trait Comparator {
             2 => Interrupt::SYSTIMER_TARGET2,
             _ => unreachable!(),
         };
+
+        #[cfg(not(esp32s2))]
         unsafe {
             interrupt::bind_interrupt(interrupt, handler.handler());
         }
+
+        #[cfg(esp32s2)]
+        {
+            // ESP32-S2 Systimer interrupts are edge triggered. Our interrupt
+            // handler calls each of the handlers, regardless of which one triggered the
+            // interrupt. This mess registers an intermediate handler that
+            // checks if an interrupt is active before calling the associated
+            // handler functions.
+
+            static mut HANDLERS: [Option<extern "C" fn()>; 3] = [None, None, None];
+
+            #[crate::prelude::ram]
+            unsafe extern "C" fn _handle_interrupt<const CH: u8>() {
+                if unsafe { &*SYSTIMER::PTR }
+                    .int_raw()
+                    .read()
+                    .target(CH)
+                    .bit_is_set()
+                {
+                    let handler = unsafe { HANDLERS[CH as usize] };
+                    if let Some(handler) = handler {
+                        handler();
+                    }
+                }
+            }
+
+            unsafe {
+                HANDLERS[self.channel() as usize] = Some(handler.handler());
+                let handler = match self.channel() {
+                    0 => _handle_interrupt::<0>,
+                    1 => _handle_interrupt::<1>,
+                    2 => _handle_interrupt::<2>,
+                    _ => unreachable!(),
+                };
+                interrupt::bind_interrupt(interrupt, handler);
+            }
+        }
         unwrap!(interrupt::enable(interrupt, handler.priority()));
     }
 }

examples/src/bin/systimer.rs

@@ -14,8 +14,7 @@ use esp_backtrace as _;
 use esp_hal::{
     clock::ClockControl,
     delay::Delay,
-    interrupt::{self, Priority},
-    peripherals::{Interrupt, Peripherals},
+    peripherals::Peripherals,
     prelude::*,
     system::SystemControl,
     timer::systimer::{
@@ -87,10 +86,6 @@ fn main() -> ! {
         ALARM2.borrow_ref_mut(cs).replace(alarm2);
     });
 
-    interrupt::enable(Interrupt::SYSTIMER_TARGET0, Priority::Priority1).unwrap();
-    interrupt::enable(Interrupt::SYSTIMER_TARGET1, Priority::Priority3).unwrap();
-    interrupt::enable(Interrupt::SYSTIMER_TARGET2, Priority::Priority3).unwrap();
-
     let delay = Delay::new(&clocks);
 
     loop {

hil-test/Cargo.toml

@@ -87,6 +87,10 @@ harness = false
 name    = "spi_half_duplex_write"
 harness = false
 
+[[test]]
+name    = "systimer"
+harness = false
+
 [[test]]
 name    = "pcnt"
 harness = false
@@ -163,6 +167,7 @@ elliptic-curve      = { version = "0.13.8", default-features = false, features =
 embassy-executor    = { version = "0.6.0", default-features = false }
 # Add the `embedded-test/defmt` feature for more verbose testing
 embedded-test       = { version = "0.4.0", default-features = false }
+fugit               = "0.3.7"
 hex-literal         = "0.4.1"
 nb                  = "1.1.0"
 p192                = { version = "0.13.0", default-features = false, features = ["arithmetic"] }

hil-test/tests/systimer.rs

@@ -0,0 +1,195 @@
+//! System Timer Test
+
+// esp32 disabled as it does not have a systimer
+//% CHIPS: esp32c2 esp32c3 esp32c6 esp32h2 esp32s2 esp32s3
+
+#![no_std]
+#![no_main]
+
+use core::cell::RefCell;
+
+use critical_section::Mutex;
+use embedded_hal::delay::DelayNs;
+use esp_hal::{
+    clock::{ClockControl, Clocks},
+    delay::Delay,
+    peripherals::Peripherals,
+    prelude::*,
+    system::SystemControl,
+    timer::systimer::{
+        Alarm,
+        FrozenUnit,
+        Periodic,
+        SpecificComparator,
+        SpecificUnit,
+        SystemTimer,
+        Target,
+    },
+    Blocking,
+};
+use fugit::ExtU32;
+use hil_test as _;
+use portable_atomic::{AtomicUsize, Ordering};
+use static_cell::StaticCell;
+
+type TestAlarm<M, const C: u8> =
+    Alarm<'static, M, Blocking, SpecificComparator<'static, C>, SpecificUnit<'static, 0>>;
+
+static ALARM_TARGET: Mutex<RefCell<Option<TestAlarm<Target, 0>>>> = Mutex::new(RefCell::new(None));
+static ALARM_PERIODIC: Mutex<RefCell<Option<TestAlarm<Periodic, 1>>>> =
+    Mutex::new(RefCell::new(None));
+
+struct Context {
+    unit: FrozenUnit<'static, SpecificUnit<'static, 0>>,
+    comparator0: SpecificComparator<'static, 0>,
+    comparator1: SpecificComparator<'static, 1>,
+    clocks: Clocks<'static>,
+}
+
+impl Context {
+    pub fn init() -> Self {
+        let peripherals = Peripherals::take();
+        let system = SystemControl::new(peripherals.SYSTEM);
+        let clocks = ClockControl::boot_defaults(system.clock_control).freeze();
+
+        let systimer = SystemTimer::new(peripherals.SYSTIMER);
+        static UNIT0: StaticCell<SpecificUnit<'static, 0>> = StaticCell::new();
+
+        let unit0 = UNIT0.init(systimer.unit0);
+        let frozen_unit = FrozenUnit::new(unit0);
+
+        Context {
+            clocks,
+            unit: frozen_unit,
+            comparator0: systimer.comparator0,
+            comparator1: systimer.comparator1,
+        }
+    }
+}
+
+#[handler(priority = esp_hal::interrupt::Priority::min())]
+fn pass_test_if_called() {
+    critical_section::with(|cs| {
+        ALARM_TARGET
+            .borrow_ref_mut(cs)
+            .as_mut()
+            .unwrap()
+            .clear_interrupt()
+    });
+    embedded_test::export::check_outcome(());
+}
+
+#[handler(priority = esp_hal::interrupt::Priority::min())]
+fn handle_periodic_interrupt() {
+    critical_section::with(|cs| {
+        ALARM_PERIODIC
+            .borrow_ref_mut(cs)
+            .as_mut()
+            .unwrap()
+            .clear_interrupt()
+    });
+}
+
+static COUNTER: AtomicUsize = AtomicUsize::new(0);
+
+#[handler(priority = esp_hal::interrupt::Priority::min())]
+fn pass_test_if_called_twice() {
+    critical_section::with(|cs| {
+        ALARM_PERIODIC
+            .borrow_ref_mut(cs)
+            .as_mut()
+            .unwrap()
+            .clear_interrupt()
+    });
+    COUNTER.fetch_add(1, Ordering::Relaxed);
+    if COUNTER.load(Ordering::Relaxed) == 2 {
+        embedded_test::export::check_outcome(());
+    }
+}
+
+#[handler(priority = esp_hal::interrupt::Priority::min())]
+fn target_fail_test_if_called_twice() {
+    critical_section::with(|cs| {
+        ALARM_TARGET
+            .borrow_ref_mut(cs)
+            .as_mut()
+            .unwrap()
+            .clear_interrupt()
+    });
+    COUNTER.fetch_add(1, Ordering::Relaxed);
+    assert!(COUNTER.load(Ordering::Relaxed) != 2);
+}
+
+#[cfg(test)]
+#[embedded_test::tests]
+mod tests {
+    use super::*;
+
+    #[init]
+    fn init() -> Context {
+        Context::init()
+    }
+
+    #[test]
+    #[timeout(3)]
+    fn target_interrupt_is_handled(ctx: Context) {
+        let alarm0 = Alarm::new(ctx.comparator0, &ctx.unit);
+
+        critical_section::with(|cs| {
+            alarm0.set_interrupt_handler(pass_test_if_called);
+            alarm0.set_target(SystemTimer::now() + SystemTimer::TICKS_PER_SECOND / 10);
+            alarm0.enable_interrupt(true);
+
+            ALARM_TARGET.borrow_ref_mut(cs).replace(alarm0);
+        });
+
+        // We'll end the test in the interrupt handler.
+        loop {}
+    }
+
+    #[test]
+    #[timeout(3)]
+    fn target_interrupt_is_handled_once(ctx: Context) {
+        let alarm0 = Alarm::new(ctx.comparator0, &ctx.unit);
+        let alarm1 = Alarm::new(ctx.comparator1, &ctx.unit);
+
+        COUNTER.store(0, Ordering::Relaxed);
+
+        critical_section::with(|cs| {
+            alarm0.set_interrupt_handler(target_fail_test_if_called_twice);
+            alarm0.set_target(SystemTimer::now() + SystemTimer::TICKS_PER_SECOND / 10);
+            alarm0.enable_interrupt(true);
+
+            let alarm1 = alarm1.into_periodic();
+            alarm1.set_interrupt_handler(handle_periodic_interrupt);
+            alarm1.set_period(100u32.millis());
+            alarm1.enable_interrupt(true);
+
+            ALARM_TARGET.borrow_ref_mut(cs).replace(alarm0);
+            ALARM_PERIODIC.borrow_ref_mut(cs).replace(alarm1);
+        });
+
+        let mut delay = Delay::new(&ctx.clocks);
+        delay.delay_ms(300);
+    }
+
+    #[test]
+    #[timeout(3)]
+    fn periodic_interrupt_is_handled(ctx: Context) {
+        let alarm1 = Alarm::new(ctx.comparator1, &ctx.unit);
+
+        COUNTER.store(0, Ordering::Relaxed);
+
+        critical_section::with(|cs| {
+            let alarm1 = alarm1.into_periodic();
+            alarm1.set_interrupt_handler(pass_test_if_called_twice);
+            alarm1.set_period(100u32.millis());
+            alarm1.enable_interrupt(true);
+
+            ALARM_PERIODIC.borrow_ref_mut(cs).replace(alarm1);
+        });
+
+        // We'll end the test in the interrupt handler.
+        loop {}
+    }
+}