Changes durations in the time module to use u64 instead of u32

[kyp44]

Summary

Currently many time durations pass/stored throughout the HAL use nanoseconds for maximum precision. However these are stored using a u32 for a maximum of about 4 seconds. Since delays/times longer than this could be useful and should be possible, this changes all type defs in the time module to use u64 instead, extending the maximum time (when stored as nanoseconds) to over 584 years.

There were also several places in the HAL that import fugit duration types directly. These were changed to use the type defs in the time time module instead so that the entire HAL uses these exclusively.

See the commit message for more details.

Note that the rate/frequency types defined in time still use u32 as rates above 4 GHz are not likely to be useful.

Also note that this is a breaking change.

Checklist

• All new or modified code is well documented, especially public items
• No new warnings or clippy suggestions have been introduced - CI will deny clippy warnings by default! You may #[allow] certain lints where reasonable, but ideally justify those with a short comment.

[ianrrees]

Interesting! I've not studied the ramifications of this change, but am curious if you've considered something like adding a 64b API to specific methods where someone would be more likely to want a long duration?

With these being 32b machines, there will be some ramifications in terms of memory usage, code size, atomicity that we might want to consider here. Particularly, I'm thinking of places like bitbanging drivers that we've seen are already are a bit marginal.

[kyp44]

Yeah that's a good point. A better approach could be to add new types in the time module instead, e.g. Nanoseconds64 and only require those when it makes sense to do so. This would certainly be preferable as far as backward compatibility is concerned.

I have another project that's keeping me busy for the next month or so, but when I get back to this stuff, how about I go through and assess where Nanoseconds is used in the HAL and what the limitations are for each when limited to a u32. We can then go through them and see where it makes sense to use Nanoseconds64 instead (or perhaps to offer both with two different methods to minimize breaking changes). How does that sound?

[ianrrees]

Great! Yeah, or perhaps there's a reasonable way to use generics to accomplish something similar.

Just to be clear - I'm not opposed to switching to 64b time types as this PR currently does, just want to make sure we understand the implications.

[kyp44]

Yeah generics may be more appropriate. I think assessing where this is used will be the first step, then we can decide where to go from there (add a generic parameter or a new Nanoseconds64 type or whatever). I pretty much just made the change from u32 to u64 willy nilly in this PR just because there is one particular spot that doesn't work correctly when restricted to u32. A more thoughtful approach will be better.

[kyp44]

@ianrrees I largely finished my other project early, so I had some time to assess this situation. The only duration type actually used within the hal is Nanoseconds, the other types (i.e. Microseconds, Milliseconds, and Seconds) are presumably just for user convenience.

Searching the HAL, the following methods take a Nanoseconds parameter so that their timers are limited to about 4 seconds:

• timer::TimerCounter::start - both for the CountDown and the timer_traits::InterruptDrivenTimer trait implementations
• rtc::Rtc<Count32Mode>::start - both for the CountDown and the timer_traits::InterruptDrivenTimer trait implementations
• timer::TimerFuture::delay - note that longer delays can be acheived using the delay_us or delay_ms methods of the DelayNs trait

There are also two helper structs that are affected as well. These calculate a clock divider and number of clock cycles to acheive certain time durations given a clock rate.

• timer_params::TimerParams - The new_ns method takes a Nanoseconds for the time, but I am not sure about the effect/limitation of a u32 beyond the 4 second limitation
• rtc::TimerParams - The new_us method takes a Nanoseconds for the time. Certain RTC clock prescalar (i.e. divider) values can never be selected with the 4 second limit for typical RTC clock rates. This is actually where I discovered the issue with the u32 limitation, see the following.

Normally, passing a duration that is too long to something that accepts an Into<NanosDurationU32> causes fugit to panic with a multiplication overflow. However, what led me to this PR is that I discovered some cases where a panic does not occur and, instead, the duration as expressed in nanoseconds is truncated to the lower 32 bits. This unexpected behavior without any kind of error is what led me to expand to u64. To be sure, this seems like a possible bug in fugit, and one I am still digging in to investigate.

