Skip to content

Add scaling_factor method to FixedPoint#113

Merged
PTaylor-us merged 2 commits into
FluenTech:release/v0.13.0from
Sh3Rm4n:fixed-point
Sep 12, 2021
Merged

Add scaling_factor method to FixedPoint#113
PTaylor-us merged 2 commits into
FluenTech:release/v0.13.0from
Sh3Rm4n:fixed-point

Conversation

@Sh3Rm4n

@Sh3Rm4n Sh3Rm4n commented Aug 20, 2021

Copy link
Copy Markdown

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

PTaylor-us commented Aug 29, 2021

Copy link
Copy Markdown
Member

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.

@PTaylor-us PTaylor-us linked an issue Aug 29, 2021 that may be closed by this pull request
@PTaylor-us PTaylor-us self-assigned this Aug 29, 2021
@PTaylor-us PTaylor-us added enhancement New feature or request good first issue Good for newcomers labels Aug 29, 2021
@PTaylor-us PTaylor-us added this to the v0.13.0 milestone Aug 29, 2021
@PTaylor-us PTaylor-us changed the base branch from master to release/v0.13.0 September 12, 2021 20:17
@PTaylor-us PTaylor-us merged commit 927e0bf into FluenTech:release/v0.13.0 Sep 12, 2021
@Sh3Rm4n Sh3Rm4n deleted the fixed-point branch September 13, 2021 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request good first issue Good for newcomers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

optimize generic Duration usage

2 participants