sys/wait: add module for backend agnostic waits #19719

maribu · 2023-06-08T12:24:41Z

Contribution description

This adds to sets of delay functions. The first kind consists of

wait_at_least_us(): delay execution in units of microseconds
wait_at_least_ms(): delay execution in units of milliseconds
wait_at_least_s(): delay execution in units of seconds

These will use ztimer as backend, if used anyways. Otherwise a trivial CPU counting loop is used.

The second kind consists of

wait_us_spinning(): busy-wait for a given number of microseconds

The benefit of this compared to ztimer_spin() is that for AVR a platform specific implementation is used that is more accurate. It allows writing drivers like the DHT11/DHT2x driver without platform specific code. Other low end platforms can follow suit and also add more accurate waiting code.

Testing procedure

A test application has been added with a python script doing automatic testing. In addition, that test app performs short pulses on the LED that should be visible via a scope / logic analyzer. Finally, it will estimate the CPU cycles required per loop iteration that can be used to fine-tune the loop.

Issues/PRs references

Deprecates: #17330

Useful for: #19718

kaspar030 · 2023-06-08T13:35:16Z

Again, I don't think that delay_us() should fall back to msec.

maribu · 2023-06-08T15:11:19Z

I dropped delay_us()

cpu/avr8_common/include/delay_arch.h

kaspar030 · 2023-06-08T19:57:08Z

I dropped delay_us()

I think that would be useful. Can't it just use ZTIMER_USEC?

"Those are trivial wrappers over ztimer_sleep() that just use the most appropriate clock that is used anyway. It gives up control over the most suitable trade-off between resolution, accuracy (clock drift) and power consumption for some flexibility / ROM size."

Repeating some arguments from #17330 ...

Didn't If they are "trivial" wrappers, make them use the corresponding clock. delay_us(N) -> ztimer_sleep(ZTIMER_USEC, N) etc.

Making this depending on which module is already used is awful, as adding modules to an already tested part of an application might add more timer modules, making the behavior change. That is especially bad for delay_s(), which might more often be used in human scale small numbers.

Like, "delay_s(2)" is +0..2seconds when using ZTIMER_SEC, +0..2 milliseconds when using ZTIMER_MSEC.

So for the timer-based delay functions, why not just:

delay_us(N) -> ztimer_sleep(ZTIMER_USEC, N)
delay_ms(N) -> ztimer_sleep(ZTIMER_MSEC, N)
delay_s(N) -> _ztimer_sleep_scale_up(ZTIMER_MSEC, secs, MS_PER_SEC) (as ZTIMER_SEC is usually implemented using ZTIMER_MSEC anyways)

Alternatively, document clearly that the delays are +0..2, and where that matters, the higher resolution delay should be used.

But please, don't make potentially hand-tuned timings change just because adding USEMODULE += some_other_module pulls in ztimer_msec.

kfessel · 2023-06-08T20:03:33Z

sys/include/delay.h

+ */
+static inline void delay_ms(uint32_t msecs)
+{
+    if (IS_USED(MODULE_ZTIMER_MSEC)) {


Suggested change

if (IS_USED(MODULE_ZTIMER_MSEC)) {

if (IS_USED(MODULE_ZTIMER_MSEC) && msec > 10) {

jitter < 10%

This would trade ROM for increased accuracy. But the whole point of this API is to trade in accuracy to safe ROM / dependencies.

ztimer_msec is not able to sleep for 1 ms ( a value of 1 is 0 usec to 2000 usec).

there will not be any ROM usage for compiletime constant delays i would such a functions be used for (static inline)

kfessel · 2023-06-08T20:04:31Z

sys/include/delay.h

+ */
+static inline void delay_s(uint32_t secs)
+{
+    if (IS_USED(MODULE_ZTIMER_SEC)) {


Suggested change

if (IS_USED(MODULE_ZTIMER_SEC)) {

if (IS_USED(MODULE_ZTIMER_SEC) && secs > 10) {

sys/include/ztimer.h

cpu/avr8_common/include/delay_arch.h

bergzand · 2023-06-09T07:38:52Z

sys/include/delay.h

+ *
+ * # Motivation
+ *
+ * @ref sys_ztimer already provides @ref ztimer_sleep to allow delaying the
+ * execution of the running thread. While @ref ztimer_sleep allows via the
+ * selection of the clock to select the best fitting trade-off between
+ * resolution, accuracy (clock drift), power consumption and so on, it is
+ * difficult to write code with flexible requirements that just picks whatever
+ * clock is available.


How about adding a paragraph on top here about what this module is supposed to do instead of having to derive it of what ztimer is not able to do. Then maybe include also a bit on when to pick this over ztimer.

kaspar030 · 2023-06-09T08:17:41Z

 * For use cases such as waiting 1 ms for a device to reset during start up it
 * matters little if the delay ends up being 1 ms, 2 ms or even 10 ms.

What's wrong with just using ZTIMER_MSEC for that?

maribu · 2023-06-09T11:09:41Z

falling back to ZTIMER_USEC is not acceptable

Why?

To be honest, I would be in favor of falling back to ZTIMER_USEC. But I'd rather have this in without falling back to ZTIMER_USEC then having PR going stale. (You could do a follow up PR, though 😉)

benpicco · 2023-06-09T11:44:06Z

sys/include/wait.h

+    if (IS_USED(MODULE_ZTIMER_MSEC)) {
+        ztimer_sleep(ZTIMER_MSEC, msecs);
+    }
+    else {
+        uint32_t iterations = msecs * (uint64_t)coreclk() / MS_PER_SEC + 1;
+        wait_busy_loop(iterations);
+    }


Are you sure that's better than a fallback to ZTIMER_USEC?
And why no wait_at_least_us()?

Are you sure that's better than a fallback to ZTIMER_USEC?

No, quite the opposite: I think falling back to ZTIMER_USEC is indeed better before falling back to the busy loop.

But I'd rather not have this PR being deadlocked. Especially since the DHT driver could profit from this getting upstream soonish.

Let's discuss this in the next sprint meeting. Maybe some agreement can be reached.

And why no wait_at_least_us()?

That would not be allowed to fall back to ZTIMER_MSEC. But now that there is a CPU loop to fall back to, it would be indeed more than an alias for ztimer_sleep(ZTIMER_USEC, ...). I'll add this again.

riot-ci · 2023-06-09T18:58:57Z

Murdock results

❌ FAILED

225c5dd examples/blinky: make use of sys/wait

Success	Failures	Total	Runtime
5551	0	6947	08m:18s

Artifacts

benpicco · 2023-06-12T09:33:19Z

I still don't see any argument against using ZTIMER_USEC, where was that brought up?
Maybe we should just add the spinning fallback to xtimer_{u,m}sleep() and use those through the compat API wrapper where we retcon xtimer to mean any timer.

kaspar030 · 2023-06-12T10:07:32Z

sys/include/ztimer.h

+ * @param[in]       scale       Each tick in @p time corresponds to @p scale
+ *                              ticks of @p clock
+ */
+void ztimer_sleep_scale_up(ztimer_clock_t *clock, uint32_t time, uint32_t scale);


this is duplicated

kaspar030 · 2023-06-12T10:08:23Z

@maribu Maybe splilt out the more accurate arch specific spinning?

The function is useful for other modules as well. It is however marked as internal, so that application developers do not expect it to be a stable, public API.

maribu · 2023-06-12T10:29:59Z

@maribu Maybe splilt out the more accurate arch specific spinning?

No need anymore to put that on a fast track. The DHT driver rewrite uses a different approach now that no longer requires ztimer_spin() anyway and also works on AVR as well.

And since there is no need to rush anyway, I'll re-add falling back to a different ztimer backend.

This adds to sets of delay functions. The first kind consists of - `wait_us()`: delay execution in units of microseconds - `wait_ms()`: delay execution in units of milliseconds - `wait_s()`: delay execution in units of seconds These will use `ztimer` as backend, if used anyways. Otherwise a trivial CPU counting loop is used. The second kind consists of - `wait_us_spinning()`: busy-wait for a given number of microseconds The benefit of this compared to `ztimer_spin()` is that platform specific implementation can be used to increase accuracy. This can allow timing critical drivers (e.g. bit-banging with accuracy requirements with in the order of a few microseconds) to scale down to slow MCUs.

benpicco · 2023-06-12T10:52:13Z

cpu/arm7_common/include/wait_arch.h

+#ifdef __cplusplus
+extern "C" {
+#endif
+#ifndef DOXYGEN
+
+/* ARM7 uses ztimer_spin for busy wait */
+
+#endif
+
+#ifdef __cplusplus
+}
+#endif


You don't have to provide this dummy file if you use

#if __has_include("wait_arch.h") #include "wait_arch.h" #endif

benpicco · 2023-06-12T10:55:28Z

sys/include/wait.h

+ *          a two instruction delay loop indeed takes only one cycle per
+ *          iteration once the dynamic branch prediction converges.
+ */
+#define CPU_CYCLES_PER_WAIT_LOOP_ITERATION 1


this isn't used anywhere

benpicco · 2023-06-14T10:58:13Z

sys/include/wait.h

+    if (IS_USED(MODULE_ZTIMER_USEC)) {
+        ztimer_sleep(ZTIMER_USEC, usecs);
+    }


How about

Suggested change

if (IS_USED(MODULE_ZTIMER_USEC)) {

ztimer_sleep(ZTIMER_USEC, usecs);

}

if (IS_USED(MODULE_ZTIMER_USEC)) {

ztimer_sleep(ZTIMER_USEC, usecs);

} else if (IS_USED(MODULE_ZTIMER_MSEC)) {

ztimer_sleep(ZTIMER_MSEC, DIV_ROUND_UP(usecs, US_PER_MS);

}

benpicco · 2023-06-14T10:58:18Z

sys/include/wait.h

+    if (IS_USED(MODULE_ZTIMER_USEC)) {
+        ztimer_sleep(ZTIMER_USEC, usecs);
+    }


How about

Suggested change

if (IS_USED(MODULE_ZTIMER_USEC)) {

ztimer_sleep(ZTIMER_USEC, usecs);

}

if (IS_USED(MODULE_ZTIMER_USEC)) {

ztimer_sleep(ZTIMER_USEC, usecs);

} else if (IS_USED(MODULE_ZTIMER_MSEC)) {

ztimer_sleep(ZTIMER_MSEC, DIV_ROUND_UP(usecs, US_PER_MS);

}

kaspar030 · 2023-06-15T11:19:28Z

I still don't see any argument against using ZTIMER_USEC, where was that brought up?
Maybe we should just add the spinning fallback to xtimer_{u,m}sleep() and use those through the compat API wrapper where we retcon _x_timer to mean any timer.

So my main gripe with this PR is the vagueness of the API.

Our "delay the current process for a while" API is ztimer. It allows arbitrary backends to be chosen. There are a couple of backends that the build system tries hard to find an implementation for (usec, msec, sec). There are multiple options for each backend (msec -> rtt, msec->usec, ...). ztimer is quantized on the clock ticks, and provides at least semantics with +0-2 guarantees. It's unsuitable for very short sleeps (<5us - <100us, depending on hardware) due to being interrupt based (meaning a context switch is always involved).

So ztimer provides the functionality that this API wraps.
For "wait at least N ms (N >2)", just use ztimer_msec() and be done. If the application is interested in low power, the user will have configured ztimer_msec to use a low-power clock.
For "wait at least N us (with N > 5), just use ztimer_usec and be done with it. Won't be low power.

"but now there's a ztimer dependency". Well. Let's make sure to get one ztimer clock that suits most needs (like, ztimer_msec), and use that as much as possible. Chances are, any sufficiently large application will use ztimer_msec somewhere. Any sufficiently minimal application won't feel the ram/rom cost.

So maybe typing ztimer_sleep(ZTIMER_MSEC, N) gets old, so having delay/wait/sleep_ms(N) *mapping to ztimer_sleep(ZTIMER_MSEC, N) does make some sense.

Why not chose any available backend? Because the call site suddenly doesn't clearly state what the function call is doing. We're basically introducing ztimer_sleep_or_maybe_spin_maybe_lowpower_with_maybe_+0..2_us_or_maybe_+0..2_ms_accuracy().
The API has uint32_t value range, so a call to wait_ms() might spin for 49 days. Or just set the thread to sleep and by extension the whole MCU. Depending on the dependency tree.

What way of "waiting" is actually used is already quite muddy when using ZTIMER_M/USEC, as there's already complex heuristics and configuration at play. This layer of abstraction adds even more muddyness. The tree of used waiting mechanisms (and watering of guarantees) grows to a forest.

For the use case most stated ("driver needs to wait a bit for the device to be ready, a bit longer doesn't matter"), if spinning is acceptable, that must be assumed to actually be used in any case, because if spinning would not be acceptable, use of wait_ms() in its current form is a bug. Because depending on the dependencies, spinning might happen. So we must be able to replace all ẁait_ms() with spin_ms() without breakage (or unacceptable behavior), because that happens implicitly if spin_ms() is one of the fallback methods. But if spin_ms() is "acceptable behavior", why not just use that? -> The doxygen states "if the application developer enabled a low power clock anyway, this can be taken as a hint that low power sleeps are preferable". That is already being dealt with at ztimer_msec level.
The same argumentation goes with low power or accuracy.
the worst incarnation of all possible outcomes (of what wait_ms...() actually calls) must be acceptable everywhere that function is used, and must be tested. (This is a high level version of "possible UB makes a case impossible so let's drop this code").

Is ztimer too fat still (implementation wise)? Is that a sentiment, or an actual issue? Because RAM/ROM use would be the only reason for not just using ztimer sleeps.

My suggestion here is:

provide delay_ms/us(). Map directly to the corresponding ztimer clock. Document that it quantizes (delay_ms/us(N) starts in the middle of a clock tick) and is generally +0..2 ticks.
provide "spin_us(uint8_t us)" for bit-banging needs (using this PR's per-CPU implementations)
replace the ztimer sleeps system wide with the corresponding delay calls
create a nice overview page that answers the question on which sleep function to use with delay_ms(N).
overall, simplify the amount of user-facing APIs in place, and the amount of dependency tree dependent behaviors
accept doing USEMODULE += delay_ms() even for simple drivers that need it
accept doing spin_us() in simple driver's boot up one time initialization

Otherwise, prove that using ztimer direct wrapping (with no fallbacks) is causing too many RAM/ROM issues in places where the delay_ms() couldn't be replaced with spin_us() without causing otherwise unacceptable behavior.

kaspar030 · 2023-06-16T08:50:09Z

Because RAM/ROM use would be the only reason for not just using ztimer sleeps.

We had some productive discussion at today's Friday meeting. Karl brought up two more aspects:

while ztimer has "at least" semantics, it actually also advertizes "+-1 tick systemic inaccuracy". Meaning, to get actual "at least", the user needs to add one to the desired period.
also, the "low resource AVR" use case was discussed, where the MCU might not have the means to pull in any ztimer
This could be mitigated by a simplified alternative spinning ztimer implementation:

enum {
  ZTIMER_USEC,
  ZTIMER_MSEC,
  ZTIMER_SEC
};

ztimer_sleep(clock, ticks) {
  switch clock {
     ZTIMER_USEC: {
       spin_us(ticks)}
       break;
       ZTIMER_MSEC: {
         for (unsigned i = 0; i < 1000, i++) {
           ztimer_sleep(ZTIMER_USEC, ticks);
           }
           break;
    ...
    }
}

(or sth smarter)

kfessel · 2023-06-16T09:35:10Z

A spinning implementation in ztimer_sleep isn't helpfull for the read ablity of ztimer_sleep, which should just wrap a ztimer_set and required data (a ztimer_t and a mutex) to make it work (we should a void having any alternative there)

that is the reason to put it somewhere else, keep ztimer_sleep simple

not having an implicit spin is one of the big advancements of ztimer over xtimer (the other one is not having a absolute time interface both these features keep ztimer simple and able to perform ist power saving)

kfessel · 2023-06-16T09:37:44Z

sys/include/wait.h

+static inline void wait_at_least_ms(uint16_t msecs)
+{
+    if (IS_USED(MODULE_ZTIMER_MSEC)) {
+        ztimer_sleep(ZTIMER_MSEC, msecs);


need to add 1 for at least semantic

Why? There is no division here.

kfessel · 2023-06-16T09:39:13Z

sys/include/wait.h

+static inline void wait_at_least_us(uint16_t usecs)
+{
+    if (IS_USED(MODULE_ZTIMER_USEC)) {
+        ztimer_sleep(ZTIMER_USEC, usecs);


add 1 for at least semantic

benpicco · 2024-01-15T13:55:10Z

sys/include/wait.h

+    }
+    else {
+        uint32_t iterations = msecs * (uint64_t)coreclk() / MS_PER_SEC;
+        wait_busy_loop(iterations);


Suggested change

wait_busy_loop(iterations);

busy_wait_us((uint32_t)msecs * US_PER_MS);

maribu added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jun 8, 2023

maribu requested review from kaspar030, bergzand, gschorcht, kYc0o and nandojve as code owners June 8, 2023 12:24

This was referenced Jun 8, 2023

sys/ztimer: add convenience functions for sleeping #17330

Closed

drivers/dht: busy wait reimplementation #19718

Merged

maribu force-pushed the delay branch from 6beee52 to c72c7ec Compare June 8, 2023 15:10

kfessel reviewed Jun 8, 2023

View reviewed changes

cpu/avr8_common/include/delay_arch.h Outdated Show resolved Hide resolved

kfessel reviewed Jun 8, 2023

View reviewed changes

sys/include/ztimer.h Outdated Show resolved Hide resolved

hugueslarrive reviewed Jun 9, 2023

View reviewed changes

cpu/avr8_common/include/delay_arch.h Outdated Show resolved Hide resolved

bergzand reviewed Jun 9, 2023

View reviewed changes

maribu requested review from smlng, leandrolanzieri and aabadie as code owners June 9, 2023 08:30

maribu force-pushed the delay branch from 2e4c4bc to 6cb2a1b Compare June 9, 2023 11:41

benpicco reviewed Jun 9, 2023

View reviewed changes

github-actions bot added the Area: examples Area: Example Applications label Jun 9, 2023

maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Jun 9, 2023

maribu changed the title ~~sys/delay: Add new convenience module~~ sys/wait: add module for backend agnostic waits Jun 9, 2023

github-actions bot added the Platform: native Platform: This PR/issue effects the native platform label Jun 9, 2023

kaspar030 reviewed Jun 12, 2023

View reviewed changes

sys/ztimer: make ztimer_sleep_scale_up() available for in-tree users

83bb3c3

The function is useful for other modules as well. It is however marked as internal, so that application developers do not expect it to be a stable, public API.

maribu added 3 commits June 12, 2023 12:36

tests: add test for sys/wait

f959e69

examples/blinky: make use of sys/wait

225c5dd

maribu force-pushed the delay branch from e06c12d to 225c5dd Compare June 12, 2023 10:36

benpicco reviewed Jun 12, 2023

View reviewed changes

benpicco reviewed Jun 14, 2023

View reviewed changes

kfessel reviewed Jun 16, 2023

View reviewed changes

benpicco mentioned this pull request Aug 31, 2023

drivers/mtd_spi_nor: fix init when only ztimer_msec is used #19908

Merged

benpicco reviewed Jan 15, 2024

View reviewed changes

benpicco mentioned this pull request Mar 20, 2024

sys/ztimer/xtimer_compat: provide more fallback options #20494

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys/wait: add module for backend agnostic waits #19719

sys/wait: add module for backend agnostic waits #19719

maribu commented Jun 8, 2023 •

edited

Loading

kaspar030 commented Jun 8, 2023

maribu commented Jun 8, 2023

kaspar030 commented Jun 8, 2023

kfessel Jun 8, 2023

maribu Jun 8, 2023 •

edited

Loading

kfessel Jun 9, 2023

kfessel Jun 8, 2023

bergzand Jun 9, 2023

maribu Jun 9, 2023

kaspar030 commented Jun 9, 2023

maribu commented Jun 9, 2023

benpicco Jun 9, 2023

maribu Jun 9, 2023

riot-ci commented Jun 9, 2023 •

edited

Loading

benpicco commented Jun 12, 2023 •

edited

Loading

kaspar030 Jun 12, 2023

kaspar030 commented Jun 12, 2023

maribu commented Jun 12, 2023

benpicco Jun 12, 2023

benpicco Jun 12, 2023

benpicco Jun 14, 2023

benpicco Jun 14, 2023

kaspar030 commented Jun 15, 2023

kaspar030 commented Jun 16, 2023

kfessel commented Jun 16, 2023 •

edited

Loading

kfessel Jun 16, 2023

benpicco Jan 15, 2024

kfessel Jun 16, 2023

benpicco Jan 15, 2024

	if (IS_USED(MODULE_ZTIMER_MSEC)) {
	if (IS_USED(MODULE_ZTIMER_MSEC) && msec > 10) {

	if (IS_USED(MODULE_ZTIMER_SEC)) {
	if (IS_USED(MODULE_ZTIMER_SEC) && secs > 10) {

	wait_busy_loop(iterations);
	busy_wait_us((uint32_t)msecs * US_PER_MS);

sys/wait: add module for backend agnostic waits #19719

Are you sure you want to change the base?

sys/wait: add module for backend agnostic waits #19719

Conversation

maribu commented Jun 8, 2023 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

kaspar030 commented Jun 8, 2023

maribu commented Jun 8, 2023

kaspar030 commented Jun 8, 2023

Choose a reason for hiding this comment

maribu Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspar030 commented Jun 9, 2023

maribu commented Jun 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riot-ci commented Jun 9, 2023 • edited Loading

Murdock results

Artifacts

benpicco commented Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

kaspar030 commented Jun 12, 2023

maribu commented Jun 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspar030 commented Jun 15, 2023

kaspar030 commented Jun 16, 2023

kfessel commented Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maribu commented Jun 8, 2023 •

edited

Loading

maribu Jun 8, 2023 •

edited

Loading

riot-ci commented Jun 9, 2023 •

edited

Loading

benpicco commented Jun 12, 2023 •

edited

Loading

kfessel commented Jun 16, 2023 •

edited

Loading