korken89 / fugit

`fugit` provides a comprehensive library of `Duration` and `Instant` for the handling of time in embedded systems, doing all it can at compile time.
Apache License 2.0
56 stars 16 forks source link

Comparison with Instant of u32::MAX is unsound #35

Closed henrikssn closed 2 years ago

henrikssn commented 2 years ago

I try this code on a thumbv7em-none-eabihf target:

defmt::println!("{:?}", TimerInstantU32::<100_000_000>::from_ticks(u32::MAX) > TimerInstantU32::<100_000_000>::from_ticks(1000));

Output:

0.099983 false

Since u32::MAX > 1000, something seems to be wrong inside of const_cmp?

korken89 commented 2 years ago

Hi, this looks correct to me. Instant supports time which can roll over, so the maximum range is MAX/2 - 1.

I think the documentation needs clarification here

korken89 commented 2 years ago

And thanks for bringing this to my attention!

henrikssn commented 2 years ago

Interesting, thanks for the explanation! What do you think about adding an Instant::MAX constant? :)

korken89 commented 2 years ago

I see what you want, but it would be difficult. As every instant is relative another instant there is no absolute MAX value, it's simply MAX/2 - 1 that is the maximum relative offset that is valid without overflow.

Does that make sense?

henrikssn commented 2 years ago

Hmm, I think so. If we would define u32::MAX/2 - 1 as MAX, would it then be true that

Instant::MAX + Instant::from_ticks(1) > Instant::MAX
korken89 commented 2 years ago

That is correct.

korken89 commented 2 years ago

You can run this test to verify:

    #[test]
    fn yolo() {
        let max = Instant::<u32, 1, 1>::from_ticks(core::u32::MAX / 2 - 1);
        let maxpone = Instant::<u32, 1, 1>::from_ticks(core::u32::MAX / 2);

        assert!(maxpone > max);
    }