From 1e6820d1a7405c738c402aef639f9e49072ff246 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Mon, 4 Nov 2024 12:36:34 +0100 Subject: [PATCH] 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 | 2 + esp-hal-embassy/src/lib.rs | 3 + esp-hal-embassy/src/time_driver.rs | 96 ++++++++------ esp-hal/CHANGELOG.md | 1 + esp-hal/src/timer/systimer.rs | 129 +++++++------------ hil-test/tests/embassy_interrupt_executor.rs | 29 ++++- hil-test/tests/embassy_interrupt_spi_dma.rs | 43 ++++++- 7 files changed, 169 insertions(+), 134 deletions(-) diff --git a/esp-hal-embassy/CHANGELOG.md b/esp-hal-embassy/CHANGELOG.md index 95d69b3dd..3449b640e 100644 --- a/esp-hal-embassy/CHANGELOG.md +++ b/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 diff --git a/esp-hal-embassy/src/lib.rs b/esp-hal-embassy/src/lib.rs index 9b8a8980a..795f4cf13 100644 --- a/esp-hal-embassy/src/lib.rs +++ b/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 diff --git a/esp-hal-embassy/src/time_driver.rs b/esp-hal-embassy/src/time_driver.rs index 997080778..6b73f8001 100644 --- a/esp-hal-embassy/src/time_driver.rs +++ b/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(); - timer.set_interrupt_handler(HANDLERS[n]); + + 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,11 +100,26 @@ impl Driver for EmbassyTimer { unsafe fn allocate_alarm(&self) -> Option { critical_section::with(|cs| { for (i, alarm) in self.alarms.borrow(cs).iter().enumerate() { - if !alarm.allocated.get() { - // set alarm so it is not overwritten - alarm.allocated.set(true); - return Some(AlarmHandle::new(i as u8)); + 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); +} diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index f4ada3c31..f742791b5 100644 --- a/esp-hal/CHANGELOG.md +++ b/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 diff --git a/esp-hal/src/timer/systimer.rs b/esp-hal/src/timer/systimer.rs index 2a666c652..fb3fdcb19 100644 --- a/esp-hal/src/timer/systimer.rs +++ b/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 { - 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); diff --git a/hil-test/tests/embassy_interrupt_executor.rs b/hil-test/tests/embassy_interrupt_executor.rs index 4aad1a386..c21fed86d 100644 --- a/hil-test/tests/embassy_interrupt_executor.rs +++ b/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()); - let timg0 = TimerGroup::new(peripherals.TIMG0); - esp_hal_embassy::init(timg0.timer0); + cfg_if::cfg_if! { + if #[cfg(timg_timer1)] { + use esp_hal::timer::timg::TimerGroup; + let timg0 = TimerGroup::new(peripherals.TIMG0); + 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::(); + esp_hal_embassy::init([ + AnyTimer::from(systimer.alarm0), + AnyTimer::from(systimer.alarm1), + ]); + } + } let sw_ints = SoftwareInterruptControl::new(peripherals.SW_INTERRUPT); Context { diff --git a/hil-test/tests/embassy_interrupt_spi_dma.rs b/hil-test/tests/embassy_interrupt_spi_dma.rs index 9ff60bfdd..fc6580a12 100644 --- a/hil-test/tests/embassy_interrupt_spi_dma.rs +++ b/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::(); + 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); - let timg0 = TimerGroup::new(peripherals.TIMG0); - esp_hal_embassy::init(timg0.timer0); + cfg_if::cfg_if! { + if #[cfg(systimer)] { + use esp_hal::timer::systimer::{SystemTimer, Target}; + let systimer = SystemTimer::new(peripherals.SYSTIMER).split::(); + 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)] {