embassy-rs / embassy

Modern embedded framework, using Rust and async.
https://embassy.dev
Apache License 2.0
5.15k stars 713 forks source link

Possible overflow issue with embassy/embassy-stm32/src/i2c/timeout.rs #1063

Open MauroMombelli opened 1 year ago

MauroMombelli commented 1 year ago

the proposed way to check for timeout seems to not work properly when Instant::now() overflow after saving deadline.

fn timeout_fn(timeout: Duration) -> impl Fn() -> Result<(), Error> {
    let deadline = Instant::now() + timeout;
    move || {
        if Instant::now() > deadline {
            Err(Error::Timeout)
        } else {
            Ok(())
        }
    }
}

Lets imagine Instant::now() is a U8 for simplicity, we are at tick 250, and set a timeout of 10tick. After 6 tick the Instant::now() overflow and return 0, causing the timeout to trigger 4 tick in advance.

afaik the proper way to handle overflow is

if Instant::now() - start > timeout {
}

this will fail only if this check is done less often than Instant overflow, at that point i don't think there is much that can be done, and was a failure case anyway

Dirbaio commented 1 year ago

Instant is 64 bits, precsiely to allow code to assume it never overflows. It's unlikely we'll ever add support for smaller Instants because the space savings aren't worth the footguns for most code.