sys: clock: add sys_clock api and remove posix from iso c time #90096
Conversation
cfriedt
force-pushed
the
remove-posix-from-time-equation
branch
2 times, most recently
from
a403e85 to
83dc940
Compare
cfriedt
changed the title
cfriedt
changed the title
cfriedt
force-pushed
the
remove-posix-from-time-equation
branch
13 times, most recently
from
db7b057 to
fe430cf
Compare
I'd like to get those in better shape before marking this ready for review. |
cfriedt
force-pushed
the
remove-posix-from-time-equation
branch
from
fe430cf to
9a4203b
Compare
cfriedt
force-pushed
the
remove-posix-from-time-equation
branch
2 times, most recently
from
5078a47 to
730558c
Compare
|
Question ... would the proposed k_clock APIs be better served somewhere under /subsys as something like sys_clock APIs? I am wary of them being placed under /kernel. |
It might be possible to reuse existing APIs for calculating some of those things though. |
cfriedt
force-pushed
the
remove-posix-from-time-equation
branch
2 times, most recently
from
793d367 to
19947db
Compare
cfriedt
changed the title
cfriedt
force-pushed
the
remove-posix-from-time-equation
branch
3 times, most recently
from
370277c to
c5bbd4f
Compare
aescolar
dismissed
their stale review
Build issue addressed. Where to place the API or its prefix pending
| sys_clock_getrtbase(&base); | ||
| if (!timespec_add(ts, &base)) { | ||
| return -EOVERFLOW; | ||
| } |
There was a problem hiding this comment.
Not a bug really, but the sense of the implementation here seems backwards to me. The idea of the API is that the "REALTIME" clock gives you what amounts to a "raw" value sourced from external inputs, even though that might be seen to go backwards sometimes if the clock needs to be synchronized, etc... The "MONOTONIC" clock is the "cooked" one that has some magic applied to store an offset so that the clock is never seen to reverse (basically, I'm sure the spec is more complicated).
But here the core value is the cooked one and we store an offset to recover the original input. Seems confusing, though obviously by the magic of 9th grade algebra neither is "wrong".
There was a problem hiding this comment.
Yeah, I think there are definite improvements to make in this area, and possibly some misnomers. The overall contract is still valid, but quality can definitely be improved over time.
For example, the monotonic clock is definitely cooked as it is, but it's cooked in such a way that it will never give sub-tick clock resolution. I think a long time ago, it was based on cycles, but there were some issues encountered.
On Linux, for example, via the VDSO, the monotonic clock has a resolution of 1ns. This is also implicitly high precision. However, the _COARSE clock only has a resolution of about 1ms by default (the tick rate) and naturally the precison is also limited.
So our SYS_CLOCK_MONOTONIC here is efffectively a SYS_CLOCK_MONOTONIC_COARSE.
Definitely agree. I'm less concerned with fixing all aspects of the clock algorithm, and naming private details correctly in this PR and more concerned with the mechanical 'what public symbol goes where', removing unnecessary dependencies, and making sure behaviour is consistent before and after this change.
There was a problem hiding this comment.
I think the naming part of this is fixed?
include/zephyr/sys/clock.h
Outdated
| * | ||
| * @param tp Pointer to memory where time will be written. | ||
| */ | ||
| __syscall void sys_clock_getrtbase(struct timespec *tp); |
There was a problem hiding this comment.
Documentation seems confusing here. The description calls this a "base time", but the way it's used below indicates (I think) that it's an offset instead?
Consider renaming to something like sys_clock_monotonic_offset() or something instead? Also because (see note below) it's really the monotonic clock that is cooked and massaged, the rt clock is "raw".
There was a problem hiding this comment.
This represents the offset of the real-time clock (from an external source) w.r.t. to the coarse monotonic clock.
Sure, this could probably be called sys_clock_getrtoffset() instead and I'll rename the internal variables to offset instead of base.
Deriving a finer monotonic clock from the coarser one should probably be a later improvement in a separate PR.
Previously, log messages would generate warnings (escalated to errors
in CI) when CONFIG_TIMEOUT_64BIT=n.
For example,
```
west build -p auto -b qemu_cortex_m0 -t run tests/posix/timers/ \
-- -DCONFIG_TIMEOUT_64BIT=n
..
warning: format '%llx' expects argument of type \
'long long unsigned int', but argument 3 has type 'k_ticks_t' \
{aka 'unsigned int'}
```
Use portable print specifiers and cast the argument to int64_t.
Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
cfriedt
force-pushed
the
remove-posix-from-time-equation
branch
2 times, most recently
from
1f11e1d to
21f5f98
Compare
|
Additional entries for the sys_clock API, comprised of: * sys_clock_gettime() * sys_clock_settime() * sys_clock_nanosleep() along with the constants * SYS_CLOCK_REALTIME * SYS_CLOCK_MONOTONIC * SYS_TIMER_ABSTIME The primary motivation for this API is so that libc and other libraries have a familiar-enough API to reach to when POSIX is not available, since POSIX is optional in Zephyr. By adding this API to lib/os, we also eliminate dependency cycles between libc and posix, as lib/os is a mutual dependency. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
Add bullets to release notes for sys_clock_gettime(), sys_clock_settime(), and sys_clock_nanosleep(). These changes were significant, as they make timekeeping dependencies more independent between libc and posix, and remove several unnecessary dependencies on posix for ISO C library routines. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
Use the newly added sys_clock_gettime() to avoid the unnecessary dependency on POSIX APIs. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
Remove POSIX clock_gettime() from the common libc time implementation, since POSIX should not be a dependency for ISO C. Instead, use the newly added lib/os sys_clock API. Specifically, sys_clock_gettime(). Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
Use the implementation of time() from the common libc, since there it no longer pulls in POSIX. Use is implied for minimal, newlib, and picolibc, and selected for IAR. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
Reduce the dependency on POSIX by taking advantage of the newly added sys_clock_nanosleep(). Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
The C standard does not guarantee that `thrd_success` is equal to zero, so ensure that the test takes that into account. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
The specification does not say that the first struct timespec passed to thrd_sleep() may be NULL. Preserving the testpoint so that a possible future testsuite can be run that expects faults to occur when undefined behaviour is used. Faults are one possible solution to handling undefined behaviour, but it would be good to come to a concensus about how that should be handled in Zephyr for standard function calls belonging to ISO C or POSIX. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
Use `timespec_from_timeout(K_MSEC(msec), &ts)` instead of leaning on lazily-crafted timespecs with invalid tv_nsec fields. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
A bit of a race condition seems to exist, but only for `qemu_x86_64/atom`. It's the same race that was attributed to scheduler noise or something along those lines when running under qemu. Most other architectures no longer see this issue, and it has not been required for some time in the `tests/benchmark/posix/threads` app, but here it seems to be pronounced again. Testing done: Before: We would see failures very reproducible, but only on this platform. Also, the failure has existed in `main` for some time, so is independent of the current PR. https://github.com/zephyrproject-rtos/zephyr/actions/runs/\ 15238463423/job/42862652065 After: ```shell COUNT=100 NOKAY=0 NFAIL=0 for i in $(seq 0 $((COUNT - 1))); do if [ $i -ne 0 ] && [ $((i % 10)) -eq 0 ]; then echo "Running test iteration $i" fi twister -i --test-only -p qemu_x86_64/atom -s \ libraries.libc.c11_threads.picolibc >/dev/null 2>&1 if [ $? -eq 0 ]; then NOKAY=$((NOKAY + 1)) else NFAIL=$((NFAIL + 1)) echo "Test failed on iteration $i" fi done PCT="$(echo "scale=0; ((100 * $NFAIL) / $COUNT) / 1" | bc)" echo "$NFAIL / $COUNT = $PCT % test failure rate" Running test iteration 10 Running test iteration 20 Running test iteration 30 Running test iteration 40 Running test iteration 50 Running test iteration 60 Running test iteration 70 Running test iteration 80 Running test iteration 90 0 / 100 = 0 % test failure rate ``` Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
Presumably the time testsuite was separate from the c library set of testsuites because it had a depedency on POSIX. Since that dependency no longer exists, colocate the time testsuite with the other c library testsuites. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
Use the newly added sys_clock API in lib/os for * clock_gettime() * clock_settime() * clock_nanosleep() and nanosleep() * gettimeofday() Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
The ISO C function time() should not depend on POSIX and this was corrected recently via the common libc time() implementation. Remove this dependency from the network subsystem where it has been unduly needed for some time. Similarly, XSI_SINGLE_PROCESS was a dependency for time() via picolibc, because the time() implementation in picolibc relies on the POSIX gettimeofday() call. However, since Zephyr's common libc time() implementation no longer depends on that, it can be removed as a dependency in the network subsystem as well. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
Remove POSIX_TIMERS and XSI_SINGLE_PROCESS dependencies from the aws cloud sample and the lwm2m client sample, as they are no longer required. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
cfriedt
force-pushed
the
remove-posix-from-time-equation
branch
from
21f5f98 to
5e38e6a
Compare
|
Previously, both the common libc
time()as well as picolibctime()(an ISO C function) relied on eitherclock_gettime()orgettimeofday(). That had the unfortunate consequence that many areas had an indirect dependency on POSIX.This change removes the dependency cycle that existed between POSIX and some libc's by creating a mutual dependency (i.e. new API) for both POSIX and the common C library under
lib/os. With this change, the ISO C functiontime()can also be used without relying on POSIX support.Areas that should no longer have POSIX dependencies for getting the "wall clock" time include, but are not limited to
lib/libc/common/source/thrdlib/libc/common/source/timetime())samples/net/cloud/aws_iot_mqttsamples/net/lwm2m_clientsubsys/logging/log_core.csubsys/net/lib/config(sntp)subsys/net/lib/lwm2mPlease see individual commits for details
Should be merged after #90060
Fixes #88555
Note
There have been many RFCs on this topic already #19030, #40099, #76335 and most of them agree on the need for an abstract clock source at the OS level. There isn't a huge need for another RFC here.
K_prefix, theSYS_prefix was suggested by @peter-mitsissys_clock_getres().