RTIC v2 RTC monotonic drivers

[kyp44]

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!

[kyp44]

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.

[jbeaurivage]

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

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

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.

[kyp44]

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]

Oh I see what you mean. I think in general unsafe should be reserved for memory unsafety, and not ensuring that other types of invariants are upheld (unless they're memory related, of course). The pacs mark all the bits register accessor functions as unsafe, because it can't know if the bits you're writing are a valid bit pattern for the specific register. In the case of COMP and PER, any bit pattern is valid, so there's no need special care.

Therefore in my opinion all methods should be marked safe, especially since they can't even be used outside the HAL.

[kyp44]

What I'll do is have every method be safe and then have a # Safety section for each that describes the conditions required before calling the method, which is usually just that the RTC has already been set to the mode by calling set_mode.

[jbeaurivage]

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::*;

[kyp44]

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]

There are two things to consider here:

• If we choose to include fugit::{ExtU64, RateExtU64} in the prelude, this might be a breaking change. Suppose a user has this bit of code:

  async fn wait_100_ms(){
      use atsamd_hal::prelude::*;
  
      Some32BitDelay::delay(100.millis()).await;
  }

  If we introduce the u64 versions, now the compiler won't be able to disambiguate between ExtU32::millis and ExtU64::millis, meaning the user will have to change their code to Some32BitDelay::delay(100_u32.millis()). Which is fine, we just have to be aware of it.

• I have no problems with adding this to the prelude:

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

  That being said I would also put this in lib.rs for completness:

  #[cfg(feature = "rtic")]
  pub use rtic_time;

[kyp44]

It looks like the ExtU32 (and RateExtU32) are used elswhere in the HAL, in timers and whatnot. But I think the u64 extensions are only used for the RTIC monotonic, so maybe we could just import these in the prelude as well only if the rtic feature is enabled to minimize the impact.

[jbeaurivage]

Sounds like a good compromise.

[jbeaurivage]

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.

[kyp44]

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]

Well, if we're going to be in the business of managing interrupts, I think it should make sense to have an interrupts module (outside of the async feature gate) to keep all the functionality in one place. I guess technically one could use the Monotonic trait without async, if they limited themselves only to the now() method.

but this is actually defined in the PAC

Sweet, I never noticed, let's use that instead then.

I made a PR that moves the interrupt functionality not specific to async into its own module - check out #807.

[kyp44]

Sounds good, we'll wait for #807 to be merged, then keep the rtic and async features independent.

I'll note that you could definitely use rtic without async if you just want to use async for delays, but don't necessarily need async interfaces to other peripherals. This may actually be the case for my project, especially since the drivers for the devices I need do not have async interfaces.

[jbeaurivage]

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.

[kyp44]

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

[jbeaurivage]

Ok, I've tested this on a SAMD21, and found it to be reasonably accurate. I found a few other minor discussion points. Once we've addressed all remaining items I'll definitely be ready to merge this.

[jbeaurivage]

Here I would suggest adding something like this, since users are much more likely to read the module docs than stumble across the docs for the set_monotonic_prio function:

// Uncomment if not using the RTIC RTOS:

// #[no_mangle]
// static RTIC_ASYNC_MAX_LOGICAL_PRIO: u8 = 1;

// This tells the monotonic driver the maximum interrupt
// priority it's allowed to use. RTIC sets it automatically,
// but you need to set it manually if you're writing
// a RTIC-less app.

[kyp44]

Yeah, that makes sense. The set_monotonic_prio function is not even public.