sys/ztimer: add ZTIMER_SEC, improve auto_init #16172

kfessel · 2021-03-09T11:03:40Z

I adopted #15715 @maribu wired up some ZTIMER_SEC support supporting ztimer_periph_rtc and ztimer_periph_rtt

Contribution description

add ZTIMER_SEC support which is currently documented but missing.
restructure the auto init. for readability an flexibility.
avoid the use of RTT for MSEC if its Frequency less than 1kHz.
puts the power mangement association to the base clk (basic timer / rtt / rtc)

Testing procedure

tests/ztimer_msg has been updated to allow testing ZTIMER_SEC. Note: Uncomment a line in the Makefile as documented there
tests/ztimer_underflow has been updated to allow testing ZTIMER_SEC
-Run make TEST_ZTIMER_CLOCK=ZTIMER_SEC -C tests/ztimer_underflow flash test
-This fails for me, which proves two things: a) wiring up the RTC worked, and b) there is a preexisting bug in master unrelated to the PR that needs fixing.

Issues/PRs references

#15715 the original
#15911 also goes for the power mangement association to the base clk
the PM naming improvement by #16160 is allready taken into account for the new defined base clock associated

wire up ZTIMER_SEC to the existing RTC backend, or RTT backend, or periph_timer backend (in this order of preference). Update sys/ztimer/auto_init.c Co-authored-by: Leandro Lanzieri <leandro.lanzieri@haw-hamburg.de>

kfessel · 2021-03-09T11:09:03Z

The initial push has the fixups, that where just made to satisfy uncrustify, removed other than that it keeps the history.

kfessel · 2021-03-09T11:11:46Z

@jue89 and @vincent-d may want to review parts of this PR and where not auto added for review

kfessel · 2021-03-09T11:18:00Z

uncrustify is wrong about wrong about sys/include/ztimer.h#L297 and sys/include/ztimer.h#L309, changing to suggestion breaks the rule written in config (there is no multiplication in that lines)

kfessel · 2021-03-29T12:34:39Z

Is there any thing need to be done before this can merge?

fjmolinas · 2021-03-29T12:53:25Z

Is there any thing need to be done before this can merge?

From my side, looking back at the original PR and comments, from #15715 (review), I still would like a test for ZTIMER_SEC.

it would be nice if the choice of initialization and backend was documented in another way as only with ifdefs, since they are really hard to read.

can we add distinctive tests for the different timers Since the application is the same mostly, I think an approach as the one used for the network driver compile test can be used

kfessel · 2021-03-29T14:47:38Z

@fjmolinas: could you please be a bit more specific about the test you like to see? maybe you can provide a path.

Marian already provided a test with "tests/ztimer_msg: Allow testing ZTIMER_SEC"
and "tests/ztimer_underflow: allow testing ZTIMER_SEC"

Am i right that with your request for more documentation you mean more of the ztimer_periphry selection exposed to Doxygen?

fjmolinas · 2021-03-29T15:25:37Z

Marian already provided a test with "tests/ztimer_msg: Allow testing ZTIMER_SEC"
and "tests/ztimer_underflow: allow testing ZTIMER_SEC"

Allow testing does not mean the test is actually run, since it depends on a commented-out module the test is never run in the CI, I would rather have a distinct separate test application. Otherwise, this code is never compiled in.

fjmolinas · 2021-03-29T15:26:46Z

Am i right that with your request for more documentation you mean more of the ztimer_periphry selection exposed to Doxygen?

Exactly, your last commit addressed it perfectly. Thanks

fjmolinas · 2021-03-30T09:15:40Z

Marian already provided a test with "tests/ztimer_msg: Allow testing ZTIMER_SEC"
and "tests/ztimer_underflow: allow testing ZTIMER_SEC"

Allow testing does not mean the test is actually run, since it depends on a commented-out module the test is never run in the CI, I would rather have a distinct separate test application. Otherwise, this code is never compiled in.

Although if @maribu think its OK as is, I won't block this, I think the PR is a very good shape, just don't like that the future is never compiled.

maribu · 2021-03-30T09:54:49Z

I'll try find some time today to extend the existing ztimer tests to just iterate over all available clocks and test each.

