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

API to correlate system time with external time sources #28977

Merged
merged 3 commits into from Jan 20, 2021

Conversation

@pabigot
Copy link
Collaborator

@pabigot pabigot commented Oct 6, 2020

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
Copy link
Collaborator Author

@pabigot pabigot commented Oct 7, 2020

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).

@pabigot 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 pabigot added this to Triage in API review/cleanup/rework via automation Oct 7, 2020
@pabigot pabigot force-pushed the pabigot:pabigot/20201005b branch from 58a5323 to b86fa43 Oct 18, 2020
@pabigot pabigot marked this pull request as ready for review Oct 18, 2020
@pabigot pabigot requested a review from henrikbrixandersen Oct 19, 2020
Copy link
Member

@henrikbrixandersen henrikbrixandersen left a comment

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

offsets separately.

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

This comment has been minimized.

@henrikbrixandersen

henrikbrixandersen Oct 25, 2020
Member

Which zephyr module?

This comment has been minimized.

@pabigot

pabigot Oct 25, 2020
Author Collaborator

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

I'll clean this up in the next push.

This comment has been minimized.

@pabigot

pabigot Jan 11, 2021
Author Collaborator

done

======================

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

This comment has been minimized.

@henrikbrixandersen

henrikbrixandersen Oct 25, 2020
Member

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

This comment has been minimized.

@pabigot

pabigot Jan 11, 2021
Author Collaborator

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

doc/reference/misc/timeutil.rst Outdated Show resolved Hide resolved
doc/reference/misc/timeutil.rst Outdated Show resolved Hide resolved
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.

Comment on lines 75 to 70

This comment has been minimized.

@henrikbrixandersen

henrikbrixandersen Oct 25, 2020
Member

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.

This comment has been minimized.

@pabigot

pabigot Oct 25, 2020
Author Collaborator

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.

This comment has been minimized.

@jfischer-no

jfischer-no Jan 4, 2021
Collaborator

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) ?

This comment has been minimized.

@pabigot

pabigot Jan 4, 2021
Author Collaborator

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.

This comment has been minimized.

@jfischer-no

jfischer-no Jan 4, 2021
Collaborator

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

@henrikbrixandersen henrikbrixandersen self-requested a review Oct 27, 2020
@pabigot pabigot force-pushed the pabigot:pabigot/20201005b branch 2 times, most recently from 5939917 to d655c60 Nov 3, 2020
@henrikbrixandersen
Copy link
Member

@henrikbrixandersen henrikbrixandersen commented Nov 9, 2020

@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
Copy link
Collaborator Author

@pabigot pabigot commented Nov 9, 2020

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 Outdated Show resolved Hide resolved
pabigot added 2 commits Oct 7, 2020
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>
@pabigot pabigot force-pushed the pabigot:pabigot/20201005b branch from 88a6a46 to dc60797 Jan 20, 2021
@jukkar
jukkar approved these changes Jan 20, 2021
Copy link
Member

@jukkar jukkar left a comment

LGTM

@nashif
nashif approved these changes Jan 20, 2021
API review/cleanup/rework automation moved this from Triage to In Progress Jan 20, 2021
@nashif nashif merged commit 08f4ce4 into zephyrproject-rtos:master Jan 20, 2021
5 checks passed
5 checks passed
@github-actions
Run compliance checks on patch series (PR)
Details
@github-actions
Documentation Build
Details
@github-actions
Pull Request Labeler
Details
@github-actions
Scan code for licenses
Details
buildkite/zephyr Build #18607 passed (38 minutes, 32 seconds)
Details
API review/cleanup/rework automation moved this from In Progress to Done Jan 20, 2021
API review/cleanup/rework automation moved this from Done to In Progress Jan 20, 2021
@pabigot pabigot deleted the pabigot:pabigot/20201005b branch Jan 22, 2021
@carlescufi carlescufi moved this from In Progress to Done in API review/cleanup/rework Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment