Skip to content

sys: sort out ztimer_xtimer_compat and ztimer64_xtimer_compat depes#17732

Merged
fjmolinas merged 4 commits intoRIOT-OS:masterfrom
fjmolinas:pr_xtimer_compat_deps
Mar 4, 2022
Merged

sys: sort out ztimer_xtimer_compat and ztimer64_xtimer_compat depes#17732
fjmolinas merged 4 commits intoRIOT-OS:masterfrom
fjmolinas:pr_xtimer_compat_deps

Conversation

@fjmolinas
Copy link
Contributor

@fjmolinas fjmolinas commented Mar 2, 2022

Contribution description

This PR splits from #17365 all modules depending on 64bit features of xtimer. It allows not using 64bit if not needed, note that because of newlib this is actually used most of the time (a nice case to optimize @kfessel)

In cases where there was already a ztimer based alternative for xtimer, those get used instead.

Note that some might still be missed as covered in the build but some of these modules.

Testing procedure

  • Green murdock

Issues/PRs references

Split from #17365

@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 2, 2022
@fjmolinas fjmolinas requested review from kaspar030 and kfessel March 2, 2022 07:51
@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration Area: network Area: Networking Area: sys Area: System Area: timers Area: timer subsystems Platform: native Platform: This PR/issue effects the native platform labels Mar 2, 2022
fjmolinas added 2 commits March 2, 2022 08:59
@fjmolinas
sys/trickle: if ztimer_xtimer_compat is used, use ztimer_msec
fc169b9 
If already using the compat layer then just use ztimer_msec otherwise
the 64bit compat layer would be needed for this module
@fjmolinas
sys: if xtimer_ztimer_compat, evtimer uses ztimer
bf83387 
The xtimer based module was only there to avoid duplicate code,
is the compat layer is used this does not make sense.
@fjmolinas fjmolinas force-pushed the pr_xtimer_compat_deps branch from fd34661 to 0bb7ecc Compare March 2, 2022 08:00
fjmolinas
fjmolinas commented Mar 2, 2022
View reviewed changes
sys/Makefile.dep Outdated
Comment on lines +338 to +343
ifneq (,$(filter sema,$(USEMODULE)))
ifneq (,$(filter ztimer_xtimer_compat,$(USEMODULE)))
# sema_timed required 64bit
USEMODULE += ztimer64_xtimer_compat
endif
endif
Copy link
Contributor Author

@fjmolinas fjmolinas Mar 2, 2022

Choose a reason for hiding this comment

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

With #17115 this dependency can be moved to only tests/sema and posix_semaphore

Copy link
Contributor

@kfessel kfessel Mar 2, 2022

Choose a reason for hiding this comment

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

lets wait for it

Copy link
Contributor Author

@fjmolinas fjmolinas Mar 2, 2022

Choose a reason for hiding this comment

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

I removed it then

@fjmolinas
Copy link
Contributor Author

fjmolinas commented Mar 2, 2022

because of newlib this is actually used most of the time (a nice case to optimize @kfessel)

I think we could make gettimeofday part of a pseudomodule newlib_syscalls_gettimeofday and native_syscalls_gettimeofday, there probably are not many users in the tree, but I would rather leave this for a follow-up, it does not make the current situation worse, and with this PR as it is I know #17365 passes, and therefore I can move forward with #17721.

The sema change I would like to address in this PR if lwip change gets in.

@fjmolinas fjmolinas mentioned this pull request Mar 2, 2022
Merged
@kfessel
Copy link
Contributor

kfessel commented Mar 2, 2022

gettimeofday might also be an easy port to ztimer_msec but this is for another PR

@fjmolinas
Copy link
Contributor Author

fjmolinas commented Mar 2, 2022

gettimeofday might also be an easy port to ztimer_msec but this is for another PR

I think it requires usec though...

@kfessel
Copy link
Contributor

kfessel commented Mar 2, 2022 •
edited
Loading

gettimeofday might also be an easy port to ztimer_msec but this is for another PR

I think it requires usec though...

https://pubs.opengroup.org/onlinepubs/009604599/functions/gettimeofday.html

"The resolution of the system clock is unspecified." -> we can do to the resolution what ever we want

also refers to ftime() which has millisecond resolution in its type (and still includes that sentence about the system clock resolution)

actually 0 should be start of unix epoch might be the bigger posix breaker (it was changed somewhen)

fjmolinas added 2 commits March 2, 2022 14:28
@fjmolinas
sys: use ztimer64_xtimer_compat when required
6151895
@fjmolinas
sys/ztimer/Kconfig: remv dep between ztimer64_compat and ztimer_compat
f1d4cc0 
This is done to avoid circular dependency, it will currently allow for
ztimer64_xtimer_compat beeing selected as well as xtimer_on_ztimer.
This altough incorrect mimics make, and will be fixed when making
ztimer_xtimer_compat the default xtimer backend
@fjmolinas fjmolinas force-pushed the pr_xtimer_compat_deps branch from 0bb7ecc to f1d4cc0 Compare March 2, 2022 13:29
@fjmolinas
Copy link
Contributor Author

fjmolinas commented Mar 3, 2022

Seems like its all green here @kfessel

kfessel
kfessel approved these changes Mar 3, 2022
View reviewed changes
Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

rollin' rollin' rollin'

@fjmolinas fjmolinas merged commit 0c166b1 into RIOT-OS:master Mar 4, 2022
@fjmolinas fjmolinas deleted the pr_xtimer_compat_deps branch March 4, 2022 07:08
@OlegHahm OlegHahm added this to the Release 2022.04 milestone Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kfessel kfessel kfessel approved these changes

@kaspar030 kaspar030 Awaiting requested review from kaspar030

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

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

@cgundogan cgundogan Awaiting requested review from cgundogan

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

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

Assignees

No one assigned

Labels

Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration Area: network Area: Networking 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 Platform: native Platform: This PR/issue effects the native platform

Projects

None yet

Milestone

Release 2022.04

Development

Successfully merging this pull request may close these issues.

3 participants

@fjmolinas @kfessel @OlegHahm