-
Notifications
You must be signed in to change notification settings - Fork 2k
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
sam0_common: make Timer implementation common across all sam0 MCUs #11336
Conversation
Could be a good idea to test (and compare) using also |
Trying around with a saml21-xpro. I have run the bench_timers on it (but see caveats and modifications needed in #10523) but the results would now look much worse. Your branch:
Master:
|
Thank you for testing! After adding it to
The discrepancy is surprising as the initialization is still the same between the new config and the old one. I'll try to reproduce this issue so I can fix it. |
Try checking out pr #11346, that should fix the test also for that board. |
Alright! master:
this branch
it sure got worse :/ |
I found the issue! I didn't check if Now it looks like before:
|
Very good, thanks for the changes!
|
I have problems after the last commits using To test it just write a simple program that uses |
e04df01
to
8be3898
Compare
/** | ||
* @brief Setup the given timer | ||
*/ | ||
int timer_init(tim_t tim, unsigned long freq, timer_cb_t cb, void *arg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a guard in the very beginning of this function to ensure tim < TIMER_NUMOF
You can use an assert like other drivers or return an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
@fedepell Do you still have pending comment ? Or everything was addressed ? |
Murdocks reports some boards miss their periph_conf update (arduino-mkr, hamilton, etc.) |
urg, Hamilton is new but the Arduino failures are due to How does this work for |
So in C++ the elements in the initializer have to be in the same order as in the struct definition. Thanks to @kaspar030 for pointing that out! |
/* interrupt function name mapping */ | ||
#define TIMER_0_ISR isr_tc4 | ||
|
||
#define TIMER_NUMOF (sizeof(timer_config) / sizeof(timer_config[0])) | ||
/** @} */ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to define TIMER_0_MAX_VALUE for this board? (as it is 16 bit needs to be here, otherwise defaults to 32)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but it's 32 bit here.
nvm,I accidentally dropped the 16 bit timer - fixed.
@dylad : went through the code now and except that little note I put looks fine to me! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK, please squash
The currently supported SAM0 MCUs (samd21, saml21, saml1x) share the same Timer peripheral, yet each of them carries it's own copy of the Timer driver. This introduces a new timer driver that is common for all sam0 MCUs and uses structs for configuration instead of defines.
squashed. |
Here we go |
Contribution description
The currently supported SAM0 MCUs (samd21, saml21, saml1x) share the same Timer peripheral, yet each of them carries it's own copy of the Timer driver.
This introduces a new timer driver that is common for all sam0 MCUs and uses structs for configuration instead of defines.
Testing procedure
Only tested it on samr21-xpro and same54-xpro with the
timer_periodic_wakeup
example.Issues/PRs references
#11305 will also use this implementation.