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

sys/ztimer/xtimer_compat: provide more fallback options #20494

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Mar 20, 2024

Contribution description

The xtimer API can be used as a backend agnostic frontend to ztimer.
To better facilitate this (and also allow the use without a timer for simple sleep operations), add more fall-back implementations.

Testing procedure

Issues/PRs references

alternative to #19719

@benpicco benpicco requested a review from maribu March 20, 2024 15:20
@github-actions github-actions bot added Area: timers Area: timer subsystems Area: sys Area: System labels Mar 20, 2024
@Teufelchen1
Copy link
Contributor

Uncrustify is pretty unhappy with this one ;)

@benpicco benpicco force-pushed the ztimer/xtimer_compat-extend branch 2 times, most recently from 0942fbb to 8712ef9 Compare November 15, 2024 16:43
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 15, 2024
@benpicco benpicco force-pushed the ztimer/xtimer_compat-extend branch from 8712ef9 to a5c385a Compare November 15, 2024 16:51
@maribu
Copy link
Member

maribu commented Nov 15, 2024

I think the convention was to use IS_USED() with modules. We couod revisit this decision and do an s/IS_USED/IS_ACTIVE/, but rather in a dedicated PR.

We could discuss this in upcoming VMA and quickly vote on this.

@benpicco benpicco force-pushed the ztimer/xtimer_compat-extend branch from a5c385a to 10888b3 Compare November 15, 2024 18:01
@riot-ci
Copy link

riot-ci commented Nov 16, 2024

Murdock results

✔️ PASSED

18c1754 ztimer_xtimer_compat: warn if backend != ztimer_usec

Success Failures Total Runtime
10249 0 10249 16m:17s

Artifacts

@kfessel
Copy link
Contributor

kfessel commented Nov 18, 2024

I like this (xtimer_compat is a nice interface to ztimer).

I just like to raise the question: "Could we somehow ensure that the user we somehow ensure the user knows about the mapping to ztimer_msec happening?"

  • eg.: simply raise a warning and a pseudo module removes it

@benpicco benpicco force-pushed the ztimer/xtimer_compat-extend branch from 10888b3 to 78be9db Compare November 18, 2024 13:18
@benpicco benpicco force-pushed the ztimer/xtimer_compat-extend branch from 78be9db to be2c455 Compare November 18, 2024 13:29
@benpicco
Copy link
Contributor Author

What is that warning supposed to look like and what is it supposed to achieve?

The xTimer API is provided by ZTimer. This should have no influence on it's functionality, carry on and have a nice day!

@benpicco benpicco enabled auto-merge November 18, 2024 17:49
@chrysn
Copy link
Member

chrysn commented Nov 18, 2024

IIUC this is causing troubles with Rust builds – but riot-wrappers doesn't wrap (and doesn't plan to wrap) XTimer.

Does switching riot-sys in .cargo/config.toml to the branch of RIOT-OS/rust-riot-sys#51 solve the build issues?

@kfessel
Copy link
Contributor

kfessel commented Nov 21, 2024

What is that warning supposed to look like and what is it supposed to achieve?

The xTimer API is provided by ZTimer. This should have no influence on it's functionality, carry on and have a nice day!

I thought of a warning in case of ztimer_msec providing the service for xtimer_usleep when it might have some influence on functionality

@benpicco
Copy link
Contributor Author

Like so?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: sys Area: System Area: timers Area: timer subsystems CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants