[riot-notifications] [RIOT-OS/RIOT] cpu/atmega_common: RTT and RTC support (#8842)

Marian Buschsieweke notifications at github.com
Tue Mar 5 13:18:21 CET 2019


maribu requested changes on this pull request.

Nice work. I found two places where `static` is missing. Also, please check the use of `volatile` (marked inline). This seems to be about the most wrongly used keyword there is. If it really is correctly used, please add comments explaining why. (Otherwise, people (including me) will suspect it is used incorrectly.)

> +#if RTC_NUMOF
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+static inline void _asynch_wait(void);
+
+typedef struct {
+    time_t time;                /* seconds since the epoch */
+    time_t alarm;               /* alarm_cb when time == alarm */
+    rtc_alarm_cb_t alarm_cb;    /* callback called from RTC alarm */
+    void *alarm_arg;            /* argument passed to the callback */
+} rtc_state_t;
+
+static volatile rtc_state_t rtc_state;

`volatile` is a beast few people fully understand. (And I admit that I do not fully understand it either ;-).) Are you sure that it is right to be used here? If so, please add comments explaining why it is needed. Otherwise, please get rid of it :-)

> +        cb(rtc_state.alarm_arg);
+
+        return;
+    }
+
+    /* Enable irq only if alarm is in the 8s period before it overflows */
+    if ((rtc_state.alarm - rtc_state.time) < 8) {
+        /* Clear interrupt flag */
+        TIFR2 = (1 << OCF2B);
+
+        /* Enable interrupt */
+        TIMSK2 |= (1 << OCIE2B);
+    }
+}
+
+void _asynch_wait(void)

`static`

> +#if MODULE_PERIPH_RTC
+extern void atmega_rtc_incr(void);
+#endif
+
+static inline void _asynch_wait(void);
+
+typedef struct {
+    uint16_t ext_cnt;           /* Counter to make 8-bit timer 24-bit */
+    uint16_t ext_comp;          /* Extend compare to 24-bits */
+    rtt_cb_t alarm_cb;          /* callback called from RTT alarm */
+    void *alarm_arg;            /* argument passed to the callback */
+    rtt_cb_t overflow_cb;       /* callback called when RTT overflows */
+    void *overflow_arg;         /* argument passed to the callback */
+} rtt_state_t;
+
+static volatile rtt_state_t rtt_state;

Is `volatile` correct here? If so, please add comments explaining it.

> +    /* Interrupt safe order of assignment */
+    rtt_state.alarm_cb = NULL;
+    rtt_state.alarm_arg = NULL;
+}
+
+void rtt_poweron(void)
+{
+    power_timer2_enable();
+}
+
+void rtt_poweroff(void)
+{
+    power_timer2_disable();
+}
+
+void _asynch_wait(void)

`static`

-- 
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/8842#pullrequestreview-210644670
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190305/57a3fc28/attachment-0001.html>


More information about the notifications mailing list