Skip to content

cpu/atmega_common: implement emulated RTC support#13394

Merged
miri64 merged 3 commits intoRIOT-OS:masterfrom
benpicco:atmega-rtc
Mar 3, 2020
Merged

cpu/atmega_common: implement emulated RTC support#13394
miri64 merged 3 commits intoRIOT-OS:masterfrom
benpicco:atmega-rtc

Conversation

@benpicco
Copy link
Contributor

@benpicco benpicco commented Feb 17, 2020

Contribution description

This re-purposes the Real Time Timer of the ATmega family as a Real Time Clock.
It is not as good as a real RTC as it's operation is entirely software based.
The RTT warps around too quickly to act as a counter, but it will act as a 1 Hz clock to increment the RTC counter.

This means the clock will not update in situations where the interrupt is not run (e.g. bootloader) or is delayed (e.g. disabled interrupts).
But it ensures compatibility with applications that rely on an RTC and the discussion in #8842 showed there is interest in this.

To mitigate being reset through reboot, the counter data is kept in the .noinit section.
There it survived at least the bootloader of the avr-rss2.

To prevent returning severely corrupted data on cold boot, a new function rtc_tm_valid() is introduced.
It checks if the struct tm would be normalized to verify the data is valid.
It might be useful for other RTC implementations as well.

Testing procedure

Run tests/periph_rtc on a supported board (atmega256rfr2-xpro, avr-rss2, derfmega128, derfmega256, mega-xplained, microduino-corerf).

The rtc command in examples/default should also work as expected.

Issues/PRs references

Alternative to #8842

@benpicco benpicco added Platform: AVR Platform: This PR/issue effects AVR-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Feb 17, 2020
@benpicco
Copy link
Contributor Author

While this is nice and generic, now that I'm thinking about it, we don't need to pull in the whole RTT abstraction to just have a timer counting to 32.
The original idea was that maybe we could use RTT and RTC at the same time, but as the RTC already claims the RTT alarm for it's own, this is not possible.

@benpicco
Copy link
Contributor Author

Just using Timer 2 directly shaves off 658 bytes of ROM for tests/periph_rtc:

before:

   text	   data	    bss	    dec	    hex	filename
  11106	    590	   1022	  12718	   31ae	/home/benpicco/dev/RIOT/tests/periph_rtc/bin/avr-rss2/tests_periph_rtc.elf

after:

   text	   data	    bss	    dec	    hex	filename
  10454	    596	   1010	  12060	   2f1c	/home/benpicco/dev/RIOT/tests/periph_rtc/bin/avr-rss2/tests_periph_rtc.elf

The RTC also lost that slight drift probably caused by repeatedly setting the RTT alarm - it's much simpler now.

@benpicco benpicco requested a review from gschorcht February 25, 2020 19:11
@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 Feb 25, 2020
@bergzand bergzand added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 26, 2020
@bergzand
Copy link
Member

Retriggered so this doesn't accidentally get stuck on #13481

Copy link
Contributor

@gschorcht gschorcht left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Unfortunatly, I don't have a board with 32k XTAL to test it.

@gschorcht gschorcht added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Feb 26, 2020
@fjmolinas
Copy link
Contributor

Tested on atmega256rfr2-xpro:

2020-02-28 16:33:26,975 # RIOT RTC low-level driver test
2020-02-28 16:33:26,981 # This test will display 'Alarm!' every 2 seconds for 4 times
2020-02-28 16:33:26,984 #   Setting clock to   2011-12-13 14:15:57
2020-02-28 16:33:26,988 # Clock value is now   2011-12-13 14:15:57
2020-02-28 16:33:26,992 #   Setting alarm to   2011-12-13 14:15:59
2020-02-28 16:33:26,995 #    Alarm is set to   2011-12-13 14:15:59
2020-02-28 16:33:26,996 # 
2020-02-28 16:33:28,984 # Alarm!
2020-02-28 16:33:30,985 # Alarm!
2020-02-28 16:33:32,985 # Alarm!
2020-02-28 16:33:34,985 # Alarm!

@fjmolinas fjmolinas added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Feb 28, 2020
@fjmolinas
Copy link
Contributor

All green, but I haven't followed the discussion to know which alternative ( this or #8842) should be preferred, @roberthartung @maribu @gschorcht thoughts?

@gschorcht
Copy link
Contributor

gschorcht commented Feb 28, 2020

I haven't read the complete discussion in PR #8842 which tried to provided RTC as well as RTT support. RTT support was added with PR #12815. A logical conclusion is to add the RTC support now.

If I'm right, the most important point in the discussion in PR #8842 was the question, whether RTC couldn't reuse the RTT and whether a common wrapper could be used for that. According to @benpicco the reason why wan't possible, was that the RTT uses a prescaler of 32 by default and the resulting clock frequency would be 1024 Hz.

I didn't think about it til now, but now the following question arises:
The prescaler is determined from RTT_FREQUENCY which can be overridden. RTC will use the same hardware timer but with a prescaler of 256. Therefore, RTT and RTC can't be used at the same time. Wouldn't it be possible to override RTT_FREQUENCY with 256 so that RTT is initialized with a prescaler of 128? The exclusion of the use of periph_rtt and periph_rtc could be by the definition of pseudomodules.

@benpicco
Copy link
Contributor Author

benpicco commented Feb 28, 2020

I didn't think about it til now, but now the following question arises:
The prescaler is determined from RTT_FREQUENCY which can be overridden. RTC will use the same hardware timer but with a prescaler of 256. Therefore, RTT and RTC can't be used at the same time. Wouldn't it be possible to override RTT_FREQUENCY with 256 so that RTT is initialized with a prescaler of 128? The exclusion of the use of periph_rtt and periph_rtc could be by the definition of pseudomodules.

The timer would still be only 8 bit. That's an 'emulated' RTT too, there are no long running counters. So there is no benefit in stacking abstractions on top of each other.

@benpicco
Copy link
Contributor Author

benpicco commented Mar 3, 2020

@fjmolinas #8842 has been abandoned, now this PR provides a new approach based on the new RTC helper functions, which makes it much smaller.

benpicco added 3 commits March 3, 2020 16:26
Add a function to verify all members of a struct tm are within
the valid range.
This implements a basic Real Time Clock based on TIM2.

As the timer is too fast and wraps around after just 8 bits, it is
not used directly. Instead TIM2 is responsible for providing a 1 Hz
tick by generating an alarm every second.

The current time data is kept in the `.noinit` section, so it will survive
a reboot, but the clock will not be updated while the bootloader runs, so
expect inaccuracies.
Every ATmega board that can run an Real Time Timer can also run
an emulated Real Time Clock.

Got all supported boards by adding `FEATURES_REQUIRED += arch_8bit`
to `tests/periph_rtt` and running

    sed -i 's/FEATURES_PROVIDED += periph_rtt/
              FEATURES_PROVIDED += periph_rtc\n
              FEATURES_PROVIDED += periph_rtt/g'

on them.
@fjmolinas
Copy link
Contributor

@fjmolinas #8842 has been abandoned, now this PR provides a new approach based on the new RTC helper functions, which makes it much smaller.

Yep, I'm fine with it just wanted to make sure not to step over other work.

Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

ACK!

@miri64 miri64 merged commit e4f27b4 into RIOT-OS:master Mar 3, 2020
@benpicco benpicco deleted the atmega-rtc branch March 3, 2020 18:08
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: AVR Platform: This PR/issue effects AVR-based platforms Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants