[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")))
 #endif
 
-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:
https://github.com/RIOT-OS/RIOT/pull/16682#pullrequestreview-715447816
-------------- 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