atsamd icon indicating copy to clipboard operation
atsamd copied to clipboard

embassy-time-driver using RTC interrupts

Open stillinbeta opened this issue 8 months ago • 1 comments

Summary

These changes implement the embassy-time-driver interface, used by Embassy for scheduling and a prerequisite for the Embassy runtime.

These changes are confined to the embassy-time feature.

I have only tested these changes on the pyportal board (see examples provided).

Checklist

  • [x] All new or modified code is well documented, especially public items
  • [x] 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.

If Adding a new Board

  • [ ] Board CI added to crates.json
  • [ ] Board is properly following "Tier 2" conventions, unless otherwise decided to be "Tier 1"

If Adding a new cargo feature to the HAL

  • [x] Feature is added to the test matrix for applicable boards / PACs in crates.json

Note

The crate changelogs should no longer be manually updated! Changelogs are now automatically generated. Instead:

  • If your PR is contained to a single crate, or a single feature:
    • Nothing else to do; your PR will likely be squashed down to a single commit.
    • Please consider using conventional commmit phrasing in the PR title.
  • If your PR brings in large, sweeping changes across multiple crates:
    • Organize your commits such that each commit only touches a single crate, or a single feature across multiple crates. Please don't create commits that span multiple features over multiple crates.
    • Use conventional commmits for your commit messages.

stillinbeta avatar Mar 11 '25 02:03 stillinbeta

As I'm interested in this getting merged, I had a good look at at and tested it. Here are some comments/questions:

  1. The implementation is well done, and I appreciate that it utilizes the RTC modes abstraction layer instead of accessing the registers directly.
  2. I tested this both on a Metro M0 (SAMD21G) and the PyGamer (SAMD51J) and it works really well.
  3. For safety reasons, it seems like the driver init function should require the pac::Rtc as not doing so allows the user to simultaneously use the Rtc, which would be bad. Doing so would also allow init to not need to be unsafe. Is there particular a reason why this was not done?
  4. init requires proof of the RTC clock being configured (e.g. RtcClock for thumbv6m chips), there are a couple of concerns with this: a. For thumbv7 chips, this requires v2 of the clock API, which is not well support yet throughout the rest of the HAL, making it difficult to actually use for most projects. Perhaps an alternative init function could be added (and later removed after full migration) that does not require proof so as to be compatible with v1 on thumbv7 chips. Edit: Issue #912 tracks the progress of the clock v2 API migration. b. This allows the user to configure the RTC clock to be whatever clock rate they like. However, embassy will always assume that the rate is 32.768 kHz due to passing the tick-hz-32_768 flag to embassy-time-drive. Unfortunately, unlike RTIC, it seems like the only way to specify the time driver tick rate is via these feature flags. We could just allow the user to set this flag themselves in their embassy-time dependency, but, annoyingly, this defaults to 1 MHz if nothing is specified, which will result in odd problems if the user is not aware they need to pass this themselves. An alternative would to for the driver to initialize the Rtc clock itself, always to the same rate, but this does not really seem possible to do for thumbv6 chips in a way that avoids at best a runtime panic. I believe with v2 (thumbv7 targets only right now) this could be done with compile-time assurances by simply passing in the RTC clock token.
  5. As the embassy time counter is inherently a u64 while the RTC only supports u32, it would be good to enhance this with half-period counting (HPC) as the RTIC monotonic uses, otherwise tasks could stall around the 36-hour mark. This is an enhancement that could be added later that would not affect the API at all. The correct way to do this would probably be a single abstraction that incorporates HPC and can be used by both the embassy time driver and the RTIC monotonic. This is something I am certainly willing to take a crack at after this gets merged.

Note: This PR makes a few changes to the RTC modes abstraction. I also made some changes and additions with my RTC fixes in PR #845. In that PR I've already merged the changes from this one, so that version should take precedence in the event of any conflicts.

Edit: For my own purposes, I'm maintaining this branch in my fork that I am keeping up to date with atsamd-rs/atsamd:master (via rebasing) and have correctly merged in the changes from PR #845 discussed above. Since this is rebased, I don't know that a PR against stillinbeta/atsamd:embassy-interrupt would work correctly.

kyp44 avatar Jun 14 '25 02:06 kyp44