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
55 stars 16 forks source link

use gcd crate & provide a way to convert between `Instant` fractions #13

Closed TDHolmes closed 2 years ago

TDHolmes commented 2 years ago

remove existing const gcd implementation in favor of the GCD crate's implementation now that it provides it, and provide const into and try_into methods for converting between Instants of different fractions

korken89 commented 2 years ago

Hi, thanks for the PR! I see the lib for gcd was fixed to be const, nice! Good update.

On the conversion of Instant it's not valid sadly. The library does not have (and cannot have) any understanding of baselines for time, so Instant only makes sense if you use it within one thing that generates said Instant. To convert and compare we have Duration, so if a user want to compare time flow from different time generators (that provide Instants), they need to use Duration. It's the same idea as the standard library has.

Thank you for you effort in implementing it! I see no problem with the implementation itself. I think documentation should be added that states why conversions of Instants are invalid and that one should move to a Duration.

TDHolmes commented 2 years ago

Thanks for the sweet library!

So the reason I'm converting Instants in the first place is the requirement that object safe traits cannot have generic parameters. My embedded-profiling trait requires a way to read the clock to get an Instant in time so you can profile some function. Since I cannot have generic parameters, I made the arbitrary decision to do microsecond resolution.

Because of this decision, I end up doing the math of Instant conversion in all of the implementers of this trait (see systick / DWT). Are you saying that because of this philosophy, this math should never exist in fugit itself, but stay in these implementors because the exact conversion of an Instants ticks could be implementation defined for that specific clock? In my case, the Instants would only be coming from one clock, so the conversion should be fine. Is the main concern using two Instants from different clocks which might have different baselines? If so that can still occur, you could create a Duration from these Instants as long as they have the same fraction.

Do you have any thoughts on my use case? Do you think leaving the logic as is is fine, or should I be working with duration_since_epoch's? that seems less ergonomic and intuitive since they are really Instants.

korken89 commented 2 years ago

Ah I see what you are trying to do! I think this will do the trick:

let inst2dur = your_instant.duration_since_epoch();
let us: DurationMicrosU32 = inst2dur.convert();

Then you have effectively an instant in a duration, and durations you can convert. :) Does this solve your usecase?

TDHolmes commented 2 years ago

It’ll work, but it feels wrong that this would require the read_clock trait method to return durations instead of instants. I think I’d just prefer to do the tick manipulation math rather than store an instant in a duration

On Wed, Dec 22, 2021 at 9:57 PM Emil Fresk @.***> wrote:

Ah I see what you are trying to do! I think this will do the trick:

let inst2dur = your_instant.duration_since_epoch(); let us: DurationMicrosU32 = inst2dur.convert();

Then you have effectively an instant in a duration, and durations you can convert. :) Does this solve your usecase?

— Reply to this email directly, view it on GitHub https://github.com/korken89/fugit/pull/13#issuecomment-1000063097, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC5YNWUC5JMJRDLRANYSRD3USK25NANCNFSM5KRLSULA . You are receiving this because you authored the thread.Message ID: @.***>

korken89 commented 2 years ago

As I see it, your read_clock trait can have a specific Instant type and you then manually convert your clock rate to the correct instant. Or you can do the following in the implementation of read_clock, which will give you what you want while upholding the invariant of fugit:

fn read_clock(..) -> Instant<1, 1_000_000, ...> {
    let now = clock.now(); // Some unknown instant we want to Instant<1, 1_000_000, _>
    let us: DurationMicrosU32 = now.duration_since_epoch().convert();
    Instant::from_ticks(us.ticks())
}

This optimizes correctly to the same instructions as adding conversions to instants.

TDHolmes commented 2 years ago

Thanks! This makes sense to me and looks like it works. Opened #15 so it can all be const