From 780a7f5e4afaf65506903185529425cf2f615017 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Tue, 7 Nov 2023 15:34:14 +0100 Subject: [PATCH] Fix `us` waits in embassy hanging (#906) * Remove unused field from AlarmState * Clean up non-idiomatic code * Simplify timg0 time driver * Better explain instant firing, hide fn, re-add check * Add changelog entry --- CHANGELOG.md | 1 + esp-hal-common/src/embassy/mod.rs | 19 ++++----- .../src/embassy/time_driver_systimer.rs | 42 ++++++++++--------- .../src/embassy/time_driver_timg.rs | 35 ++++++---------- 4 files changed, 44 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 70adf1ee5..6cce48a3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - ESP32-C2/C3 examples: fix build error (#899) - ESP32-S3: Fix GPIO interrupt handler crashing when using GPIO48. (#898) +- Fixed short wait times in embassy causing hangs (#906) ### Removed diff --git a/esp-hal-common/src/embassy/mod.rs b/esp-hal-common/src/embassy/mod.rs index aafbe8ab9..7c466acd0 100644 --- a/esp-hal-common/src/embassy/mod.rs +++ b/esp-hal-common/src/embassy/mod.rs @@ -101,7 +101,6 @@ pub fn init(clocks: &Clocks, td: time_driver::TimerType) { } pub struct AlarmState { - pub timestamp: Cell, pub callback: Cell>, pub allocated: Cell, } @@ -111,7 +110,6 @@ unsafe impl Send for AlarmState {} impl AlarmState { pub const fn new() -> Self { Self { - timestamp: Cell::new(0), callback: Cell::new(None), allocated: Cell::new(false), } @@ -124,18 +122,17 @@ impl Driver for EmbassyTimer { } unsafe fn allocate_alarm(&self) -> Option { - return critical_section::with(|cs| { - let alarms = self.alarms.borrow(cs); - for i in 0..time_driver::ALARM_COUNT { - let c = alarms.get_unchecked(i); - if !c.allocated.get() { + 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 - c.allocated.set(true); - return Option::Some(AlarmHandle::new(i as u8)); + alarm.allocated.set(true); + self.on_alarm_allocated(i); + return Some(AlarmHandle::new(i as u8)); } } - return Option::None; - }); + None + }) } fn set_alarm_callback( diff --git a/esp-hal-common/src/embassy/time_driver_systimer.rs b/esp-hal-common/src/embassy/time_driver_systimer.rs index 2b461bd47..a29927702 100644 --- a/esp-hal-common/src/embassy/time_driver_systimer.rs +++ b/esp-hal-common/src/embassy/time_driver_systimer.rs @@ -32,15 +32,23 @@ impl EmbassyTimer { SystemTimer::now() } - pub(crate) fn trigger_alarm(&self, n: usize, cs: CriticalSection) { + fn trigger_alarm(&self, n: usize, cs: CriticalSection) { let alarm = &self.alarms.borrow(cs)[n]; - alarm.timestamp.set(u64::MAX); if let Some((f, ctx)) = alarm.callback.get() { f(ctx); } } + pub(super) fn on_alarm_allocated(&self, n: usize) { + match n { + 0 => self.alarm0.enable_interrupt(true), + 1 => self.alarm1.enable_interrupt(true), + 2 => self.alarm2.enable_interrupt(true), + _ => {} + } + } + fn on_interrupt(&self, id: usize) { critical_section::with(|cs| { self.clear_interrupt(id); @@ -83,17 +91,20 @@ impl EmbassyTimer { alarm: embassy_time::driver::AlarmHandle, timestamp: u64, ) -> bool { - critical_section::with(|cs| { + critical_section::with(|_cs| { let n = alarm.id() as usize; - let alarm_state = &self.alarms.borrow(cs)[n]; // The hardware fires the alarm even if timestamp is lower than the current - // time. - alarm_state.timestamp.set(timestamp); + // time. In this case the interrupt handler will pend a wakeup when we exit the + // critical section. self.arm(n, timestamp); + }); - true - }) + // In theory, the above comment is true. However, in practice, we seem to be + // missing interrupt for very short timeouts, so let's make sure and catch + // timestamps that already passed. Returning `false` means embassy will + // run one more poll loop. + Self::now() < timestamp } fn clear_interrupt(&self, id: usize) { @@ -107,18 +118,9 @@ impl EmbassyTimer { fn arm(&self, id: usize, timestamp: u64) { match id { - 0 => { - self.alarm0.set_target(timestamp); - self.alarm0.enable_interrupt(true); - } - 1 => { - self.alarm1.set_target(timestamp); - self.alarm1.enable_interrupt(true); - } - 2 => { - self.alarm2.set_target(timestamp); - self.alarm2.enable_interrupt(true); - } + 0 => self.alarm0.set_target(timestamp), + 1 => self.alarm1.set_target(timestamp), + 2 => self.alarm2.set_target(timestamp), _ => {} } } diff --git a/esp-hal-common/src/embassy/time_driver_timg.rs b/esp-hal-common/src/embassy/time_driver_timg.rs index d83615507..ea0002507 100644 --- a/esp-hal-common/src/embassy/time_driver_timg.rs +++ b/esp-hal-common/src/embassy/time_driver_timg.rs @@ -33,15 +33,16 @@ impl EmbassyTimer { unsafe { Timer0::::steal() }.now() } - pub(crate) fn trigger_alarm(&self, n: usize, cs: CriticalSection) { + fn trigger_alarm(&self, n: usize, cs: CriticalSection) { let alarm = &self.alarms.borrow(cs)[n]; - alarm.timestamp.set(u64::MAX); if let Some((f, ctx)) = alarm.callback.get() { f(ctx); } } + pub(super) fn on_alarm_allocated(&self, _n: usize) {} + fn on_interrupt(&self, id: u8, mut timer: Timer) { critical_section::with(|cs| { timer.clear_interrupt(); @@ -89,29 +90,19 @@ impl EmbassyTimer { timestamp: u64, ) -> bool { critical_section::with(|cs| { - let n = alarm.id() as usize; - let alarm_state = &self.alarms.borrow(cs)[n]; - + // The hardware fires the alarm even if timestamp is lower than the current + // time. In this case the interrupt handler will pend a wakeup when we exit the + // critical section. #[cfg(any(esp32, esp32s2, esp32s3))] - if n == 1 { - let tg = unsafe { Timer1::::steal() }; - return Self::set_alarm_impl(tg, alarm_state, timestamp); + if alarm.id() == 1 { + let mut tg = unsafe { Timer1::::steal() }; + Self::arm(&mut tg, timestamp); + return; } - let tg = unsafe { Timer0::::steal() }; - Self::set_alarm_impl(tg, alarm_state, timestamp) - }) - } - - fn set_alarm_impl( - mut tg: Timer, - alarm_state: &AlarmState, - timestamp: u64, - ) -> bool { - // The hardware fires the alarm even if timestamp is lower than the current - // time. - alarm_state.timestamp.set(timestamp); - Self::arm(&mut tg, timestamp); + let mut tg = unsafe { Timer0::::steal() }; + Self::arm(&mut tg, timestamp); + }); true }