RTIC v2 RTC monotonic drivers #804

kyp44 · 2024-12-21T00:08:43Z

Summary

Adds RTC-based RTIC v2 monotonics drivers.

This addresses issue #765 and is the culmination of a lot of discussion there.

Note that it was decided along with the RTIC team that ATSAMD RTIC monotonics should be released as part of the HAL instead of in rtic-monotonics.

Note that this release of the monotonics is opinionated in the sense that only the best monotonic is provided for each chip variant. See the module documentation of rtc::rtic for details. For reference, the alternative of releasing two monotonics, one for each RTC mode, is preserved in the rtic-v2-rtc-monotonics-full branch. Compare the module documentation of rtc::rtic in that branch for details on why there is clearly a best choice for each chip variant.

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!

…d mode1 monotonics.

… and macros, now uses type-level programming to specify the RTC clock source at compile time.

…lication in the HAL. * Moves the two monotonics into their own files so that the `rtc::rtic` main module file is not so large and unwieldy. * Adds thorough documentation to the `rtc::rtic` module and its items, discussing the differences between the two monotonics, and how to use the macros to create them.

…lication. * Finally achieves ostensibly robust operation for both monotonics in which they can operate solidly for at least 12 hours under very heavy task load. * Adds in instrumentation to monitor the monotonics and panic with useful debugging messages if an abnormal condition or stall is detected.

…lication. * Major refactor to decouple the RTC mode from the monotonic backend type (basic or half-period counting). * This enables using half-period counting in mode 0 for SAMx5x chips without duplicating as much mode. * Updates and expands the documentation to account for the changes. * Leaves in the monitor instrumentation so it is captured in the new form.

… that the monotonics have been thoroughly tested.

…ons and moves RTIC-specific things, so they can be used for other purposes as well.

…sing the best RTC mode for the chip variant. Updates the documentation accordingly.

…ng things with the `build-all.py` script. * Renames the `wait_for_count_change` to `_wait_for_count_change` function name to suppress an unused warning for SAMD11/21 chips. * Fixes the `metro_m0` example `blinky_rtic.rs` to fix an RTIC v1 item location change.

kyp44 · 2024-12-21T18:08:59Z

Ok, I just discovered a stress test freezing issue when building in release mode, which I had not really tested before. Please do not merge until I get this fixed.

…ess a freezing issue in release builds.

jbeaurivage · 2024-12-22T23:46:14Z

@kyp44 does the last commit address all the freezing issues? I'm hoping to have the time to read the PR in detail soon™.

kyp44 · 2024-12-23T02:11:52Z

@jbeaurivage Yeah, this does fix the issue, which, despite taking me hours to troubleshoot, was a very simple fix. I am still letting the stress test run for a long period of time, but I am 98% confident that there will not be further issues. I like to wait at least 36 hours to get a hardware counter rollover to be sure. Here is a timer showing how much time is left. If you want to be sure not to waste any of your time, wait until this timer is done, and I will check in and confirm that it's still going.

jbeaurivage

I have a few comments, some of which require changes, and some of which are at your discretion. But overall well done, this is some quality work!

Ideally I'd like to somehow get this tested on a thumbv6 target as a sanity check before merging.

By the way @kyp44, am I correct in my understanding that there are no breaking changes in this PR?

jbeaurivage · 2024-12-23T03:15:04Z

hal/src/rtc/modes.rs

+    /// Sets this mode in the CTRL register.
+    unsafe fn set_mode(rtc: &Rtc);
+    /// Sets a compare value.
+    unsafe fn set_compare(rtc: &Rtc, number: usize, value: Self::Count);


Here, I would suggest adding a # Safety section to the doc comments, explaining why these functions are unsafe and what are the invariants that must be upheld.

These particular methods are unsafe because writing to the required registers is unsafe in the PAC (e.g. the compare registers) and this was simply passed up to the enclosing function. However, there are other registers that are safe to write to in the PAC (e.g. the interrupt flag register). It is not clear to me why some are safe and others unsafe.

Really this entire low-level interface (i.e. the RtcMode trait) is unsafe from the perspective of ensuring that the RTC is used correctly, and not necessary unsafe from a memory perspective. This unsafety is necessary because the interface has to be static because the RTIC monotonic has a static interface. By static I of course mean that all methods are associated functions and no state is maintained.

