sys/walltime: add module for system time #21343

benpicco · 2025-04-01T16:43:24Z

Contribution description

Currently applications use the periph_rtc API directly if they want to track wall clock time.
This leads to problems when the time is not provided by periph_rtc but some external RTC or when we want to perform actions e.g. on setting the time - then this has to be hooked up with every periph_rtc implementation.

Instead, provide a wrapper API that applications can use to obtain the current system time without accessing the RTC API directly.

This allows to write applications that are agnostic to the availability of periph_rtc and still can provide some proper timestamps.

Testing procedure

Enable the walltime module.
This will provide a walltime command to get / set the time and display the time since last boot.

Issues/PRs references

Alternative to #17416, #21366

crasbe

I like this addition a lot, some small comments and observations from testing the PR below.

Edit: For reference: I tested this with an nRF52840DK with USEMODULE+=walltime BOARD=nrf52840dk make -C tests/sys/shell flash term.

sys/walltime/shell.c

sys/include/walltime.h

sys/walltime/shell.c

sys/include/walltime.h

crasbe · 2025-04-01T20:07:06Z

sys/include/walltime.h

+ * @brief       Common functions to access the wall-clock / real time clock
+ * @{
+ * @file
+ *


It would be nice to have some documentation about what this module does and how to use it. More or less what the PR description says for example.

Currently it's a bit sparse and if you don't know what it does, it's a bit hard to get into it (IMO):

Also since there is no documentation for struct tm (I think time.h is part of the standard library?), it might be good to include a link to a reference for it.

This is the link used by drivers/periph/rtc.h: https://pubs.opengroup.org/onlinepubs/7908799/xsh/time.h.html

RIOT/drivers/include/periph/rtc.h

Lines 17 to 20 in 85dc9be

* @note

* The values used for setting and getting the time/alarm should

* conform to the `struct tm` specification.

* Compare: http://pubs.opengroup.org/onlinepubs/7908799/xsh/time.h.html

riot-ci · 2025-04-01T20:31:24Z

Murdock results

✔️ PASSED

90fc5e4 tests/walltime: add test for the walltime module

Success	Failures	Total	Runtime
10316	0	10317	11m:02s

Artifacts

Documentation preview

mguetschow

Nice, I like this! Some first thoughts on it below.

sys/include/walltime.h

sys/walltime/shell.c

mguetschow · 2025-04-02T11:42:53Z

sys/walltime/walltime.c

+    struct tm now;
+    walltime_get(&now, ms);
+    return mktime(&now);


Since you are providing a ZTIMER_FALLBACK above, what would happen here if periph_rtc and rtt_rtc are not available?

The system time will always start at RIOT_EPOCH at boot, but still can by synced later on e.g. via NTP - it's just not persistent across reboots.

mguetschow · 2025-04-02T11:43:42Z

sys/walltime/walltime.c

+#endif
+
+static uint32_t _boottime;
+#ifdef BACKUP_RAM


Is that macro documented somewhere?

Does this count?

$ rg BACKUP_RAM -B 5 features.yaml 417-- title: RAM Related Features 418- help: These features indicate presence of special RAM regions or features 419- features: 420- - name: backup_ram 421- help: A special portion of RAM is retained during deep sleep. 422: Variables can be placed there by annotating them with the `BACKUP_RAM`

See https://doc.riot-os.org/feature-list.html#autotoc_md2404 for the output

Hum, that's indeed something, but probably nothing doxygen would find when using @ref BACKUP_RAM

mguetschow · 2025-04-02T11:46:04Z

sys/walltime/walltime.c

+    uint32_t msec = 0;
+#ifdef ZTIMER_FALLBACK
+    rtc_localtime(walltime_get_riot(ms), time);
+#elif defined(MODULE_PERIPH_RTC_MS)


Is it on purpose that you check for MODULE_PERIPH_RTC above for ZTIMER_FALLBACK?

sys/walltime/walltime.c

sys/include/walltime.h

sys/walltime/shell.c

kfessel · 2025-04-03T11:26:12Z

sys/walltime/walltime.c

+static uint32_t _boottime_bkup;
+#endif
+#ifdef ZTIMER_FALLBACK
+uint32_t ztimer_offset;


even though i rarely run my devboards for 50 days or longer there should be a "mitigation for the 49day overflow"
eg a overflow counter

ah here: uint32_t now = ztimer_now(ZTIMER_MSEC); you mean.

sys/include/walltime.h

sys/walltime/shell.c

fabian18 · 2025-04-07T14:26:45Z

sys/walltime/walltime.c

+
+void walltime_set(struct tm *time)
+{
+    int32_t diff = rtc_mktime(time) - walltime_get_riot(NULL);


I think the int32_t could lead to unexpected results. For example the extreme case 0 - 4294967295 becomes 1.
I would expect -4294967295
You could figure out the sign and compute the number as uint32_t higher - lower.

diff --git a/sys/walltime/walltime.c b/sys/walltime/walltime.c index 903e856a16..5171cb0770 100644 --- a/sys/walltime/walltime.c +++ b/sys/walltime/walltime.c @@ -17,6 +17,8 @@ * * @} */ +#include <stdio.h> + #include "auto_init.h" #include "auto_init_utils.h" #include "auto_init_priorities.h" @@ -96,7 +98,10 @@ time_t walltime_get_unix(uint16_t *ms) void walltime_set(struct tm *time) { - int32_t diff = rtc_mktime(time) - walltime_get_riot(NULL); + uint32_t t; + int32_t w; + int32_t diff = (t = rtc_mktime(time)) - (w = walltime_get_riot(NULL)); + printf("t %"PRIu32" - w %"PRId32" = diff %"PRId32"\n", t, w, diff); _boottime += diff; #ifdef BACKUP_RAM _boottime_bkup += diff;

> 2025-05-06 15:29:20,168 # main(): This is RIOT! (Version: 2025.04-devel-522-g90fc5e-walltime) > walltime set 4000-01-01 00:00:00 2025-05-06 15:29:23,464 # walltime set 4000-01-01 00:00:00 2025-05-06 15:29:23,480 # t 2353209856 - w 3 = diff -1941757443 2025-05-06 15:29:23,481 # time changed by -1941757443 sec, 0 ms

So when going very very very far in the future, the diff becomes negative.
I know that no one actually cares about the year 4000, but consider some user input or malformed network packet ... whatever.

sys/walltime/shell.c

fabian18 · 2025-04-07T14:36:18Z

sys/walltime/walltime.c

+ *
+ * @}
+ */
+#include "auto_init.h"


