Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cpu/fe310: RTC: use rtc_mktime() #14878

Merged
merged 1 commit into from
Sep 25, 2020
Merged

Conversation

benpicco
Copy link
Contributor

Contribution description

Use RTC helper functions instead of libc functions.
This gives us y2038 safety by the extended epoch and saves a good chunk of memory:

picolibc mktime():

   text	   data	    bss	    dec	    hex	filename
  15048	    520	   2504	  18072	   4698	tests/periph_rtc/bin/hifive1/tests_periph_rtc.elf

rtc_mktime():

   text	   data	    bss	    dec	    hex	filename
   7632	     40	   2452	  10124	   278c	tests/periph_rtc/bin/hifive1/tests_periph_rtc.elf

Testing procedure

tests/periph_rtc should still work.
(untested, I don't have the hardware)

Issues/PRs references

#13277

@benpicco benpicco added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: cpu Area: CPU/MCU ports labels Aug 27, 2020
@benpicco 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 27, 2020
Use RTC helper functions instead of libc functions.
This gives us y2038 safety by the extended epoch and saves
a good chunk of memory:

picolibc mktime():

   text	   data	    bss	    dec	    hex	filename
  15048	    520	   2504	  18072	   4698	tests/periph_rtc/bin/hifive1/tests_periph_rtc.elf

rtc_mktime():

   text	   data	    bss	    dec	    hex	filename
   7632	     40	   2452	  10124	   278c	tests/periph_rtc/bin/hifive1/tests_periph_rtc.elf
@aabadie
Copy link
Contributor

aabadie commented Sep 25, 2020

Just tested on hifive1b and the test application is failing:

RIOT RTC low-level driver test
This test will display 'Alarm!' every 2 seconds for 4 times
  Setting clock to   2020-02-28 23:59:57
Clock value is now   2020-02-29 00:00:05

The time is not set correctly.

@aabadie
Copy link
Contributor

aabadie commented Sep 25, 2020

It's also completely broken on master...

@aabadie
Copy link
Contributor

aabadie commented Sep 25, 2020

I found the culprit: 442b11d from #14741

Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this patch:

diff --git a/cpu/fe310/include/periph_cpu.h b/cpu/fe310/include/periph_cpu.h
index dcc468fce1..18d7af039f 100644
--- a/cpu/fe310/include/periph_cpu.h
+++ b/cpu/fe310/include/periph_cpu.h
@@ -173,7 +173,7 @@ typedef struct {
 #define RTT_MIN_FREQUENCY   (1U)                    /* in Hz */
 
 #ifndef RTT_FREQUENCY
-#define RTT_FREQUENCY       (RTT_MAX_FREQUENCY)     /* in Hz */
+#define RTT_FREQUENCY       (RTT_MIN_FREQUENCY)     /* in Hz */
#

I'm able to have periph_rtc to work (it reverts the situation to what it was before #14741 was merged).
So it's not worse than master (it fails badly) but it also works the same as master under the same conditions, when master was still working.

I also confirm the code size decrease compared to master (from 21419B to 12102B for tests/periph_rtc).

ACK

@aabadie aabadie merged commit a147343 into RIOT-OS:master Sep 25, 2020
@benpicco benpicco deleted the cpu/fe310/rtc_mktime branch September 25, 2020 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants