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

Add scaling_factor method to FixedPoint #113

Closed Sh3Rm4n closed 3 years ago

Sh3Rm4n commented 3 years ago

For duration::Generic or rate::Generic getting the scaling factor is as easy as calling rate.scaling_factor().

For examle today I tried to muliply a Hertz variable with a Microsecond variable. To do this now, needs the following

use embedded_time::{
    fixed_point::FixedPoint,
    time::Microseconds,
    rate::Hertz,
};

// not really needed and sublte bugs are introduced if the type (and its internal scaling factor) will change
let time = Microseconds(10);
let rate = Hertz(1_000);

let calculation_result = time.integer() * rate.integer() * <Microseconds as FixedPoint>::SCALING_FACTOR;

This could also introduce subtle bugs, as the SCALING_FACTOR call is not bound to the time variable.

Providing a method, which just returns the value of the associated constant, allows this:


-let calculation_result = time.integer() * rate.integer() * <Microseconds as FixedPoint>::SCALING_FACTOR;
+let calculation_result = time.integer() * rate.integer() * time.scaling_factor();

The ergonomics of the Generic types are quite nice, but I miss it for the other types.

PTaylor-us commented 3 years ago

Thank you for the PR! There are a few great points here. First, we are fixing the need to create an object just to get the integer() by returning a copy of the value rather than a reference.

I hadn't really considered the need to multiply a rate and duration, so thank you for bringing that to my attention. You shouldn't have to get the integer values to do so. That will be rectified.

In addition, #92 will likely add a required scaling_factor() function implementation (or comparable) to the Duration and Rate types, effectively moving the implementation of your added function to the implementation of those traits.

I will accept your PR shortly and target the next release.