Skip to content

embassy-time-driver using RTC interrupts - #825

Open
stillinbeta wants to merge 21 commits into
atsamd-rs:masterfrom
stillinbeta:embassy-interrupt
Open

embassy-time-driver using RTC interrupts#825
stillinbeta wants to merge 21 commits into
atsamd-rs:masterfrom
stillinbeta:embassy-interrupt

Conversation

@stillinbeta

@stillinbeta stillinbeta commented Mar 11, 2025

Copy link
Copy Markdown
Contributor

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

  • 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! 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

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

Comment thread hal/src/rtc/mod.rs Outdated
Comment thread hal/src/rtc/embassy/mod.rs Outdated
if RtcMode0::check_interrupt_flag::<Compare0>(&rtc) {
// Due to synchronization delay, the RTC may be slightly behind
// Assume the current time is the time the interrupt is set for
let now = RtcMode0::get_compare(&rtc, 0) as u64;

@stillinbeta stillinbeta Mar 25, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sjroe solved this by setting the prescaler to four, thus ensuring the resolution was coarse enough to not cause issues. I like my solution better, but am open to suggestions

@stillinbeta
stillinbeta force-pushed the embassy-interrupt branch 3 times, most recently from 9182bd0 to f2af62e Compare March 27, 2025 02:42
@stillinbeta
stillinbeta marked this pull request as ready for review March 27, 2025 02:42
Comment thread boards/pyportal/examples/blinky_interrupt.rs Outdated
Comment thread hal/src/rtc/modes.rs Outdated
@stillinbeta
stillinbeta marked this pull request as draft March 27, 2025 03:46
@stillinbeta stillinbeta changed the title (wip) embassy-time-driver using RTC interrupts embassy-time-driver using RTC interrupts Mar 27, 2025
@stillinbeta
stillinbeta force-pushed the embassy-interrupt branch 3 times, most recently from 1a81ae9 to 3d7a961 Compare March 28, 2025 02:43
@stillinbeta
stillinbeta marked this pull request as ready for review March 28, 2025 02:47

@jbeaurivage jbeaurivage left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @stillinbeta! I intend to review this shortly. In the meantime, could you revert these two changes? Thanks!

Comment thread boards/pyportal/.cargo/config.toml
Comment thread boards/pyportal/Cargo.toml Outdated
ritikmishra added a commit to ritikmishra/atsamd that referenced this pull request Jun 4, 2025
Comment thread hal/src/rtc/embassy.rs
Comment on lines +34 to +41
let at = match u32::try_from(at) {
Ok(at) => at,
_ if at == u64::MAX => u32::MAX,
Err(_) => return false,
};

RtcMode0::set_compare(rtc, 0, at);
true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am experiencing race condition in this implementation where it is possible for the RTC counter to advance past at while COMP0 is being set

the [embassy timer queue implementation for stm32 microcontrollers] tries to prevent this from happening by avoiding setting alarms for timestamps in the very near future:

kyp44 added a commit to kyp44/atsamd that referenced this pull request Jun 13, 2025
…some minor changes made for the embassy time driver (see PR atsamd-rs#825).
kyp44 added a commit to kyp44/atsamd that referenced this pull request Jun 13, 2025
…some minor changes made for the embassy time driver (see PR atsamd-rs#825).
@kyp44

kyp44 commented Jun 14, 2025

Copy link
Copy Markdown
Contributor

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 clock::v2 API migration discussion and tracking #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 added a commit to kyp44/atsamd that referenced this pull request Jun 18, 2025
…some minor changes made for the embassy time driver (see PR atsamd-rs#825).
kyp44 added a commit to kyp44/atsamd that referenced this pull request Jun 20, 2025
…some minor changes made for the embassy time driver (see PR atsamd-rs#825).
ritikmishra added a commit to ritikmishra/atsamd that referenced this pull request Jul 9, 2025
ritikmishra added a commit to ritikmishra/atsamd that referenced this pull request Jul 9, 2025
update interrupt handling code

close up modes

Remove unneeded examples

don't need semihosting

remove rtt-target

roll back config.toml
@stillinbeta

Copy link
Copy Markdown
Contributor Author

@kyp44 thanks for doing the hard work of rebasing this back onto master! I've gone ahead and removed all the pyportal changes (they're in #1008 now). I'm going to see what I can do about the race condition @ritikmishra found.

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.

4 participants