Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RTIC Monotonic for RTC and TIMER #427

Merged
merged 79 commits into from May 24, 2024
Merged

RTIC Monotonic for RTC and TIMER #427

merged 79 commits into from May 24, 2024

Conversation

Kyrne
Copy link
Contributor

@Kyrne Kyrne commented Oct 3, 2023

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

@qwandor
Copy link
Member

qwandor commented May 23, 2024

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?

@ivario123
Copy link
Contributor

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
Copy link
Member

Thanks for working on this!

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

@ivario123
Copy link
Contributor

ivario123 commented May 23, 2024

@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.

@ivario123
Copy link
Contributor

ivario123 commented May 23, 2024

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
Copy link
Member

qwandor commented May 23, 2024

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

@qwandor
Copy link
Member

qwandor commented May 23, 2024

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?

@ivario123
Copy link
Contributor

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.

ivario123 and others added 2 commits May 23, 2024 20:07
* Test monotonic feature in CI.

* needs testing

* remove excessive unsafe

* Fix build.

* correct docs

---------

Co-authored-by: Andrew Walbran <qwandor@google.com>
@ivario123
Copy link
Contributor

ivario123 commented May 23, 2024

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
Copy link
Member

@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?

@ivario123
Copy link
Contributor

@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
Copy link
Member

qwandor commented May 24, 2024

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.

@qwandor qwandor merged commit 090ce0e into nrf-rs:master May 24, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants