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/sam3: added RTT driver #7583

Merged
merged 4 commits into from Mar 12, 2020
Merged

Conversation

haukepetersen
Copy link
Contributor

Found and finished this code in one of my branches...

@haukepetersen haukepetersen added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 8, 2017
@haukepetersen haukepetersen added this to the Release 2017.10 milestone Sep 8, 2017
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.

2 minor comments. I'll see if I can test this week.

cpu/sam3/cpu.c Outdated
/* enable external oscillator */
SUPC->SUPC_CR = (SUPC_CR_KEY(SUPCKEY) | SUPC_CR_XTALSEL);
while (!(SUPC->SUPC_OSCSEL & SUPC_SR_OSCSEL)) {}

Copy link
Contributor

Choose a reason for hiding this comment

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

This extra line can be removed I think

#include "cpu.h"
#include "periph/rtt.h"

#define ENABLE_DEBUG (1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Debug should be disabled

@aabadie
Copy link
Contributor

aabadie commented Sep 13, 2017

@haukepetersen, I gave a try to this one with tests/periph_rtt: it seems to work. Not sure if it's a problem or not but I noticed a (nearly) 1s time shift for each Hello triggered. See my output below:

2017-09-13 13:43:40,729 - INFO # Initializing the RTT driver
2017-09-13 13:43:40,730 - INFO # [rtt] setting prescaler to 32768
2017-09-13 13:43:40,731 - INFO # Setting initial alarm
2017-09-13 13:43:40,733 - INFO # [rtt] set new alarm to trigger at 10
2017-09-13 13:43:40,735 - INFO # Done setting up the RTT, wait for many Hellos
2017-09-13 13:43:51,954 - INFO # [rtt] ISR: state is 0x00000003
2017-09-13 13:43:51,958 - INFO # [rtt] set new alarm to trigger at 20
2017-09-13 13:43:51,959 - INFO # Hello
2017-09-13 13:44:02,875 - INFO # [rtt] ISR: state is 0x00000003
2017-09-13 13:44:02,877 - INFO # [rtt] set new alarm to trigger at 30
2017-09-13 13:44:02,878 - INFO # Hello
2017-09-13 13:44:13,792 - INFO # [rtt] ISR: state is 0x00000003
2017-09-13 13:44:13,795 - INFO # [rtt] set new alarm to trigger at 40
2017-09-13 13:44:13,795 - INFO # Hello
2017-09-13 13:44:24,712 - INFO # [rtt] ISR: state is 0x00000003
2017-09-13 13:44:24,715 - INFO # [rtt] set new alarm to trigger at 50
2017-09-13 13:44:24,715 - INFO # Hello

@jnohlgard
Copy link
Member

@aabadie check the reference manual for your CPU, it could be the same problem as for kinetis where the alarm is triggered when the counter equals the alarm time and the counter increments. See #7476, #6331

@aabadie
Copy link
Contributor

aabadie commented Sep 13, 2017

Thanks for pointing me to this @gebart. It indeed makes more sense to me what behavior is expected in #6331. And I'm not the only one apparently.

@aabadie
Copy link
Contributor

aabadie commented Oct 9, 2017

@haukepetersen, any chance that you addressed the comments here before feature freeze ?
We also merge #7476 I think.

@haukepetersen
Copy link
Contributor Author

Will try. Tried to debug the behavior further last week, but so far I am completely in the blank why the RTT is to imprecise on the arduino-due. Will take more debugging...

#include "debug.h"

/* guard file to prevent file being build if no RTT was configured */
#if RTT_NUMOF
Copy link
Member

Choose a reason for hiding this comment

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

is this still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, its not. Will remove once I find time to actually get this driver working correctly...

@smlng
Copy link
Member

smlng commented Jan 15, 2018

looks like this still needs some work and testing, postpone to 2018.04.

@kaspar030 kaspar030 removed this from the Release 2018.04 milestone Apr 11, 2018
@PeterKietzmann
Copy link
Member

@haukepetersen ping

@haukepetersen
Copy link
Contributor Author

I am aware of this PR, though its not very high up my prio list...

@haukepetersen
Copy link
Contributor Author

Finally found a minute: this PR should be all good now:

  • code is cleaned up
  • clock init is fixed
  • tests/periph_rtt runs as expected

BUT note: per default, the slow clock driving the RTT is clocked by an internal 22 to 42 kHz Low Power RC Oscillator (-> see datasheet sec. 16.2). So as observed above, the timings are quite off w.r.t. to the 5s interval the examples should run at.

It turns out (at least my google search), that many arduino-due boards do not have the external 32khz oscillator equipped, which would be needed for better timings. So I left the default configuaration to use the internal RC-oscillator. Simply compile with CFLGAS=-DCLOCK_SCLK_XTAL=1 to switch to the external osciallator (if your board has it equipped...).

@haukepetersen
Copy link
Contributor Author

got hold of a due board with the external oscillator equipped:

  • the init function now waits for the prescaler to be applied
  • fixed a 1-off error when setting the alarm value

Now the output of the periph_rtt test looks like expected (building with CFLAGS=-DCLOCK_SCLK_XTAL:

2020-03-11 11:13:15,401 # main(): This is RIOT! (Version: 2020.04-devel-1110-g4113a-add_sam3_rtt)
2020-03-11 11:13:15,401 # 
2020-03-11 11:13:15,402 # RIOT RTT low-level driver test
2020-03-11 11:13:15,406 # This test will display 'Hello' every 5 seconds
2020-03-11 11:13:15,406 # 
2020-03-11 11:13:15,410 # Initializing the RTT driver
2020-03-11 11:13:15,410 # RTT now: 0
2020-03-11 11:13:15,414 # Setting initial alarm to now + 5 s (5)
2020-03-11 11:13:15,418 # Done setting up the RTT, wait for many Hellos
2020-03-11 11:13:20,410 # Hello
2020-03-11 11:13:25,408 # Hello
2020-03-11 11:13:30,409 # Hello
2020-03-11 11:13:35,410 # Hello
2020-03-11 11:13:40,411 # Hello
2020-03-11 11:13:45,409 # Hello
2020-03-11 11:13:50,410 # Hello

* @name RTT configuration
* @{
*/
#define RTT_FREQUENCY (1U) /* 1Hz */
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you default this to 1024 or 32768?

Copy link
Contributor

Choose a reason for hiding this comment

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

scratch that, need to solve this globally.

@kaspar030
Copy link
Contributor

Tested using tests/periph_rtt, works fine! I'm not entirely happy that this important fix is optional and hidden behind a global define, but at least now there's a way to use it somehow.
I guess there's no easy way to check if the XTAL is present?

@haukepetersen
Copy link
Contributor Author

I guess there's no easy way to check if the XTAL is present?

nope, not that I know of.

@haukepetersen
Copy link
Contributor Author

should I squash?

@haukepetersen
Copy link
Contributor Author

squashed.

@aabadie
Copy link
Contributor

aabadie commented Mar 11, 2020

@fjmolinas can you test this PR ? There should be an arduino-due on my desk.

Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

Tested on arduino-due. ACK.

@kaspar030 kaspar030 merged commit 7be303f into RIOT-OS:master Mar 12, 2020
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Mar 13, 2020
@leandrolanzieri leandrolanzieri added the Area: cpu Area: CPU/MCU ports label Mar 13, 2020
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: ARM Platform: This PR/issue effects ARM-based platforms State: don't stale State: Tell state-bot to ignore this issue 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.

None yet

7 participants