Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: rtos-trace time missing #2541

Merged
merged 4 commits into from
Feb 9, 2024

Conversation

xgroleau
Copy link
Contributor

@xgroleau xgroleau commented Feb 6, 2024

Could not compile when rtos-trace was enabled due to missing embassy_time feature. Note that I added embassy-time for the rtos-trace feature

We could probably derive from embassy-time-driver::now() if it's preferred and don't want to depend on embassy-time

@xgroleau xgroleau changed the title fix: rtos-usage time missing fix: rtos-trace time missing Feb 6, 2024
@Dirbaio
Copy link
Member

Dirbaio commented Feb 6, 2024

embassy-executor intentionally doesn't depend on embassy-time, only on embassy-time-driver, so that it works with any embassy-time version (as long as both have the same embassy-time-driver version in common). Can you change this to use the "raw" embassy-time-driver API to get the current time?

Also can you add one line with the right features to ci.sh so that this doesn't regress again? thanks!

@xgroleau
Copy link
Contributor Author

xgroleau commented Feb 6, 2024

That's what I was feeling. Move the GCDs to the embassy-time-driver, but didn't reexport it in the embassy-time crate.

Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

thanks!

#[cfg(feature = "rtos-trace")]
impl rtos_trace::RtosTraceOSCallbacks for Executor {
fn task_list() {
// We don't know what tasks exist, so we can't send them.
}
#[cfg(feature = "integrated-timers")]
fn time() -> u64 {
Instant::now().as_micros()
const GCD_1M: u64 = gcd(embassy_time_driver::TICK_HZ, 1_000_000);
let us_per_tick = (1000 / GCD_1M) / (embassy_time_driver::TICK_HZ / GCD_1M);
Copy link
Member

@Dirbaio Dirbaio Feb 7, 2024

Choose a reason for hiding this comment

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

1000 -> 1_000_000

also: the order of operations here will accumulate error if tick rate is not a clean divisor of 1e6. Note:

  • embassy_time: (ticks * (1000 / GCD_1K)) / (embassy_time_driver::TICK_HZ / GCD_1K);
  • this PR: ticks * ((1000 / GCD_1K) / (embassy_time_driver::TICK_HZ / GCD_1K));

if you do the division first, it'll get rounded a bit, and that error will accumulate for every second passed. Or in extreme cases, if the tick rate is higher than 1MHz us_per_tick will be exactly 0. If you multiply first then divide, the rounding is just +/- 1us independently of the absolute value of the uptime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, just fixed it.

Sorry about the back in forth in this PR., I'll take my time for the future ones

@xgroleau xgroleau force-pushed the fix/executor-rtos-usage-time branch from 9c6322a to d48620d Compare February 8, 2024 14:00
Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

thanks!

@Dirbaio Dirbaio added this pull request to the merge queue Feb 9, 2024
Merged via the queue into embassy-rs:main with commit 5e82e32 Feb 9, 2024
6 checks passed
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.

2 participants