Add RTC time change notifications

[DanielLockau-MLPA]

Contribution description

Relating to real (wall clock) time is required in many applications. Knowing about shifts in real time due to setting the clock can be crucial in some application contexts and building up a monitoring within the application can be considered too expensive. Applications also can may have several sources for changing the system's wall time, e.g. the console, a GNSS module or NTP.

As all sources for time changes to the wall time will use the periph/rtc or drivers/rtt_rtc APIs, we propose to add time change notifications to the time setter API functions within those modules. Notifications are distributed to subscribers through a system bus for RTC events.

To enable queuing of multiple additive time change events, we propose to add the amount of time difference created between old wall time and new wall time events as int32_t content in milliseconds to each message. This enables communication of time differences in milliseconds within about +-24.8 days and an out of range special value (INT32_MIN). One might instead consider a singleton to signal larger time differences as multiple queuing is quite improbable. However, wall time changes of more than 24 days are also quite improbable in a running system and by enabling queuing of multiple additive changes we can achieve consistency within that period even if the application shows some misbehavior.

Testing procedure

• the unit test for periph/rtc was extended to test the time difference computation
• the test for periph/rtc was extended to test the time change notifications

Issues/PRs references

None.

[kaspar030]

Would it make sense to rename the existing rtc_set_time(), and do the sysbus stuff once, calling the renamed ones?

[DanielLockau-MLPA]

Would it make sense to rename the existing rtc_set_time(), and do the sysbus stuff once, calling the renamed ones?

I also thought about having a single place for the implementation but some of the rtc implementations support sub-second precision and others don't. I would also like to support rtt_rtc which has an additional setter method apart from the rtc interface which is why I set this idea aside.

[kaspar030]

I'm not entirely sure if these notification should be part of periph_rtc.

[maribu]

I'm not entirely sure if these notification should be part of periph_rtc.

How about providing a callback (no need to waste RAM and ROM on function pointers here IMO, just extern void rtc_changed_callback(time_old, time_new)) and hook the bus interface in there (by implementing that callback)? If someone wants the callback without the bus, they could implement their own callback function with this.

[DanielLockau-MLPA]

I finally got back to this and addressed existing comments. Not sure whether this proposal will fit coding conventions though.

• @kaspar030 I reduced code impact by wrapping rtc_set_time into another function call instead of creating copy-paste content
• @maribu I separated the time change notification on the system bus from periph/rtc by adding a callback as proposed by you
• @kaspar030 the first commit addresses a probable issue in the implementation of cpu/lpc23xx, however I don't have the means to test it. Comparing to the data sheet I seems like a typo was made during the rtc implementation. If you confirm, I can move this to another branch as it is unrelated.

Generally a good number of rtc implementations seem to reset the sub-second timer whenever the time is reset. A few don't seem to do that. I would generally argue that the provided date should be precise and sub-second timing should be reset to the provided time stamp's precision. I actually encountered a test failure on native following an unexpected rollover due to this issue (notice the seconds value following No alarm at which was expected to be identical to the alarm value previously set):

 % make test                                                       
r
/home/daniel_lockau/src/IoT_RIOT_Products/ext/RIOT/tests/periph_rtc/bin/native/tests_periph_rtc.elf /dev/ttyACM0 
RIOT native interrupts/signals initialized.
LED_RED_OFF
LED_GREEN_ON
RIOT native board initialized.
RIOT native hardware initialization complete.

Help: Press s to start test, r to print it is ready
READY
s
START
main(): This is RIOT! (Version: 2022.01-devel-1319-g5bf5ae1-feat/20211108__sys-bus__add_rtc_bus)

RIOT RTC low-level driver test
This test will display 'Alarm!' every 2 seconds for 4 times
Native RTC initialized.
  Setting clock to   2020-02-28 23:59:57
Clock value is now   2020-02-28 23:59:57
Resetting clock to   2020-02-29 00:00:39
    Time change of   42000 milliseconds
  Setting alarm to   2020-02-29 00:00:41
   Alarm is set to   2020-02-29 00:00:41
  Alarm cleared at   2020-02-29 00:00:39
       No alarm at   2020-02-29 00:00:42
  Setting alarm to   2020-02-29 00:00:44


Traceback (most recent call last):
  File "/home/daniel_lockau/src/IoT_RIOT_Products/ext/RIOT/tests/periph_rtc/tests/01-run.py", line 52, in <module>
    sys.exit(run(testfunc))
  File "/home/daniel_lockau/src/IoT_RIOT_Products/ext/RIOT/dist/pythonlibs/testrunner/__init__.py", line 30, in run
    testfunc(child)
  File "/home/daniel_lockau/src/IoT_RIOT_Products/ext/RIOT/tests/periph_rtc/tests/01-run.py", line 44, in testfunc
    assert alarm_value == no_alarm_value
AssertionError
make: *** [/home/daniel_lockau/src/IoT_RIOT_Products/ext/RIOT/makefiles/tests/tests.inc.mk:22: test] Error 1

[benpicco]

looks good, but needs a rebase. Code was moved from periph_common/rtc.c to sys/rtc_utils

[maribu]

Hmm, I wonder if it wouldn't be better to rename rtc_set_time() to e.g. rtc_set_time_dev() or whatever. That could also be extended to directly provide the previous values (including the ms parameter) for improved robustness. And rtc_set_time() would than be a common wrapper on top, which either just replicates the old behavior (if module periph_rtc_setter_callback is not used), or calls the callback.

I fear that this feature might be a footgun if one has to ensure that all calls of rtc_set_time() will be replaced with rtc_set_time_with_callback() to work correctly.

I am aware that this would involve touching all RTC implementations...

[DanielLockau-MLPA]

I completely agree with you on the footgun. I just wanted to reduce my impact on code with this second proposal. I am willing to rework this PR accordingly but it might take some weeks until I get back to it as I am currently under a relatively high load.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

[riot-ci]

Murdock results

❌ FAILED

ae804c1 sys/bus: implement time change notification

Success	Failures	Total	Runtime
3621	0	9394	04m:24s

Artifacts

[fabian18]

I think this needs an update. The rtc_setter_callback is no longer compiled in to our app, because when the module is enabled, the build system complains about an unknown feature. There is now a features.yaml configuration file to generate Makefile.features. Every periph_ module is a feature. I think a normal pseudomodule rtc_setter_callback would be more appropriate. Because the callback is not provided by MCU or board, but by application.

[DanielLockau-MLPA]

I think this needs an update.

At the time of opening this PR, it seemed sensible to add this feature to RIOT but the number of use cases for this might not justify adding this feature on OS level. Adding an external wrapper layer to be used in specific applications should do the trick as well.