maybe guard with IS_USED(MODULE_AUTO_INIT)

Because when it would be used with unittest it would fail to compile, or when someone does not want auto_init.
I would say if the auto init stuff is not guarded, USEMODULE += auto_init would belong in Makefile.dep.
But auto_init is not necessarily required.

Too much worries or do you agree that any auto init stuff should be guarded?

fabian18 · 2025-04-07T14:37:41Z

sys/walltime/walltime.c

+    return now - _boottime;
+}
+
+static void auto_init_uptime(void)


maybe guard with IS_USED(MODULE_AUTO_INIT_WALLTIME/UPTIME)

sys/walltime/walltime.c

fabian18 · 2025-04-07T14:54:25Z

This allows to write applications that are independent of the RTC
peripheral and allows the implementation of on-time change callbacks.

How would I implement an on-time change callback?
You expect that the backend for walltime implements the RTC API, don't you?
This limits to only only one RTC API. If I want to implement my application rtc_set_time around the RTC rtc_set_time, it does not work. I would have gone with function pointers for walltime_backend->rtc_set_time and walltime_backend->rtc_get_time

benpicco · 2025-04-07T15:05:47Z

How would I implement an on-time change callback?

We can add that to the walltime module as a 2nd step, then we would only need to add it in one place instead of adding it to every RTC implementation.

I wanted to get the core API in first, but might as well already add this here.

If I want to implement my application rtc_set_time around the RTC rtc_set_time, it does not work.

Yea applications must not use the periph_rtc API if the walltime module is used to access the RTC instead - but that should be a simple change.

I would have gone with function pointers for walltime_backend->rtc_set_time and walltime_backend->rtc_get_time

There only is a single wall time/system time. I'm happy to hook up different backends like ds1307 or even dcf77 or gnss. The nice thing about the high level API is that the internals can be as complex as needed, to the user it's still just a simple get time / set time API.

sys/walltime/walltime.c

fabian18 · 2025-05-06T13:43:57Z

sys/include/walltime.h

+ *
+ * @param[in] time  The current data / time to set
+ */
+void walltime_set(struct tm *time);


If rtc_set_time returns an int, then walltime_set also should be capable to return an error.
There could be an external I2C RTC which could report a communication error.

