Systimer improvements (#2451)

* Do not read to set update bit

* Deduplicate

* Try to bind interrupts to the correct core

* Inline poll_count into read_count

* Clean up

* Make sure only a single update is done at a time

* Changelog

* Fix docs

* Correct the channel count

* Assign enough timers for HIL test

* Use a lock to prevent re-update

* Remove locking, use esp-idf implementation

* Document timer count requirement

esp-hal-embassy/CHANGELOG.md

@@ -17,6 +17,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- Alarm interrupts are now handled on the core that allocated them. (For executors created on the second core after calling `esp_hal_embassy::init`) (#2451)
+
 ### Removed
 
 ## 0.4.0 - 2024-10-10

esp-hal-embassy/src/lib.rs

@@ -149,6 +149,9 @@ impl_array!(4);
 /// - A mutable static array of `OneShotTimer` instances
 /// - A 2, 3, 4 element array of `AnyTimer` instances
 ///
+/// Note that if you use the `integrated-timers` feature,
+/// you need to pass as many timers as you start executors.
+///
 /// # Examples
 ///
 /// ```rust, no_run

esp-hal-embassy/src/time_driver.rs

@@ -49,48 +49,20 @@ impl EmbassyTimer {
             );
         }
 
-        static HANDLERS: [InterruptHandler; MAX_SUPPORTED_ALARM_COUNT] = [
-            handler0, handler1, handler2, handler3, handler4, handler5, handler6,
-        ];
-
         critical_section::with(|cs| {
             timers.iter_mut().enumerate().for_each(|(n, timer)| {
                 timer.enable_interrupt(false);
                 timer.stop();
+
+                if DRIVER.alarms.borrow(cs)[n].allocated.get() {
+                    // FIXME: we should track which core allocated an alarm and bind the interrupt
+                    // to that core.
                     timer.set_interrupt_handler(HANDLERS[n]);
+                }
             });
 
             TIMERS.replace(cs, Some(timers));
         });
-
-        #[handler(priority = Priority::max())]
-        fn handler0() {
-            DRIVER.on_interrupt(0);
-        }
-        #[handler(priority = Priority::max())]
-        fn handler1() {
-            DRIVER.on_interrupt(1);
-        }
-        #[handler(priority = Priority::max())]
-        fn handler2() {
-            DRIVER.on_interrupt(2);
-        }
-        #[handler(priority = Priority::max())]
-        fn handler3() {
-            DRIVER.on_interrupt(3);
-        }
-        #[handler(priority = Priority::max())]
-        fn handler4() {
-            DRIVER.on_interrupt(4);
-        }
-        #[handler(priority = Priority::max())]
-        fn handler5() {
-            DRIVER.on_interrupt(5);
-        }
-        #[handler(priority = Priority::max())]
-        fn handler6() {
-            DRIVER.on_interrupt(6);
-        }
     }
 
     fn on_interrupt(&self, id: usize) {
@@ -128,12 +100,27 @@ impl Driver for EmbassyTimer {
     unsafe fn allocate_alarm(&self) -> Option<AlarmHandle> {
         critical_section::with(|cs| {
             for (i, alarm) in self.alarms.borrow(cs).iter().enumerate() {
-                if !alarm.allocated.get() {
+                if alarm.allocated.get() {
+                    continue;
+                }
+                let mut timer = TIMERS.borrow_ref_mut(cs);
+                // `allocate_alarm` may be called before `esp_hal_embassy::init()`, so
+                // we need to check if we have timers.
+                if let Some(timer) = &mut *timer {
+                    // If we do, bind the interrupt handler to the timer.
+                    // This ensures that alarms allocated after init are correctly bound to the
+                    // core that created the executor.
+                    let timer = unwrap!(
+                        timer.get_mut(i),
+                        "There are not enough timers to allocate a new alarm. Call `esp_hal_embassy::init()` with the correct number of timers."
+                    );
+                    timer.set_interrupt_handler(HANDLERS[i]);
+                }
+
                 // set alarm so it is not overwritten
                 alarm.allocated.set(true);
                 return Some(AlarmHandle::new(i as u8));
             }
-            }
             None
         })
     }
@@ -172,3 +159,36 @@ impl Driver for EmbassyTimer {
         true
     }
 }
+
+static HANDLERS: [InterruptHandler; MAX_SUPPORTED_ALARM_COUNT] = [
+    handler0, handler1, handler2, handler3, handler4, handler5, handler6,
+];
+
+#[handler(priority = Priority::max())]
+fn handler0() {
+    DRIVER.on_interrupt(0);
+}
+#[handler(priority = Priority::max())]
+fn handler1() {
+    DRIVER.on_interrupt(1);
+}
+#[handler(priority = Priority::max())]
+fn handler2() {
+    DRIVER.on_interrupt(2);
+}
+#[handler(priority = Priority::max())]
+fn handler3() {
+    DRIVER.on_interrupt(3);
+}
+#[handler(priority = Priority::max())]
+fn handler4() {
+    DRIVER.on_interrupt(4);
+}
+#[handler(priority = Priority::max())]
+fn handler5() {
+    DRIVER.on_interrupt(5);
+}
+#[handler(priority = Priority::max())]
+fn handler6() {
+    DRIVER.on_interrupt(6);
+}

esp-hal/CHANGELOG.md

@@ -53,6 +53,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - 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)
 - Fixed `TWAI::transmit_async`: bus-off state is not reached when CANH and CANL are shorted. (#2421)
 - ESP32: added UART-specific workaround for https://docs.espressif.com/projects/esp-chip-errata/en/latest/esp32/03-errata-description/esp32/cpu-subsequent-access-halted-when-get-interrupted.html (#2441)
+- Fixed some SysTimer race conditions and panics (#2451)
 
 ### Removed
 

esp-hal/src/timer/systimer.rs

@@ -170,13 +170,7 @@ impl<'d> SystemTimer<'d> {
         // an older time stamp
 
         let unit = unsafe { SpecificUnit::<'_, 0>::conjure() };
-
-        unit.update();
-        loop {
-            if let Some(value) = unit.poll_count() {
-                break value;
-            }
-        }
+        unit.read_count()
     }
 }
 
@@ -248,37 +242,29 @@ pub trait Unit {
                     _ => unreachable!(),
                 },
                 UnitConfig::DisabledIfCpuIsStalled(cpu) => match self.channel() {
-                    0 => w
-                        .timer_unit0_work_en()
-                        .set_bit()
-                        .timer_unit0_core0_stall_en()
-                        .bit(cpu == Cpu::ProCpu)
-                        .timer_unit0_core1_stall_en()
-                        .bit(cpu != Cpu::ProCpu),
-                    1 => w
-                        .timer_unit1_work_en()
-                        .set_bit()
-                        .timer_unit1_core0_stall_en()
-                        .bit(cpu == Cpu::ProCpu)
-                        .timer_unit1_core1_stall_en()
-                        .bit(cpu != Cpu::ProCpu),
+                    0 => {
+                        w.timer_unit0_work_en().set_bit();
+                        w.timer_unit0_core0_stall_en().bit(cpu == Cpu::ProCpu);
+                        w.timer_unit0_core1_stall_en().bit(cpu != Cpu::ProCpu)
+                    }
+                    1 => {
+                        w.timer_unit1_work_en().set_bit();
+                        w.timer_unit1_core0_stall_en().bit(cpu == Cpu::ProCpu);
+                        w.timer_unit1_core1_stall_en().bit(cpu != Cpu::ProCpu)
+                    }
                     _ => unreachable!(),
                 },
                 UnitConfig::Enabled => match self.channel() {
-                    0 => w
-                        .timer_unit0_work_en()
-                        .set_bit()
-                        .timer_unit0_core0_stall_en()
-                        .clear_bit()
-                        .timer_unit0_core1_stall_en()
-                        .clear_bit(),
-                    1 => w
-                        .timer_unit1_work_en()
-                        .set_bit()
-                        .timer_unit1_core0_stall_en()
-                        .clear_bit()
-                        .timer_unit1_core1_stall_en()
-                        .clear_bit(),
+                    0 => {
+                        w.timer_unit0_work_en().set_bit();
+                        w.timer_unit0_core0_stall_en().clear_bit();
+                        w.timer_unit0_core1_stall_en().clear_bit()
+                    }
+                    1 => {
+                        w.timer_unit1_work_en().set_bit();
+                        w.timer_unit1_core0_stall_en().clear_bit();
+                        w.timer_unit1_core1_stall_en().clear_bit()
+                    }
                     _ => unreachable!(),
                 },
             });
