[riot-notifications] [RIOT-OS/RIOT] rtt_rtc: add rtc_settimeofday() & rtc_gettimeofday() (#16682)

Marian Buschsieweke notifications at github.com
Tue Jul 27 05:19:55 CEST 2021

@maribu commented on this pull request.

see inline

> @@ -50,7 +51,7 @@
 #define BACKUP_RAM      __attribute__((section(".noinit")))
-static uint32_t rtc_now BACKUP_RAM;         /**< The RTC timestamp when the last RTT alarm triggered */
+static volatile uint32_t rtc_now BACKUP_RAM; /**< The RTC timestamp when the last RTT alarm triggered */

I really dislike using `volatile` on a variable that does not refer to memory-mapped I/O.

1. It feeds into the misinformation campaign that `volatile` magically solves all data race issues that is commonly spread by embedded developers
2. It results in more overhead in both terms of ROM and CPU cycles, as `volatile` really only is about disabling optimizations. E.g. withing `irq_disable()` ... `irq_restore()`, memory mapped I/O register still can change (hence, every access is `volatile`), but ordinary SRAM will not magically change.
    - To some part this is IMO an oversight of the C spec, as it would have made much more sense to specify for specific accesses to be `volatile` rather than making this a type qualifier.
3. It is extremely hard to reason when one really only needs `volatile`, or also atomic access or sequential consistency. And this has to be done for *every* access to the variable. It is extremely easy to overlook an instance where more than just `volatile` is needed or to introduce bugs in refactoring. And these data-race bugs are a pain in the ass to debug.

>      do {
-        uint32_t now = rtt_get_counter();
-        uint32_t tmp = _rtc_now(now);
+        uint32_t now;
-        rtc_localtime(tmp, time);
-        *ms = (SUBSECONDS(now) * MS_PER_SEC) / RTT_SECOND;
+        prev = rtc_now;
+        now  = rtt_get_counter();
+        tmp  = _rtc_now(now);
+        *ms = (SUBSECONDS(now - last_alarm) * MS_PER_SEC) / RTT_SECOND;
     } while (prev != rtc_now);

OK, let's assume that `last_alarm` would be `volatile` here as well and we are on an AVR with a 16 bit RTT.

1. `rtc_now` has a value of `0x00ff`
2. `prev = rtc_now;` requires two 8 bit loads and in between these loads `rtc_now` is set to `0x4200`. As a result, `prev` has a value of `0x42ff`
3. The read of `last_alarm` gets similarly corrupted by the value changing in between the reads of the two least significant bytes
4. The timer fires and `rtc_now` gets a value of `0x42ff`
5. `while (prev != rtc_now)` doesn't detect the corruptions, as `prev` has a corrupted value of `0x42ff`

I agree that the chance of something like this happening is pretty low. But this only makes the issue harder to debug.

You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210726/5745532b/attachment.htm>

More information about the notifications mailing list