FluenTech / embedded-time

Time(ing) library (Instant/Duration/Clock/Timer/Period/Frequency) for bare-metal embedded systems
Apache License 2.0
90 stars 17 forks source link

Comparison of sub-second duration::Generic is broken #127

Open DrTobe opened 1 year ago

DrTobe commented 1 year ago

When two Instants are subtracted, this results in a duration::Generic. This happens, for example, if we want to measure how much time has passed between two moments in time. To compare such a duration to another duration, the other one has to be converted into a duration::Generic, too. Unfortunately, comparison between two duration::Generics is broken:

use embedded_time::{clock::Clock, fraction::Fraction, Instant, duration::Milliseconds};

struct TwoInstantClock {
    instants: core::cell::RefCell<Vec<u32>>,
}

impl Clock for TwoInstantClock {
    type T = u32;
    const SCALING_FACTOR: Fraction = Fraction::new(1, 1000);
    fn try_now(&self) -> Result<Instant<Self>, embedded_time::clock::Error> {
        let i = self.instants.borrow_mut().remove(0);
        Ok(Instant::new(i))
    }
}

fn main() {
    let clock = TwoInstantClock {
        instants: core::cell::RefCell::new(vec![0, 5]),
    };
    let zero = clock.try_now().unwrap();
    let five = clock.try_now().unwrap();
    println!("{}", five-zero > Milliseconds(0u32).into()); // prints true!
}

The problem here is that for comparison, the integer part of a duration::Generic is multiplied by the scaling factor, i.e. this roughly floors the duration to full seconds. For sub-second durations, which can be expected to happen a lot in embedded contexts, this just does not work because both values will be zero.

This could be changed to compare self.integer * self.numerator * rhs.denominator to rhs.integer * rhs.numerator * self.denominator but this drastically increases the possibility of overflows. More elaborate comparison schemes are imaginable but may be costly. I would assume that overflows or errors can not be avoided completely without increasing the bit width of the numbers involved.