@@ -317,48 +303,30 @@ pub trait Unit {
         }
     }
 
-    /// Update the value returned by [Self::poll_count] to be the current value
-    /// of the counter.
-    ///
-    /// This can be used to read the current value of the timer.
-    fn update(&self) {
-        let systimer = unsafe { &*SYSTIMER::ptr() };
-        systimer
-            .unit_op(self.channel() as _)
-            .modify(|_, w| w.update().set_bit());
-    }
-
-    /// Return the count value at the time of the last call to [Self::update].
-    ///
-    /// Returns None if the update isn't ready to read if update has never been
-    /// called.
-    fn poll_count(&self) -> Option<u64> {
-        let systimer = unsafe { &*SYSTIMER::ptr() };
-        if systimer
-            .unit_op(self.channel() as _)
-            .read()
-            .value_valid()
-            .bit_is_set()
-        {
-            let unit_value = systimer.unit_value(self.channel() as _);
-
-            let lo = unit_value.lo().read().bits();
-            let hi = unit_value.hi().read().bits();
-
-            Some(((hi as u64) << 32) | lo as u64)
-        } else {
-            None
-        }
-    }
-
-    /// Convenience method to call [Self::update] and [Self::poll_count].
+    /// Reads the current counter value.
     fn read_count(&self) -> u64 {
         // This can be a shared reference as long as this type isn't Sync.
 
-        self.update();
+        let channel = self.channel() as usize;
+        let systimer = unsafe { SYSTIMER::steal() };
+
+        systimer.unit_op(channel).write(|w| w.update().set_bit());
+        while !systimer.unit_op(channel).read().value_valid().bit_is_set() {}
+
+        // Read LO, HI, then LO again, check that LO returns the same value.
+        // This accounts for the case when an interrupt may happen between reading
+        // HI and LO values (or the other core updates the counter mid-read), and this
+        // function may get called from the ISR. In this case, the repeated read
+        // will return consistent values.
+        let unit_value = systimer.unit_value(channel);
+        let mut lo_prev = unit_value.lo().read().bits();
         loop {
-            if let Some(count) = self.poll_count() {
-                break count;
+            let lo = lo_prev;
+            let hi = unit_value.hi().read().bits();
+            lo_prev = unit_value.lo().read().bits();
+
+            if lo == lo_prev {
+                return ((hi as u64) << 32) | lo as u64;
             }
         }
     }
@@ -895,13 +863,7 @@ where
         // This should be safe to access from multiple contexts; worst case
         // scenario the second accessor ends up reading an older time stamp.
 
-        self.unit.update();
-
-        let ticks = loop {
-            if let Some(value) = self.unit.poll_count() {
-                break value;
-            }
-        };
+        let ticks = self.unit.read_count();
 
         let us = ticks / (SystemTimer::ticks_per_second() / 1_000_000);
 
@@ -933,11 +895,6 @@ where
         } else {
             // Target mode
 
-            self.unit.update();
-            while self.unit.poll_count().is_none() {
-                // Wait for value registers to update
-            }
-
             // The counters/comparators are 52-bits wide (except on ESP32-S2,
             // which is 64-bits), so we must ensure that the provided value
             // is not too wide:
@@ -946,7 +903,7 @@ where
                 return Err(Error::InvalidTimeout);
             }
 
