sys/ztimer: add ZTIMER_SEC #15715

maribu · 2021-01-07T09:33:53Z

Contribution description

Wired up ZTIMER_SEC to the existing RTC backend, or RTT backend, or periph_timer backend (in this order of preference).

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

None

maribu · 2021-01-07T09:34:58Z

@kfessel might want to have a look at this

sys/ztimer/auto_init.c

leandrolanzieri · 2021-01-07T09:52:42Z

sys/ztimer/auto_init.c

+static ztimer_convert_frac_t _ztimer_convert_frac_sec;
+ztimer_clock_t *const ZTIMER_SEC = &_ztimer_convert_frac_sec.super.super;
+ztimer_clock_t *const ZTIMER_SEC_BASE = &_ztimer_periph_timer_msec.super;
+#    define ZTIMER_SEC_CONVERT_LOWER        (&_ztimer_periph_timer_rtt_msec)


is it ok to use this symbol here? doesn't it need the rtt?

Good catch. I changed the preprocessor conditional to only uses this if both ztimer_msec and ztimer_periph_rtt are used.

Maybe it makes sense to decouple this from ztimer_msec to also profit from the power savings when ztimer_sec but not ztimer_msec is used. I'll fix this.

Done. RTT is now used for ZTIMER_SEC when available regardless of ZTIMER_MSEC, unless the application/user explicitly pulled in ztimer_periph_rtc.

kaspar030 · 2021-01-07T10:16:53Z

Long overdue! But I think we should try to not use RTC. On most platforms, it shares the same hardware with RTT anyways (requiring to chose one). Also the RTC interface is clumsy to use if only a seconds granularity counter is desired (like in this case).

So, please change to use RTT as preference (possibly a converted ZTIMER_MSEC), and only fall back to RTC if no RTT is available.

kaspar030 · 2021-01-07T10:19:19Z

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.

Yeah, the RTC backend probably doesn't yet deal with setting 0 or 1 properly. It would need to set at least +2 to be safe.

maribu · 2021-01-07T11:21:52Z

So, please change to use RTT as preference (possibly a converted ZTIMER_MSEC), and only fall back to RTC if no RTT is available.

Hmm, if one pulls in ztimer_periph_rtc, IMO we really should use this for ZTIMER_SEC. So far there is no logic that every pulls in ztimer_periph_rtc, so it is up to the user to add this module. And for the reasons you provided against using the RTC, I don't think we should automatically pull it in.

Btw.: Shouldn't we (unrelated of this PR) use FEATURES_OPTIONAL += periph_rtt for ztimer_msec and ztimer_sec and automatically use ztimer_periph_rtt if periph_rtt is used?

maribu · 2021-01-07T11:41:19Z

Oh, I chose the wrong commit with --fixup=. I hope I remember to manually squash to fix this :-D

kaspar030 · 2021-01-07T12:53:10Z

Btw.: Shouldn't we (unrelated of this PR) use FEATURES_OPTIONAL += periph_rtt for ztimer_msec and ztimer_sec and automatically use ztimer_periph_rtt if periph_rtt is used?

That's an FAQ by now. The issue is that if ztimer_periph_rtt is used, it'll be used for ztimer_msec. But not all periph_rtt can actually do 1000 or 1024 Hz (or are configured that way). I think we need to add more fine-grained rtt capabilities as features.

maribu · 2021-01-12T15:43:17Z

I think we need to add more fine-grained rtt capabilities as features.

👍

kfessel

I did not want to respond without measuring. Yesterday i found my nucleo-l073rz having a IDD bride. I tested different configuration (non extra, rtt, rtc, rtt and rtc). Not every configuration could be built first I fixed that, patch attached an added the pm_layerd configuration for ZTIMER_SEC

i will add the patch later

maribu · 2021-02-05T13:54:13Z

@kfessel: Care to also squash when rebasing?

maribu · 2021-02-05T15:35:45Z

sys/ztimer/auto_init.c

+#  ifdef MODULE_PM_LAYERED
+    LOG_DEBUG("ztimer_init(): ZTIMER_SEC setting required_pm_mode to %i\n",
+              CONFIG_ZTIMER_SEC_REQUIRED_PM_MODE);
+    ZTIMER_SEC->required_pm_mode = CONFIG_ZTIMER_SEC_REQUIRED_PM_MODE;


@kfessel: Can you take a look at #15911 I think this needs to be applied to the hardware clock. In case of the RTC, this is already the hardware clock, but in case of RTT, it should be applied to the RTT clock sitting below the convert frac clock.

you are right and while writing that lines i thought this is wrong but its the current way to go and i was already searching if there is an issue or PR in this direction.
so these PRs are in "conflict". one has to merged the other has to follow

kfessel · 2021-02-05T15:36:53Z

my Test Example

IDD on nucleo-l073rz waitfor rtc or rtt 1.5µA (30€ Multimeter)

ztimer_pm.zip

kfessel · 2021-02-09T21:55:43Z

with restructure for flexibility i also fixed the issue of #15911

kfessel · 2021-02-09T22:07:18Z

uncrustify keeps calling out:
sys/include/ztimer.h#L295
it thinks the * is an operation but it is a pointer-marker (i like the space after the * but it is to my understanding against the uncrustify rules)

kfessel · 2021-03-04T11:27:32Z

the fixup also adds the comments
@fjmolinas: you are right the "rename reorder uncrustify" is a fixup but i also think its title is better matching (and much easyser to see in qgit fixup can be anything

kfessel · 2021-03-04T12:34:41Z

uncrustfy is just plain wrong wrong about sys/include/ztimer.h#L297 and sys/include/ztimer.h#L309 its against the riot uncrustfy rules

and no one in their right mind would consider to do that

--- a/sys/include/ztimer/config.h
+++ b/sys/include/ztimer/config.h
@@ -109,10 +109,10 @@ extern "C" {
 #ifndef CONFIG_ZTIMER_TIMER_REQUIRED_PM_MODE
 #  ifdef CONFIG_ZTIMER_USEC_REQUIRED_PM_MODE
 #    define CONFIG_ZTIMER_TIMER_REQUIRED_PM_MODE \
-            CONFIG_ZTIMER_USEC_REQUIRED_PM_MODE
+    CONFIG_ZTIMER_USEC_REQUIRED_PM_MODE
 #  else
 #    define CONFIG_ZTIMER_TIMER_REQUIRED_PM_MODE \
-            ZTIMER_CLOCK_NO_REQUIRED_PM_MODE
+    ZTIMER_CLOCK_NO_REQUIRED_PM_MODE
 #  endif
 #endif

it kills the last bit of readablity that was left (after a line breake that it sugested there for 2-5 chars

kaspar030 · 2021-03-04T13:22:37Z

uncrustfy is just plain wrong wrong about sys/include/ztimer.h#L297 and sys/include/ztimer.h#L309 its against the riot uncrustfy rules

uncrustify is currently only warning. just use up-to-date uncrustify and ignore the warnings.

it kills the last bit of readablity that was left (after a line breake that it sugested there for 2-5 chars

main reason to use uncrustify is to not deal with formatting anymore but still have consistent looks. right now, that might be at the cost of some readability sometimes.

maribu · 2021-03-04T13:26:53Z

Maybe we could configure uncrustify to never ever suggest a line break. We have other tools already complaining about long lines and I agree that uncrustify really does a poor job at breaking lines.

kaspar030 · 2021-03-04T13:31:04Z

Maybe we could configure uncrustify to never ever suggest a line break. We have other tools already complaining about long lines and I agree that uncrustify really does a poor job at breaking lines.

I do like being able to do "strg-f" (my shortcut for uncrustify) and be done with it, and enjoy not going through a CI static-test fail/look at/manually fix cycle...

We should definitely update the uncrustify on CI. if it takes too long to go to 2020.04 LTS, maybe self-compile the latest version.

kfessel · 2021-03-04T13:37:18Z

but even the version that comes with 2020.10 wants to make that define less readable

ATM i think changing CONFIG_ZTIMER_TIMER_REQUIRED_PM_MODE to CONFIG_ZTIMER_TIM_REQ_PM_MODE to cicumvent that uncrustify problem

for the gcc warning you can state don't do that here, is there something that does it for uncrustify

maribu · 2021-03-04T13:45:53Z

I do like being able to do "strg-f" (my shortcut for uncrustify) and be done with it, and enjoy not going through a CI static-test fail/look at/manually fix cycle...

Add set cc=101 to your .vimrc / init.vim and you'll never accidentally surpass the limit.

Also: uncrustify still is at 80 chars (not 100). Fixing this would solve the issue at hand.

But generally: The coding convention only recommends to keep lines shorter than up to 100 chars - as otherwise this sets and insensitive to use short but unreadable names.

kfessel · 2021-03-04T13:50:29Z

I do like being able to do "strg-f" (my shortcut for uncrustify) and be done with it, and enjoy not going through a CI static-test fail/look at/manually fix cycle...

Add set cc=101 to your .vimrc / init.vim and you'll never accidentally surpass the limit.

Also: uncrustify still is at 80 chars (not 100). Fixing this would solve the issue at hand.

But generally: The coding convention only recommends to keep lines shorter than up to 100 chars - as otherwise this sets and insensitive to use short but unreadable names.

so i remove the linebreakes and ignore the uncrustify?

kaspar030 · 2021-03-04T13:50:35Z

ATM i think changing CONFIG_ZTIMER_TIMER_REQUIRED_PM_MODE to CONFIG_ZTIMER_TIM_REQ_PM_MODE to cicumvent that uncrustify problem

you can also use code-style-ignore comments.

maribu · 2021-03-04T13:56:48Z

so i remove the linebreakes and ignore the uncrustify?

Let's wait a bit. I'll PR a change to 100 chars. That should go in without discussion, as our coding convention was updated to allow this and we just forgot to also touch uncrustify. Maybe the issue is gone with that already.

kfessel · 2021-03-05T10:19:44Z

#16149 will update riot-uncrustify to 100 chars (riot-coding convention)

kfessel · 2021-03-05T10:49:29Z

just checked the complains by uncrustify:
-it 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 multipication in that line)
-sys/include/ztimer/config.h#L108 #L119 #L130 are complient with riot coding convention of upto 100 chars in a line
breaking them hinders readability (especially if the uncrustify indention-suggestion is used)

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>

rename ztimer_periph_timer_rtt for readability fix debugmessage reorder defines

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

better go for readybility and convention than uncrustify

BLOCK instead of require

kfessel · 2021-03-08T14:12:54Z

rebased for uncrustify config update

maribu · 2021-03-09T10:27:27Z

At this point I'm still formally author of this PR, but haven't really authored much of the changes. IMO it would make sense if I close the PR and @kfessel opens a new one with the current state (which references this one). This would IMO better document the development history and authorship.

kfessel · 2021-03-17T09:25:18Z

Please continue the review in #16172

maribu requested review from kaspar030, leandrolanzieri and jue89 January 7, 2021 09:33

maribu requested review from aabadie, bergzand, fjmolinas, MichelRottleuthner and smlng as code owners January 7, 2021 09:33

maribu added Area: sys Area: System Area: timers Area: timer subsystems labels Jan 7, 2021

leandrolanzieri reviewed Jan 7, 2021

View reviewed changes

sys/ztimer/auto_init.c Outdated Show resolved Hide resolved

leandrolanzieri reviewed Jan 7, 2021

View reviewed changes

leandrolanzieri mentioned this pull request Jan 7, 2021

sys/ztimer: add modules to Kconfig #15716

Merged

maribu requested a review from vincent-d January 12, 2021 15:42

kfessel requested changes Feb 4, 2021

View reviewed changes

kfessel force-pushed the ztimer_sec branch 2 times, most recently from 75b1781 to 13cbca8 Compare February 5, 2021 14:33

maribu commented Feb 5, 2021

View reviewed changes

kfessel mentioned this pull request Feb 5, 2021

ztimer: fix required_pm_mode initialization #15911

Closed

kfessel mentioned this pull request Mar 5, 2021

sys/ztimer: rename required_pm_mode to block_pm_mode #16160

Merged

maribu and others added 10 commits March 8, 2021 15:11

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>

tests/ztimer_msg: Allow testing ZTIMER_SEC

c2b1344

tests/ztimer_underflow: allow testing ZTIMER_SEC

ca6929d

some fixes and power management for ZTIMER_SEC

6627ce2

rename reorder uncrustify

4c6f58d

rename ztimer_periph_timer_rtt for readability fix debugmessage reorder defines

ztimer/auto_init.c restructure for flexibitily

6d047b6

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 and be a bit explicit about the choice taken

e00373f

fixup! ztimer/auto_init.c restructure for flexibitily

3d47991

fixup! fixup! ztimer/auto_init.c restructure for flexibitily

59f7123

better go for readybility and convention than uncrustify

fixup! ztimer/auto_init.c restructure for flexibitily

80932dd

BLOCK instead of require

kfessel force-pushed the ztimer_sec branch from 23261d8 to 80932dd Compare March 8, 2021 14:12

maribu closed this Mar 9, 2021

kfessel mentioned this pull request Mar 9, 2021

sys/ztimer: add ZTIMER_SEC, improve auto_init #16172

Merged

maribu deleted the ztimer_sec branch March 31, 2021 08:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys/ztimer: add ZTIMER_SEC #15715

sys/ztimer: add ZTIMER_SEC #15715

maribu commented Jan 7, 2021

maribu commented Jan 7, 2021

leandrolanzieri Jan 7, 2021

maribu Jan 7, 2021

maribu Jan 7, 2021

kaspar030 commented Jan 7, 2021

kaspar030 commented Jan 7, 2021 •

edited

Loading

maribu commented Jan 7, 2021

maribu commented Jan 7, 2021

kaspar030 commented Jan 7, 2021

maribu commented Jan 12, 2021

kfessel left a comment •

edited

Loading

maribu commented Feb 5, 2021

maribu Feb 5, 2021

kfessel Feb 5, 2021 •

edited

Loading

kfessel commented Feb 5, 2021 •

edited

Loading

kfessel commented Feb 9, 2021

kfessel commented Feb 9, 2021

kfessel commented Mar 4, 2021

kfessel commented Mar 4, 2021

kaspar030 commented Mar 4, 2021

maribu commented Mar 4, 2021

kaspar030 commented Mar 4, 2021

kfessel commented Mar 4, 2021 •

edited

Loading

maribu commented Mar 4, 2021

kfessel commented Mar 4, 2021

kaspar030 commented Mar 4, 2021

maribu commented Mar 4, 2021

kfessel commented Mar 5, 2021 •

edited

Loading

kfessel commented Mar 5, 2021 •

edited

Loading

kfessel commented Mar 8, 2021 •

edited

Loading

maribu commented Mar 9, 2021

kfessel commented Mar 17, 2021

sys/ztimer: add ZTIMER_SEC #15715

sys/ztimer: add ZTIMER_SEC #15715

Conversation

maribu commented Jan 7, 2021

Contribution description

Testing procedure

Issues/PRs references

maribu commented Jan 7, 2021

leandrolanzieri Jan 7, 2021

Choose a reason for hiding this comment

maribu Jan 7, 2021

Choose a reason for hiding this comment

maribu Jan 7, 2021

Choose a reason for hiding this comment

kaspar030 commented Jan 7, 2021

kaspar030 commented Jan 7, 2021 • edited Loading

maribu commented Jan 7, 2021

maribu commented Jan 7, 2021

kaspar030 commented Jan 7, 2021

maribu commented Jan 12, 2021

kfessel left a comment • edited Loading

Choose a reason for hiding this comment

maribu commented Feb 5, 2021

maribu Feb 5, 2021

Choose a reason for hiding this comment

kfessel Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

kfessel commented Feb 5, 2021 • edited Loading

kfessel commented Feb 9, 2021

kfessel commented Feb 9, 2021

kfessel commented Mar 4, 2021

kfessel commented Mar 4, 2021

kaspar030 commented Mar 4, 2021

maribu commented Mar 4, 2021

kaspar030 commented Mar 4, 2021

kfessel commented Mar 4, 2021 • edited Loading

maribu commented Mar 4, 2021

kfessel commented Mar 4, 2021

kaspar030 commented Mar 4, 2021

maribu commented Mar 4, 2021

kfessel commented Mar 5, 2021 • edited Loading

kfessel commented Mar 5, 2021 • edited Loading

kfessel commented Mar 8, 2021 • edited Loading

maribu commented Mar 9, 2021

kfessel commented Mar 17, 2021

kaspar030 commented Jan 7, 2021 •

edited

Loading

kfessel left a comment •

edited

Loading

kfessel Feb 5, 2021 •

edited

Loading

kfessel commented Feb 5, 2021 •

edited

Loading

kfessel commented Mar 4, 2021 •

edited

Loading

kfessel commented Mar 5, 2021 •

edited

Loading

kfessel commented Mar 5, 2021 •

edited

Loading

kfessel commented Mar 8, 2021 •

edited

Loading