Given this, I'm not sure what the approach should be. Should every method be marked as unsafe, or should they all be safe? Is unsafe in Rust supposed to just refer to memory safety or does safety at the peripheral usage level count? Either way, probably every method should at least have a # Safety section to document how the peripheral can be used safely. Thoughts?

jbeaurivage · 2024-12-23T03:27:32Z

hal/src/rtc/rtic/mod.rs

+/// TODO: This probably needs to be modernized (e.g. it does not implement
+/// half-period counting) or deprecated/removed.


I'm partial to adding a #[deprecated] attribute. Then we can yeet it at the next opportune occasion.

If we choose to deprecate the item, its removal should be requested in #784.

👍 I agree that deprecation makes sense here rather than putting forth the effort to modernize.

I'm assuming that it only makes sense to add the item to #784 once this gets merged.

EDIT: The v1 module was deprecated in commit 71b308.

jbeaurivage · 2024-12-23T03:30:26Z

hal/src/rtc/rtic/mod.rs

+pub mod prelude {
+    pub use super::rtc_clock;
+    pub use crate::rtc_monotonic;
+    pub use rtic_time::Monotonic;
+
+    pub use fugit::{self, ExtU32, ExtU32Ceil, ExtU64, ExtU64Ceil};
+}


I don't want to start bikeshedding here, but I'm not a big fan of preludes in general. Although here, we already reexport fugit in lib.rs, so there's potential for conflict if a user does this:

use atsamd_hal::fugit::ExtU32; use atsamd_hal::rtc::rtic::prelude::*;

I think it's fine to remove the RTIC-specific prelude, though I would recommend adding fugit::ExtU64 and fugit::RateExtU64 to the overall HAL prelude since the u32 versions are there. I would also opt to add the following to the HAL prelude:

#[cfg(feature = "rtic")] pub use rtic_time::Monotonic as _;

This is just so the methods of the monotonic can be used without users having to add rtic_time as a dependency. An alternative would be to just re-export this trait somewhere rather than having it in the prelude. Thoughts?

jbeaurivage · 2024-12-23T03:37:16Z

hal/src/rtc/rtic/mod.rs

+const fn cortex_logical2hw(logical: u8, nvic_prio_bits: u8) -> u8 {
+    ((1 << nvic_prio_bits) - logical) << (8 - nvic_prio_bits)
+}


There is already a very similar function in the HAL (here). If you're up for it, it would be nice to consolidate them to avoid duplication.

The similar function is gated behind the async feature. Currently, the rtic feature is independent from this, but perhaps it makes sense for rtic to also enable async?

Also, I wanted to point out that here defines NVIC_PRIO_BITS based on the chip variant, but this is actually defined in the PAC (e.g. here or here), so does not need to be defined manually in the HAL. I'll note that the modified set_monotonic_prio would also need to depend on NVIC_PRIO_BITS.

jbeaurivage · 2024-12-23T03:42:01Z

hal/src/rtc/rtic/mod.rs