With a whitelist the periph_rtt backend for ZTIMER_MSEC can be selected, for the others periph_timer is just used for both ZTIMER_USEC and ZTIMER_MSEC. (Using FEATURES_OPTIONAL += periph_rtt doesn't work, as an RTT needs to run at least with 1 kHz to be usable.)

How about this: If I manage to get this done today, @kfessel could rebase and extend the tests in the same fashion to also cover ZTIMER_SEC. And I give my best that we don't need to worry about the other case :-)

kfessel · 2021-03-30T12:03:46Z

I just fished a really simple High level ztimer test that compiles all high level timers and uses them to unlock some mutexes.
For testing rtt and rtc the respective module has to used.

maribu

The test looks good to me. IMO it is always good to (also) have simple tests. If a complex tests fails, one has to first understand the test before one can start debugging.

We can still independently of this PR extend the other ztimer tests to cover all clocks, to increase test coverage under more challenging situations.

tests/ztimer_xsec/main.c

kfessel · 2021-04-01T14:12:42Z

squashed,

the last two (new) commits make sure that no struct tm extra information (TZ and DST) leakes from riot to rtc or from rtc native to riot

maribu

Some typos, you can directly squash. (Ideally even before pushing.)

Note: If you split out the last two commits, they could go in right away.

cpu/native/periph/rtc.c

kfessel · 2021-04-01T15:00:09Z

i am unsure is it likely that this merges soon?
if not i would split out the rtc fixes

kfessel · 2021-04-01T15:22:49Z

typo sorry murdock

MSEC and SEC are now usable on TIMER(0) without having USEC pm is configured by used hardware OLD configuration values are translated for backward compatibility prefer rtt for ZTIMER_SEC avoid doing partial ztimer setup if auto_init_ztimer is disabled before this patch some const pointers might have been definend to values that a user who disables auto_init for ztimer does not like.

provide more information about the selection procedure to Doxygen

contrary to the western civilised world struct tm requires us to count months from 0

{ .elem = 0 } initializes the complete struct to 0 {} is not ISO C conform

kfessel · 2021-04-01T16:23:22Z

commit message typo

fjmolinas · 2021-04-01T16:59:14Z

@maribu have your changes been addressed?

fjmolinas

ACK!, Thanks for pushing for this and sticking through @kfessel!

maribu · 2021-04-02T07:54:37Z

🎉 (Btw: I just noticed that I put a confused smiley to this PR. I hit the wrong icon, it should have been the thumbs up.)

kfessel · 2021-04-02T08:11:22Z

thanks 🎉

next stop #16160 and #16126

sys/ztimer: add ZTIMER_SEC

da76ca6

wire up ZTIMER_SEC to the existing RTC backend, or RTT backend, or periph_timer backend (in this order of preference). Update sys/ztimer/auto_init.c Co-authored-by: Leandro Lanzieri <leandro.lanzieri@haw-hamburg.de>

kfessel requested review from aabadie, bergzand, fjmolinas, kaspar030, leandrolanzieri, MichelRottleuthner and smlng as code owners March 9, 2021 11:03

kfessel changed the title ~~sys/ztimer: add ZTIMER_SEC~~ sys/ztimer: add ZTIMER_SEC improve auto_init Mar 9, 2021

kfessel changed the title ~~sys/ztimer: add ZTIMER_SEC improve auto_init~~ sys/ztimer: add ZTIMER_SEC, improve auto_init Mar 9, 2021

jeandudey added Area: timers Area: timer subsystems Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 10, 2021

kfessel mentioned this pull request Mar 17, 2021

sys/ztimer: add ZTIMER_SEC #15715

Closed

maribu mentioned this pull request Mar 30, 2021

tests/ztimer_periodic: iterate over clocks #16254

Merged

kfessel requested a review from miri64 as a code owner March 30, 2021 11:57

kfessel force-pushed the p-zimer-sec branch from e60f1be to df16d6f Compare March 30, 2021 11:58

maribu reviewed Mar 30, 2021

View reviewed changes

tests/ztimer_xsec/main.c Outdated Show resolved Hide resolved

tests/ztimer_xsec/main.c Outdated Show resolved Hide resolved

tests/ztimer_xsec/main.c Outdated Show resolved Hide resolved

tests/ztimer_xsec/main.c Outdated Show resolved Hide resolved

maribu reviewed Apr 1, 2021

View reviewed changes

cpu/native/periph/rtc.c Outdated Show resolved Hide resolved

cpu/native/periph/rtc.c Outdated Show resolved Hide resolved

cpu/native/periph/rtc.c Outdated Show resolved Hide resolved

kfessel force-pushed the p-zimer-sec branch from c848216 to e897533 Compare April 1, 2021 14:57

kfessel force-pushed the p-zimer-sec branch from e897533 to 842d878 Compare April 1, 2021 15:32

kfessel added 8 commits April 1, 2021 18:19

sys/ztimer: enhance periph selection documentation

c9e1719

provide more information about the selection procedure to Doxygen

tests/ztimer_xsec: add a simple high level ztimer test

febfa5b

native/rtc: avoid dealing with DST

72213ec

ztimer/periph_rtc: count months from 0

0b5709a

contrary to the western civilised world struct tm requires us to count months from 0

sys/ztimer: add ZTIMER_SEC to Kconfig

9e7f1d0

native/rtc: ensure no struct tm extra information is used

15f2d00

ztimer/periph_rtc: intialize struct tm to avoid TZ and IDST problems

3f66735

{ .elem = 0 } initializes the complete struct to 0 {} is not ISO C conform

kfessel force-pushed the p-zimer-sec branch from 842d878 to 3f66735 Compare April 1, 2021 16:22

fjmolinas approved these changes Apr 2, 2021

View reviewed changes

fjmolinas merged commit 9d1d2f9 into RIOT-OS:master Apr 2, 2021

This was referenced Apr 4, 2021

sys/ztimer: rename required_pm_mode to block_pm_mode #16160

Merged

ztimer: fix required_pm_mode initialization #15911

Closed

kfessel mentioned this pull request Apr 16, 2021

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

Closed

kaspar030 added this to the Release 2021.04 milestone Apr 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys/ztimer: add ZTIMER_SEC, improve auto_init #16172

sys/ztimer: add ZTIMER_SEC, improve auto_init #16172

kfessel commented Mar 9, 2021 •

edited

Loading

kfessel commented Mar 9, 2021

kfessel commented Mar 9, 2021

kfessel commented Mar 9, 2021 •

edited

Loading

kfessel commented Mar 29, 2021

fjmolinas commented Mar 29, 2021

kfessel commented Mar 29, 2021

fjmolinas commented Mar 29, 2021

fjmolinas commented Mar 29, 2021

fjmolinas commented Mar 30, 2021

maribu commented Mar 30, 2021

kfessel commented Mar 30, 2021 •

edited

Loading

maribu left a comment

kfessel commented Apr 1, 2021 •

edited

Loading

maribu left a comment

kfessel commented Apr 1, 2021

kfessel commented Apr 1, 2021 •

edited

Loading

kfessel commented Apr 1, 2021

fjmolinas commented Apr 1, 2021

fjmolinas left a comment

maribu commented Apr 2, 2021

kfessel commented Apr 2, 2021

sys/ztimer: add ZTIMER_SEC, improve auto_init #16172

sys/ztimer: add ZTIMER_SEC, improve auto_init #16172

Conversation

kfessel commented Mar 9, 2021 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

kfessel commented Mar 9, 2021

kfessel commented Mar 9, 2021

kfessel commented Mar 9, 2021 • edited Loading

kfessel commented Mar 29, 2021

fjmolinas commented Mar 29, 2021

kfessel commented Mar 29, 2021

fjmolinas commented Mar 29, 2021

fjmolinas commented Mar 29, 2021

fjmolinas commented Mar 30, 2021

maribu commented Mar 30, 2021

kfessel commented Mar 30, 2021 • edited Loading

maribu left a comment

Choose a reason for hiding this comment

kfessel commented Apr 1, 2021 • edited Loading

maribu left a comment

Choose a reason for hiding this comment

kfessel commented Apr 1, 2021

kfessel commented Apr 1, 2021 • edited Loading

kfessel commented Apr 1, 2021

fjmolinas commented Apr 1, 2021

fjmolinas left a comment

Choose a reason for hiding this comment

maribu commented Apr 2, 2021

kfessel commented Apr 2, 2021

kfessel commented Mar 9, 2021 •

edited

Loading

kfessel commented Mar 9, 2021 •

edited

Loading

kfessel commented Mar 30, 2021 •

edited

Loading

kfessel commented Apr 1, 2021 •

edited

Loading

kfessel commented Apr 1, 2021 •

edited

Loading