A time/rate solution

[MabezDev]

We currently use fugit, which has served us well, and works nicely overall. I do see a few issues with it though.

• It's unstable, I asked about this here: 1.0 release korken89/fugit#52
• I'm not sold on the extension traits. Whilst it can look nice to magically get this .khz() method on a u32 it's:
  • Not very discoverable, if you call it without the trait imported, the compiler will most likely throw a fit with a not very clear error message
  • There are ambiguous types involved when the backing for the type can be a u32 or a u64

I'm wondering if we should provide our own time types (which may very well use fugit under the hood), but we'll have more control on what we expose, and what we expect users to do.

For example, whilst it might be a bit more verbose, I think I'd prefer an API that looked like this:

pub struct Hertz(/* some type here, probably a `HertzU32` from fugit */);

impl Hertz {
	fn khz(khz: u32 /* unsure of type here, but u32 seems appropriate */) -> Hertz {
		// internally we can use fugit stuff, like the ext traits
		Hertz(khz.khz())
	}
}

The difference in use:

Before:

use fugit::ExtRate; // somewhere
// ..
.with_frequency(1000.khz())
// ..

After:

use esp_hal::time::Hertz; // somewhere
// ..
.with_frequency(Hertz::khz(1000))
// ..

^^ above is just an idea, I'd appreciate thoughts on this going forward.

[bugadani]

Well the structure should probably be called Rate or Frequency, as Hertz is a unit of measurement so Hertz::kHz is already a confusion :)

We're mostly working with u64 as timestamp, which means using u64 under Duration and Instant is logical. For Rate, I'm not sure, depends whether we want to work with rates faster than 4GHz :)

[burrbull]

After:

https://docs.rs/fugit/latest/fugit/struct.Rate.html#method.kHz

[MabezDev]

Well the structure should probably be called Rate or Frequency

Agree with this, thanks for pointing that out.

docs.rs/fugit/latest/fugit/struct.Rate.html#method.kHz

Thanks! I wasn't aware of those.

This has been on my mind this past week, and I'm leaning towards new typing fugit with a u64 for Duration/Instant and a u32 for Rate. Hopefully delegate will make this straight forward.

I'd appreciate any other input or suggestions here.

[bjoernQ]

I'm wondering if we should provide our own time types (which may very well use fugit under the hood), but we'll have more control on what we expose, and what we expect users to do.

...

This has been on my mind this past week, and I'm leaning towards new typing fugit with a u64 for Duration/Instant and a u32 for Rate. Hopefully delegate will make this straight forward.

I completely agree with this - this should be the road forward

[bugadani]

I think it's a reasonable approach. In the best case, we'll be able to just reexport fugit if it goes stable as is (and since the crate hasn't seen any change in the past 2 years, why would it not?). Worst case, we'll have the compatibility layer in place and we can even decide to move away to some other internal representation. I'm not sure why we'd want to, but we could.

[MabezDev]

Thanks for the input, I think we should go ahead with this. It's the last big API change in the 1.0-blockers list, so I think we should try and get this in for the beta. I think we should still be using fugit, as it is a very flexible, well tested library, but often a bit tricky for users to wrap their head around. Adding our own types wrapping it up should put us in a great position. There is a possibility that we may need more APIs than I've listed below, but I think this should cover most of it, and we can address the others in the implementation PR(s).

I have also been debating whether this belongs in esp-hal or new crate like esp-time because we can add From/Into impls for embassy-time and other time types if we wish, which might look a bit weird to have as features on esp-hal. I think I'm currently against this, as converting between these time types should be trivial, i.e embassy_time::Duration::from_millis(hal_duration.as_millis()). In the future we can split it out and rexport from the hal if we want to.

The plan

1. The types

pub struct Rate(fugit::Rate<u32, 1, 1>); // resolution: hz
pub struct Instant(fugit::Instant<u64, 1, 1_000_000>); // resolution: micros
pub struct Duration(fugit::Duration<u64, 1, 1_000_000>); // resolution: micros

2. Delegating to fugit: Rate

I think we want to be really small with the APIs we delegate to initially. I think the following should suffice for Rate:

Note: imo I see no reason to capitalize the MHz, imo we should expose this as mHz to be consistent with kHz

Update: with the discussion below, lets use lowercase for all here, and we'll bikeshed this in the PR.

3. Delegating to fugit: Instant

• duration_since_epoch/duration_since_boot?
  • alternatively we could expose as_millis, as_secs which on our time system is the same thing (I'll leave this open to the implementer and we can discuss in the PR)
• any Add or Sub trait impls to do Instant::now() - last_instant to get a duration etc

4. Delegating to fugit: Duration

I think we should adopt the from_ prefix that std and embassy-time use instead of the raw fugit api here.

• Duration::from_nanos()
• Duration::from_millis()
• Duration::from_secs()
• Duration::as_nanos()
• Duration::as_millis()
• Duration::as_secs()
• any Add or Sub trait impls etc that make sense

5. I don't think we should expose the RateExt traits etc anywhere. We can add this later of course, but currently I'm not a huge fan of having to import a magic trait to get an extra method on a primitive.

[jessebraham]

Note: imo I see no reason to capitalize the MHz, imo we should expose this as mHz to be consistent with kHz

This means milli-hertz, though. The kilo prefix is not capitalized but Mega is to differentiate it from milli.

[MabezDev]

This means milli-hertz, though. The kilo prefix is not capitalized but Mega is to differentiate it from milli.

Fair enough, I didn't know that was a unit until now :D. Does millihertz make sense as a unit for anything we might want to do? If not I propose we stick the plan and document that it is actually megahertz, this is what they're doing in https://docs.embassy.dev/embassy-stm32/git/stm32f030rc/time/struct.Hertz.html#method.mhz.

We can also bikeshed this in the impl PR, my main goal with that comment was consistency, I'm not particularly bothered about the exact route we choose.

[jessebraham]

These are globally agreed upon prefixes, and formalized in ISO/IEC 80000. I'd really rather not deviate from that, Embassy is wrong to do so IMO.

[bjoernQ]

I agree that mHz/kHz is not a good choice - mhz and khz wouldn't be an eye-sore to me however

We can also bikeshed this in the impl PR, my main goal with that comment was consistency, I'm not particularly bothered about the exact route we choose.

👍

[bugadani]

Does millihertz make sense as a unit for anything we might want to do?

I don't think anyone uses milli-hertz as a real unit. At that point you count things in seconds, bpm/rpm, or something more natural/intuitive. In general I wouldn't fight for this, but to me there is an appeal to physically correct capitalization, but lower case everything would be equally obvious I think.

[MabezDev]

Let's go with lowercase everything for now, and we can bikeshed it in the PR.