Sneak peak: I'm completely aware of ztimer_acquire() and ztimer_release() introducing more complexity. But this interface could be just a foundation for something like this:

typedef 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 ztimer_now() migrate to an interface as shown above.

Or we could introduce a global ztimer_watch_t uptime that is backed by ZTIMER_MSEC and started during auto_init. For time measurement, one may call ztimer_watch_read(ZTIMER_MSEC, &uptime).

Is this really an API change? The use of ztimer_now has a pending PR #17298 documenting previously silent assumptions made; I think that's the real API chan^Wfix here, whereas this PR just makes ztimer use its potential.

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 ztimer_t with a very long offset can also be a solution to make the pm_layered approach work. But it feels more like a workaround than a proper solution. TBH, I'm regretting that I've introduced this approach in the first place. It seemed to be easy and elegant one but I've shot myself into the foot several times :D

You may have noticed: This PR disables all these mechanics, once you pull in ztimer_ondemand (I'm still not sure if this name is verbose enough ... but I wasn't able to come up with a better one, yet.) Once we found a working solution that doesn't require any workarounds, I'd say we should introduce pm_layered into all periph_timer implementation and then deprecate the mechanics around direct interaction between ztimer and pm_layered.

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 ztimer_acquire() is missing, you'll notice pretty fast because the periph_timer is disabled even if the MCU is still on fire and hasn't entered any power-saving modes, yet. I'd say those bugs that show themselves only by accident are really annoying type of bugs. (I.e. you changed timing of your application and, thus, different pm modes are entered at different times and see stuff like networking starting to behave weirdly. This will happen if ztimer_now() is used to time stuff - and it is!)

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 ztimer_now64. That should create enough room that timer_now() never wraps.

Your approach clearly detects wraps: But what shall we do if we detected wrapping timers? Extend like ztimer_now64 does?

Some additional work: I've introduced ztimer_ondemand_strict with 04607ed. It will make sure ztimer_now() is only called if the timer is active. Of course, this pseudomodule brakes almost every module using ztimer. But it already showed me that I used ztimer_now() in my patch before turning on the clock! This has been fixed with ba77e7b.

Furthermore, I wrote a little example: tests/ztimer_ondemand for you to run on your boards :)

Would anybody mind if I squash commits?

I think I have finished for the first round and implemented everything necessary for the ztimer_ondemand driver to work properly.

I wouldn't mind, and I think that kfessel's comment is high-level enough that a squash wouldn't disturb it.

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.

This is running tests/bench_xtimer with USEMODULE=ztimer_xtimer_compat on nrf52840dk, first on master, then this PR (rebased on master):

main(): This is RIOT! (Version: buildtest)
xtimer benchmark application.

                     set() one     6501 / 1000 = 6
                  remove() one      943 / 1000 = 0
          set() + remove() one    10690 / 1000 = 10
  set() many increasing target   112317 / 1000 = 112
               re-set()  first     6564 / 1000 = 6
               re-set() middle   169686 / 1000 = 169
               re-set()   last   331090 / 1000 = 331
       remove() + set()  first    11479 / 1000 = 11
       remove() + set() middle   175118 / 1000 = 175
       remove() + set()   last   336466 / 1000 = 336
      remove() many decreasing    60732 / 1000 = 60
                  xtimer_now()     1189 / 1000 = 1
              sizeof(xtimer_t)    16000 / 1000 = 16
done.
Help: Press s to start test, r to print it is ready
START
main(): This is RIOT! (Version: buildtest)
xtimer benchmark application.

                     set() one     6501 / 1000 = 6
                  remove() one      959 / 1000 = 0
          set() + remove() one    10752 / 1000 = 10
  set() many increasing target   112286 / 1000 = 112
               re-set()  first     6564 / 1000 = 6
               re-set() middle   193808 / 1000 = 193
               re-set()   last   343817 / 1000 = 343
       remove() + set()  first    11417 / 1000 = 11
       remove() + set() middle   175090 / 1000 = 175
       remove() + set()   last   336404 / 1000 = 336
      remove() many decreasing    60732 / 1000 = 60
                  xtimer_now()     1189 / 1000 = 1
              sizeof(xtimer_t)    16000 / 1000 = 16
done.

There seems to be some minor impact on the re-set middle and re-set last. Accteptable I'd say.

edit I didn't enable ztimer_ondemand, so it probably wasn't used. Makes me wonder where the difference comes from. :/

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.

May I squash? (CC: @maribu)

This PR actually brings useful power management to the nrf5x CPU once #18954 and #18953 are merged 🥳

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 ztimer_acquire() / ztimer_release() API - although ztimer_ondemand is still opt-in at least until 2023.01 has been released. No one should be bothered by this PR, yet. Cf. my proposed road map.

Thank you very much for your valuable feedback and the support to push this PR forward! :-)