sys/trickle: migrate from xtimer to ZTIMER_MSEC by haukepetersen · Pull Request #16322 · RIOT-OS/RIOT

haukepetersen · 2021-04-13T12:27:20Z

Contribution description

This PR migrates trickle from xtimer to ZTIMER_MSEC. This allows for a) slightly simpler code, and b) for potential energy savings as xtimer can (at one point) be left out of a GNRC build.

The acutal changes are pretty straight forward, no surprises here :-)

Testing procedure

Running the test/trickle test should suffice.

Issues/PRs references

none

jue89 · 2021-04-14T09:18:53Z

All in all a valid change. But your PR shows up a problem, if just ztimer_msec is required. It tries to back its time by ztimer_periph_timer - but that module is missing. This must be fixed before merging this PR.

haukepetersen · 2021-04-15T08:53:37Z

Yap, but this is IMHO more a generic problem with the ztimer deps. I proposed a generic fix in #16334, lets see if it gets in :-)

haukepetersen · 2021-04-15T08:55:52Z

On top there is a problem in the rpl shell command, as it accesses internals of the trickle data structure directly, very good coding style :-)

But as it seems the calculation of the remaining trickle time is based on a xtimer function that does not exist for ztimer, so there is no easy drop-in replacement. Will need to think of a viable approach to migrate that piece of code.

haukepetersen · 2021-04-15T13:12:01Z

After thinking about it, I will adapt this PR as well to be 'dual-timer', so to use ztimer only if its included in the build anyway and fallback to xtimer in any other case...

kfessel · 2021-04-15T17:39:32Z

#13667

haukepetersen · 2021-04-16T07:32:59Z

Adapted this PR to follow the same style as #16340 and #16339: ztimer_msec is now used only if available, whereas the default dependencies are not changed - despite including xtimer as dep only in the case ztimer_msec is not present in the build.

@jue89 This should also rule out the dependency problem we discussed.

haukepetersen · 2021-05-01T10:06:44Z

rebased

haukepetersen · 2021-06-11T06:08:35Z

rebased

cgundogan · 2021-06-11T09:51:01Z

The code changes look reasonable. Just a quick question on how to test this .. I wanted to run the trickle test in tests/ and modified the Makefile to include USEMODULE += ztimer_msec. But I get an error from the linker:

RIOT/sys/ztimer/auto_init.c:224: undefined reference to `ztimer_periph_timer_init'

I was testing with native; is this expected because of missing adaptations?

miri64 · 2021-06-11T09:54:48Z

RIOT/sys/ztimer/auto_init.c:224: undefined reference to `ztimer_periph_timer_init'
I was testing with native; is this expected because of missing adaptations?

For native you need to also manually include ztimer_usec at the moment.

cgundogan · 2021-06-11T10:01:26Z

