sys/ztimer: add ztimer64_xtimer_compat complete xtimer replace module by fjmolinas · Pull Request #17670 · RIOT-OS/RIOT

fjmolinas · 2022-02-17T09:34:21Z

Contribution description

This PR makes ztimer_xtimer_compat use ztimer64_usec implementing all xtimer api, with this ztimer64_usec can be used as a drop-in replacement for xtimer.

Testing procedure

Green Murdock should be enough, sys/ztimer: make ztimer_xtimer_compat use ztimer64_usec #17365 will flag possible remaining offenders.

Issues/PRs references

Split from #17365

fjmolinas · 2022-02-18T16:10:13Z

@leandrolanzieri do you mind taking a quick look I changed some of the xtimer/ztimer modelings, and I want to be sure I'm not re-introducing issues.

leandrolanzieri · 2022-02-21T08:10:31Z

sys/xtimer/Kconfig

-
-    # use timer peripheral unless ztimer compatibility module is used
-    select MODULE_PERIPH_TIMER if HAS_PERIPH_TIMER && !MODULE_XTIMER_ON_ZTIMER && !MODULE_ZTIMER_XTIMER_COMPAT
+    select MODULE_PERIPH_TIMER if HAS_PERIPH_TIMER


I think the idea of this was not to force the timer backend when compatibility is in place, leaving the option of the hardware to the Ztimer config. Why does this change?

Hmmm let me revert it and remember why hahaha

will split out in another commit if justfied

Hmm well all green

@leandrolanzieri does everything else Kconfig related look good to you?

fjmolinas · 2022-02-21T13:23:21Z

sys/xtimer/Kconfig

    depends on HAS_PERIPH_TIMER
    depends on TEST_KCONFIG
-    select MODULE_PERIPH_TIMER if HAS_PERIPH_TIMER
+    # select MODULE_PERIPH_TIMER if HAS_PERIPH_TIMER


Should remove this line then

@leandrolanzieri

The comment? yes, that means you still keep

select MODULE_PERIPH_TIMER if HAS_PERIPH_TIMER && !MODULE_XTIMER_ON_ZTIMER && !MODULE_ZTIMER_XTIMER_COMPAT

right?

Yes, I meant removing the comments, ti was a note for myself when rebasing.

fjmolinas · 2022-02-21T13:24:31Z

@kaspar030 could you give this one a look? IMO its missing only a rebase, squash, uncrustify the header file and this #17670 (comment)

fjmolinas · 2022-02-21T13:27:38Z

Maybe @kfessel wants to take a look as well

sys/net/dsm/Makefile.dep

fjmolinas · 2022-02-23T08:46:57Z

I added as asked for a ztimer64/xtimer_compat.h" header, but I also removed compatusage fromevent/timeout`, its honestly simply not worth the effort anymore to keep both. Let's see what Murdock thinks.

In a follow up what I'll do is enable ztimer_xtimer_compat by default, see which modules actually require the whole API, and have those explicitly select ztimer64_xtimer_compat.

fjmolinas · 2022-02-23T09:10:11Z

In a follow up what I'll do is enable ztimer_xtimer_compat by default, see which modules actually require the whole API, and have those explicitly select ztimer64_xtimer_compat.

So I'm modeling ztimer64_xtimer_compat as an "extension", both modules are pseudomodules anyway, but having it this way makes it easy to model in Make and Kconfig, since there will be no ifeq dependencies.

fjmolinas · 2022-02-23T11:10:38Z

@kaspar030 @kfessel I'll split the event_timeout not supporting ztimer PR out.

kfessel · 2022-02-23T10:47:07Z

sys/include/ztimer64/xtimer_compat.h

+    return ztimer64_now(ZTIMER64_USEC);
+}
+
+


Suggested change

some static test does not like that empty line

kfessel · 2022-02-23T12:52:46Z

somthing went wrong (event/timeout.c got added)

fjmolinas · 2022-02-23T13:21:45Z

somthing went wrong (event/timeout.c got added)

ups wrong rebase, re-added

fjmolinas · 2022-02-23T14:55:30Z

Well all green

kaspar030

ACK.

kfessel

two nitpicks,
i think you can squash them right in and skip the murdock build

sys/include/ztimer/xtimer_compat.h

sys/include/ztimer64/xtimer_compat.h

The ztimer64_xtimer_compat modules implements the whole xtimer api on top of ztimer64_usec

fjmolinas · 2022-02-24T08:11:47Z

Squashed in both nitpicks @kfessel

fjmolinas · 2022-02-24T10:38:20Z

All green @kaspar030 @kfessel

fjmolinas · 2022-02-24T11:10:54Z

Thanks, @kaspar030 @kfessel!

fjmolinas requested review from MichelRottleuthner, aabadie, bergzand, cgundogan, jia200x, kaspar030, leandrolanzieri and smlng as code owners February 17, 2022 09:34

fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 17, 2022

github-actions bot added Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: tests Area: tests and testing framework Area: timers Area: timer subsystems labels Feb 17, 2022

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 Feb 18, 2022

fjmolinas mentioned this pull request Feb 18, 2022

pkg/tinydtls/sock_dtls: use ztimer_usec #17677

Merged

fjmolinas force-pushed the pr_ztimer_xtimer_copmat_64 branch from 4e6f9f7 to 28b6c93 Compare February 21, 2022 07:31

fjmolinas removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 21, 2022

fjmolinas requested review from PeterKietzmann, haukepetersen and miri64 as code owners February 21, 2022 08:13

fjmolinas added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 21, 2022

github-actions bot added the Area: network Area: Networking label Feb 21, 2022

leandrolanzieri reviewed Feb 21, 2022

View reviewed changes

fjmolinas commented Feb 21, 2022

View reviewed changes

kaspar030 reviewed Feb 21, 2022

View reviewed changes

sys/net/dsm/Makefile.dep Outdated Show resolved Hide resolved

github-actions bot removed the Area: network Area: Networking label Feb 23, 2022

github-actions bot added the Area: build system Area: Build system label Feb 23, 2022

fjmolinas removed the Area: build system Area: Build system label Feb 23, 2022

kfessel self-requested a review February 23, 2022 10:45

kfessel changed the title ~~sys/ztimer: update ztimer_xtimer_compat_64 to use ztimer64~~ sys/ztimer: add ztimer64_xtimer_compat complete xtimer replace module Feb 23, 2022

fjmolinas mentioned this pull request Feb 23, 2022

sys/event/timeout: remove alternative xtimer implementation #17689

Merged

kfessel reviewed Feb 23, 2022

View reviewed changes

fjmolinas force-pushed the pr_ztimer_xtimer_copmat_64 branch from 67a3e10 to 8555df0 Compare February 23, 2022 12:45

github-actions bot added the Area: build system Area: Build system label Feb 23, 2022

fjmolinas force-pushed the pr_ztimer_xtimer_copmat_64 branch from 8555df0 to 03cec76 Compare February 23, 2022 13:21

kfessel mentioned this pull request Feb 23, 2022

sys/ztimer: cleanup xtimer_compat.h for 32Bit only #17690

Merged

kaspar030 approved these changes Feb 23, 2022

View reviewed changes

kfessel approved these changes Feb 23, 2022

View reviewed changes

sys/include/ztimer/xtimer_compat.h Show resolved Hide resolved

sys/include/ztimer64/xtimer_compat.h Outdated Show resolved Hide resolved

kaspar030 and others added 3 commits February 24, 2022 09:10

sys/ztimer: add ztimer64_xtimer_compat

df80860

The ztimer64_xtimer_compat modules implements the whole xtimer api on top of ztimer64_usec

sys/Makefile.dep: add missing newlib_syscalls dep

106db46

tests/xtimer_drift: add missing dependency

46fe917

fjmolinas force-pushed the pr_ztimer_xtimer_copmat_64 branch from 03cec76 to 46fe917 Compare February 24, 2022 08:11

kaspar030 merged commit f2d7a04 into RIOT-OS:master Feb 24, 2022

kfessel mentioned this pull request Feb 28, 2022

sys/ztimer: make ztimer_xtimer_compat use ztimer64_usec #17365

Closed

fjmolinas deleted the pr_ztimer_xtimer_copmat_64 branch March 24, 2022 08:31

OlegHahm added this to the Release 2022.04 milestone Apr 25, 2022

Conversation

fjmolinas commented Feb 17, 2022

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

fjmolinas commented Feb 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Feb 21, 2022

Uh oh!

fjmolinas commented Feb 21, 2022

Uh oh!

Uh oh!

fjmolinas commented Feb 23, 2022

Uh oh!

fjmolinas commented Feb 23, 2022

Uh oh!

fjmolinas commented Feb 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfessel commented Feb 23, 2022

Uh oh!

fjmolinas commented Feb 23, 2022

Uh oh!

fjmolinas commented Feb 23, 2022

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

kfessel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fjmolinas commented Feb 24, 2022

Uh oh!

fjmolinas commented Feb 24, 2022

Uh oh!

fjmolinas commented Feb 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants