<p></p>
<p><b>@maribu</b> commented on this pull request.</p>

<p>see inline</p><hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/16682#discussion_r677084467">drivers/rtt_rtc/rtt_rtc.c</a>:</p>
<pre style='color:#555'>> @@ -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 */
</pre>
<p>I really dislike using <code>volatile</code> on a variable that does not refer to memory-mapped I/O.</p>
<ol>
<li>It feeds into the misinformation campaign that <code>volatile</code> magically solves all data race issues that is commonly spread by embedded developers</li>
<li>It results in more overhead in both terms of ROM and CPU cycles, as <code>volatile</code> really only is about disabling optimizations. E.g. withing <code>irq_disable()</code> ... <code>irq_restore()</code>, memory mapped I/O register still can change (hence, every access is <code>volatile</code>), but ordinary SRAM will not magically change.
<ul>
<li>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 <code>volatile</code> rather than making this a type qualifier.</li>
</ul>
</li>
<li>It is extremely hard to reason when one really only needs <code>volatile</code>, or also atomic access or sequential consistency. And this has to be done for <em>every</em> access to the variable. It is extremely easy to overlook an instance where more than just <code>volatile</code> is needed or to introduce bugs in refactoring. And these data-race bugs are a pain in the ass to debug.</li>
</ol>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/16682#discussion_r677086890">drivers/rtt_rtc/rtt_rtc.c</a>:</p>
<pre style='color:#555'>>      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);
</pre>
<p>OK, let's assume that <code>last_alarm</code> would be <code>volatile</code> here as well and we are on an AVR with a 16 bit RTT.</p>
<ol>
<li><code>rtc_now</code> has a value of <code>0x00ff</code></li>
<li><code>prev = rtc_now;</code> requires two 8 bit loads and in between these loads <code>rtc_now</code> is set to <code>0x4200</code>. As a result, <code>prev</code> has a value of <code>0x42ff</code></li>
<li>The read of <code>last_alarm</code> gets similarly corrupted by the value changing in between the reads of the two least significant bytes</li>
<li>The timer fires and <code>rtc_now</code> gets a value of <code>0x42ff</code></li>
<li><code>while (prev != rtc_now)</code> doesn't detect the corruptions, as <code>prev</code> has a corrupted value of <code>0x42ff</code></li>
</ol>
<p>I agree that the chance of something like this happening is pretty low. But this only makes the issue harder to debug.</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/RIOT-OS/RIOT/pull/16682#pullrequestreview-715447816">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABE7WYFQAVA66GT4B7XPWDTTZYQVXANCNFSM5BAIVSLA">unsubscribe</a>.<img src="https://github.com/notifications/beacon/ABE7WYDBOSKHT3WEX7WMO4TTZYQVXA5CNFSM5BAIVSLKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOFKSN4CA.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/RIOT-OS/RIOT/pull/16682#pullrequestreview-715447816",
"url": "https://github.com/RIOT-OS/RIOT/pull/16682#pullrequestreview-715447816",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>