periph/rtc: normalize struct tm before usage by benpicco · Pull Request #11413 · RIOT-OS/RIOT

benpicco · 2019-04-17T17:09:44Z

Contribution description

A naive implementation may set a RTC alarm in 30s by calling

struct tm now;
rtc_get_time(&now);
now.tm_sec += 30;
rtc_set_alarm(&now, _cb, NULL);

This works for RTC implementations that use a RTT internally and call mktime() to convert the struct tm to a unix timestamp, as mktime() will normalize the struct in the process.

On a RTC implementation where there is a separate register for minutes / seconds the alarm will never trigger if the current seconds are > 30.

Always call mktime() even when the RTC uses separate registers for time / date components to ensure it is normalized.

This also modifies tests/periph_rtc to exercise this case.

Testing procedure

Run tests/periph_rtc on a board with RTC.
4 Alarm!s should be triggered.

~~[ ] Board with CPU lpc2387 (@maribu)~~ (Broken anyway)
Board with STM32-* CPU (@fjmolinas)
Board with SAM0-CPU (@fjmolinas)
~~[ ] Board with cc430-CPU (@kaspar030)~~ Not needed

fjmolinas

Nice catch, the current tests don't take this into account since only 4 alarms are triggered there never is an overflow. I tested your changes with and without normalizing on nucleo-l073rz, without this PR it just triggers one alarm and with this PR it works as expected. I also tested that there was no regression for samr21-xpro.

Everything looks good, I would just add a comment on rtc_set_alarm and rtc_set_time that states that the input time structure gets normalized.

I don't have any board using the other CPU's @maribu has a msba2 according to the wiki and @kaspar030 has a chronos, maybe they can test for those cpu's?

maribu · 2019-07-24T15:14:23Z

I tested with the MSB-A2. The periph_rtc is completely broken (unrelated to this PR).

I checked the manpage of mktime() and it should indeed normalize the time. Thus, it should theoretically fix the bug.

I'll have a look at the broken periph_rtc of the lpc2387 CPU.

+1 for merging this without having it tested on the MSB-A2 (but on the other three implementations). This fixes a real issue for the other CPU-families and should not be stalled IMO due to an unrelated bug. Also, this PR is not really scary and very unlikely to break anything.

benpicco · 2019-08-13T23:09:46Z

Well it turns out not every platform has mktime() and for some other platforms, including mktime() tips them over the ROM size limit.

I've now introduced a rtc_tm_normalize() that should behave as a minimal mktime().
I will add testcases too if this approach is sensible.

Calculating tm_yday and tm_wday is optional, the RTC does not need it.

For examples/default and BOARD=chronos this gives:

before:

   text	   data	    bss	    dec	    hex	filename
  13116	     90	   1032	  14238	   379e	bin/chronos/default.elf

after:

   text	   data	    bss	    dec	    hex	filename
  13486	     90	   1032	  14608	   3910	bin/chronos/default.elf

after with RTC_NORMALIZE_COMPAT = 1

   text	   data	    bss	    dec	    hex	filename
  13786	     90	   1032	  14908	   3a3c	bin/chronos/default.elf

fjmolinas · 2019-08-14T10:00:06Z

Code looks good. msp430-gcc seems to be doing a worse job at optimizing the rtc_tm_normalize(), 502 bytes vs 242/280 bytes for arm-gcc (depending on platform), both compiled in docker.

fjmolinas · 2019-08-14T10:04:46Z

For chronos rtc code size has almost doubled with this addition. But I still think we should include this. Timing is important enough to spend the memory on getting it correctly, I might want to check if we can optimize it a bit though.

benpicco · 2019-08-14T12:01:17Z

Using div() to get both quotient and remainder in one go sheds some more bytes. Down to ~~458~~ 370 bytes extra on BOARD=chronos now.

fjmolinas · 2019-09-12T07:35:38Z

Tests are still passing for me. Changes are looking good to, thanks for the unit-test addition.

@kaspar030 can you find time to test on chronos? If not I think we should just move forward with the PR.

maribu

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.

drivers/periph_common/rtc.c

maribu · 2019-09-12T09:05:50Z

Btw: Please squash right away (including the added comments).

benpicco · 2019-09-12T09:31:16Z

@maribu thank you for the comments! I added them all except the inline one inside the array members , that one just adds clutter imho and the logic is already explained above.

A naive implementation may set a RTC alarm in 30s by calling struct tm now; rtc_get_time(&now); now.tm_sec += 30; rtc_set_alarm(&now, _cb, NULL); This works for RTC implementations that use a RTT internally and call mktime() to convert the struct tm to a unix timestamp, as mktime() will normalize the struct in the process. Call rtc_tm_normalize() when the RTC uses separate registers for time / date components to ensure it is normalized. This also modifies tests/periph_rtc to exercise this case.

maribu

ACK!

benpicco force-pushed the rtc_sanitize branch 2 times, most recently from 01147d2 to 2d69d84 Compare April 17, 2019 17:59

miri64 added Area: drivers Area: Device drivers Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Apr 18, 2019

fjmolinas reviewed Jul 24, 2019

View reviewed changes

maribu mentioned this pull request Jul 24, 2019

cpu/lpc2387: periph_rtc completely broken #11908

Closed

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 5, 2019

benpicco force-pushed the rtc_sanitize branch 2 times, most recently from da98ddd to 15748e7 Compare August 13, 2019 23:03

benpicco force-pushed the rtc_sanitize branch 2 times, most recently from 9379591 to 97667fc Compare August 13, 2019 23:38

benpicco force-pushed the rtc_sanitize branch from 70cf91d to b877c59 Compare August 14, 2019 11:45

benpicco force-pushed the rtc_sanitize branch 2 times, most recently from 83ae633 to a605c44 Compare August 14, 2019 12:25

benpicco requested a review from fjmolinas September 11, 2019 13:43

maribu reviewed Sep 12, 2019

View reviewed changes

maribu added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Sep 12, 2019

maribu added the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Sep 12, 2019

benpicco and others added 3 commits September 12, 2019 11:32

periph_common/rtc: add rtc_tm_normalize()

fa9e906

unittests: add tests for rtc_tm_normalize()

90a8a96

benpicco force-pushed the rtc_sanitize branch from 1858467 to b1724a7 Compare September 12, 2019 09:32

maribu approved these changes Sep 12, 2019

View reviewed changes

maribu merged commit c262770 into RIOT-OS:master Sep 12, 2019

benpicco deleted the rtc_sanitize branch September 12, 2019 11:05

kb2ma added this to the Release 2019.10 milestone Sep 16, 2019

benpicco mentioned this pull request Sep 30, 2019

cpu/saml21: Make Low-Power SRAM available to programs #11486

Merged

benpicco mentioned this pull request Oct 26, 2019

cpu/lpc2387: enable RTC on rtc_init() #12580

Merged

Conversation

benpicco commented Apr 17, 2019 • edited by maribu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

maribu commented Jul 24, 2019

Uh oh!

benpicco commented Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjmolinas commented Aug 14, 2019

Uh oh!

fjmolinas commented Aug 14, 2019

Uh oh!

benpicco commented Aug 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjmolinas commented Sep 12, 2019

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maribu commented Sep 12, 2019

Uh oh!

benpicco commented Sep 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

benpicco commented Apr 17, 2019 •

edited by maribu

Loading

benpicco commented Aug 13, 2019 •

edited

Loading

benpicco commented Aug 14, 2019 •

edited

Loading

benpicco commented Sep 12, 2019 •

edited

Loading