API to correlate system time with external time sources #28977

pabigot · 2020-10-06T21:08:58Z

This PR has content I've been working on for about six months, to locally timestamp sensor observations with standard time rather than time-since boot.

It provides data structures to capture a timestamp in two different clocks, monitor the drift between those clocks, and using a base instant with estimated drift convert between the clocks. This provides the core technology to convert between system uptime and an external continuous time scale like TAI (UTC without applying leap seconds).

A sample is provided that demonstrates using this technology to measure local clock error between Nordic HFCLK and LFCLK.

There are some questions in the first comment that need to be resolved. There are also some follow-on commits that will be submitted separately after this seems to be going forward:

support for Bluetooth Current Time Service as a time source (working, needs cleanup and documentation)
conversion of the similar functionality in the Maxim DS3231 driver to use the new API

pabigot · 2020-10-07T18:18:37Z

Some notes on the design, with a few questions that should be answered before it's ready to merge:

~~confirm name for timeutil_realtime_* functions~~ that isn't here anymore.

What is this?

There's already a timeutil_ namespace for civil-time operations. This PR extends it with data structures and functions that allow skewed translation between time scales. This is the timeutil_sync_* API.

It then adds API under CONFIG_TIMEUTIL_REALTIME that supports system-wide translation from the current local tick counter to the corresponding civil time, including accounting for local clock skew (which has significant impact on accuracy).

The API does not automatically do time maintenance. Something has to invoke timeutil_realtime_set() at least once, and preferably periodically, as well as using the timeutil_sync_* capabilities to calculate and the skew that adjusts for local clock error.

Why isn't this `clock_gettime()`, `gettimeofday()`, or `k_realtime_get()`?

The first two belong to libc, and attempting to override them is mis-use. It's possible it could be done for newlib using internal hooks, but that wouldn't help for toolchains that don't use newlib. It seems better to provide an external solution.

It's not k_realtime_*() mostly because it's entirely library code. I'm open to renaming that part of it.

Is `timeutil_realtime_*` userspace-safe?

No. It requires a spinlock to protect the synchronization state from simultaneous access. Spin locks can't be used from unprivileged contexts as locking disables interrupts on the running processor. (It's also unclear how much userspace should be given rights to manipulate kernel-supported concepts of local/external time synchronization.)

It could be reasonable to add a userspace wrapper to read the current time and the synchronization state. That's an enhancement that shouldn't block merge of the core capability.

The time_sync_* library code should be usable from userspace.

Any other concerns?

This use floating point to do the time translation, because calculating skews in integer-only math isn't feasible due to the magnitude of the values and scale factors. It does this math under lock to prevent simultaneous update of the synchronization state. IIRC FP in the kernel was once an issue for Linux. Clearly it works fine in supervisor-only Zephyr. It may be that something about userspace might introduce a problem (e.g. failing to restore FPU state).

henrikbrixandersen

Just reviewed the documentation for now. I will try to find the time to review the rest within the next week.

henrikbrixandersen · 2020-10-25T17:34:47Z

doc/reference/misc/timeutil.rst

+offsets separately.
+
+The inverse transformation is not standardized: APIs like ``mktime()``
+expect information about time zones.  Zephyr module provides this


Which zephyr module?

s/module//. It's from lib/os, which doesn't have a module name.

I'll clean this up in the next push.

henrikbrixandersen · 2020-10-25T17:39:26Z

doc/reference/misc/timeutil.rst

+======================
+
+The :option:`CONFIG_TIMEUTIL_REALTIME` flag enables API that allows
+zephyr to provide the current civil ("real") time that advances aligned


Suggested change

zephyr to provide the current civil ("real") time that advances aligned

zephyr to provide the current civil ("wall clock") time that advances aligned

Made it both, since absent conflicting consensus "real" is the name currently used for this timescale.

doc/reference/misc/timeutil.rst

henrikbrixandersen · 2020-10-25T17:51:34Z

lib/os/Kconfig

+config TIMEUTIL_REALTIME
+	bool "Support conversion between local and civil time"
+	help
+	  When true API is provided that enables applications to read
+	  the current time based on maintained offsets and skews
+	  between the local tick clock and an external time source,
+	  possibly an RTC.
+