Assuming this is something that can be resolved in fugit to eliminate this unexpected behavior, the question for us then simply becomes whether or not we want the HAL timers to be able to support times longer than 4 seconds and, if so, how this should best be achieved. The list above is exhaustive so this just affects the above timers and their helpers.

EDIT: I filed an issue with fugit for this to find out if it's a bug or not. Per the example there, passing 1.minutes() to any of the above timers will result in no errors and a ~4.2 second delay.

As a side note, regarding both the TimerParams structs above, probably for historic reasons the new method takes a frequency as it's main parameter, and calculates the cycles/divider for the period of that frequency. I would argue that this method should instead be called new_rate, new_rate_period, or new_period. In constrast the new_ns or new_us methods calculate the cycles/divider given an arbitrary duration of time. Though these both take a Nanoseconds argument, the time can be expressed in any units via the various methods of fugit::NanosDurationU32, e.g. 5.minutes() or 7.seconds(). As such, I would argue that these methods should be renamed to new_time or new_duration. However, this would be a breaking change since these are publically exposed.

Really, the TimerParams structs should probably also be refactored for better code re-use. Currently, timer_params::TimerParams is used for timer peripherals and for PWM. The rtc::TimerParams is (obviously) used for the RTC peripheral. These are implemented completely independently but there is substantial commonality. Refactoring, however, would also likely be a breaking change. Obviously these would belong in a separate PR if we wanted to improve them.

[ianrrees]

Thanks for the excellent research and writeup, @kyp44 ! I'm curious to see what results from that fugit issue.

Wouldn't worry too much about the CountDown implementations, as they're the old embedded-hal 0.2 which we're moving away from.

The HAL API in general isn't set in stone, and personally I'm a big fan of readable code, so like the ideas of renaming that new() and refactoring TimerParams as you suggest. Guess it would be best to do the refactor before sorting the 32b overflow, since there would presumably be less code churn that way, but in terms of the HAL release cadence it would be best if there's just one user-facing change - so maybe one PR is OK (or, two that we merge in sequence).

A low effort thought: it seems like this API is designed around an assumption that the Into impl will handle overflow in some reasonable way, and some extra effort will be required for longer delays:

pub trait InterruptDrivenTimer {
    ...
    fn start<T: Into<NanosDurationU32>>(&mut self, timeout: T);
    ...
}

Maybe we need separate methods which are a bit more explicit - something like the DelayNs API:

fn start_ns(&mut self, timeout: u32);
fn start_ms(&mut self, timeout: u32); // 49 days should be enough...

[kyp44]

There is a synchronous version of the DelayNs trait as part of embedded-hal v1.0, so it probably makes sense to implement that on these timers and the RTC. The way these work in both cases (sync and async) is that you just have to implement the delay_ns and the other methods then work properly by calling delay_ns repeatedly if needed so that all durations can be passed as u32.

There seem to be two ways to specify delay/timer durations: one is the above, and the other is by passing fugit duration types, which has the benefit that the same function can take different units of time using the fugit::ExtU32 methods. However, only the latter has the issue with u32. It seems as though the HAL generally supports both of these (e.g. TimerFuture uses fugit for delay() and also implements the async DelayNs).

I would propose the following, which I can work on:

• Wait to see what happens with the fugit issue before making any decisions about expanding Nanoseconds to u64 and resolving this PR. I think the unexpected behavior is bad, but if overflows always panic, then maybe we don't need to expand the Nanoseconds type. We still may consider adding a new Nanoseconds64 type and additional methods if we want to allow for delays longer than 4 seconds while preserving backward compatibilty.
• Propably also wait on the fugit issue to decide about new InterruptDrivenTimer methods. If we end up expanding the Nanoseconds type, then the additional methods (e.g. start_ms) won't be needed, unless we just want to have a DelayNs-like API here.
• Refactor and test the TimerParams to maximize code re-use and have more appropriate method names in its own PR.
• Implement DelayNs for the synchronous timer and Rtc types in a new PR. I'm actually working on fixing and refactoring the implementation for the Rtc abstraction after working on the RTIC RTC monotonic, so this could be rolled into that.

Thoughts?

[ianrrees]

That sounds like a good plan to me, thanks!