RTIC Monotonic for RTC and TIMER

Kyrne commented 11 months ago

This PR aims to add rtic monotonic implementations for the RTC and TIMER. As #384 requested. This would allow users of the hal to use native implementations of the Monotonic trait rather than third-party ones and therefore ensure future support for the feature.

Added

monotonic.rs ( holds all of the rtic monotonic related implementations )
2 new optional dependencies, fugit and rtic-monotonic
1 new feature flag for all hals --features monotonic
1 new example crate ( monotonic-blinky )

Features

Configurable frequency ( can use all valid frequencies for the rtc and timer respectively )
Compile time or initiation time guarantees that the frequency is valid.

Usage

There exists an example of how to instantiate the monotonic rtc and timer in nrf-hal/examples/monotonic-blinky.

RTC

The RTC cannot at compile time guarantee that the frequency is correct. It does however check this at initiation time, so for any invalid frequency it will throw an error leaving the user to correct the frequency. By taking a reference to the clock object we can ensure that the lfclk is enabled.

// RTC0 with a frequency of 32 768 Hz
type MyMono = MonotonicRtc<RTC0, 32_768>;

let clocks = hal::clocks::Clocks::new(cx.device.CLOCK);
let clocks = clocks.start_lfclk();

let mono = MyMono::new(cx.device.RTC0, &clocks).unwrap();

Timer

The timer abstraction supports frequency gating since there are only 9 different prescaler values. This ensures that the MonotonicTimer object is not constructible for any frequency that does not yield a valid prescaler. The documentation for the MonotonicTimer lists all valid frequencies in a table with their corresponding time until overflow.

// RTC0 with a frequency of 16 MHz
type MyMono = MonotonicTimer<TIMER0, 16_000_000>;
let mono = MyMono::new(cx.device.TIMER0).unwrap();

Recommended actions

Squash and merge to remove the long commit history.

Alternatives

There are already a few third-party implementations of these features that are less configurable

ivajon commented 11 months ago

@diondokter Not quite sure how the review process works but figured I'd mention you here since we have discussed earlier PRs

diondokter commented 11 months ago

Hey, this looks pretty nice! However...

I'm not a maintainer here (though I think I might have some limited permissions). In fact, this crate has no active maintainers at this point...

So I don't know how to continue. Best thing would be to find a new maintainer. Lately in the embedded rust matrix chat, the recommendation has been to go with embassy-nrf which you can use blockingly as well.

ivajon commented 6 months ago

@qwandor Would you mind looking through this?

qwandor commented 3 months ago

Thanks! I've updated this to work with current master and fixed a bunch of small things. It looks good, except that it doesn't build for nrf51 or nrf52832. Is that expected?

ivajon commented 3 months ago

Thanks! I've updated this to work with the current master and fixed a bunch of small things. It looks good, except that it doesn't build for nrf51 or nrf52832. Is that expected?

This should not be the case, at least not for the nrf52832 chip as the docs specify the same field, I think that it is simply a matter of the field names either not existing in the pac or being different from that of the nrf52840 that we tested on. I will look into it

BartMassey commented 3 months ago

Thanks for working on this!

I'm easily confused. Should this trait be named rtic-monotonic? Or is it more general than that?

ivajon commented 3 months ago

@qwandor it seems that we might need to modify the bits manually or re-write parts of the PAC. As this is a small feature I think that it is better to simply unsafely modify the bits and not re-write the PAC for this.

I implemented a short fix that should be tested before including as it adds three extra bits of unchecked unsafe code. https://github.com/Kyrne/nrf-hal/tree/fix_missing_parts_of_pac

I do, however, not have any direct way of testing this. So if you have time it would be great if you looked over the modifications so we at least have two sets of eyes on the unsafe bits before calling it good. Also if we don't want to include the extra unsafe bitmodifcations it might just be good to leave them out as you did. However, we should feature gate the entire monotonic mod, in that case, to not throw errors during future development.

ivajon commented 3 months ago

Thanks for working on this!

I'm easily confused. Should this trait be named rtic-monotonic? Or is it more general than that?

This is probably a good idea as it describes the feature better.

qwandor commented 3 months ago

Thanks! We should probably fix the PAC to add the missing fields, but I'm alright with this for now.

qwandor commented 3 months ago

Thanks for working on this!

I'm easily confused. Should this trait be named rtic-monotonic? Or is it more general than that?

Do you mean the feature flag?

ivajon commented 3 months ago

Thanks! We should probably fix the PAC to add the missing fields, but I'm alright with this for now.

If at all possible it would be great to test it on these devices though, I only have access to 52840 devkit so I cannot test it on any other devices.

ivajon commented 3 months ago

Excuse the force push, I resolved the conflicts with a PR instead. I also renamed the user-facing feature to rtic-monotonic as I find it describes the feature better. The feature in nrf-hal-common is still simply monotonic to not conflict with the name of the dependency. And I modified the tests to reflect the naming change.

Did not know feat:... was a thing, the more you know!

BartMassey commented 3 months ago

@qwandor Yeah, meant the feature flag.

I think we should probably have someone test any unsafe and get some good eyes on it before we merge that. I'm bad at Github: is there a specific commit/diff that I'm missing that does that part?

ivajon commented 3 months ago

@qwandor Yeah, meant the feature flag.

I think we should probably have someone test any unsafe and get some good eyes on it before we merge that. I'm bad at Github: is there a specific commit/diff that I'm missing that does that part?

Testing the unsafe parts should be as simple as porting the blinky example to the affected devices and running it, does it work it is correct.

qwandor commented 3 months ago

Unfortunately I don't have any nRF51 or nRF52832 board either. I've tested it on an nRF52833 and that works at least, and the code looks reasonable to me.

nrf-rs / nrf-hal