Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers/rtt64: add 64 bit RTT extension for timekeeping #18120

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

benpicco
Copy link
Contributor

Contribution description

A common format for accurate time synchronization across the network is a 64 bit timestamp where the upper 48 bits count the seconds since the Unix epoch and the lower 16 bits are used for the fractions of the current second, yielding 1/64k accuracy.

This format can be evasively mapped to an RTT by shifting the value to match the RTT width and overflow accounting.

For this it is better suited than rtt_rtc since it does not rely on conversions to seconds (and often you don't want to have the RTC functionality at all and are just interested in a counter).

Testing procedure

For easier interfacing with the driver I added a periph_rtc emulation on top. This allows it to be used with the rtc shell command for some interactive testing.

Issues/PRs references

@kaspar030
Copy link
Contributor

Huh, why not implement this using ztimer64 on top of rtt?

@benpicco
Copy link
Contributor Author

benpicco commented May 19, 2022

Isn't the ZTimer RTT backed limited to 1ms accuracy since it only provides ms as a unit? And it requires a unit conversion each time the time is read / updated.

See #18121 for how this can be used.

@maribu
Copy link
Member

maribu commented May 19, 2022

Isn't the ZTimer RTT backed limited to 1ms accuracy

Not really, the auto init stuff just uses it to provide ZTIMER_MSEC clock. You could just as well provide a ZTIMER_TIMESYNC clock that has a different granularity of time than millisecond.

@kfessel
Copy link
Contributor

kfessel commented May 20, 2022

ztimer64 is very capable to do what this does, but this is so much on point providing a absolute measure of time in a binary partition of seconds (there are less surprises for a user and this matches many hardware RTT/C very well).

  • 48Bits of Seconds are a very long time so this can stay within unix epoch and doesn't need riot epoch (if this would be signed this could reach our whole current period , while 32Bit signed are not enough to reach + and -100years.
  • While e.g.:ZTIMER64_1_32KI_SEC (1/(32*1024) of a second) is createable not many people would expect that, this is just so much on that point.
  • Shifts instead of division might yield performance and avoid possible loss of accuracy.
  • I think this also good to use for a RTC (considering ztimer64 _SEC (low_res) _MSEC (still not) _USEC (more power and likely to be drifty (temperature compensated RC)).
  • MSEC and SEC and THIS are very likely to be driven by a low power clock-crystal, while USEC is either (TC)RC or a power hungry crystal.

One thing i am missing (that ztimer64 does), is this working with ZTIMER_MSEC sharing the same low power Timer (RTT).

I like this.

@fjmolinas
Copy link
Contributor

One thing i am missing (that ztimer64 does), is this working with ZTIMER_MSEC sharing the same low power Timer (RTT).

Just because of this I think I would rather want to see the alternative of using both.

  • if no one uses RTC then use it
  • if no one uses RTC then use it
  • if ztimer uses RTC or RTT, then use ztimer64

You could then also allow for external RTC to be used e.g.DS3231. The only thing I would find a pity is getting all this nice functionality to have a system time, and then having to choose between that and using RTT for ztimer (e.g. for the nrf52.

@kaspar030
Copy link
Contributor

kaspar030 commented May 20, 2022

A common format for accurate time synchronization across the network is a 64 bit timestamp where the upper 48 bits count the seconds since the Unix epoch and the lower 16 bits are used for the fractions of the current second, yielding 1/64k accuracy.

This just means, "2**16 Hz ticks since Unix epoch", right?

(assuming we add the offset API to ztimer64):

  • which RTT can actually do 64khz?

  • there'll be users of the single rtt for msec stuff (like, ztimer_msec), and using it here makes it unavailable there

  • ztimer can actually abstract a single 32khz rtt (very common) and use it as base for ztimer_msec, ztimer_rtt32khz and ztimer_rtt64hz, the latter done by shifting

  • ztimer can also use a 64khz rtt and use it for ztimer_msec and ztimer_rtt32khz (and ztimer64_64khz) at the same time

  • this can then be used to provide ztimer64_64khz (the timestampin clock this PR wants to introduce)

  • the rtt can still be used for other things. edit through the ztimer_clocks to e.g., set timers edit

all of the extension from any value to 64bit, overflow handling, is already implemented in ztimer(64). and, ztimer allows then to actually set timers on the clock.

ztimer64 is very capable to do what this does

exactly. why another timer api?

From my side, NACK.

@maribu
Copy link
Member

maribu commented May 20, 2022

I also want to point out that hardware abstraction APIs have to be implemented for every peripheral. While I do agree that some design decisions (e.g. a separate timer API for low power and high frequency timers) haven't aged well, I'd rather not have competing APIs for the same hardware on the long term.

I would however support evolving the timer abstraction APIs to something that matches the hardware better and exposes more capabilities.

@benpicco
Copy link
Contributor Author

  • which RTT can actually do 64khz?

I didn't come up with this format - it's what's used by RFC 3971 (and probably some others too). Most RTTs do 32kHz but that extra bit doesn't hurt and the 48:16 split feels most natural.

  • there'll be users of the single rtt for msec stuff (like, ztimer_msec), and using it here makes it unavailable there

That's already the case with existing users of RTT/RTC. ztimer_msec is incompatible with Deep Sleep operation in it's current form.

The underlying problem is that with the current RTT/timer API there can only ever be a single implementation of each (and only a single instance of RTT) even though many MCUs provide multiple low-power timers.

  • ztimer can actually abstract a single 32khz rtt (very common) and use it as base for ztimer_msec, ztimer_rtt32khz and ztimer_rtt64hz, the latter done by shifting

ztimer_rtt64hz sounds like something that would work as well.

I just wanted to try out time-sync via router advertisements and use a better replacement for rtt_rtc while I'm at it, as this was not designed with sub-second accuracy in mind but instead should run on any RTT with the least common feature set (no overflow detection, no set).
I'm not sure what kind of overhead / conversion losses would be involved by using ztimer, but if it can make use of real timer ticks (instead of ms) that could also be a way forward.

@benpicco benpicco changed the title drivers/rtt_long: add 64 bit RTT extension for timekeeping drivers/rtt64: add 64 bit RTT extension for timekeeping May 30, 2022
@github-actions github-actions bot added the Area: pkg Area: External package ports label May 31, 2022
@kaspar030
Copy link
Contributor

I'm not sure what kind of overhead / conversion losses would be involved by using ztimer, but if it can make use of real timer ticks (instead of ms) that could also be a way forward.

Of course it can! In ztimer, there's what ztimer can do (works with arbitrary ticks, extension to 32bit (and then 64bit), conversion by constant fraction, multiplication/division or shifting, multiplexing), and what the pre-defined clocks do (ZTIMER_USEC, ZTIMER_MSEC, ZTIMER_SEC with their automatically configured conversion steps).

The one thing that can't be disabled (currently) is the extension, as in, if a backend clock is not 32bit, ztimer will extend it if told, or break if the clock is not extended. (ztimer64 always extends, and expects a ztimer32 as underlying clock).

The predefined clocks are there to have some common ground that applications can be written against, they're pre-configured clocks that (might) make some sense. They might come as a package of configuration, availability and guarantees ("ztimer_msec uses a low power timer if the hardware supports it", "ztimer_usec will block low power"), but those are just convention & configuration. We can define any timer in addition to those, and even share backends.

There's no reason for not configuring a ztimer with any HZ that a backend spits out.

As for conversion overhead, if no conversion is done, it's probably hard to notice.

I totally do see the need and use-case for 48/16 bit timestamps, but if ztimer can't provide those, IMO, ztimer needs to be fixed (or if unfixable, replaced). Adding extension, multiplexing, conversion, periodic timers, epochs, 64bit support to periph_timer, periph_rtt, ... would duplicate a lot of efforts. And lead to a multitude of APIs, with different usage, feature set, ...

@maribu
Copy link
Member

maribu commented Jun 7, 2022

I was incorrectly assuming that rtt64 would be a peripheral API to be implemented for each driver (I haven't looked at the source yet). Out of band it was explained to me that this instead implemented only once on top of the peripheral RTT API. I am no longer opposed to this addition.

@kaspar030
Copy link
Contributor

kaspar030 commented Jun 7, 2022

  1. I like the way time is represented as "seconds" and "us", with "us" seamlessly only showing the accuracy that the rtt provides.
  2. This quite a bit simpler than ztimer even when used in it's simplest form (no stacking/conversion).
  3. I think the extension is racey. IIUC, it depends on the overflow callback being called in time. That might not happen, e.g., when calling rtt64_get_counter() with interrupts disabled, or from ISR. In that case, the rtt would overflow, but that overflow would be missed.

What's it gonna take to more seriously consider ztimer for this? With #18122, we could even trivially just implement this API on top of ztimer.

drivers/rtt64/rtt64.c Outdated Show resolved Hide resolved
@kaspar030
Copy link
Contributor

kaspar030 commented Jun 7, 2022

That's already the case with existing users of RTT/RTC. ztimer_msec is incompatible with Deep Sleep operation in it's current form.

this I don't understand. ztimer_msec, when configured to use the RTT and the rtt allows running in deep sleep, will allow deep sleep, no? Or do you mean "deep sleep" -> "no ram retention"?

But, rtt64 will basically make the rtt unavailable for ztimer_msec, and thus prevent low-power sleep for the whole system, if ztimer_msec now needs to use a non-lower timer.

@benpicco
Copy link
Contributor Author

benpicco commented Jun 7, 2022

Deep Sleep is without RAM retention, yes.
The issue with ztimer_msec is that we lack an API that lets us expose multiple low-power timers, there can only be a single RTT and a single type of timer.

My plan for ztimer_msec currently is to just configure a periph_timer to be clocked by the 32kHz clock and see how that goes. Another option would be to add rtt64 as a ztimer backend (but that will cause trouble when the time is synced with an external reference, we might need something like #17416 here)

@kaspar030
Copy link
Contributor

The issue with ztimer_msec is that we lack an API that lets us expose multiple low-power timers, there can only be a single RTT and a single type of timer.

What's your goal? Is the issue here that if ztimer is using the rtt, the rtt cannot be used anymore for wakeup from deep sleep?

@benpicco
Copy link
Contributor Author

benpicco commented Jun 7, 2022

Yes or rather it becomes inherently racy to use the RTT alarm to wake from Deep Sleep as some random driver might set it's own RTT alarm inbetween.

@kaspar030
Copy link
Contributor

Yes or rather it becomes inherently racy to use the RTT alarm to wake from Deep Sleep as some random driver might set it's own RTT alarm inbetween.

you mean the random driver might set an alarm on the RTT through ztimer_msec?

The reason why ztimer doesn't play nicely with Deep Sleep is that a Deep Sleep is essentially a reboot from the system perspective that just happens at a time that makes sense to whatever set that alarm. And keeping state between reboots is something that we don't have a nice API for (yet).

But, there's no reason for e.g., the ztimer_periph_rtt backend to not expose hooks and API to allow setting an alarm (if no other ztimer alarms are queued), and lowering the pm layer blocks.

@benpicco
Copy link
Contributor Author

benpicco commented Jun 7, 2022

The thing is that often you don't need to keep state at all, so reboot after Deep Sleep is the right thing to do.

@kaspar030
Copy link
Contributor

I think I'm realizing what you're trying to accomplish. You'd like to be able to set "wake me up in N days" and deep sleep in the meantime. ztimer is "racey" in the sense that currently there's no way to prevent ztimer from overriding an rtt alarm.
And you'd like to have some extension inside RTT to be able to track continuous time through those deep sleep phases that extends the MAX_VALUE of a given RTT.

@kaspar030
Copy link
Contributor

Why didn't you say so ... :)

How about this:

  1. with sys/ztimer64: initial epoch / time reference support #18122, ztimer64 can make use of an epoch.
  2. we add an API do expose forced deep sleep. (e.g., pm_deep_sleep_for(seconds)).
  3. we add hooks to that API that other parts of the system can register. cb_mux comes to mind, or something that doesn't use utlist
  4. we make ztimer64 store it's offset in backup ram (or whereever it survives deep sleep), and restore on boot
  5. we rig up the ztimer_periph_rtt backend with the deep sleep API so when deep sleep is initiated, it doesn't allow setting any new timers anymore (via some flag), and just sets the desired sleep time. (this maybe needs handling if the required sleep time is longer than the rtt can handle). another deep sleep hook would work here.

This might sound complicated, but I doubt it would end up being more code than rtt64 is. The upsides would be that the method for storing state is naturally abstracted (just use different ztimer64 hooks), that the ztimer extension mechanism can be re-used (it is (disabled)ISR safe and can be and is tested as such), and that we end up with one ztimer64 rtt that's 64bit extended, allows being used with an epoch, naturally shares the usually one RTT with the rest of the system, or can be used with a low-power periph_timer, ...

@benpicco benpicco requested a review from gschorcht June 12, 2022 16:05
@benpicco
Copy link
Contributor Author

My take on this is that it's a cleaner alternative to rtt_rtc that provides sub-second time-stamping.

rtt_rtc stared as a compatibility layer for MCUs that lack a hardware RTC to still run software that makes use of the periph_rtc API.
It was then extended to provide sub-second timestamping, but was never designed for that, which just turned into a mess.

This is not supposed to be an alternative to ztimer.

@benpicco benpicco requested a review from kfessel June 24, 2022 16:39
Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @benpicco:

i think rtt64_t (aka uint64_t) should be the programmer facing interface typ of this. accompanied by a set of static inline conversion functions that look like this

static inline uint64_t rtt64_sec(uint64_t sec);
static inline uint64_t rtt64_usec(uint64_t usec);

static inline uint32_t usec_rtt64(uint64_t rtt64);
static inline uint64_t sec_rtt64(uint64_t rtt64);

rtt64_get_time() should just return a uint64_t
rtt64_set_alarm(uint64_t rtt64, cb , arg) should just take uint64_t

rtt64_set(uint64_t rtt64) should just take uint64_t

emphasize and embrace the rtt64_t aka uint64_t

@benpicco
Copy link
Contributor Author

What are

static inline uint64_t rtt64_sec(uint64_t sec);
static inline uint64_t rtt64_usec(uint32_t usec);

supposed to do?

@benpicco
Copy link
Contributor Author

I'm fine with exporting the helper functions, but I would like to keep the convenience functions (and the periph_rtt aligned naming) in place.

Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some ideas for that rtt64 time format or rt64 (realtime 64) or wt64 (walltime 64)

typedef uint64_t rtt64_t;

/**
* @brief Get seconds from RTT64 time format
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @brief Get seconds from RTT64 time format
* @brief Get RTT64 time format from seconds

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sec_rtt64 would: Get seconds from RTT64 time format

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we use prefixes for namespacing

}

/**
* @brief Get microseconds from RTT64 time format
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @brief Get microseconds from RTT64 time format
* @brief Get RTT64 time format from microseconds

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usec_rtt64 would: Get microseconds from RTT64 time format

and i should get all the rtt64 in microseconds (the mask may be apllyed by the user like)

usec_rtt64(RTT64SUBSECONDS & rtt64_timestamp)

*
* @return Timestamp in RTT64 format
*/
static inline rtt64_t rtt64_counter(uint64_t secs, uint32_t us)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timex_t timex_rtt64(rtt64_t time)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but timex is 32 bit so we have a problem in 2038

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timex has that problem not rt64 :)

maybe we need a static inline void rt64_to_time(rt64_t timestamp, uint64_t *secs, uint32_t *us)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's wrong with rtt64_sec() and rtt64_usec()?

*
* @return Timestamp in RTT64 format
*/
static inline rtt64_t rtt64_counter(uint64_t secs, uint32_t us)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static inline rtt64_t rtt64_counter(uint64_t secs, uint32_t us)
static inline rtt64_t rtt64_set(uint64_t secs, uint32_t us)

and here i thin the type might need a different name maybe rt64 (realtime 64) or wt64 (walltime 64)

Suggested change
static inline rtt64_t rtt64_counter(uint64_t secs, uint32_t us)
static inline rt64_t rt64_set(uint64_t secs, uint32_t us)

or

Suggested change
static inline rtt64_t rtt64_counter(uint64_t secs, uint32_t us)
static inline rtt64_t rtt64_time(uint64_t secs, uint32_t us)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you have an aversion against the counter?
One tick is 1/65536 seconds, so it's not a 'time' and it this function does not set anything, it only converts units as you requested.

*
* @param now timestamp in the 48:16 format
*/
void rtt64_set_counter(rtt64_t now);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i do not like the word counter (if it is a clock)

Suggested change
void rtt64_set_counter(rtt64_t now);
void rtt64_set_clock(rtt64_t now);

or better

Suggested change
void rtt64_set_counter(rtt64_t now);
void rtt64_set(rtt64_t now);

Copy link
Contributor Author

@benpicco benpicco Jul 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it's already used by the RTT API, I think we should keep consistency in naming here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that one is about the hardware (which has an oscillator and a counter an some other stuff) which you may configure to count other things than time
But this is a clock it counts time.

Copy link
Contributor Author

@benpicco benpicco Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use counter to make the distinction between ticks and milliseconds clear.
Is rtt64_set() in seconds? Milliseconds? Ticks?

We already use rtt_sec_counter() so it's just consistent.
And unless you use the periph_rtc emulation on top there is nothing that says those ticks have to mean a wall time.

*
* @return uint64_t current time in the 48:16 format
*/
rtt64_t rtt64_get_counter(void);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
rtt64_t rtt64_get_counter(void);
rtt64_t rtt64_get(void);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Area: pkg Area: External package ports Area: sys Area: System
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants