Fix systimer timestamp conversion #4634
Conversation
bugadani
force-pushed
the
xtal
branch
4 times, most recently
from
d01810c to
ca1a887
Compare
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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| multiplied / UNEVEN_MULTIPLIER as u64 | ||
| } | ||
| } | ||
| _ => us * correction as u64, |
There was a problem hiding this comment.
Potential arithmetic overflow when multiplying large microsecond values. If the us parameter approaches large values (e.g., close to u64::MAX), the multiplication us * correction as u64 can overflow, resulting in incorrect tick calculations. While overflow may be unlikely in typical usage, consider using checked_mul() or saturating_mul() to handle edge cases safely, similar to the pattern used in the timg module (see timeout_to_ticks in esp-hal/src/timer/timg.rs line 640).
There was a problem hiding this comment.
If anything, we should use wrapping_mul et al, because we've documented wrapping behaviour. But wrapping behaviour is annoying for comparisons. I'm not really concerned by this given the large timespans involved, but this IS a problem we should define better.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
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.