-
Notifications
You must be signed in to change notification settings - Fork 7.4k
sys: realtime: Add native real-time library and API #72173
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
sys: realtime: Add native real-time library and API #72173
Conversation
e8599ff
to
8dd18c1
Compare
UpdateAdded counter supportUsing a low-power counter / real-time counter as timekeeper is now supported! Added test suite for time conversion functionsThe test suite tests conversions from timestamps all the way back from year -1 to year 2399 |
f54f82b
to
93c64e3
Compare
93c64e3
to
88f71ee
Compare
Note to self, namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok to me. In terms of timestamp, is that seconds since the epoch?
@cfriedt yep :) we could up the resolution to ms as well, we got the bits for it |
include/zephyr/sys/time.h
Outdated
int sys_time_set_timestamp(const int64_t *timestamp); | ||
|
||
/** Get universal coordinated time datetime */ | ||
int sys_time_get_datetime(struct rtc_time *datetime); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, you argued that rtc.h is closely coupled to a specific type of hardware - which I consider essential as otherwise it shouldn't be in the driver subsystem.
Therefore please call this sys_rtc_time...
or any other _rtc_
prefix or infix and place it outside of the generic sys/time.h
(here and elsewhere in this PR where you're not in the direct realm of the RTC driver API but reference drivers/rtc.h
.
The time namespace is so crowded already in Zephyr that generic namespaces like sys_time should not be occupied by specific libraries.
Alternatively use generic POSIX tm and refer to it in the docs. I know that the two concepts are compatible but the conversion should then be done in the implementation IMHO.
In any case the API lacks parameter documentation, it seems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its the other way around really :)
The RTC API itself is closely coupled to time.h, struct rtc_time is a 1-1 match to struct tm. The reason for this choice was to keep compatibility with POSIX, while not being dependent on it. This is the reason for using it over struct tm in the general API, not because of RTC drivers. We could move the struct out of rtc.h, have it in sys/time.h instead to make this clearer, actually, we should so that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, you can keep them separate in both directions and only have a specific implementation refer to both at the same time, but if you intend to implement within a POSIX namespace (or at least a namespace that can easily be confused with <sys/time.h>, then you should steer clear from any imports deep into a single driver subsystem from a generic header file. If this was in sys/rtc_time.h
or ...rtc/time.h
(haven't thought much about the specific locations), it would be much clearer which kind of dependencies you imply here.
lib/time/Kconfig
Outdated
# Copyright (c) 2024 Bjarki Arge Andreasen | ||
# SPDX-License-Identifier: Apache-2.0 | ||
|
||
menuconfig TIME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, the configuration should either be done in the RTC driver realm or refer to generic time concepts independent of a specific hardware type. This API is not general enough to represent general POSIX time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will make an attempt to use this API as the basis for posix time, then we can discuss further. I will probably meet some of the hurdles you foresee :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, then please keep this experimental/unstable API encapsulated in the appropriate subsystem until it keeps the promise of being generic enough to be placed in a generic namespace. You can always move it later one once you make it represent general POSIX time.
include/zephyr/sys/time.h
Outdated
int sys_time_set_datetime(const struct rtc_time *datetime); | ||
|
||
/** Convert universal coordinated time datetime to timestamp */ | ||
int sys_time_datetime_to_timestamp(int64_t *timestamp, const struct rtc_time *datetime); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make it clear that we're dealing with Unix timestamps here, as the timestamp name is also highly fraught in Zepyhr already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will update the comments to specify unix time
lib/time/counter.c
Outdated
int ret; | ||
uint32_t counter_ticks; | ||
uint32_t counter_center_bottom_ticks; | ||
uint32_t counter_center_top_ticks; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: reversed x-mas - here and elsewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will reverse the reversed names :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Full disclosure: There's no official consent about this in the project AFAIK, I just used to get comments in that direction myself and got to like it. But I think no one really has a strong opinion about this (myself definitely not).
lib/time/counter.c
Outdated
|
||
static void add_ticks_to_seconds(int64_t *seconds, const uint64_t *ticks) | ||
{ | ||
*seconds += (int64_t)((*ticks) / frequency); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You seem to be making implicit rounding assumptions here, please document these assumptions and name the function accordingly. Also document your assumptions about overflow (if any), as well as any other concept relevant to counter-to-time-conversion (see the well-defined concepts and nomenclature in #19030 (comment)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converting between ticks and seconds is quite trivial, in physics terms:
frequency = ticks / second
ticks / (ticks / second) = second
One may describe the fact that the ticks will always be rounded down to the nearest second as an assumption, but this is simply the expected behavior of a UTC timestamp, I would hardly describe this as an assumption.
Regarding roll-over, the counter may roll over, the implementation expects it to. When it does, the counter will start from 0, and the total ticks are added to an accumulated ticks since the counter was started. There are no assumptions made here that are not defined by the counter API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm aware of the formula.
Have a look at how the well-conceived timepoint and timeout concepts in the kernel, the Linux kernel clocksource and my own counter-to-time conversion API proposal are all extremely careful to keep the "tick-version" well apart from the "rounded version" (potentially non-deterministic, accruing rounding errors over subsequent conversions, ...). They all go to great lengths in explaining in which situations the different "worlds" actually make sense and can be used legitimately. It's usually better to stick to the tick representation internally and make all conversions "ephemeral" with a very well defined epoch, rounding approach, syntonization requirements, etc. You should not give up the original "wrapped" tick-value "truth" prematurely, but if you have to then the chosen approach should be very well defined.
These things have been discussed many times before in the project, that's why I invited @andyross to this PR and pointed you to the Clock Architecture PR which documents that discussion quite well, I hope.
Some (not all) of these requirements go away as soon as you conceive your API as something specific to a specific driver universe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andyross Would be great if you could give this a quick review wrt tick-to-counter conversion, duplication with other concepts and encapsulation/openness towards other system timepoint representations.
lib/time/counter.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
counter != time-to-counter conversion - I suggest you name this properly as a conversion library (with very specific underlying assumptions) - again to not occupy generic namespaces with specific implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of the source file just needs to be specific enough to separate it from other files in the same folder, the name is not exposed anywhere outside the folder, so it is not occupying any generic namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't agree. We have been there and it has been highly confusing to users. We are speaking about highly visible header files. I vote for code that is not only "formally" correct but also helps users to not be trapped by misleading names.
lib/time/counter.c
Outdated
return sys_time_validate_timestamp(timestamp) ? 0 : -EAGAIN; | ||
} | ||
|
||
int sys_time_set_timestamp(const int64_t *timestamp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The specific kind of counter and conversion assumptions should be decoupled from sys_time. Either make it clear where future decoupling can be done (even if not required now) or name the function more specifically and place it somewhere close to the specific type of counter API you're expecting here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree with this comment. The counter API has no relevance to the sys_time API, neither does the struct rtc_time have any relevance to the RTC API. Implementations of the sys_time API may use either, but the entire point is to abstract this away from the user, who simply wants to get/set and convert time.
I understand that the fact that we are reusing the struct rtc_time
makes it seem like this is directly related to the rtc device driver API, it is not, sys_time
works just fine on systems that don't have an RTC...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to keep up the API as being generic, then please rename the struct such that the name speaks of itself and remove the rtc_time
definition from the driver subsystem rtc.h
to a more general place. This is not my recommendation, though, see above, as it will be very hard to reach the level of openness to cover the kinds of use cases we already know about. Like it is now it's unnecessarily confusing in a place where people could expect well-known concepts and names.
How about naming it sys_realtime? Its a library handling interactions with and conversions between real-time clocks and real-time counters? |
It would be kind of great to have 1 header file for converting between zephyr time representations. Naming things is hard, but if it can be added to an existing header, it's much easier. |
include/zephyr/sys/time.h
Outdated
int sys_time_set_timestamp(const int64_t *timestamp); | ||
|
||
/** Get universal coordinated time datetime */ | ||
int sys_time_get_datetime(struct rtc_time *datetime); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, you can keep them separate in both directions and only have a specific implementation refer to both at the same time, but if you intend to implement within a POSIX namespace (or at least a namespace that can easily be confused with <sys/time.h>, then you should steer clear from any imports deep into a single driver subsystem from a generic header file. If this was in sys/rtc_time.h
or ...rtc/time.h
(haven't thought much about the specific locations), it would be much clearer which kind of dependencies you imply here.
lib/time/Kconfig
Outdated
# Copyright (c) 2024 Bjarki Arge Andreasen | ||
# SPDX-License-Identifier: Apache-2.0 | ||
|
||
menuconfig TIME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, then please keep this experimental/unstable API encapsulated in the appropriate subsystem until it keeps the promise of being generic enough to be placed in a generic namespace. You can always move it later one once you make it represent general POSIX time.
lib/time/counter.c
Outdated
int ret; | ||
uint32_t counter_ticks; | ||
uint32_t counter_center_bottom_ticks; | ||
uint32_t counter_center_top_ticks; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Full disclosure: There's no official consent about this in the project AFAIK, I just used to get comments in that direction myself and got to like it. But I think no one really has a strong opinion about this (myself definitely not).
lib/time/counter.c
Outdated
|
||
static void add_ticks_to_seconds(int64_t *seconds, const uint64_t *ticks) | ||
{ | ||
*seconds += (int64_t)((*ticks) / frequency); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm aware of the formula.
Have a look at how the well-conceived timepoint and timeout concepts in the kernel, the Linux kernel clocksource and my own counter-to-time conversion API proposal are all extremely careful to keep the "tick-version" well apart from the "rounded version" (potentially non-deterministic, accruing rounding errors over subsequent conversions, ...). They all go to great lengths in explaining in which situations the different "worlds" actually make sense and can be used legitimately. It's usually better to stick to the tick representation internally and make all conversions "ephemeral" with a very well defined epoch, rounding approach, syntonization requirements, etc. You should not give up the original "wrapped" tick-value "truth" prematurely, but if you have to then the chosen approach should be very well defined.
These things have been discussed many times before in the project, that's why I invited @andyross to this PR and pointed you to the Clock Architecture PR which documents that discussion quite well, I hope.
Some (not all) of these requirements go away as soon as you conceive your API as something specific to a specific driver universe.
lib/time/counter.c
Outdated
return sys_time_validate_timestamp(timestamp) ? 0 : -EAGAIN; | ||
} | ||
|
||
int sys_time_set_timestamp(const int64_t *timestamp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to keep up the API as being generic, then please rename the struct such that the name speaks of itself and remove the rtc_time
definition from the driver subsystem rtc.h
to a more general place. This is not my recommendation, though, see above, as it will be very hard to reach the level of openness to cover the kinds of use cases we already know about. Like it is now it's unnecessarily confusing in a place where people could expect well-known concepts and names.
I agree, that would be great, but the problem has been underestimated by many in the project already. There are lots of different ways to convert between pairs of time representations, especially when they come from different layers in the clock hierarchy as is the case here: counter layer vs. synchronized time layer - leaving out the syntonization layer in between w/o even mentioning it. If you don't have good intermediate representations (single point of truth) on those three layers as well as APIs to manipulate them properly, then you get a combinatoric explosion of time types and conversion approaches. Hub and spoke is the only approach that works for proper time conversion IMHO. Just think of leap second smearing (or whatever of the many approaches) in the UTC clock and the large amount of established syntonization approaches across system borders which are both huge problems for long-running high-precision timing systems as those driven by Zephyr. And there are many more. Due to our lack of proper time representation in Zephyr, we currently experience that kind of combinatoric explosion. This PR is just one example of many. Linux has gone a long way to converge towards proper intermediate time representations. But our challenge is even bigger, as we cannot sensibly let go of truely deterministic timing even in higher clock layers (syntonization, synchronization), something Linux mostly fails to deliver today. Wrt this PR: If we cannot avoid another arbitrary experimental time conversion library, then I vote for keeping it somewhere down in one driver subsystem where it originates. At least this would properly document the dependency of POSIX time (which seems to be the implied use case) onto one or two driver subsystems. In any case I vote against another flawed approach of "general" time conversion clogging our common namespace, |
"Realtime" is still much too overloaded as a namespace. As long as you don't keep the promise of actually dealing with what "realtime" means in other systems, as long as we don't even reach the Linux benchmark for real time (much less deterministic synchronized clocks as required by several RTOS use cases), then this name promises far too much. Let's face it: This is an RTC driver/hardware (plus maybe counter subsystem) specific conversion library that might even solve an immediate problem in the POSIX layer for a little while but that will have to be replaced as soon as POSIX APIs want to expose something that is similar to what "real time" means on other systems. But it doesn't even remotely integrate the existing time representations in Zephyr not to speak of those that are clearly on the horizon. I'm totally open to help you start with the nucleus of a well-defined time representation in Zephyr, but this PR is not the way to go to achieve this IMO. We have experimental implementations and specific APIs that go far beyond this PR already, so you'd first have to go beyond those. |
@cfriedt Just so I don't pull too much in the wrong direction. Another idea: What if we conceived this API as being part of the internal POSIX layer implementation (but not more), just remove the dependencies on specific hardware from the header files and hide the RTC (or other current hardware dependencies) in the .c files rather than in the .h files and let the .h files refer to POSIX concepts only. Then this API would again be well encapsulated inside a subsystem - which would also be fine from my pov. If we ever introduce proper intermediate time representations we can just introduce them in the implementation w/o having to change anything in the header files or outside the POSIX subsystem. |
I would say, the basis for POSIX time is
That would provide a kind of coarse granularity "clock recovery" on power-on (in the absence of synchronizing via e.g. NTP or PTP). Additionally, I think that would make a great Hard to say where to park this, but since it's not part of any standard, I would steer clear of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answering the summons, though I'll note that while I know timing hardware and common traps with timer drivers and timing-sensitive app code, I'm really not much of an expert on Gregorian calendar stuff, except to say that I know it's historically difficult to get right.
Basically the worry here is that we're inventing a novel implementation of a library that the rest of the world has messed up repeatedly. :)
One thing that would be good to see is a host-side (or native_posix) unit test that exhaustively runs this and compares it against e.g. the glibc gmtime()
implementation. Like, pick random timestamps in the range of "all the times we'd expect this code to ever be used" and verify that it never differs over a few hundred billion iterations or whatever.
lib/time/Kconfig
Outdated
|
||
config TIME_MIN_YEAR | ||
int "Minimum year to support for real-time services" | ||
default 2000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really understanding why the defaults are so limited here. The rtc_time struct is basically just a Unix struct tm and the year field is a 32 bit signed int with the zero at 1900.
lib/time/convert.c
Outdated
datetime->tm_min >= 0 && | ||
datetime->tm_min <= 59 && | ||
datetime->tm_sec >= 0 && | ||
datetime->tm_sec <= 59; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this isn't something called only when e.g. CONFIG_ASSERT=y, it's on the path to every call. And this is actually a lot of code. What is it trying to protect agaist? If the user passes an incorrectly constructed date, it's IMHO perfectly legal to hand them back a nonsense timestamp. Likewise if the RTC driver gives you a broken struct, it's not your problem to solve here.
lib/time/convert.c
Outdated
datetime->tm_sec <= 59; | ||
} | ||
|
||
void sys_time_init_datetime(struct rtc_time *datetime) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API nitpick: why does this function exist? What does it mean to "initialize" a date that doesn't actually (by construction!) refer to an actual date. This literally produces an invalid struct per the validate() call above. ;)
As I see it this should just be removed. You initialize a struct rtc_time by getting one from a driver or by converting a timestamp.
f98ba17
to
608bb34
Compare
What about "rtcutil" as a prefix instead of "sys_realtime"? Just a thought |
4268b07
to
5eb3bed
Compare
I propose If the POSIX layer or an application want to introduce a direct dependency to the RTC driver subsys (rtc.h) as one of several possible realtime sources then it can still do so - but then it's not a forced dependency and one that is well visible as being bound to a specific hardware type by looking at the names. If that's not fine, then let's rediscuss in the architecture WG. But please document the rtc.h dependency in your diagram above, so we can properly see what's actually going on in that API. |
I propose we wait until I have had time to address the existing comments, before discussing further :) I'm working on it ;) |
5eb3bed
to
0b90924
Compare
Add user facing API for getting and setting system realtime and converting between datetime (clock and calendar) and timestamp (unixtime in milliseconds). Signed-off-by: Bjarki Arge Andreasen <[email protected]>
0b90924
to
29a63a7
Compare
Add native implementation of realtime conversion functions. Signed-off-by: Bjarki Arge Andreasen <[email protected]>
5ab0e18
to
33e42c4
Compare
Add system realtime clock implementation based on the system clock (k_uptime) Signed-off-by: Bjarki Arge Andreasen <[email protected]>
Add sys_time conversion test suite testing conversion between and validation of datetimes and UTC timestamps, all the way from year -1, to year 2399. Signed-off-by: Bjarki Arge Andreasen <[email protected]>
Add test suite which tests the timestamp and datetime get/set APIs work from userspace and the implementation of the system realtime clock is settable and incrementing correctly. Signed-off-by: Bjarki Arge Andreasen <[email protected]>
The real time clock is almost never synced with the monotonic clock, nor k_uptime which the monotonic clock is tied to. Update the nanosleep test suite to set the real time clock to a realistic value, apart from the monotonic clock. This requires passing two new params to the common lower bound check function, the requested time to sleep, which is now different from the absolute k_uptime/monotonic clock time. This update validates the the real time clock is settable and indeed set to requested value, and that is running as expected, and that the real time clock a different clock than monotonic clock. Signed-off-by: Bjarki Arge Andreasen <[email protected]>
The posix clock implementation uses k_uptime as its only source of truth for time, tying clock monotonic 1-1 to k_uptime, and clock realtime to (k_uptime + a reference/offset). sys_realtime provides the zephyr version of a realtime clock, which is managed by the kernel. This commit refactors the posix clock implementation to offload the realtime clock to sys_realtime, converting between a unix timestamp in ms (sys_realtime) to a unix timestamp in struct timespec (posix time). A small adjustment to the posix common clock test suite to set the realtime clock to the same time as clock monotonic was required since the test suite can not support a clock which is not tied 1-1 to k_uptime. Signed-off-by: Bjarki Arge Andreasen <[email protected]>
Add rtc shell command "sync" which synchronizes the system realtime with the RTC time of the given RTC. The command is used like: $uart: rtc sync rtc to set the system realtime clock to the time of the rtc. Signed-off-by: Bjarki Arge Andreasen <[email protected]>
The hwclock is selected using the chosen node "zephyr,rtc". It will automatically set the system real-time clock on boot if the RTCs time is valid. Signed-off-by: Bjarki Arge Andreasen <[email protected]>
fb31000
to
89f7ac4
Compare
Something is causing a test to sleep forever in CI, It can not be reproduces locally, so a bunch of logs have been added to try to see at least where it is failing. Signed-off-by: Bjarki Arge Andreasen <[email protected]>
89f7ac4
to
b337bba
Compare
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
Is this still moving forward? Right now we're having to do yet another roll-your-own solution to handle the fact that the only concept of time Zephyr has is its own uptime. Uptime alone doesn't really work for network enabled applications or even for system event logging, and something better than an application-specific offset + uptime workaround would be greatly appreciated. |
Its quite dormant since I am spending most of my time with power management currently, I think the solution in this PR is nice, the only thing I remember needing to add was handling day of week and day of year conversions (if anyone wants that), would you like to help finalize this? |
oh, and also, there was a posix clock test which was failing for some target run in Renode, 99% certain it was just some CI instability, but that was the last thing I was dealing with before switching tasks :) |
Introduction
This PR adds native APIs for setting, getting and working with real-time as unix timestamps and
struct tm
like datetimes.Overview
The following diagram presents an overview of the system realtime library.

The system realtime clock, which is the base for
sys_realtime_get_/set_
APIs, is a settable complimentary clock to the sys_clock, which is the base fork_uptime()
. The system realtime clock can be implemented using any high accuracy clock. This PR provides an implementation which uses the sys_clock as the base clock. Similarly, the implementation of the conversion functionssys_realtime_timestamp_to_datetime
and opposite can be selected at build time. This PR provides a native implementation of said APIs.System realtime API
The top level API exposed to the user provides the following APIs
Thje get/set functions are syscalls, making them available to userspace, the conversion functions don't touch any kernel objects so they are not implemented as syscalls.
Configuring initial time source
Setting realtime automatically on boot from a hardware RTC can be configured by using the chosen
zephyr,rtc
.fixes: #35333