Skip to content

Commit e8e4bba

Browse files
committed
Rewrite time, fix timestamp conversion
1 parent 9371137 commit e8e4bba

File tree

6 files changed

+193
-98
lines changed

6 files changed

+193
-98
lines changed

esp-hal/CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
3838
- RMT: When dropping a Tx channel, the driver now disconnects the output pin from the peripheral. (#4302)
3939
- I2C: avoid potential infinite loop while checking for command completion (#4519)
4040
- UART: correct documentation of `read` which incorrectly stated that it would never block (#4586)
41+
- Fixed System Timer timestamp inaccuracy when using uncommon crystal frequencies (#4634)
42+
- `SystemTimer::ticks_per_second()` now correctly returns the number of ticks per second. (#4634)
4143

4244
### Removed
4345

esp-hal/src/clock/mod.rs

Lines changed: 0 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -990,40 +990,6 @@ impl Clocks {
990990
pub fn get<'a>() -> &'a Clocks {
991991
unwrap!(Self::try_get())
992992
}
993-
994-
/// Returns the xtal frequency.
995-
///
996-
/// This function will run the frequency estimation if called before
997-
/// [`crate::init()`].
998-
#[cfg(systimer)]
999-
#[inline]
1000-
pub(crate) fn xtal_freq() -> Rate {
1001-
if let Some(clocks) = Self::try_get() {
1002-
return clocks.xtal_clock;
1003-
}
1004-
1005-
Self::measure_xtal_frequency().frequency()
1006-
}
1007-
1008-
#[cfg(not(esp32))]
1009-
fn measure_xtal_frequency() -> XtalClock {
1010-
cfg_if::cfg_if! {
1011-
if #[cfg(esp32h2)] {
1012-
XtalClock::_32M
1013-
} else if #[cfg(not(esp32c2))] {
1014-
XtalClock::_40M
1015-
} else {
1016-
// TODO: we should be able to read from a retention register, but probe-rs flashes a
1017-
// bootloader that assumes a frequency, instead of choosing a matching one.
1018-
let mhz = RtcClock::estimate_xtal_frequency();
1019-
1020-
debug!("Working with a {}MHz crystal", mhz);
1021-
1022-
// Try to guess the closest possible crystal value.
1023-
XtalClock::closest_from_mhz(mhz)
1024-
}
1025-
}
1026-
}
1027993
}
1028994

1029995
impl Clocks {

esp-hal/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -754,7 +754,7 @@ pub fn init(config: Config) -> Peripherals {
754754
#[cfg(timergroup_timg1)]
755755
crate::timer::timg::Wdt::<crate::peripherals::TIMG1<'static>>::new().disable();
756756

757-
crate::time::time_init();
757+
crate::time::implem::time_init();
758758

759759
crate::gpio::interrupt::bind_default_interrupt_handler();
760760

esp-hal/src/time.rs

Lines changed: 44 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,6 @@
66
77
use core::fmt::{Debug, Display, Formatter, Result as FmtResult};
88

9-
#[cfg(esp32)]
10-
use crate::peripherals::TIMG0;
11-
129
type InnerRate = fugit::Rate<u32, 1, 1>;
1310
type InnerInstant = fugit::Instant<u64, 1, 1_000_000>;
1411
type InnerDuration = fugit::Duration<u64, 1, 1_000_000>;
@@ -264,7 +261,7 @@ impl Instant {
264261
/// ```
265262
#[inline]
266263
pub fn now() -> Self {
267-
now()
264+
implem::now()
268265
}
269266

270267
#[inline]
@@ -710,10 +707,33 @@ impl core::ops::Div<Duration> for Duration {
710707
}
711708
}
712709

713-
#[inline]
714-
fn now() -> Instant {
715-
#[cfg(esp32)]
716-
let (ticks, div) = {
710+
#[cfg(esp32)]
711+
pub(crate) mod implem {
712+
use super::Instant;
713+
use crate::peripherals::TIMG0;
714+
715+
#[cfg(feature = "rt")]
716+
pub(crate) fn time_init() {
717+
let apb = crate::Clocks::get().apb_clock.as_hz();
718+
719+
let tg0 = TIMG0::regs();
720+
721+
tg0.lactconfig().write(|w| unsafe { w.bits(0) });
722+
tg0.lactalarmhi().write(|w| unsafe { w.bits(u32::MAX) });
723+
tg0.lactalarmlo().write(|w| unsafe { w.bits(u32::MAX) });
724+
tg0.lactload().write(|w| unsafe { w.load().bits(1) });
725+
726+
// 16 MHz counter
727+
tg0.lactconfig().write(|w| {
728+
unsafe { w.divider().bits((apb / 16_000_000u32) as u16) };
729+
w.increase().bit(true);
730+
w.autoreload().bit(true);
731+
w.en().bit(true)
732+
});
733+
}
734+
735+
#[inline]
736+
pub(super) fn now() -> Instant {
717737
// on ESP32 use LACT
718738
let tg0 = TIMG0::regs();
719739
tg0.lactupdate().write(|w| unsafe { w.update().bits(1) });
@@ -733,39 +753,27 @@ fn now() -> Instant {
733753
let hi = tg0.lacthi().read().bits();
734754

735755
let ticks = ((hi as u64) << 32u64) | lo as u64;
736-
(ticks, 16)
737-
};
738-
739-
#[cfg(not(esp32))]
740-
let (ticks, div) = {
741-
use crate::timer::systimer::{SystemTimer, Unit};
742-
// otherwise use SYSTIMER
743-
let ticks = SystemTimer::unit_value(Unit::Unit0);
744-
(ticks, (SystemTimer::ticks_per_second() / 1_000_000))
745-
};
746756

747-
Instant::from_ticks(ticks / div)
757+
Instant::from_ticks(ticks / 16)
758+
}
748759
}
749760

750-
#[cfg(feature = "rt")]
751-
pub(crate) fn time_init() {
752-
#[cfg(esp32)]
753-
{
754-
let apb = crate::Clocks::get().apb_clock.as_hz();
761+
#[cfg(soc_has_systimer)]
762+
pub(crate) mod implem {
763+
use super::Instant;
764+
use crate::timer::systimer::{SystemTimer, Unit};
755765

756-
let tg0 = TIMG0::regs();
766+
#[cfg(feature = "rt")]
767+
pub(crate) fn time_init() {
768+
SystemTimer::init_timestamp_scaler();
769+
}
757770

758-
tg0.lactconfig().write(|w| unsafe { w.bits(0) });
759-
tg0.lactalarmhi().write(|w| unsafe { w.bits(u32::MAX) });
760-
tg0.lactalarmlo().write(|w| unsafe { w.bits(u32::MAX) });
761-
tg0.lactload().write(|w| unsafe { w.load().bits(1) });
771+
#[inline]
772+
pub(super) fn now() -> Instant {
773+
let ticks = SystemTimer::unit_value(Unit::Unit0);
762774

763-
// 16 MHz counter
764-
tg0.lactconfig().write(|w| {
765-
unsafe { w.divider().bits((apb / 16_000_000u32) as u16) };
766-
w.increase().bit(true);
767-
w.autoreload().bit(true);
768-
w.en().bit(true)
769-
});
775+
let micros = SystemTimer::ticks_to_us(ticks);
776+
777+
Instant::from_ticks(micros)
770778
}
771779
}

esp-hal/src/timer/systimer.rs

Lines changed: 140 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
//! [`PeriodicTimer`](super::PeriodicTimer). Using the System timer directly is
1818
//! only possible through the low level [`Timer`](crate::timer::Timer) trait.
1919
20-
use core::{fmt::Debug, marker::PhantomData};
20+
use core::{fmt::Debug, marker::PhantomData, num::NonZeroU32};
2121

2222
use esp_sync::RawMutex;
2323

@@ -26,10 +26,50 @@ use crate::{
2626
asynch::AtomicWaker,
2727
interrupt::{self, InterruptHandler},
2828
peripherals::{Interrupt, SYSTIMER},
29+
soc::clocks::ClockTree,
2930
system::{Cpu, Peripheral as PeripheralEnable, PeripheralClockControl},
3031
time::{Duration, Instant},
3132
};
3233

34+
// System Timer is only clocked by XTAL divided by 2 or 2.5 (or RC_FAST_CLK which is not supported
35+
// yet). Some XTAL options (most, in fact) divided by this value may not be an integer multiple of
36+
// 1_000_000. Because the timer API works with microseconds, we need to correct for this. To avoid
37+
// u64 division as much as possible, we use the two highest bits of the divisor to determine the
38+
// division method.
39+
// - If these flags are 0b00, we divide by the divisor.
40+
// - If these flags are 0b01, we multiply by a constant before dividing by the divisor to improve
41+
// accuracy.
42+
// - If these flags are 0b10, we shift the timestamp by the lower bits.
43+
//
44+
// Apart from the S2, the System Timer clock divider outputs a 16MHz timer clock when using
45+
// the "default" XTAL configuration, so this method will commonly use the shifting based
46+
// division.
47+
//
48+
// On a 26MHz C2, the divider outputs 10.4MHz. On a 32MHz C3, the divider outputs 12.8MHz.
49+
//
50+
// Time is unreliable before `init_timestamp_scaler` is called.
51+
//
52+
// Because a single crate version can have "rt" enabled, `ESP_HAL_SYSTIMER_CORRECTION` needs
53+
// to be shared between versions. This aspect of this driver must be therefore kept stable.
54+
#[unsafe(no_mangle)]
55+
#[cfg(feature = "rt")]
56+
static mut ESP_HAL_SYSTIMER_CORRECTION: NonZeroU32 = NonZeroU32::new(SHIFT_TIMESTAMP_FLAG).unwrap(); // Shift-by-0 = no-op
57+
58+
#[cfg(not(feature = "rt"))]
59+
unsafe extern "Rust" {
60+
static mut ESP_HAL_SYSTIMER_CORRECTION: NonZeroU32;
61+
}
62+
63+
const SHIFT_TIMESTAMP_FLAG: u32 = 0x8000_0000;
64+
const SHIFT_MASK: u32 = 0x0000_FFFF;
65+
// If the tick rate is not an integer number of microseconds: Since the divider is 2.5,
66+
// and we assume XTAL is an integer number of MHz, we can multiply by 5, then divide by
67+
// 5 to improve accuracy. On H2 the divider is 2, so we can multiply by 2, then divide by
68+
// 2.
69+
const UNEVEN_DIVIDER_FLAG: u32 = 0x4000_0000;
70+
const UNEVEN_MULTIPLIER: u32 = if cfg!(esp32h2) { 2 } else { 5 };
71+
const UNEVEN_DIVIDER_MASK: u32 = 0x0000_FFFF;
72+
3373
/// The configuration of a unit.
3474
#[derive(Copy, Clone)]
3575
pub enum UnitConfig {
@@ -70,29 +110,107 @@ impl<'d> SystemTimer<'d> {
70110
}
71111
}
72112

113+
/// One-time initialization for the timestamp conversion/scaling.
114+
#[cfg(feature = "rt")]
115+
pub(crate) fn init_timestamp_scaler() {
116+
// Maximum tick rate is 80MHz (S2), which fits in a u32, so let's narrow the type.
117+
let systimer_rate = Self::ticks_per_second();
118+
119+
// Select the optimal way to divide timestamps.
120+
let packed_rate_and_method = if systimer_rate.is_multiple_of(1_000_000) {
121+
let ticks_per_us = systimer_rate as u32 / 1_000_000;
122+
if ticks_per_us.is_power_of_two() {
123+
// Turn the division into a shift
124+
SHIFT_TIMESTAMP_FLAG | (ticks_per_us.ilog2() & SHIFT_MASK)
125+
} else {
126+
// We need to divide by an integer :(
127+
ticks_per_us
128+
}
129+
} else {
130+
// The rate is not a multiple of 1 MHz, we need to scale it up to prevent precision
131+
// loss.
132+
let multiplied_ticks_per_us = (systimer_rate * UNEVEN_MULTIPLIER as u64) / 1_000_000;
133+
UNEVEN_DIVIDER_FLAG | (multiplied_ticks_per_us as u32)
134+
};
135+
136+
// Safety: we only ever write ESP_HAL_SYSTIMER_CORRECTION in `init_timestamp_scaler`, which
137+
// is called once and only once during startup, from `time_init`.
138+
unsafe {
139+
let correction_ptr = &raw mut ESP_HAL_SYSTIMER_CORRECTION;
140+
*correction_ptr = unwrap!(NonZeroU32::new(packed_rate_and_method));
141+
}
142+
}
143+
144+
#[inline]
145+
pub(crate) fn ticks_to_us(ticks: u64) -> u64 {
146+
// Safety: we only ever write ESP_HAL_SYSTIMER_CORRECTION in `init_timestamp_scaler`, which
147+
// is called once and only once during startup.
148+
let correction = unsafe { ESP_HAL_SYSTIMER_CORRECTION };
149+
150+
let correction = correction.get();
151+
match correction & (SHIFT_TIMESTAMP_FLAG | UNEVEN_DIVIDER_FLAG) {
152+
v if v == SHIFT_TIMESTAMP_FLAG => ticks >> (correction & SHIFT_MASK),
153+
v if v == UNEVEN_DIVIDER_FLAG => {
154+
// Not only that, but we need to multiply the timestamp first otherwise
155+
// we'd count slower than the timer.
156+
let multiplied = if UNEVEN_MULTIPLIER.is_power_of_two() {
157+
ticks << UNEVEN_MULTIPLIER.ilog2()
158+
} else {
159+
ticks * UNEVEN_MULTIPLIER as u64
160+
};
161+
162+
let divider = correction & UNEVEN_DIVIDER_MASK;
163+
multiplied / divider as u64
164+
}
165+
_ => ticks / correction as u64,
166+
}
167+
}
168+
169+
#[inline]
170+
fn us_to_ticks(us: u64) -> u64 {
171+
// Safety: we only ever write ESP_HAL_SYSTIMER_CORRECTION in `init_timestamp_scaler`, which
172+
// is called once and only once during startup.
173+
let correction = unsafe { ESP_HAL_SYSTIMER_CORRECTION };
174+
175+
let correction = correction.get();
176+
match correction & (SHIFT_TIMESTAMP_FLAG | UNEVEN_DIVIDER_FLAG) {
177+
v if v == SHIFT_TIMESTAMP_FLAG => us << (correction & SHIFT_MASK),
178+
v if v == UNEVEN_DIVIDER_FLAG => {
179+
let multiplier = correction & UNEVEN_DIVIDER_MASK;
180+
let multiplied = us * multiplier as u64;
181+
182+
// Not only that, but we need to divide the timestamp first otherwise
183+
// we'd return a slightly too-big value.
184+
if UNEVEN_MULTIPLIER.is_power_of_two() {
185+
multiplied >> UNEVEN_MULTIPLIER.ilog2()
186+
} else {
187+
multiplied / UNEVEN_MULTIPLIER as u64
188+
}
189+
}
190+
_ => us * correction as u64,
191+
}
192+
}
193+
73194
/// Returns the tick frequency of the underlying timer unit.
74195
#[inline]
75196
pub fn ticks_per_second() -> u64 {
76-
cfg_if::cfg_if! {
77-
if #[cfg(esp32s2)] {
78-
const MULTIPLIER: u32 = 2;
79-
const DIVIDER: u32 = 1;
80-
} else if #[cfg(esp32h2)] {
81-
// The counters and comparators are driven using `XTAL_CLK`.
82-
// The average clock frequency is fXTAL_CLK/2, which is 16 MHz.
83-
// The timer counting is incremented by 1/16 μs on each `CNT_CLK` cycle.
84-
const MULTIPLIER: u32 = 1;
85-
const DIVIDER: u32 = 2;
86-
} else {
87-
// The counters and comparators are driven using `XTAL_CLK`.
88-
// The average clock frequency is fXTAL_CLK/2.5, which is 16 MHz.
89-
// The timer counting is incremented by 1/16 μs on each `CNT_CLK` cycle.
90-
const MULTIPLIER: u32 = 4;
91-
const DIVIDER: u32 = 10;
197+
// FIXME: this requires a critical section. We can probably do better, if we can formulate
198+
// invariants well.
199+
ClockTree::with(|clocks| {
200+
cfg_if::cfg_if! {
201+
if #[cfg(esp32s2)] {
202+
crate::soc::clocks::apb_clk_frequency(clocks) as u64
203+
} else if #[cfg(esp32h2)] {
204+
// The counters and comparators are driven using `XTAL_CLK`.
205+
// The average clock frequency is fXTAL_CLK/2, (16 MHz for XTAL = 32 MHz)
206+
(crate::soc::clocks::xtal_clk_frequency(clocks) / 2) as u64
207+
} else {
208+
// The counters and comparators are driven using `XTAL_CLK`.
209+
// The average clock frequency is fXTAL_CLK/2.5 (16 MHz for XTAL = 40 MHz)
210+
(crate::soc::clocks::xtal_clk_frequency(clocks) * 10 / 25) as u64
211+
}
92212
}
93-
}
94-
let xtal_freq_mhz = crate::clock::Clocks::xtal_freq().as_hz();
95-
((xtal_freq_mhz * MULTIPLIER) / DIVIDER) as u64
213+
})
96214
}
97215

98216
/// Create a new instance.
@@ -519,7 +637,7 @@ impl super::Timer for Alarm<'_> {
519637

520638
let ticks = self.unit.read_count();
521639

522-
let us = ticks / (SystemTimer::ticks_per_second() / 1_000_000);
640+
let us = SystemTimer::ticks_to_us(ticks);
523641

524642
Instant::from_ticks(us)
525643
}
@@ -528,7 +646,7 @@ impl super::Timer for Alarm<'_> {
528646
let mode = self.mode();
529647

530648
let us = value.as_micros();
531-
let ticks = us * (SystemTimer::ticks_per_second() / 1_000_000);
649+
let ticks = SystemTimer::us_to_ticks(us);
532650

533651
if matches!(mode, ComparatorMode::Period) {
534652
// Period mode

qa-test/src/bin/embassy_executor_benchmark.rs

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -36,15 +36,16 @@ const TEST_MILLIS: u64 = 500;
3636
fn timer_handler() {
3737
let c = unsafe { COUNTER } as u64;
3838
let cpu_clock = CLOCK as u64 * 1_000_000;
39-
40-
let timer_ticks_per_second = SystemTimer::ticks_per_second();
41-
let cpu_cycles_per_timer_ticks = cpu_clock / timer_ticks_per_second;
4239
println!("task2 count={}", unsafe { T2_COUNTER });
4340
println!("task3 count={}", unsafe { T3_COUNTER });
41+
let total_test_cpu_cycles = cpu_clock * TEST_MILLIS / 1000;
42+
// Average cycles per task execution, with a precision of 2 decimal places.
43+
let centicycles = (100 * total_test_cpu_cycles) / c;
4444
println!(
45-
"Test OK, count={}, cycles={}/100",
45+
"Test OK, count={}, cycles={}.{}",
4646
c,
47-
(100 * timer_ticks_per_second * cpu_cycles_per_timer_ticks * TEST_MILLIS / 1000) / c
47+
centicycles / 100,
48+
centicycles % 100
4849
);
4950
loop {}
5051
}

0 commit comments

Comments
 (0)