What is the rationale behind the CONFIG_TIMEUTIL_REALTIME naming? "Real time" may be confusing here (Zephyr is a Real-Time Operating System).

Have you considered CONFIG_TIMEUTIL_CIVIL_TIME? Same goes for the naming of the API functions.

Yes, but the consensus interpretation of "civil time" is the local time, which is almost always UTC plus an offset (that generally changes twice a year).

I'm not entirely bound to that decision. Options I've considered include:

civil time (implies a local time zone time scale)

calendar time (implies a representation, not a time scale)

wall clock

timescale

real time

none of which are wonderful. I went with REALTIME because it's what CLOCK_REALTIME on a POSIX system gives you, which is UTC, and I think that the most likely use of these functions will be to produce something that approximates or provides a capability similar to CLOCK_REALTIME. So: timeutil_realtime_.

If "realtime" is too confusing because Zephyr is a "real-time operating system" (it's not), then we can come up with a consensus alternative.

I think it is confusing because realtime term is subjective, from the designation alone, everyone could understand something different about what for example timeutil_realtime_get() does. Maybe CONFIG_TIMEUTIL_CTS/timeutil_cts_get(), for civil time scale (144c887#diff-ca1507f5fcb1d2e792a8c80442ab64038f0e5d9939a38e6015b1f1b5d28a8431R34) ?

CTS in Bluetooth is "Current Time Service", which is one of the potential sources for "real" time data. So I think that spelling would be confusing.

It is less confusing than REALTIME, at least for me :-)

henrikbrixandersen · 2020-11-09T17:46:55Z

@pabigot Would it be feasible to drop the float skew (I am a little worried about introducing floating point requirements in the kernel code) with a PPB representation? Like turning it upside down and do all internal calculations based on an integer PPB value? Do we loose too much precision?

pabigot · 2020-11-09T17:55:54Z

Would it be feasible to drop the float skew?

I tried that. The dynamic range due to the need to translate between different frequencies causes problems even with 64-bit integer math. There are overflows and underflows during the intermediate calculation that makes the skew estimate horribly unstable even when the clocks are stable.

This ties to #29569 in that while this is proposed to be in lib/os it's unclear whether it's "kernel" code. It maybe used in but doesn't require supervisor privileges. I'm thinking we need a "support library" category (#29876 also falls into that category).

doc/reference/misc/timeutil.rst

Describe the role of these APIs, key concepts that they depend on, and expose the low-level API. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>

Provide a demonstration of using the timeutil skew infrastructure to measure the relative error of the two clock sources on Nordic boards. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>

jukkar

LGTM

github-actions bot added area: API Changes to public APIs area: Samples Samples area: Shell Shell subsystem area: Tests Issues related to a particular existing or missing test labels Oct 6, 2020

pabigot force-pushed the pabigot/20201005b branch 2 times, most recently from cf4b874 to 58a5323 Compare October 7, 2020 17:01

github-actions bot added the area: Documentation label Oct 7, 2020

pabigot changed the title ~~WIP: API to correlate system time with external time sources~~ API to correlate system time with external time sources and translate uptime to wall-clock time Oct 7, 2020

pabigot added this to Triage in Architecture Project via automation Oct 7, 2020

pabigot force-pushed the pabigot/20201005b branch from 58a5323 to b86fa43 Compare October 18, 2020 15:23

pabigot marked this pull request as ready for review October 18, 2020 18:28

pabigot requested review from andrewboie, andyross, carlescufi, galak, jakub-uC, MaureenHelm, nashif and nordic-krch as code owners October 18, 2020 18:28

pabigot requested a review from henrikbrixandersen October 19, 2020 14:55

henrikbrixandersen reviewed Oct 25, 2020

View reviewed changes

henrikbrixandersen self-requested a review October 27, 2020 18:21

pabigot force-pushed the pabigot/20201005b branch 2 times, most recently from 5939917 to d655c60 Compare November 3, 2020 14:41

pabigot mentioned this pull request Dec 2, 2020

clock_control: Allow reconfiguration clock [after exiting low-power mode] #28572

Open

pabigot mentioned this pull request Dec 14, 2020

kernel idling / timer API, idle parameter limitations #30590

Open

pabigot added this to the v2.5.0 milestone Dec 26, 2020

pabigot removed area: Devicetree area: Kconfig area: Kernel area: Samples Samples area: Shell Shell subsystem area: X86 x86 Architecture (32-bit) manifest manifest-hal_stm32 manifest-mcumgr manifest-openthread platform: NXP NXP platform: nRF Nordic nRFx west labels Jan 19, 2021

pabigot force-pushed the pabigot/20201005b branch 2 times, most recently from d7dddb4 to 88a6a46 Compare January 19, 2021 14:23

jukkar reviewed Jan 20, 2021

View reviewed changes

doc/reference/misc/timeutil.rst Outdated Show resolved Hide resolved

pabigot added 2 commits January 20, 2021 05:53

doc: add a section for timeutil APIs

bc8cbe6

Describe the role of these APIs, key concepts that they depend on, and expose the low-level API. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>

samples: boards: nrf: add clock_skew

dc60797

Provide a demonstration of using the timeutil skew infrastructure to measure the relative error of the two clock sources on Nordic boards. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>

pabigot force-pushed the pabigot/20201005b branch from 88a6a46 to dc60797 Compare January 20, 2021 11:54

github-actions bot added the area: Samples Samples label Jan 20, 2021

jukkar approved these changes Jan 20, 2021

View reviewed changes

nashif approved these changes Jan 20, 2021

View reviewed changes

Architecture Project automation moved this from Triage to In Progress Jan 20, 2021

nashif merged commit 08f4ce4 into zephyrproject-rtos:master Jan 20, 2021

Architecture Project automation moved this from In Progress to Done Jan 20, 2021

Architecture Project automation moved this from Done to In Progress Jan 20, 2021

jfischer-no approved these changes Jan 20, 2021

View reviewed changes

pabigot deleted the pabigot/20201005b branch January 22, 2021 14:20

carlescufi moved this from In Progress to Done in Architecture Project Feb 16, 2021

erwango mentioned this pull request Dec 20, 2021

stm32: counter: making counter date-time friendly #41318

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API to correlate system time with external time sources #28977

API to correlate system time with external time sources #28977

pabigot commented Oct 6, 2020 •

edited

pabigot commented Oct 7, 2020 •

edited

henrikbrixandersen left a comment

henrikbrixandersen Oct 25, 2020

pabigot Oct 25, 2020

pabigot Jan 11, 2021

henrikbrixandersen Oct 25, 2020

pabigot Jan 11, 2021

henrikbrixandersen Oct 25, 2020

pabigot Oct 25, 2020 •

edited

jfischer-no Jan 4, 2021

pabigot Jan 4, 2021

jfischer-no Jan 4, 2021

henrikbrixandersen commented Nov 9, 2020

pabigot commented Nov 9, 2020 •

edited

jukkar left a comment

	zephyr to provide the current civil ("real") time that advances aligned
	zephyr to provide the current civil ("wall clock") time that advances aligned

API to correlate system time with external time sources #28977

API to correlate system time with external time sources #28977

Conversation

pabigot commented Oct 6, 2020 • edited

pabigot commented Oct 7, 2020 • edited

What is this?

Why isn't this clock_gettime(), gettimeofday(), or k_realtime_get()?

Is timeutil_realtime_* userspace-safe?

Any other concerns?

henrikbrixandersen left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pabigot Oct 25, 2020 • edited

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

henrikbrixandersen commented Nov 9, 2020

pabigot commented Nov 9, 2020 • edited

jukkar left a comment

Choose a reason for hiding this comment

pabigot commented Oct 6, 2020 •

edited

pabigot commented Oct 7, 2020 •

edited

Why isn't this `clock_gettime()`, `gettimeofday()`, or `k_realtime_get()`?

Is `timeutil_realtime_*` userspace-safe?

pabigot Oct 25, 2020 •

edited

pabigot commented Nov 9, 2020 •

edited