ztimer: add ztimer_ondemand module for implicit power management #17607
Conversation
jue89
added
Process: API change
github-actions
bot
added
Area: sys
|
Sneak peak: I'm completely aware of ztimer_watchtypedef struct {
ztimer_now_t checkpoint; /**< last seen timer now value */
ztimer_now_t value; /**< watch time at checkpoint */
bool running; /**< flag showing if the watch is running */
} ztimer_watch_t;
void ztimer_watch_start(ztimer_clock_t *clock, ztimer_watch_t *watch)
{
if (watch->running) {
return;
}
ztimer_acquire(clock);
watch->checkpoint = ztimer_now(clock);
running = true;
}
void ztimer_watch_set(ztimer_clock_t *clock, ztimer_watch_t *watch, ztimer_now_t val)
{
if (watch->running) {
watch->checkpoint = ztimer_now(clock);
}
watch->value = val;
}
ztimer_now_t ztimer_watch_read(ztimer_clock_t *clock, ztimer_watch_t *watch)
{
if (watch->running) {
ztimer_now_t now = ztimer_now(clock);
return now - watch->checkpoint + watch->value;
}
else {
return watch->value;
}
}
void ztimer_watch_stop(ztimer_clock_t *clock, ztimer_watch_t *watch)
{
if (!watch->running) {
return;
}
watch->value = ztimer_watch_read(clock, watch);
watch->running = false;
ztimer_release(clock);
}We should make every user of Or we could introduce a global |
|
Is this really an API change? The use of On acquire and release, I wonder if they are actually relevant (and warrant their counter). Any user just comparing the ztimer_now outputs will find they have a hard time knowing whether it wrapped, whereas if someone acquiring the ztimer just placed a maximal timeout and then checked for it still being set after obtaining a later now value would be sure that it did not wrap (or, it may have wrapped but the wrapping difference never overflew). Their timer callback could be a no-op (the task will conclude, and the measured time will be "exceeding the scale"), or do some cancellation ("don't bother, it has already taken too long"). (The uptime counter would need special handling then, though.) |
|
Thank you for your response! Yes, I agree that setting up a You may have noticed: This PR disables all these mechanics, once you pull in After having made my first baby steps with this approach, I've already noticed that this implicit power management fails fast, which is a good thing! If a |
|
An addition to the wrapped ztimer_now() values - I just understood your point after reading your response a second time: If there must the guarantee that it hasn't wrapped during time measurement, we would have to use Your approach clearly detects wraps: But what shall we do if we detected wrapping timers? Extend like |
|
Your approach clearly detects wraps: But what shall we do if we
detected wrapping timers? Extend like `ztimer_now64` does?
Something like a watch, when detecting a wrapped timer, should IMO just
report "longer than I can represent". A user who asked for a 32bit value
in some units is not prepared for any larger values anyway.
|
jue89
requested review from
aabadie,
leandrolanzieri,
MichelRottleuthner and
smlng
as code owners
|
Some additional work: I've introduced Furthermore, I wrote a little example: |
|
Would anybody mind if I squash commits? I think I have finished for the first round and implemented everything necessary for the |
|
I wouldn't mind, and I think that kfessel's comment is high-level enough that a squash wouldn't disturb it. |
jue89
force-pushed
the
feature/ztimer-ondemand
branch
2 times, most recently
from
371045e
to
309cbfe
Compare
|
And squashed. I'd say this PR is ready to be reviewed. I've fixed typos and added some breaks to long lines. Static tests are green now. |
jue89
added
the
CI: ready for build
|
I updated the initial PR message with my testing procedure on already working boards and families :) I'd say this is ready to be reviewed. |
sys/include/ztimer.h
Outdated
| unsigned state = irq_disable(); | ||
| if (_ztimer_acquire(clock) == true) { | ||
| /* if the clock just has been enabled, make sure to set possibly | ||
| * required checkpoints for clock extension */ | ||
| _ztimer_update(clock); | ||
| } | ||
| irq_restore(state); |
There was a problem hiding this comment.
Wouldn't it make sense to move all the IRQ management and the call to _ztimer_update() into _ztimer_acquire() to safe a bit of ROM?
There was a problem hiding this comment.
I'd rather remove the inline statement and move ztimer_acquire() into core.c.
The separation between ztimer_acquire() and _ztimer_acquire() saves some time: the _ztimer_update() call on the first acquisition isn't required if it is called by ztimer_set() ... it'll set the right timer in the process.
Or am I underestimating the time required for a function call?
There was a problem hiding this comment.
I hope, I got you right ... I added two fixup commits.
There was a problem hiding this comment.
Could you have another look onto the code if I understood your suggestion correctly?
| #if MODULE_ZTIMER_ONDEMAND_RTT | ||
| static void _ztimer_periph_rtt_start(ztimer_clock_t *clock) | ||
| { | ||
| (void)clock; | ||
| rtt_poweron(); | ||
| } | ||
|
|
||
| static void _ztimer_periph_rtt_stop(ztimer_clock_t *clock) | ||
| { | ||
| (void)clock; | ||
| rtt_poweroff(); | ||
| } | ||
| #endif /* MODULE_ZTIMER_ONDEMAND_RTT */ |
There was a problem hiding this comment.
| #if MODULE_ZTIMER_ONDEMAND_RTT | |
| static void _ztimer_periph_rtt_start(ztimer_clock_t *clock) | |
| { | |
| (void)clock; | |
| rtt_poweron(); | |
| } | |
| static void _ztimer_periph_rtt_stop(ztimer_clock_t *clock) | |
| { | |
| (void)clock; | |
| rtt_poweroff(); | |
| } | |
| #endif /* MODULE_ZTIMER_ONDEMAND_RTT */ | |
| __attribute__((unused)) | |
| static void _ztimer_periph_rtt_start(ztimer_clock_t *clock) | |
| { | |
| (void)clock; | |
| rtt_poweron(); | |
| } | |
| __attribute__((unused)) | |
| static void _ztimer_periph_rtt_stop(ztimer_clock_t *clock) | |
| { | |
| (void)clock; | |
| rtt_poweroff(); | |
| } |
Would be an alternative. The advantage would be that the start and stop functions would be compile-tested no matter what. The __attribute__((unused)) is pretty ugly, though.
If you take the suggestion, please do so consistently for all clock backends.
There was a problem hiding this comment.
I've thought about this. It'll add compile time for systems that doesn't make use of ztimer_ondemand_rtt. Do we want that cost? I'm not sure ...
There was a problem hiding this comment.
The general trend is to go to e.g. rust where a lot more compile time checks are happening. Also GCC and clang etc became slower and slower over the years, but generate faster and smaller machine code and better diagnostics.
I'd say it is consensus that trading in compile time for faster/smaller/safer code is a good trade off. It certainly is reducing maintenance costs by having larger portions compile time tested.
There was a problem hiding this comment.
Are you okay to address this in a follow-up? There's a lot of code in ztimer applying the same pattern.
There was a problem hiding this comment.
Yes! Let's get this in first and address the nit picks afterwards :) I may just open a PR to address this here and elsewhere in ztimer :)
|
May I squash? (CC: @maribu) |
Enforce ztimer_clock_t to be active (i.e. clock->users > 0) before ztimer_now() can be called.
The start/stop overhead that might by introduced by ztimer_acquire() and ztimer_release() called during ztimer_set() resp. ztimer_handler() should not be mesured here. It has its own adjustment field. Furthermore, the overhead mesaurement uses ztimer_now(). It is allowed to called it only after the clock has been acquired.
jue89
force-pushed
the
feature/ztimer-ondemand
branch
from
eaa816e
to
8d6d6f2
Compare
|
May I gently ping? Soft freeze of 2023.01 will happen in under 3 weeks - and I'd love to get this one in :-) |
|
Some maintainers (@chrysn @maribu @kfessel) had a closer look into this PR. The remaining objection is complaining about RIOT's power management architecture and not this particular implementation. Thus, I'm pressing the shiny green merge button. This introduces the Thank you very much for your valuable feedback and the support to push this PR forward! :-) |
Contribution description
This PR implements the approach proposed in #16327. Basically it extends
ztimerto start and stopztimer_periph_%depending on the count of currently active users. This solves problems like described in #16891.The
ztimer_ondemandfeature can be enabled per peripheral driver. To start/stopperiph_timer,ztimer_ondemand_timerhas to be pulled in.periph_rttis started/stopped byztimer_ondemand_rttandperiph_rtcbyztimer_ondemand_rtc. I think you get the idea ;-)To help
ztimerto understand if someone is currently relaying on a certainztimer_clock_t,ztimer_acquire()andztimer_release()have been introduced. These methods are required for users ofztimer_now(). The interface works like this:(
ztimer_set()andztimer_remove()will callztimer_acquire()andztimer_release()internally.)First of all, I'd love to hear your opinion about the direction I'm heading in.
I also came up with a possible migration path: #17607 (comment)
Testing procedure
I've added unit tests.
During the next days I'll come up with some sample code that you may run on your device.A demo showcasing ztimer_ondemand:
Diff for the demo rebased on current master
Some tests on boards that already have power management on
periph_timerandperiph_rtt:samr30-xpro:xg23-pk6068a:nrf52dk:They're all entering lowest power mode after all the hard hello world work is done \o/
Issues/PRs references