Fix C2 delays (esp-rs#1981)

* Re-enable delay tests on S2 and C2

* Systimer: use fn instead of constant to retrieve tick freq

* Reformulate delay using current_time

* Take actual XTAL into account

* Re-enable tests

* Fix changelog

* Disable defmt

* Remove unused esp32 code

* Update esp-hal/src/delay.rs

Co-authored-by: Jesse Braham <[email protected]>

---------

Co-authored-by: Jesse Braham <[email protected]>

esp-hal/CHANGELOG.md

@@ -287,6 +287,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Auto detect crystal frequency based on `RtcClock::estimate_xtal_frequency()` (#1165)
 - ESP32-S3: Configure 32k ICACHE (#1169)
 - Lift the minimal buffer size requirement for I2S (#1189)
+- Replaced `SystemTimer::TICKS_PER_SEC` with `SystemTimer::ticks_per_sec()` (#1981)
 
 ### Removed
 

esp-hal/src/clock/mod.rs

@@ -71,6 +71,8 @@
 //! ```
 
 use fugit::HertzU32;
+#[cfg(esp32c2)]
+use portable_atomic::{AtomicU32, Ordering};
 
 #[cfg(any(esp32, esp32c2))]
 use crate::rtc_cntl::RtcClock;
@@ -333,6 +335,26 @@ pub struct RawClocks {
     pub pll_96m_clock: HertzU32,
 }
 
+cfg_if::cfg_if! {
+    if #[cfg(esp32c2)] {
+        static XTAL_FREQ_MHZ: AtomicU32 = AtomicU32::new(40);
+
+        pub(crate) fn xtal_freq_mhz() -> u32 {
+            XTAL_FREQ_MHZ.load(Ordering::Relaxed)
+        }
+    } else if #[cfg(esp32h2)] {
+        pub(crate) fn xtal_freq_mhz() -> u32 {
+            32
+        }
+    } else if #[cfg(any(esp32, esp32s2))] {
+        // Function would be unused
+    } else {
+        pub(crate) fn xtal_freq_mhz() -> u32 {
+            40
+        }
+    }
+}
+
 /// Used to configure the frequencies of the clocks present in the chip.
 ///
 /// After setting all frequencies, call the freeze function to apply the
@@ -429,6 +451,7 @@ impl<'d> ClockControl<'d> {
         } else {
             26
         };
+        XTAL_FREQ_MHZ.store(xtal_freq, Ordering::Relaxed);
 
         ClockControl {
             _private: clock_control.into_ref(),
@@ -452,6 +475,7 @@ impl<'d> ClockControl<'d> {
         } else {
             XtalClock::RtcXtalFreq26M
         };
+        XTAL_FREQ_MHZ.store(xtal_freq.mhz(), Ordering::Relaxed);
 
         let pll_freq = PllClock::Pll480MHz;
 

esp-hal/src/delay.rs

@@ -30,36 +30,25 @@
 //! [DelayUs]: embedded_hal_02::blocking::delay::DelayUs
 //! [embedded-hal]: https://docs.rs/embedded-hal/1.0.0/embedded_hal/delay/index.html
 
-use fugit::HertzU64;
 pub use fugit::MicrosDurationU64;
 
+use crate::clock::Clocks;
+
 /// Delay driver
 ///
 /// Uses the `SYSTIMER` peripheral internally for RISC-V devices, and the
 /// built-in Xtensa timer for Xtensa devices.
 #[derive(Clone, Copy)]
-pub struct Delay {
-    freq: HertzU64,
-}
-
-impl Delay {
-    /// Delay for the specified number of milliseconds
-    pub fn delay_millis(&self, ms: u32) {
-        for _ in 0..ms {
-            self.delay_micros(1000);
-        }
-    }
-}
+#[non_exhaustive]
+pub struct Delay;
 
 #[cfg(feature = "embedded-hal-02")]
 impl<T> embedded_hal_02::blocking::delay::DelayMs<T> for Delay
 where
     T: Into<u32>,
 {
     fn delay_ms(&mut self, ms: T) {
-        for _ in 0..ms.into() {
-            self.delay_micros(1000);
-        }
+        self.delay_millis(ms.into());
     }
 }
 
@@ -80,83 +69,48 @@ impl embedded_hal::delay::DelayNs for Delay {
     }
 }
 
-#[cfg(riscv)]
-mod implementation {
-    use super::*;
-    use crate::{clock::Clocks, timer::systimer::SystemTimer};
-
-    impl Delay {
-        /// Create a new `Delay` instance
-        pub fn new(clocks: &Clocks<'_>) -> Self {
-            // The counters and comparators are driven using `XTAL_CLK`.
-            // The average clock frequency is fXTAL_CLK/2.5, which is 16 MHz.
-            // The timer counting is incremented by 1/16 μs on each `CNT_CLK` cycle.
-            Self {
-                #[cfg(not(esp32h2))]
-                freq: HertzU64::MHz(clocks.xtal_clock.to_MHz() as u64 * 10 / 25),
-                #[cfg(esp32h2)]
-                // esp32h2 TRM, section 11.2 Clock Source Selection
-                freq: HertzU64::MHz(clocks.xtal_clock.to_MHz() as u64 * 10 / 20),
-            }
-        }
-
-        /// Delay for the specified time
-        pub fn delay(&self, time: MicrosDurationU64) {
-            let t0 = SystemTimer::now();
-            let rate: HertzU64 = MicrosDurationU64::from_ticks(1).into_rate();
-            let clocks = time.ticks() * (self.freq / rate);
+impl Delay {
+    /// Creates a new `Delay` instance.
+    // Do not remove the argument, it makes sure that the clocks are initialized.
+    pub fn new(_clocks: &Clocks<'_>) -> Self {
+        Self {}
+    }
 
-            while SystemTimer::now().wrapping_sub(t0) & SystemTimer::BIT_MASK <= clocks {}
-        }
+    /// Delay for the specified time
+    pub fn delay(&self, delay: MicrosDurationU64) {
+        let start = crate::time::current_time();
 
-        /// Delay for the specified number of microseconds
-        pub fn delay_micros(&self, us: u32) {
-            let t0 = SystemTimer::now();
-            let clocks = us as u64 * (self.freq / HertzU64::MHz(1));
+        while elapsed_since(start) < delay {}
+    }
 
-            while SystemTimer::now().wrapping_sub(t0) & SystemTimer::BIT_MASK <= clocks {}
+    /// Delay for the specified number of milliseconds
+    pub fn delay_millis(&self, ms: u32) {
+        for _ in 0..ms {
+            self.delay_micros(1000);
         }
+    }
 
-        /// Delay for the specified number of nanoseconds
-        pub fn delay_nanos(&self, ns: u32) {
-            let t0 = SystemTimer::now();
-            let clocks = ns as u64 * (self.freq / HertzU64::MHz(1)) / 1000;
+    /// Delay for the specified number of microseconds
+    pub fn delay_micros(&self, us: u32) {
+        let delay = MicrosDurationU64::micros(us as u64);
+        self.delay(delay);
+    }
 
-            while SystemTimer::now().wrapping_sub(t0) & SystemTimer::BIT_MASK <= clocks {}
-        }
+    /// Delay for the specified number of nanoseconds
+    pub fn delay_nanos(&self, ns: u32) {
+        let delay = MicrosDurationU64::nanos(ns as u64);
+        self.delay(delay);
     }
 }
 
-#[cfg(xtensa)]
-mod implementation {
-    use super::*;
-    use crate::clock::Clocks;
-
-    impl Delay {
-        /// Create a new `Delay` instance
-        pub fn new(clocks: &Clocks<'_>) -> Self {
-            Self {
-                freq: clocks.cpu_clock.into(),
-            }
-        }
-
-        /// Delay for the specified time
-        pub fn delay(&self, time: MicrosDurationU64) {
-            let rate: HertzU64 = MicrosDurationU64::from_ticks(1).into_rate();
-            let clocks = time.ticks() * (self.freq / rate);
-            xtensa_lx::timer::delay(clocks as u32);
-        }
-
-        /// Delay for the specified number of microseconds
-        pub fn delay_micros(&self, us: u32) {
-            let clocks = us as u64 * (self.freq / HertzU64::MHz(1));
-            xtensa_lx::timer::delay(clocks as u32);
-        }
+fn elapsed_since(start: fugit::Instant<u64, 1, 1_000_000>) -> MicrosDurationU64 {
+    let now = crate::time::current_time();
 
-        /// Delay for the specified number of nanoseconds
-        pub fn delay_nanos(&self, ns: u32) {
-            let clocks = ns as u64 * (self.freq / HertzU64::MHz(1)) / 1000;
-            xtensa_lx::timer::delay(clocks as u32);
-        }
+    if start.ticks() <= now.ticks() {
+        now - start
+    } else {
+        // current_time specifies at least 7 happy years, let's ignore this issue for
+        // now.
+        panic!("Time has wrapped around, which we currently don't handle");
     }
 }

esp-hal/src/time.rs

@@ -50,7 +50,7 @@ pub fn current_time() -> fugit::Instant<u64, 1, 1_000_000> {
         let ticks = crate::timer::systimer::SystemTimer::now();
         (
             ticks,
-            (crate::timer::systimer::SystemTimer::TICKS_PER_SECOND / 1_000_000),
+            (crate::timer::systimer::SystemTimer::ticks_per_second() / 1_000_000),
         )
     };
 

esp-hal/src/timer/systimer.rs

@@ -112,20 +112,37 @@ impl<'d> SystemTimer<'d> {
         if #[cfg(esp32s2)] {
             /// Bitmask to be applied to the raw register value.
             pub const BIT_MASK: u64 = u64::MAX;
-            /// The ticks per second the underlying peripheral uses.
-            pub const TICKS_PER_SECOND: u64 = 80_000_000;
             // Bitmask to be applied to the raw period register value.
             const PERIOD_MASK: u64 = 0x1FFF_FFFF;
         } else {
             /// Bitmask to be applied to the raw register value.
             pub const BIT_MASK: u64 = 0xF_FFFF_FFFF_FFFF;
-            /// The ticks per second the underlying peripheral uses.
-            pub const TICKS_PER_SECOND: u64 = 16_000_000;
             // Bitmask to be applied to the raw period register value.
             const PERIOD_MASK: u64 = 0x3FF_FFFF;
         }
     }
 
+    /// Returns the tick frequency of the underlying timer unit.
+    pub fn ticks_per_second() -> u64 {
+        cfg_if::cfg_if! {
+            if #[cfg(esp32s2)] {
+                80_000_000
+            } else if #[cfg(esp32h2)] {
+                // The counters and comparators are driven using `XTAL_CLK`.
+                // The average clock frequency is fXTAL_CLK/2, which is 16 MHz.
+                // The timer counting is incremented by 1/16 μs on each `CNT_CLK` cycle.
+                const MULTIPLIER: u64 = 10_000_000 / 20;
+                crate::clock::xtal_freq_mhz() as u64 * MULTIPLIER
+            } else {
+                // The counters and comparators are driven using `XTAL_CLK`.
+                // The average clock frequency is fXTAL_CLK/2.5, which is 16 MHz.
+                // The timer counting is incremented by 1/16 μs on each `CNT_CLK` cycle.
+                const MULTIPLIER: u64 = 10_000_000 / 25;
+                crate::clock::xtal_freq_mhz() as u64 * MULTIPLIER
+            }
+        }
+    }
+
     /// Create a new instance.
     pub fn new(_systimer: impl Peripheral<P = SYSTIMER> + 'd) -> Self {
         #[cfg(soc_etm)]
@@ -810,7 +827,7 @@ where
         }
 
         let us = period.ticks();
-        let ticks = us * (SystemTimer::TICKS_PER_SECOND / 1_000_000) as u32;
+        let ticks = us * (SystemTimer::ticks_per_second() / 1_000_000) as u32;
 
         self.comparator.set_mode(ComparatorMode::Period);
         self.comparator.set_period(ticks);
@@ -879,7 +896,7 @@ where
             }
         };
 
-        let us = ticks / (SystemTimer::TICKS_PER_SECOND / 1_000_000);
+        let us = ticks / (SystemTimer::ticks_per_second() / 1_000_000);
 
         Instant::<u64, 1, 1_000_000>::from_ticks(us)
     }
@@ -888,7 +905,7 @@ where
         let mode = self.comparator.get_mode();
 
         let us = value.ticks();
-        let ticks = us * (SystemTimer::TICKS_PER_SECOND / 1_000_000);
+        let ticks = us * (SystemTimer::ticks_per_second() / 1_000_000);
 
         if matches!(mode, ComparatorMode::Period) {
             // Period mode

examples/src/bin/systimer.rs

@@ -74,11 +74,11 @@ fn main() -> ! {
         alarm0.enable_interrupt(true);
 
         alarm1.set_interrupt_handler(systimer_target1);
-        alarm1.set_target(SystemTimer::now() + (SystemTimer::TICKS_PER_SECOND * 2));
+        alarm1.set_target(SystemTimer::now() + (SystemTimer::ticks_per_second() * 2));
         alarm1.enable_interrupt(true);
 
         alarm2.set_interrupt_handler(systimer_target2);
-        alarm2.set_target(SystemTimer::now() + (SystemTimer::TICKS_PER_SECOND * 3));
+        alarm2.set_target(SystemTimer::now() + (SystemTimer::ticks_per_second() * 3));
         alarm2.enable_interrupt(true);
 
         ALARM0.borrow_ref_mut(cs).replace(alarm0);

hil-test/tests/delay.rs

@@ -1,7 +1,6 @@
 //! Delay Test
 
-// esp32c2 is disabled currently as it fails
-//% CHIPS: esp32  esp32c3 esp32c6 esp32s3
+//% CHIPS: esp32 esp32c2 esp32c3 esp32c6 esp32s2 esp32s3
 
 #![no_std]
 #![no_main]
@@ -37,25 +36,33 @@ mod tests {
     }
 
     #[test]
-    #[timeout(1)]
+    #[timeout(2)]
     fn delay_ns(mut ctx: Context) {
         let t1 = esp_hal::time::current_time();
         ctx.delay.delay_ns(600_000_000);
         let t2 = esp_hal::time::current_time();
 
         assert!(t2 > t1);
-        assert!((t2 - t1).to_nanos() >= 600_000_000u64);
+        assert!(
+            (t2 - t1).to_nanos() >= 600_000_000u64,
+            "diff: {:?}",
+            (t2 - t1).to_nanos()
+        );
     }
 
     #[test]
-    #[timeout(1)]
+    #[timeout(2)]
     fn delay_700millis(ctx: Context) {
         let t1 = esp_hal::time::current_time();
         ctx.delay.delay_millis(700);
         let t2 = esp_hal::time::current_time();
 
         assert!(t2 > t1);
-        assert!((t2 - t1).to_millis() >= 700u64);
+        assert!(
+            (t2 - t1).to_millis() >= 700u64,
+            "diff: {:?}",
+            (t2 - t1).to_millis()
+        );
     }
 
     #[test]
@@ -66,7 +73,11 @@ mod tests {
         let t2 = esp_hal::time::current_time();
 
         assert!(t2 > t1);
-        assert!((t2 - t1).to_micros() >= 1_500_000u64);
+        assert!(
+            (t2 - t1).to_micros() >= 1_500_000u64,
+            "diff: {:?}",
+            (t2 - t1).to_micros()
+        );
     }
 
     #[test]
@@ -77,6 +88,10 @@ mod tests {
         let t2 = esp_hal::time::current_time();
 
         assert!(t2 > t1);
-        assert!((t2 - t1).to_millis() >= 3000u64);
+        assert!(
+            (t2 - t1).to_millis() >= 3000u64,
+            "diff: {:?}",
+            (t2 - t1).to_millis()
+        );
     }
 }

hil-test/tests/embassy_timers_executors.rs

@@ -1,7 +1,6 @@
 //! Embassy timer and executor Test
 
-// esp32c2 is disabled currently as it fails
-//% CHIPS: esp32 esp32c3 esp32c6 esp32h2 esp32s3
+//% CHIPS: esp32 esp32c2 esp32c3 esp32c6 esp32h2 esp32s2 esp32s3
 
 #![no_std]
 #![no_main]