smoltcp

smoltcp copied to clipboard

https://github.com/smoltcp-rs/smoltcp/blob/master/src/time.rs#L71

While reviewing the crate a bit, I noticed this implementation, which seems a bit suspicious to me. It creates a smoltcp::time::Instant from an std::time::Instant by looking at how much time has elapsed since said instant was created? This seems super wrong to me.

smoltcp::time::Instant is supposed to represent the number of milliseconds passed since an arbitrary point in time (e.g. be monotically increasing, with a frequency of 1000Hz). Calling smoltcp::Instant::from(std::Instant) twice with the same argument should always return the same value. This cannot be done with elapsed, since it returns the amount of time since the previous instant. See this playground

I expected From<Instant> to be implemented along the lines of:

#[cfg(feature = "std")]
impl From<::std::time::Instant> for Instant {
    fn from(other: ::std::time::Instant) -> Instant {
        lazy_static! {
            static ref REFERENTIAL: Instant = Instant::now();
        }
        let elapsed = other - *REFERENTIAL;
        Instant::from_millis((elapsed.as_secs() * 1_000) as i64 + (elapsed.subsec_nanos() / 1_000_000) as i64)
    }
}

Oh yeah that's a bug.

@roblabla Great catch! I like your solution. Only nit is that I'd prefer other.duration_since(*REFERENTIAL), but that is just a nit. A PR with a test would be appreciated!

If you don't have the time to submit one, it is totally understandable and I'd be happy to implement this.