Skip to content

net/gnrc/rpl: use ztimer_msec if available#16339

Merged
haukepetersen merged 1 commit intoRIOT-OS:masterfrom
haukepetersen:opt_rpl_ztimer
Jun 17, 2021
Merged

net/gnrc/rpl: use ztimer_msec if available#16339
haukepetersen merged 1 commit intoRIOT-OS:masterfrom
haukepetersen:opt_rpl_ztimer

Conversation

@haukepetersen
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen commented Apr 15, 2021

Contribution description

With this PR RPL will use ztimer_msec as timer whenever ztimer_msec is included in the build. This is needed for xtimer-free GNRC builds for getting to lower power consumption.

One small quirk: when building with ztimer, the RPL shell commands does not show the remaining seconds until the trickle timer triggers next anymore. This is due to missing functionality in the ztimer, and IMO it would be overkill to add this functionality into ztimer (or any other workaround in the trickle timer) just to get this debug information...

Testing procedure

The default builds (e.g. examples/gnrc_networking) should not change, as per default RPL will still use xtimer. But when building with ztimer_msec included in the build, RPL should switch to use ztimer_msec.

This can be tested e.g. with two samr21-xpro nodes by running the examples/gnrc_networking example while building it with USEMODULE="ztimer_msec ztimer_periph_rtt" make ....

Issues/PRs references

none

@haukepetersen haukepetersen added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Area: timers Area: timer subsystems labels Apr 15, 2021
@jue89
Copy link
Copy Markdown
Contributor

jue89 commented Apr 15, 2021

Why have you decided against a hard switch to ztimer_msec? Are applications already using this code affected negatively when ztimer_msec is used under the hood instead of xtimer? Or just because of the rpl command thing?

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Apr 15, 2021

Because I wanted to keep the impact on existing applications as small as possible. I think the hard switch only makes sense once we have a joined decision to completely phase out xtimer once and for all...

@kfessel
Copy link
Copy Markdown
Contributor

kfessel commented Apr 15, 2021

#13667

@kfessel kfessel mentioned this pull request Apr 15, 2021
Open
22 tasks
This was referenced Apr 16, 2021
Merged
Merged
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented May 1, 2021

rebased

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Jun 11, 2021

rebased

@github-actions github-actions bot added Area: sys Area: System and removed Area: timers Area: timer subsystems labels Jun 11, 2021
@cgundogan
Copy link
Copy Markdown
Member

cgundogan commented Jun 14, 2021

Another rebase needed .. will look at this PR afterwards :)

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Jun 14, 2021

rebased :-)

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 15, 2021
cgundogan
cgundogan approved these changes Jun 15, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@cgundogan cgundogan left a comment

Choose a reason for hiding this comment

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

Tested with native and USEMODULE+="ztimer_msec ztimer_usec". The timer operation looks still valid in wireshark.

@cgundogan cgundogan 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 labels Jun 15, 2021
@cgundogan
Copy link
Copy Markdown
Member

cgundogan commented Jun 15, 2021

Murdock is still complaining, though.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Jun 16, 2021

Found the issue, there was #endif one line too early in gnrc_rpl.c. Fixed it and squashed, hopefully Murdock is now happy.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Jun 17, 2021

Alright, Murdock is happy now and everything green -> go.

@haukepetersen haukepetersen merged commit 1df6ee3 into RIOT-OS:master Jun 17, 2021
@haukepetersen haukepetersen deleted the opt_rpl_ztimer branch June 17, 2021 06:15
@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cgundogan cgundogan cgundogan approved these changes

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

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

Assignees

No one assigned

Labels

Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR 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 Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2021.07

Development

Successfully merging this pull request may close these issues.

7 participants

@haukepetersen @jue89 @kfessel @cgundogan @miri64 @MrKevinWeiss @fjmolinas