[riot-notifications] [RIOT-OS/RIOT] periph/rtc: normalize struct tm before usage (#11413)

Marian Buschsieweke notifications at github.com
Thu Sep 12 11:03:18 CEST 2019


maribu commented on this pull request.

OK, codewise I'd ACK. With the PR not changing hardware interactions, I think there is no reason to test it on every board, but testing on one should be more that enough.

Some of the code is pretty magic, which needs more explanation. I marked those sections inline and tried to already suggest comments that could explain the magic. Afterwards, I will ACK.

> + *
+ * @}
+ */
+
+#include <stdint.h>
+#include <stdlib.h>
+#include "periph/rtc.h"
+
+#ifndef RTC_NORMALIZE_COMPAT
+#define RTC_NORMALIZE_COMPAT (0)
+#endif
+
+static int _is_leap_year(int year)
+{
+    if (year & 0x3) {
+        return 0;

Add a comment e.g.

``` C
if (year & 0x3) {
    /* not a multiple of four, so not a leap year */
    return 0;
}
```

> +
+#include <stdint.h>
+#include <stdlib.h>
+#include "periph/rtc.h"
+
+#ifndef RTC_NORMALIZE_COMPAT
+#define RTC_NORMALIZE_COMPAT (0)
+#endif
+
+static int _is_leap_year(int year)
+{
+    if (year & 0x3) {
+        return 0;
+    }
+
+    return !!(year % 25) || !(year & 15);

OK, this needs some more explanation. The rules here are (to be checked in that explicit order):

1. If the year is not a multiple of four, it is not a leap year.
2. If the year is a multiple of 400, it is a leap year
3. If the year is a multiple of 100, it is not a leap year.

It is clear that rule 1 is checked in line 31. So we know that year is a multiple of four by now. If it is also a multiple of 25, it is therefore a multiple of hundred. Thus (with `year` being a multiple of four), `!!(year %25)` is `0` if and only if `year` is a multiple of `100`. `!(year & 15)` is `0` for all `year` that are not a multiple of 16.

As `!!(year %25)` only returns `0` for years that are a multiple of 100 (out of all the multiples of four), `|| !(year &15)` is only evaluated for these. Out of all multiples of 100, `!(year &15)` is `0` except for those that are a multiple of 400.

So the function is indeed fully correct and more efficient than the idiomatic version. But this really needs more explanation. It took me at least 5 minutes to verify the correctness.

> +static int _is_leap_year(int year)
+{
+    if (year & 0x3) {
+        return 0;
+    }
+
+    return !!(year % 25) || !(year & 15);
+}
+
+static int _month_length(int month, int year)
+{
+    if (month == 1) {
+        return 28 + _is_leap_year(year);
+    }
+
+    return 31 - ((month % 7) & 1);

OK. Again add a comment. Something like:

>From January (0) to July (6), with the exception of the already handled February (`month == 1`), the months have 31 and 30 days in turns. Thus, subtracting 1 day if the month is odd works here.

>From August (7) to December (11) again we have 31 and 30 days in turns. If we reset the counter at August (7) to 0, we can therefore again subtract 1 day if the month counter is odd. `(month % 7)` here is used to reset the counter to zero at August (7).

> +    return !!(year % 25) || !(year & 15);
+}
+
+static int _month_length(int month, int year)
+{
+    if (month == 1) {
+        return 28 + _is_leap_year(year);
+    }
+
+    return 31 - ((month % 7) & 1);
+}
+
+#if RTC_NORMALIZE_COMPAT
+static int _wday(int day, int month, int year)
+{
+    /* Tomohiko Sakamoto's Algorithm */

Add this link to the comment please https://en.wikipedia.org/wiki/Determination_of_the_day_of_the_week#Sakamoto's_methods

> +
+    return 31 - ((month % 7) & 1);
+}
+
+#if RTC_NORMALIZE_COMPAT
+static int _wday(int day, int month, int year)
+{
+    /* Tomohiko Sakamoto's Algorithm */
+    static const uint8_t t[] = {0, 3, 2, 5, 0, 3, 5, 1, 4, 6, 2, 4};
+    year -= month < 2;
+    return (year + year/4 - year/100 + year/400 + t[month] + day) % 7;
+}
+
+static int _yday(int day, int month, int year)
+{
+    static const uint8_t d[] = { 0,  31,  59, 90, 120, 151,

Add comment, such as: Cumulative number of days since start of the year. In order to squeeze this into 8 bits, we wrap around at 255 and handle this later.

> +    return 31 - ((month % 7) & 1);
+}
+
+#if RTC_NORMALIZE_COMPAT
+static int _wday(int day, int month, int year)
+{
+    /* Tomohiko Sakamoto's Algorithm */
+    static const uint8_t t[] = {0, 3, 2, 5, 0, 3, 5, 1, 4, 6, 2, 4};
+    year -= month < 2;
+    return (year + year/4 - year/100 + year/400 + t[month] + day) % 7;
+}
+
+static int _yday(int day, int month, int year)
+{
+    static const uint8_t d[] = { 0,  31,  59, 90, 120, 151,
+                               181, 212, 243, 17,  48,  78};

```patch
-                                181, 212, 243, 17,  48,  78};
+                                181, 212, 243, 17/* == 273 % 256*/,  48/* == 304 % 256*/,  78/*== 334 % 256*/};
```

> +    static const uint8_t t[] = {0, 3, 2, 5, 0, 3, 5, 1, 4, 6, 2, 4};
+    year -= month < 2;
+    return (year + year/4 - year/100 + year/400 + t[month] + day) % 7;
+}
+
+static int _yday(int day, int month, int year)
+{
+    static const uint8_t d[] = { 0,  31,  59, 90, 120, 151,
+                               181, 212, 243, 17,  48,  78};
+
+    if (month > 1) {
+        day += _is_leap_year(year);
+    }
+
+    /* at this point we can be sure that day <= 31 */
+    if (month > 8) {

Add comment, such as: 
```C
/* For months October (9), November (10), and December (11) number in d was wrapped around to fit into 8 bits. We undo this now */
```

> +
+static int _yday(int day, int month, int year)
+{
+    static const uint8_t d[] = { 0,  31,  59, 90, 120, 151,
+                               181, 212, 243, 17,  48,  78};
+
+    if (month > 1) {
+        day += _is_leap_year(year);
+    }
+
+    /* at this point we can be sure that day <= 31 */
+    if (month > 8) {
+        day |= 0x100;
+    }
+
+    return d[month] + day - 1;

Add comment, e.g. `/* subtract 1 as counting starts at zero, e.g. 2019-01-01 will be be day 0 in year 2019 */`

-- 
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/11413#pullrequestreview-287250133
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190912/6ea648db/attachment.htm>


More information about the notifications mailing list