sys/ztimer: add convenience functions for sleeping #17330
Conversation
I think this is a footgun. If Like, start developing driver using IMO, the different hardware is difficult to abstract, and guarantees or bounds on accuracy / power consumption are hard to make. If clocks are freely switched depending on the whole dependency tree, it is even harder to make guarantees. Using a more accurate clock is fine accuracy wise (more accuracy is acceptable), e.g., use ZTIMER_USEC if MSEC is not available. That breaks So in short, I'm OK with mapping |
Although it is a contrieved example, the current implementation would allow a |
cpu/stm32/periph/adc_f3.c
Outdated
| #if IS_USED(MODULE_ZTIMER_USEC) | ||
| ztimer_sleep(ZTIMER_USEC, ADC_T_ADCVREG_STUP_US); | ||
| #else | ||
| /* to avoid using ZTIMER_USEC unless already included round up the | ||
| internal voltage regulator start up to 1ms */ | ||
| ztimer_sleep(ZTIMER_MSEC, 1); | ||
| #endif | ||
| ztimer_sleep_usecs(ADC_T_ADCVREG_STUP_US); |
There was a problem hiding this comment.
Note that with the exception of the possibility to degrade to second resolution, this is already doing what the convenience functions do. So the question on whether automatic clock selection is acceptable needs to be answered independent of this PR.
There are a lot of drivers that call into ztimer exactly once for a sleep after a reset, and the drivers don't care at all if the delay gets rounded up to milliseconds, or even seconds. (But I agree that for user experience, rounding up to seconds might indeed be a footgun.) For low power use cases it could be beneficial to allow applications to run without pulling in ztimer_usec at all. And on the other hand, a user with a low-on-memory device might care not about the power consumption benefit of adding ztimer_msec, but care a lot about the additional memory this would use.
So I think there are use cases for this. So why not provide a convenience function for this to make it easier to cater these use cases.
There was a problem hiding this comment.
I dropped the degrading to seconds. It is now identical to the code before, just easier to read. I also added some more visible warnings.
There was a problem hiding this comment.
I see the use case. I still think that we cannot have a "ztimer_sleep_usecs()" that behaves as specified. (the name alone would make users use it, and probably be surprised.)
Can we boil this down to just ztimer_sleep_msec() that just falls back to use ZTIMER_USEC if MSEC is not available?
If users don't care about the accuracy, that should be fine.
And, multiplication is faster than division. :)
Having an API that is called "sleep for usecs" but sometimes it actually sleeps quantized to an msec timer is weird.
There was a problem hiding this comment.
And, multiplication is faster than division.
For the use case it has this shouldn't matter, there should be a DRIVER_FOO_RESET_SLEEP defined somewhere, so that the compiler does constant folding anyway.
the name alone would make users use it, and probably be surprised
Maybe we should just find a better name. ztimer_sleep_usec_with_footgun() or ztimer_sleep_usec_unprecise()?
There was a problem hiding this comment.
ztimer_sleep_at_least() because when initializing drivers, I don't care how long the sleep takes as long as it sleeps for at least the required amount of time.
There was a problem hiding this comment.
Note that also ztimer_sleep() has at-least-semantics, as there is little ztimer could do for low prio tasks in case higher priority tasks would kick in when the sleep is over. The difference is only that this sleep can take longer even in absence of load.
I fear if the is a special at-least function, users start assuming the regular version has no at-least-semantics.
There was a problem hiding this comment.
ztimer_sleep() has at-least semantic but also best effort (if there isn't a higher prio task and the sleep isn't to short to manage it should be within 0-2 units)
There was a problem hiding this comment.
I added a _relaxed postfix to all functions, as for each of them at least one assumption has to be relaxed regarding the behavior.
|
Why have this a ztimer function, what it provides can be implemented in different ways ( eg checking ARMs syscounter) as you said it is often used just once -> i think we could use some convinece sleep functions that use whatever source is available. (making them independent of ztimer (even if there isn't a second implementation now) sleep_usec |
I'm not sure what the benefit of a meta-timer API would be.
I don't think it makes sense to add on-least functions, as "least" (+/- rounding error and clock drift) is the only sane guarantee one can make. Providing an "exact" or "at most" guarantee will not work if there is load on the system, unless one adds an immensely complex task admission and timer admission system that will only allow to add threads and timer, if the system can be guaranteed to remain real-time capable. |
|
I'm fine with the |
|
Profit of a "meta" api you will have to rewrite less if you want to switch the timer below it: a API (similar) to what you are proposing is exiting in these late days of xtimer, in the form of just had a look into the diff an saw you a reimplemting the very API that shows how this is independent o ztimer the at_least is about relaxing the resoulution and best effort to keep within - constrains @kaspar030 just wrote: i am still for the meta part since this api needs not to be bound to ztimer as its implemetation |
There was a problem hiding this comment.
I like the API, this will simplify things a lot.
Since I envision those functions being used most of the time, I would have preferred the shorter names (ztimer_usleep(), ztimer_msleep(), ztimer_sleep()).
Bike-shedding aside, anything needed to get this unblocked?
This adds three additional wrapper-convenience functions for when drivers have relaxed requirements and don't want to pull in additional clocks, but rather use whatever clock is available. Also refactored the xtimer_compat code to make use of the new functions.
The issue is that there already is a Maybe |
How about grouping the functions instead in the documentation (see fixup commit)? That structures the doc and group those sleep functions to easy grasping the documentation. As @benpicco says, these helpers may become popular ;) Having to use and extra module or |
|
how about removing the while writing i think |
|
tbh I find it not immediately clear what 'relaxed' means in the context of timers. How about |
|
@benpicco: just "relax some seconds" :) I also don't think of this in term of a timer, but in more in the sleeping (or relaxing) (realworld example: more like the laying down onto bed/couch and rest, than the setting an alarm clock) I don't see a reason to tie this API to ztimer. but sadly xtimer_msleep (x as in any) is also taken (by x as in this is using the xtimer backend (or an api emuation) (there is some old API we are in the process of replacing) |
but wouldn't it be super convenient if the old API just so happens to be compatible with the new API and will automatically do the right thing? |
but we can't -since the old API is defined and expected to do the best thing it can do to hit that timing (may need some extra cylces for the list magement or if the CPU is occupied otherwise some extra cylcles of that), but with this API the very common case of requesting something and checkback later should be addressed. e.g. Hardware Initialisation Datasheet: power this thing up and it will be ready in max 1sec typical 400ms) e.g.: Onewire Temperature Messurement while the protocol needs µsec timing the meassurment periode can have a sleeping cpu (no exact timing need just sleep longer than 750ms (of my head this is for 10Bit??)) (wakeup takes less than a msec (stm32) so 749ms of just a LPTIMER do the waiting and this can easily be relaxed to to 1-2 seconds which would be RTC realm (no need for a ztimer_msec accuracy so just |
|
So, the verdict is to move this to |
maybe |
|
... but I still think having those functions switch their accuracy to "lower" by an order of magnitude, depending on the dependency tree, is a footgun ... |
|
|
|
I think this is still very much useful. |
|
I picked up the idea here again in #19719 and this time hopefully picked a API names that are resistant to bike-shedding |
Contribution description
This adds three additional wrapper-convenience functions for when drivers have relaxed requirements and don't want to pull in additional clocks, but rather use whatever clock is available.
The xtimer compat code and the STM32 ADC drivers are added as the first users of the new convenience API.
Testing procedure
Sleeping for minimum durations specified in usecs, msecs, and secs should work now regardless of which ztimer clocks are available, as long at least one is present. If multiple options are present, the most suitable clock should be used.
Issues/PRs references
None