fabian18 · 2025-05-06T13:44:41Z

sys/include/walltime.h

+ * @param[out] ms   current milliseconds output, may be NULL
+ *                  always returns 0 if the backend supports no millisecond resolution
+ */
+void walltime_get(struct tm *time, uint16_t *ms);


also here I would vote for an int return type

fabian18 · 2025-05-06T13:45:07Z

sys/fmt/fmt.c

+    return 8;
+}
+
+


double empty line

fabian18 · 2025-06-19T08:44:46Z

The walltime is calling rtc API.

Consider the case of an external RTC.

There is the option to wrap the driver RTC API in our periph RTC API.

Then periph_rtc must get substituted with the external RTC.

If the walltime would not use the RTC API but use a driver struct with function callbacks, then the walltime could be instanciated with either the periph RTC or the external RTC driver. Then periph RTC features could still be used.

github-actions bot added the Area: sys Area: System label Apr 1, 2025

benpicco force-pushed the walltime branch from c7303e5 to 128f19f Compare April 1, 2025 16:54

maribu self-requested a review April 1, 2025 17:25

crasbe reviewed Apr 1, 2025

View reviewed changes

crasbe added Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 1, 2025

mguetschow reviewed Apr 2, 2025

View reviewed changes

maribu reviewed Apr 2, 2025

View reviewed changes

sys/include/walltime.h Outdated Show resolved Hide resolved

maribu reviewed Apr 2, 2025

View reviewed changes

sys/walltime/shell.c Outdated Show resolved Hide resolved

sys/walltime/shell.c Outdated Show resolved Hide resolved

kfessel reviewed Apr 3, 2025

View reviewed changes

mguetschow reviewed Apr 3, 2025

View reviewed changes

sys/include/walltime.h Outdated Show resolved Hide resolved

mguetschow reviewed Apr 3, 2025

View reviewed changes

sys/include/walltime.h Outdated Show resolved Hide resolved

benpicco requested a review from fabian18 April 7, 2025 13:51

benpicco mentioned this pull request Apr 7, 2025

periph/rtc: add rtc_pre_set_time() and rtc_post_set_time() #21366

Open

fabian18 reviewed Apr 7, 2025

View reviewed changes

sys/walltime/walltime.c Outdated Show resolved Hide resolved

sys/walltime/walltime.c Outdated Show resolved Hide resolved

fabian18 mentioned this pull request Apr 8, 2025

sys/fmt: add {fmt,scn}_time_tm_iso8601 #21391

Merged

benpicco added 3 commits April 11, 2025 19:36

sys/fmt: split up scn_time_tm_iso8601()

1b42c47

sys/walltime: add module for system time

d46d166

sys/walltime: add shell commands

28e964b

benpicco force-pushed the walltime branch from 873243a to 28e964b Compare April 11, 2025 17:37

benpicco requested a review from kaspar030 as a code owner April 11, 2025 17:37

benpicco requested review from leandrolanzieri, aabadie and MichelRottleuthner as code owners April 24, 2025 16:13

github-actions bot added the Area: tests Area: tests and testing framework label Apr 24, 2025

sys/walltime: add time change notification callbacks

26b3f05

benpicco force-pushed the walltime branch from 73922af to d3999cd Compare April 24, 2025 16:20

tests/walltime: add test for the walltime module

90fc5e4

benpicco force-pushed the walltime branch from d3999cd to 90fc5e4 Compare April 24, 2025 16:57

fabian18 reviewed May 6, 2025

View reviewed changes

sys/fmt/fmt.c

return 8;

}

Copy link

Contributor

fabian18 May 6, 2025

Choose a reason for hiding this comment

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

double empty line

	* @note
	* The values used for setting and getting the time/alarm should
	* conform to the `struct tm` specification.
	* Compare: http://pubs.opengroup.org/onlinepubs/7908799/xsh/time.h.html

sys/walltime: add module for system time #21343

Are you sure you want to change the base?

sys/walltime: add module for system time #21343

Uh oh!

Conversation

benpicco commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

crasbe left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

riot-ci commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Murdock results

Artifacts

Uh oh!

mguetschow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fabian18 commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benpicco commented Apr 1, 2025 •

edited

Loading

crasbe left a comment •

edited

Loading

riot-ci commented Apr 1, 2025 •

edited

Loading

fabian18 commented Apr 7, 2025 •

edited

Loading

fabian18 commented Jun 19, 2025 •

edited

Loading