Skip to content

cpu/atmega_common: RTT support#12815

Merged
benpicco merged 2 commits intoRIOT-OS:masterfrom
benpicco:atmega-rtt
Dec 1, 2019
Merged

cpu/atmega_common: RTT support#12815
benpicco merged 2 commits intoRIOT-OS:masterfrom
benpicco:atmega-rtt

Conversation

@benpicco
Copy link
Contributor

@benpicco benpicco commented Nov 26, 2019

Contribution description

This is just the RTT commit split off from the RTT/RTC PR by @ZetaR60.
I made small adjustments & cleanups to make it compile with the latest master branch and made the RTT frequency configurable.

Without the RTC emulation I hope this should be less controversial.

The RTT requires a 32kHz oscillator to be present on the board.
The only boards I know where this is the case are mega-xplained, atmega256rfr2-xpro and avr-rss2. It's possible that I missed some.
#12799 will add two more.

Testing procedure

Run tests/periph_rtt.
The RTT on ATmega will only work if the board is equipped with a 32kHz crystal.

Issues/PRs references

split off from #8842 which is based on #7604

@benpicco benpicco added Type: new feature The issue requests / The PR implemements a new feature for RIOT Platform: AVR Platform: This PR/issue effects AVR-based platforms CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: cpu Area: CPU/MCU ports labels Nov 26, 2019
maribu
maribu previously requested changes Nov 26, 2019
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

Code contains a data race, see inline explanation. But trivial to fix :-)

Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

With the current implementation the returned time could be up to 255 ticks lower than correct in a race condition. See inline comment

Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

Two more suggestions to get _safe_cnt_get() race-free (at least unless I didn't overlook anything).

@maribu maribu dismissed their stale review November 29, 2019 10:28

My comments have been addressed

@maribu maribu 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 Nov 29, 2019
@maribu
Copy link
Member

maribu commented Nov 29, 2019

I'll test tonight.

@maribu
Copy link
Member

maribu commented Nov 29, 2019

(If noone does so sooner ;-))

@maribu
Copy link
Member

maribu commented Nov 29, 2019

Sorry, I'm too tired to think straight now. I'll test tomorrow.

@benpicco
Copy link
Contributor Author

Don't worry, there is no ticking clock 😆

@maribu
Copy link
Member

maribu commented Nov 30, 2019

OK. I hooked up a 32.768kHz and two 10 pF capacitors to TOSC1 and TOSC2 as pictured in the schematic in the data sheet to an ATmega1284P. Seems to work as advertised :-)

Output of tests/periph_rtt on the ATmega1284P on a breadboard
2019-11-30 23:39:57,760 # main(): This is RIOT! (Version: 2020.01-devel-1017-gdabc8-ben)
2019-11-30 23:39:57,761 # 
2019-11-30 23:39:57,794 # RIOT RTT low-level driver test
2019-11-30 23:39:57,842 # This test will display 'Hello' every 5 seconds
2019-11-30 23:39:57,843 # 
2019-11-30 23:39:57,872 # Initializing the RTT driver
2019-11-30 23:39:57,884 # RTT now: 0
2019-11-30 23:39:57,927 # Setting initial alarm to now + 5 s (5120)
2019-11-30 23:39:57,975 # Done setting up the RTT, wait for many Hellos
2019-11-30 23:40:02,877 # Hello
2019-11-30 23:40:07,877 # Hello
2019-11-30 23:40:12,877 # Hello
2019-11-30 23:40:17,878 # Hello
2019-11-30 23:40:22,878 # Hello
2019-11-30 23:40:27,878 # Hello
2019-11-30 23:40:32,878 # Hello
2019-11-30 23:40:37,878 # Hello
2019-11-30 23:40:42,878 # Hello
2019-11-30 23:40:47,878 # Hello
2019-11-30 23:40:52,878 # Hello
2019-11-30 23:40:57,878 # Hello
2019-11-30 23:41:02,879 # Hello
2019-11-30 23:41:07,879 # Hello
2019-11-30 23:41:12,879 # Hello
2019-11-30 23:41:17,879 # Hello
2019-11-30 23:41:22,879 # Hello
2019-11-30 23:41:27,879 # Hello
2019-11-30 23:41:32,879 # Hello
2019-11-30 23:41:37,879 # Hello
2019-11-30 23:41:42,879 # Hello
2019-11-30 23:41:47,880 # Hello
2019-11-30 23:41:52,880 # Hello
2019-11-30 23:41:57,880 # Hello
2019-11-30 23:42:02,880 # Hello
2019-11-30 23:42:07,880 # Hello
2019-11-30 23:42:12,880 # Hello
2019-11-30 23:42:17,880 # Hello
2019-11-30 23:42:22,880 # Hello
2019-11-30 23:42:27,880 # Hello
2019-11-30 23:42:32,881 # Hello
2019-11-30 23:42:37,881 # Hello
2019-11-30 23:42:42,881 # Hello
2019-11-30 23:42:47,881 # Hello
2019-11-30 23:42:52,881 # Hello
2019-11-30 23:42:57,881 # Hello
2019-11-30 23:43:02,881 # Hello
2019-11-30 23:43:07,881 # Hello
2019-11-30 23:43:12,881 # Hello
2019-11-30 23:43:17,882 # Hello
2019-11-30 23:43:22,882 # Hello
2019-11-30 23:43:27,882 # Hello
2019-11-30 23:43:32,882 # Hello
2019-11-30 23:43:37,882 # Hello
2019-11-30 23:43:42,882 # Hello
2019-11-30 23:43:47,882 # Hello
2019-11-30 23:43:52,882 # Hello
2019-11-30 23:43:57,882 # Hello

(I might add a follow up PR to extend the doc of the ATmega1284P on a breadboard and of the ATmega328P on a breadboard to cover this feature. And also a way how to export an environment variable (e.g. ATMEGA1284P_HAS_TIMER_OSC=1) to let the board provide periph_rtt as feature.)

@maribu maribu added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Nov 30, 2019
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

Please squash. (Also, please squash the fix for the nitpick right away.)

Comment on lines +69 to +76
/* Overflow occurred just after we read `TCNT2`
so it has overflown back to zero now */
if (cnt == 255) {
cnt = 0;
}
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: The comment should be moved inside the body of the if. (Or reword the comment, e.g."Check if overflow occurred just after reading TCNT2, in which case it will now have a value of zero and update cnt to correspondingly.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reworded.

Matthew Blue and others added 2 commits December 1, 2019 17:26
The RTT on ATmega only works if the board provides a 32kHz oscillator.
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK. The code is well written and documented, it worked as expected during testing, and I gave my best to check that the code is race free.

@benpicco
Copy link
Contributor Author

benpicco commented Dec 1, 2019

Thank you for the thorough review & suggestions w.r.t. race conditions!

@benpicco benpicco merged commit c9e9e04 into RIOT-OS:master Dec 1, 2019
@benpicco benpicco deleted the atmega-rtt branch December 1, 2019 18:31
@fjmolinas fjmolinas added this to the Release 2020.01 milestone Dec 13, 2019
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 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.

4 participants