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.

Sign up for GitHub

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

Jump to bottom

sys/*timer: rework dependencies to ease backend switch, prefer ztimer_xtimer_compat over xtimer_on_ztimer #17811

Merged
merged 7 commits into from
Mar 18, 2022
Merged

sys/*timer: rework dependencies to ease backend switch, prefer ztimer_xtimer_compat over xtimer_on_ztimer #17811

merged 7 commits into from
Mar 18, 2022

Conversation

fjmolinas
Copy link
Contributor

@fjmolinas fjmolinas commented Mar 16, 2022 •
edited
Loading

Contribution description

Although I prefer how it's done in #17809, I realize I'm changing a lot of things when the initial objective was only to have an easy-to-switch backend default for xtimer, so let's do it simple, even if it keeps all the indirections...

Note this PR also changes the default prefered xtimer backend to ztimer_on_xtimer, that can also be an individual PR.

Marking as WIP will I let Murdock check I didn't make a fundamentally wrong change.

Testing procedure

  • Green Murdock

Issues/PRs references

#17721

@fjmolinas fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 16, 2022
@github-actions github-actions bot added Area: build system Area: Build system Area: CI Area: Continuous Integration of RIOT components Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: tests Area: tests and testing framework Area: timers Area: timer subsystems labels Mar 16, 2022
@fjmolinas fjmolinas force-pushed the pr_xtimer_dep_rework_simple branch 2 times, most recently from ba51ac8 to 073e369 Compare March 16, 2022 08:08
@github-actions github-actions bot removed the Area: CI Area: Continuous Integration of RIOT components label Mar 16, 2022
@github-actions github-actions bot added the Area: CI Area: Continuous Integration of RIOT components label Mar 16, 2022
@fjmolinas fjmolinas force-pushed the pr_xtimer_dep_rework_simple branch 2 times, most recently from 4990961 to f29d966 Compare March 16, 2022 08:35
@fjmolinas
Copy link
Contributor Author

If the ci passes the diff switching over would be:

diff --git a/sys/xtimer/Kconfig b/sys/xtimer/Kconfig
index f47f7bdc8d..e1d2dbd3b1 100644
--- a/sys/xtimer/Kconfig
+++ b/sys/xtimer/Kconfig
@@ -18,7 +18,6 @@ menuconfig MODULE_XTIMER

 config MODULE_XTIMER_NO_ZTIMER_DEFAULT
     bool "xtimer does not select ztimer"
-    default y

 config MODULE_AUTO_INIT_XTIMER
     bool "Auto-init xtimer"
diff --git a/sys/xtimer/Makefile.dep b/sys/xtimer/Makefile.dep
index 8590f5b14d..03d3e97c6d 100644
--- a/sys/xtimer/Makefile.dep
+++ b/sys/xtimer/Makefile.dep
@@ -7,7 +7,6 @@ DEFAULT_MODULE += auto_init_xtimer
 # dependency inclusion order issues
 FEATURES_REQUIRED += periph_timer

-USEMODULE += xtimer_no_ztimer_default
 ifeq (,$(filter xtimer_no_ztimer_default,$(USEMODULE)))
   ifeq (,$(filter xtimer_on_ztimer,$(USEMODULE)))
     USEMODULE += ztimer_xtimer_compat

@fjmolinas fjmolinas added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Mar 16, 2022
@fjmolinas fjmolinas changed the title sys/*timer: rework dependecies to ease backend switch sys/*timer: rework dependecies to ease backend switch, prefer ztimer_xtimer_compat over xtimer_on_ztimer Mar 16, 2022
@fjmolinas fjmolinas removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Mar 16, 2022
@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers and removed Area: CI Area: Continuous Integration of RIOT components labels Mar 16, 2022
leandrolanzieri
leandrolanzieri reviewed Mar 17, 2022
View reviewed changes
cpu/efm32/periph/Kconfig Show resolved Hide resolved
sys/xtimer/Kconfig Show resolved Hide resolved
sys/xtimer/Makefile.dep Show resolved Hide resolved
leandrolanzieri
leandrolanzieri reviewed Mar 17, 2022
View reviewed changes
Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

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

Just a small comment, otherwise the changes in Kconfig make sense. Now MODULE_ZTIMER_XTIMER_COMPAT is the default choice option for xtimer when both have to co-exist (or when !MODULE_XTIMER_NO_ZTIMER_DEFAULT).

sys/xtimer/Kconfig Outdated Show resolved Hide resolved
@fjmolinas
Copy link
Contributor Author

Just a small comment, otherwise the changes in Kconfig make sense. Now MODULE_ZTIMER_XTIMER_COMPAT is the default choice option for xtimer when both have to co-exist (or when !MODULE_XTIMER_NO_ZTIMER_DEFAULT).

Yes, this was done in this commit cfbebb3, should I split nthat one out?

@leandrolanzieri
Copy link
Contributor

No no, I was just confirming that the expected behaviour seems to work. From my side you can squash now!

@fjmolinas
treewide: make all modules use Kconfig ZTIMER_USEC indirection
2cc5af6
@fjmolinas
cpu/efm32: periph_rtt_series0/1 are not modules
d86828c
@fjmolinas
sys/sc_gnrc_udp: fix *timer include
be6a76a
@fjmolinas
sys/xtimer: move dependencies to xtimer/Makefile.dep
7227d1b
@fjmolinas
sys/xtimer-ztimer: rework dependecies to ease backend switch
0b14536 
This adds a xtimer_no_ztimer_default that is currently always
selected in Makefile, but that can be switched off in Kconfig.
Removing its inclusion will allow switching the default xtimer
backend to ztimer, while allowing for an easy way back.
@fjmolinas
sys/xtimer: prefer ztimer_xtimer_compat
9260cfb
@fjmolinas
tests/xtimer*: select xtimer_no_ztimer_default for tests requiring it
46ae295 
These tests need to be tested on xtimer, and if using ztimer_xtimer_compat
the full api will not be provided, therefore also blacklist BOARDs
that will en up selecting ztimer by default.
@fjmolinas
Copy link
Contributor Author

Squashed!

@fjmolinas fjmolinas added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 17, 2022
@fjmolinas
Copy link
Contributor Author

All green @kaspar030 @leandrolanzieri

@leandrolanzieri leandrolanzieri added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Mar 18, 2022
@leandrolanzieri
Copy link
Contributor

Then let's go!

@leandrolanzieri leandrolanzieri merged commit 202fb26 into RIOT-OS:master Mar 18, 2022
@fjmolinas fjmolinas deleted the pr_xtimer_dep_rework_simple branch March 18, 2022 07:20
@OlegHahm OlegHahm added this to the Release 2022.04 milestone Apr 8, 2022
@benpicco benpicco changed the title sys/*timer: rework dependecies to ease backend switch, prefer ztimer_xtimer_compat over xtimer_on_ztimer sys/*timer: rework dependencies to ease backend switch, prefer ztimer_xtimer_compat over xtimer_on_ztimer Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrKevinWeiss MrKevinWeiss MrKevinWeiss left review comments

@leandrolanzieri leandrolanzieri leandrolanzieri approved these changes

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

@cgundogan cgundogan Awaiting requested review from cgundogan

@smlng smlng Awaiting requested review from smlng

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@kaspar030 kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

@bergzand bergzand Awaiting requested review from bergzand bergzand is a code owner

Assignees
No one assigned
Labels
Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework 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 Platform: ARM Platform: This PR/issue effects ARM-based platforms Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines
Projects
None yet
Milestone
Release 2022.04
Development

Successfully merging this pull request may close these issues.
4 participants
@fjmolinas @leandrolanzieri @MrKevinWeiss @OlegHahm