-            let v = self.unit.poll_count().unwrap();
+            let v = self.unit.read_count();
             let t = v + ticks;
 
             self.comparator.set_target(t);

hil-test/tests/embassy_interrupt_executor.rs

@@ -16,7 +16,7 @@ use esp_hal::{
         software::{SoftwareInterrupt, SoftwareInterruptControl},
         Priority,
     },
-    timer::timg::TimerGroup,
+    timer::AnyTimer,
 };
 use esp_hal_embassy::InterruptExecutor;
 use hil_test as _;
@@ -56,8 +56,31 @@ mod test {
     fn init() -> Context {
         let peripherals = esp_hal::init(esp_hal::Config::default());
 
+        cfg_if::cfg_if! {
+            if #[cfg(timg_timer1)] {
+                use esp_hal::timer::timg::TimerGroup;
                 let timg0 = TimerGroup::new(peripherals.TIMG0);
-        esp_hal_embassy::init(timg0.timer0);
+                esp_hal_embassy::init([
+                    AnyTimer::from(timg0.timer0),
+                    AnyTimer::from(timg0.timer1),
+                ]);
+            } else if #[cfg(timg1)] {
+                use esp_hal::timer::timg::TimerGroup;
+                let timg0 = TimerGroup::new(peripherals.TIMG0);
+                let timg1 = TimerGroup::new(peripherals.TIMG1);
+                esp_hal_embassy::init([
+                    AnyTimer::from(timg0.timer0),
+                    AnyTimer::from(timg1.timer0),
+                ]);
+            } else if #[cfg(systimer)] {
+                use esp_hal::timer::systimer::{SystemTimer, Target};
+                let systimer = SystemTimer::new(peripherals.SYSTIMER).split::<Target>();
+                esp_hal_embassy::init([
+                    AnyTimer::from(systimer.alarm0),
+                    AnyTimer::from(systimer.alarm1),
+                ]);
+            }
+        }
 
         let sw_ints = SoftwareInterruptControl::new(peripherals.SW_INTERRUPT);
         Context {

hil-test/tests/embassy_interrupt_spi_dma.rs

@@ -17,7 +17,7 @@ use esp_hal::{
         master::{Spi, SpiDma},
         SpiMode,
     },
-    timer::{timg::TimerGroup, AnyTimer},
+    timer::AnyTimer,
     Async,
 };
 use esp_hal_embassy::InterruptExecutor;
@@ -60,12 +60,26 @@ mod test {
     #[timeout(3)]
     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);
-        esp_hal_embassy::init([AnyTimer::from(timg0.timer0), AnyTimer::from(timg0.timer1)]);
-
         let dma = Dma::new(peripherals.DMA);
 
+        cfg_if::cfg_if! {
+            if #[cfg(systimer)] {
+                use esp_hal::timer::systimer::{SystemTimer, Target};
+                let systimer = SystemTimer::new(peripherals.SYSTIMER).split::<Target>();
+                esp_hal_embassy::init([
+                    AnyTimer::from(systimer.alarm0),
+                    AnyTimer::from(systimer.alarm1),
+                ]);
+            } else {
+                use esp_hal::timer::timg::TimerGroup;
+                let timg0 = TimerGroup::new(peripherals.TIMG0);
+                esp_hal_embassy::init([
+                    AnyTimer::from(timg0.timer0),
+                    AnyTimer::from(timg0.timer1),
+                ]);
+            }
+        }
+
         cfg_if::cfg_if! {
             if #[cfg(pdma)] {
                 let dma_channel1 = dma.spi2channel;
@@ -164,8 +178,23 @@ mod test {
         let peripherals = esp_hal::init(esp_hal::Config::default());
         let dma = Dma::new(peripherals.DMA);
 
+        cfg_if::cfg_if! {
+            if #[cfg(systimer)] {
+                use esp_hal::timer::systimer::{SystemTimer, Target};
+                let systimer = SystemTimer::new(peripherals.SYSTIMER).split::<Target>();
+                esp_hal_embassy::init([
+                    AnyTimer::from(systimer.alarm0),
+                    AnyTimer::from(systimer.alarm1),
+                ]);
+            } else {
+                use esp_hal::timer::timg::TimerGroup;
                 let timg0 = TimerGroup::new(peripherals.TIMG0);
-        esp_hal_embassy::init(timg0.timer0);
+                esp_hal_embassy::init([
+                    AnyTimer::from(timg0.timer0),
+                    AnyTimer::from(timg0.timer1),
+                ]);
+            }
+        }
 
         cfg_if::cfg_if! {
             if #[cfg(pdma)] {