RIOT/sys/ztimer/auto_init.c:224: undefined reference to `ztimer_periph_timer_init'
I was testing with native; is this expected because of missing adaptations?
For native you need to also manually include ztimer_usec at the moment.

mhh, okay. Works with the following diff:

diff --git a/tests/trickle/Makefile b/tests/trickle/Makefile
index 72efd754bc..5512f1cb0d 100644
--- a/tests/trickle/Makefile
+++ b/tests/trickle/Makefile
@@ -1,5 +1,7 @@
 include ../Makefile.tests_common
 
 USEMODULE += trickle
+USEMODULE += ztimer_usec
+USEMODULE += ztimer_msec
 
 include $(RIOTBASE)/Makefile.include

The print output of the test also looks correct. I'd be fine with merging if there are no unresolved discussions (from above).

cgundogan

Reasonable code changes and the tests/trickle test shows expected results.

miri64 · 2021-06-11T11:10:30Z

The Github action figured out just fine that the PR needs squashing. No need to set the label and possibly waste valuable CI time as such ;-).

haukepetersen · 2021-06-14T07:05:02Z

squashed.

haukepetersen · 2021-06-14T09:10:56Z

Some gnrc_networking example builds failed due to sc_gnrc_rpl using the timer member of the trickle timer directly, which lead to conflicts between ztimer and xtimer. I fixed this by pulling in the sc_gnrc_rpl fixes already included in #16339.

cgundogan · 2021-06-14T09:19:08Z

sys/shell/commands/sc_gnrc_rpl.c

                gnrc_rpl_instances[i].mop, gnrc_rpl_instances[i].of->ocp,
                gnrc_rpl_instances[i].min_hop_rank_inc, gnrc_rpl_instances[i].max_rank_inc);

+#if !IS_USED(MODULE_XTIMER)


Hmm, if there is no better way to access the current time-to-fire of the timer, then we should probably remove the TC print in both cases. It was there for debugging purposes and is not very important to have that around.

No, to my knowledge there is not really a decent way with ztimer to get the TC. Removing all-together sounds good to me, will adapt.

cgundogan · 2021-06-14T11:12:37Z

The changes still look good after the amendments.

haukepetersen added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 13, 2021

haukepetersen requested review from bergzand and cgundogan April 13, 2021 12:27

haukepetersen requested review from MichelRottleuthner, aabadie, fjmolinas, leandrolanzieri and smlng as code owners April 13, 2021 12:27

haukepetersen 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 Apr 15, 2021

haukepetersen mentioned this pull request Apr 15, 2021

make/ztimer: auto-pull timer backend deps for ztimer_msec #16334

Closed

haukepetersen removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 15, 2021

kfessel mentioned this pull request Apr 15, 2021

[Tracking] modules depending on 64-bit xtimer implementation #13667

Open

22 tasks

haukepetersen force-pushed the opt_trickle_ztimer branch from af0a5bb to 54f177e Compare April 16, 2021 07:29

kfessel mentioned this pull request Apr 16, 2021

sys/ztimer: auto_init react to possibly missing ztimer_periph_timer #16342

Closed

haukepetersen mentioned this pull request Apr 16, 2021

examples/gnrc_networking: use ztimer_msec if available #16343

Merged

haukepetersen force-pushed the opt_trickle_ztimer branch from 54f177e to 6d6a28f Compare May 1, 2021 10:06

haukepetersen force-pushed the opt_trickle_ztimer branch from 6d6a28f to 511aebd Compare June 11, 2021 06:07

github-actions bot added Area: sys Area: System Area: tests Area: tests and testing framework and removed Area: network Area: Networking labels Jun 11, 2021

cgundogan approved these changes Jun 11, 2021

View reviewed changes

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Jun 11, 2021

haukepetersen added 2 commits June 14, 2021 09:04

sys/trickle: migrate to ZTIMER_MSEC

c5d1a34

tests/trickle: use ztimer instead of xtimer

ef8753a

haukepetersen force-pushed the opt_trickle_ztimer branch from 511aebd to ef8753a Compare June 14, 2021 07:04

cgundogan reviewed Jun 14, 2021

View reviewed changes

sys/shell/sc_gnrc_rpl: do not print tricke TC

1fb34b3

haukepetersen force-pushed the opt_trickle_ztimer branch from 84b5384 to 1fb34b3 Compare June 14, 2021 09:23

cgundogan merged commit d4997d7 into RIOT-OS:master Jun 14, 2021

cgundogan deleted the opt_trickle_ztimer branch June 14, 2021 11:13

MrKevinWeiss added this to the Release 2021.07 milestone Jul 15, 2021

Conversation

haukepetersen commented Apr 13, 2021

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

jue89 commented Apr 14, 2021

Uh oh!

haukepetersen commented Apr 15, 2021

Uh oh!

haukepetersen commented Apr 15, 2021

Uh oh!

haukepetersen commented Apr 15, 2021

Uh oh!

kfessel commented Apr 15, 2021

Uh oh!

haukepetersen commented Apr 16, 2021

Uh oh!

haukepetersen commented May 1, 2021

Uh oh!

haukepetersen commented Jun 11, 2021

Uh oh!

cgundogan commented Jun 11, 2021

Uh oh!

miri64 commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cgundogan commented Jun 11, 2021

Uh oh!

cgundogan left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Jun 11, 2021

Uh oh!

haukepetersen commented Jun 14, 2021

Uh oh!

haukepetersen commented Jun 14, 2021

Uh oh!

cgundogan Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

haukepetersen Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

cgundogan commented Jun 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

miri64 commented Jun 11, 2021 •

edited

Loading