+/// This function was copied from the private function in `rtic-monotonics`.
+unsafe fn set_monotonic_prio(prio_bits: u8, interrupt: impl cortex_m::interrupt::InterruptNumber) {
+    extern "C" {
+        static RTIC_ASYNC_MAX_LOGICAL_PRIO: u8;


An extern static is pretty unusual in Rust. I've personally scratched my head for a while on exactly this RTIC_ASYNC_MAX_LOGICAL_PRIO thing not so long ago. I would definitely explain in the docs somewhere what this u8 means, and that rtic will initialize the static, but users writing rtic-less apps should set the static themselves using

#[no_mangle] pub static RTIC_ASYNC_MAX_LOGICAL_PRIO: u8 = (something);

The requested documentation was added with commit 71b308.

jbeaurivage · 2024-12-23T04:01:31Z

Oh also, not a big deal at all, but #[hal_macro_helper] is meant to be able to add the #[hal_cfg] attribute to blocks and statements inside functions, since that's not currently supported by the compiler (apart from the #[cfg] attribute) . For items outside of functions, simply doing use atsamd_hal_macros::hal_cfg is sufficient.

…ing the PR (atsamd-rs#804) * Adds #[hal_macro_helper] to functions as required and removes them from other, non-function items. * Adds documentation to `rtc::rtic::set_monotonic_prio` about the dependence on the RTIC static variable `RTIC_ASYNC_MAX_LOGICAL_PRIO`.

…ing the PR (atsamd-rs#804) * Deprecates the entire `rtc::rtic::v1` module, which primarily just implements the old RTIC v1 `Monotonic` trait for `Rtc<Count32Mode>`. * Adds #[hal_macro_helper] to functions as required and removes them from other, non-function items. * Adds documentation to `rtc::rtic::set_monotonic_prio` about the dependence on the RTIC static variable `RTIC_ASYNC_MAX_LOGICAL_PRIO`.

kyp44 · 2025-01-02T22:26:46Z

@jbeaurivage I just added comments to the particular items above as some require further discussion I think.

The latest commit (71b308) addresses/fixes some items as noted above, including your note about #[hal_macro_helper].

Regarding testing on thumbv6 targets, I may be getting a Feather M0 in the next couple of weeks to carry out and test the clocks v2 API work. If someone is able to test the monotonic in the meantime that would be great! I will also note that I just had the monotonic on the PyGamer running for over a week straight with no issues.

As for breaking changes, this PR does technically have breaking changes, but it is something minor and silly. In particular, in the current master with rtic enabled, the types rtc::Duration and rtc::Instant are publicly defined, I suppose to support the the RTIC v1 Monotonic implementation. In the current PR branch, these are moved to rtc::rtic::v1 as they are specific to RTIC v1.

Dan Whitman and others added 9 commits December 11, 2024 14:43

feat(rtic-v2-rtc-monotonics): Initial import of both the RTC mode0 an…

85413e3

…d mode1 monotonics.

feat(rtic-v2-rtc-monotonics): Refactors and simplifies the monotonics…

ac0b428

… and macros, now uses type-level programming to specify the RTC clock source at compile time.

feat(rtic-v2-rtc-monotonics): Removes the monitor instrumentation now…

e423c13

… that the monotonics have been thoroughly tested.

feat(rtic-v2-rtc-monotonics): Factors out the low-level RTC abstracti…

00081de

…ons and moves RTIC-specific things, so they can be used for other purposes as well.

feat(rtic-v2-rtc-monotonics): Only includes a single RTC monotonic, u…

9d77960

…sing the best RTC mode for the chip variant. Updates the documentation accordingly.

kyp44 mentioned this pull request Dec 21, 2024

atsamd21: initial implementation usign TC4/5 rtic-rs/rtic#994

Open

feat(rtic-v2-rtc-monotonics): Extends the MIN_COMPARE_TICKS to addr…

d05b300

…ess a freezing issue in release builds.

jbeaurivage requested changes Dec 23, 2024

View reviewed changes

kyp44 force-pushed the rtic-v2-rtc-monotonics branch from 78700c6 to 71b308c Compare January 2, 2025 22:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RTIC v2 RTC monotonic drivers #804

RTIC v2 RTC monotonic drivers #804

kyp44 commented Dec 21, 2024 •

edited

Loading

kyp44 commented Dec 21, 2024

jbeaurivage commented Dec 22, 2024

kyp44 commented Dec 23, 2024 •

edited

Loading

jbeaurivage left a comment •

edited

Loading

jbeaurivage Dec 23, 2024

kyp44 Jan 2, 2025

jbeaurivage Dec 23, 2024

jbeaurivage Dec 23, 2024

kyp44 Jan 2, 2025 •

edited

Loading

jbeaurivage Dec 23, 2024

kyp44 Jan 2, 2025

jbeaurivage Dec 23, 2024

kyp44 Jan 2, 2025

jbeaurivage Dec 23, 2024

kyp44 Jan 2, 2025

jbeaurivage commented Dec 23, 2024

kyp44 commented Jan 2, 2025

		/// TODO: This probably needs to be modernized (e.g. it does not implement
		/// half-period counting) or deprecated/removed.

RTIC v2 RTC monotonic drivers #804

Are you sure you want to change the base?

RTIC v2 RTC monotonic drivers #804

Conversation

kyp44 commented Dec 21, 2024 • edited Loading

Summary

Checklist

kyp44 commented Dec 21, 2024

jbeaurivage commented Dec 22, 2024

kyp44 commented Dec 23, 2024 • edited Loading

jbeaurivage left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyp44 Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbeaurivage commented Dec 23, 2024

kyp44 commented Jan 2, 2025

kyp44 commented Dec 21, 2024 •

edited

Loading

kyp44 commented Dec 23, 2024 •

edited

Loading

jbeaurivage left a comment •

edited

Loading

kyp44 Jan 2, 2025 •

edited

Loading