atsamd
atsamd copied to clipboard
embassy-time-driver using RTC interrupts
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.
As I'm interested in this getting merged, I had a good look at at and tested it. Here are some comments/questions:
- The implementation is well done, and I appreciate that it utilizes the RTC modes abstraction layer instead of accessing the registers directly.
- I tested this both on a Metro M0 (SAMD21G) and the PyGamer (SAMD51J) and it works really well.
- For safety reasons, it seems like the driver
initfunction should require thepac::Rtcas not doing so allows the user to simultaneously use theRtc, which would be bad. Doing so would also allowinitto not need to beunsafe. Is there particular a reason why this was not done? initrequires proof of the RTC clock being configured (e.g.RtcClockforthumbv6mchips), there are a couple of concerns with this: a. Forthumbv7chips, 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 alternativeinitfunction could be added (and later removed after full migration) that does not require proof so as to be compatible with v1 onthumbv7chips. 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 thetick-hz-32_768flag toembassy-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 theirembassy-timedependency, 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 forthumbv6chips in a way that avoids at best a runtime panic. I believe with v2 (thumbv7targets only right now) this could be done with compile-time assurances by simply passing in the RTC clock token.- As the embassy time counter is inherently a
u64while the RTC only supportsu32, 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.