-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Sub<Instant> for Instant should saturate instead of panicking (like std::time::Instant since Rust 1.60) #5545
Description
Summary
embassy_time::Instant's Sub<Instant> implementation panics when self.ticks < earlier.ticks. This causes firmware crashes in real-world embedded applications where the timer driver's now() may occasionally return non-monotonic values due to lock-free read races.
The Rust standard library had the same design and changed Instant::duration_since / Sub<Instant> to saturating in Rust 1.60 (rust-lang/rust#89926) for the same class of bugs. The justification from that PR:
Panicking on monotonic clock violations that the user cannot control is hostile.
Crash scenario
We hit this panic on an STM32F103 (Cortex-M3, 16-bit timer, embassy-stm32 0.5, embassy-time 0.5, default 1 MHz tick rate) running a DALI bus adapter with USB HID. The crash occurs during a time-critical GPIO bit-bang TX path:
DEBUG gpio: send fwd 16b [B1 79], send_twice=false, priority 3
ERROR panicked at embassy-time-0.5.0/src/instant.rs:108:20:
unwrap of `self.ticks.checked_sub(earlier.ticks)` failed: NoneError
The pattern is:
let earlier = Instant::now();
// ... Timer::at(target).await (yields to executor, ISRs fire) ...
let now = Instant::now();
let elapsed = (now - earlier).as_micros(); // PANIC: now.ticks < earlier.ticksRoot cause analysis
The embassy-stm32 time driver uses a lock-free half-period scheme in now():
fn now(&self) -> u64 {
let period = self.period.load(Ordering::Relaxed);
compiler_fence(Ordering::Acquire);
let counter = r.cnt().read().cnt();
calc_now(period, counter)
}This algorithm handles single-period races correctly via XOR compensation in calc_now. However, on STM32F103 with 16-bit timers and heavy interrupt activity (USB + EXTI + timer), we observe non-monotonic ticks. The exact mechanism is still under investigation (possible interactions with critical sections from defmt-rtt or CriticalSectionRawMutex channels masking timer ISRs), but the user-facing impact is clear: Instant::now() occasionally returns a value lower than a previously captured Instant.
Proposal
Make Sub<Instant> for Instant saturating, returning Duration::ZERO when self < rhs, matching std::time::Instant behavior since Rust 1.60:
impl ops::Sub<Instant> for Instant {
type Output = Duration;
fn sub(self, rhs: Instant) -> Duration {
self.saturating_duration_since(rhs)
}
}checked_duration_since() and saturating_duration_since() already exist and are correct. Only the Sub operator and duration_since() need the change.
Workaround
Users can replace (a - b).as_micros() with a.saturating_duration_since(b).as_micros() at every call site. We've done this in our project, but:
- Every embassy user working with time intervals is exposed to this
- The
-operator is the natural and most discoverable API - Nothing in the docs warns that it panics (unlike
duration_sincewhich at least mentions the precondition)
References
- Rust std precedent: rust-lang/rust#89926 (make
Instantsubtraction saturating) embassy_time::Instant::saturating_duration_sincealready exists- Affected platform: STM32F103CB, embassy-stm32 0.5, embassy-time 0.5,
time-driver-any