Fix systimer timestamp conversion #4634
Conversation
bugadani
force-pushed
the
xtal
branch
4 times, most recently
from
d01810c to
ca1a887
Compare
| #[unsafe(no_mangle)] | ||
| #[cfg(feature = "rt")] | ||
| static mut ESP_HAL_SYSTIMER_CORRECTION: NonZeroU32 = NonZeroU32::new(SHIFT_TIMESTAMP_FLAG).unwrap(); // Shift-by-0 = no-op | ||
|
|
||
| #[cfg(not(feature = "rt"))] | ||
| unsafe extern "Rust" { | ||
| static mut ESP_HAL_SYSTIMER_CORRECTION: NonZeroU32; | ||
| } |
There was a problem hiding this comment.
This being insta-stable-and-unchangeable is bothering me
There was a problem hiding this comment.
Hmm, it is a bit scary. Can't we use some of the rcntl retention registers for storing this calibration?
There was a problem hiding this comment.
Then we'd still have the format and location of the data fixed, just not in memory. Is that any better?
There was a problem hiding this comment.
Pull request overview
This PR aims to fix systimer timestamp conversion accuracy and improve performance, particularly for uncommon crystal frequencies like 26MHz on ESP32-C2. However, the implementation contains critical bugs that prevent it from working correctly.
Key Issues:
ticks_per_second()incorrectly returns MHz instead of Hz (off by factor of 1,000,000)- Precision loss in uneven divider calculation due to premature integer division
- These bugs would cause division by zero or highly inaccurate timestamp conversions
Intended Changes:
- Introduced optimization to use bit shifts for power-of-2 tick rates
- Added special handling for fractional dividers (2.5x) using multiply-then-divide approach
- Refactored time initialization into chip-specific modules
- Removed unused
xtal_freq()helper function
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| esp-hal/src/timer/systimer.rs | Adds timestamp conversion optimization with shift/multiply-divide logic, but contains critical bugs in ticks_per_second() and uneven divider calculation |
| esp-hal/src/time.rs | Refactors time_init() and now() into chip-specific implem modules (ESP32 vs systimer-based chips) |
| esp-hal/src/lib.rs | Updates unconditional call to time_init() to use new module path |
| esp-hal/src/clock/mod.rs | Removes unused xtal_freq() helper function |
| esp-hal/CHANGELOG.md | Documents the fix for systimer timestamp inaccuracy |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let cpu_cycles_per_timer_ticks = cpu_clock / timer_ticks_per_second; | ||
| println!("task2 count={}", unsafe { T2_COUNTER }); | ||
| println!("task3 count={}", unsafe { T3_COUNTER }); | ||
| let total_test_cpu_cycles = cpu_clock * TEST_MILLIS / 1000; |
There was a problem hiding this comment.
I was wondering why the ticks_per_sec change had no effect here, then I saw we multiplied, then divided by it. So I simplified this part a bit.
|
/hil full |
|
Triggered full HIL run for #4634. Run: https://github.com/esp-rs/esp-hal/actions/runs/20106494606 Status update: ❌ HIL (full) run failed (conclusion: failure). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/hil full |
|
Triggered full HIL run for #4634. Run: https://github.com/esp-rs/esp-hal/actions/runs/20245654039 Status update: ❌ HIL (full) run failed (conclusion: failure). |
|
/hil esp32 I want to see if that hang reproduces today |
|
Triggered HIL run for #4634 (chips: esp32). Run: https://github.com/esp-rs/esp-hal/actions/runs/20260954095 Status update: ✅ HIL (per-chip) run succeeded. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // | ||
| // On a 26MHz C2, the divider outputs 10.4MHz. On a 32MHz C3, the divider outputs 12.8MHz. | ||
| // | ||
| // Time is unreliable before `init_timestamp_scaler` is called. |
There was a problem hiding this comment.
is this something we should document in a user-facing way?
I wouldn't expect many users trying to do a lot before init but having something documented wouldn't hurt I guess
There was a problem hiding this comment.
Yes, good catch. We should make a note on the time types themselves.
3 participants
Perf improvement, improved systimer accuracy on 26MHz C2, one step towards removing
Clocks, what else do we need? (a way to do all this without taking a critical section to read frequencies).This PR replaces the old assumption that the systimer counter divides evenly into microseconds with a bit of precomputation that ensures precision. The PR also introduces an optimized method for the more common cases, where the end result can be obtained by shifting instead of 64-bit integer division.