cpu/stm32: implement periph_timer_periodic #14391
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
b82ef02 to
2e1b835
Compare
|
samr21-xpro All boards tests/periph_timer |
|
samr21-xpro All boards tests/periph_timer_periodic |
MrKevinWeiss
reviewed
Jul 1, 2020
1d7053f to
ac6014e
Compare
c6662bc to
b05638e
Compare
maribu
reviewed
Jul 23, 2020
cpu/stm32/periph/timer.c
Outdated
Comment on lines
128
to
151
| /** | ||
| * @brief Set timer TOP to the lowest reset on match channel | ||
| */ | ||
| static void _set_arr(tim_t tim) | ||
| { | ||
| uint32_t dir = dev(tim)->DIER; | ||
| uint32_t arr = timer_config[tim].max; | ||
|
|
||
| /* find the lowest reset value */ | ||
| for (unsigned int i = 0; i < TIMER_CHANNEL_NUMOF; i++) { | ||
|
|
||
| /* skip disabled channels */ | ||
| if ((dir & (TIM_DIER_CC1IE << i)) == 0) { | ||
| continue; | ||
| } | ||
|
|
||
| uint32_t cc = TIM_CHAN(tim, i); | ||
| if (is_reset_on_match(tim, i) && cc < arr) { | ||
| arr = cc; | ||
| } | ||
| } | ||
|
|
||
| dev(tim)->ARR = arr; | ||
| } |
There was a problem hiding this comment.
Is there a reason why TIM_FLAG_RESET_ON_MATCH is allowed for multiple channels? Otherwise, I wouldn't mind adding an @pre to the API doc that at most one channel per timer may be used with TIM_FLAG_RESET_ON_MATCH.
There was a problem hiding this comment.
The documentation for TIM_FLAG_RESET_ON_MATCH says
When set on multiple channels, only the channel with the lowest match value will be reached.
so if you could have two channels with this flag and only the lower one would be reached.
Then if you disable the lower channel, the higher one would be used.
However, this is not used anywhere and with the Atmel timers (both on AVR & ARM) only channel 0 can be used as TOP value.
So yea, we could extend the API documentation here.
There was a problem hiding this comment.
Maybe we could even restrict it to a certain channel, then? E.g.:
#ifndef HAVE_TIMER_GET_PERIODIC_CHANNEL
static inline int timer_get_periodic_channel(tim_t tim)
{
(void)tim;
return 0;
}
#endif /* HAVE_TIMER_GET_PERIODIC_CHANNEL */There was a problem hiding this comment.
Care to create a PR limiting the use of TIM_FLAG_RESET_ON_MATCH by extending the doc?
There was a problem hiding this comment.
Can I just piggy-back it on this PR?
There was a problem hiding this comment.
I would prefer a separate PR, as it is formally an API change.
There was a problem hiding this comment.
I moved it to a separate PR.
This doesn't change anything - TIM_FLAG_RESET_ON_MATCH is already documented on it's own.
There was a problem hiding this comment.
With #14619 in, you should be able to simplify the code here.
There was a problem hiding this comment.
Alright!
Still untested though
|
Feel free to squash |
d67c160 to
c70b272
Compare
|
Squashed. |
0d4cdb2 to
c70b272
Compare
maribu
reviewed
Aug 7, 2020
cpu/stm32/periph/timer.c
Outdated
Comment on lines
190
to
162
| if (flags & TIM_FLAG_RESET_ON_SET) { | ||
| /* setting COUNT gives us an interrupt on all channels */ | ||
| set_ignore_irq(tim); | ||
| dev(tim)->CNT = 0; | ||
| } |
There was a problem hiding this comment.
Have you tried if wrapping this is irq_disable() ... irq_restore() and clearing the interrupt flags from the status registers works? IMO, that would be favorable over conditionally ignoring IRQs
There was a problem hiding this comment.
Thank you, that simplifies things quite a bit.
|
|
|
Yea I lowered the cycle duration without adjusting the testcase - fixed. |
maribu
approved these changes
Aug 9, 2020
Reduce CYCLE_MS to 25 ms so the period also fits into a 16 bit 1 MHz timer.
Seems like the Interrupt flag for a Capture/Compare channel gets set when - the CC-value is reached - the timer resets before the CC value is reached. We only want the first event and ignore the second one. Unfortunately I did not find a way to disable the second event type, so it is filtered in software. That is we need to - ignore the CC-interrupts when the COUNT register register is reset - ignore the CC-interrupts > TOP value/ARR (auto-reload register)
7e57e6b to
29f83a6
Compare
fjmolinas
reviewed
Oct 7, 2020
Comment on lines
+19
to
+21
| # test should run 10 cycles with 25ms each | ||
| assert (end - start) > 0.25 | ||
| assert (end - start) < 0.35 |
There was a problem hiding this comment.
On atmega (arduino-uno and arduino-mega2560) it takes 0.3523.... can we simply increase the margin to fix the test or might something else be going on? Found while running RIOT-OS/Release-Specs#192.
There was a problem hiding this comment.
Yea pushing it to 40 ms won't hurt much if it helps with slow UARTs.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
This implements
timer_set_periodic()for STM32.Seems like the Interrupt flag for a Capture/Compare channel gets set when
We only want the first event and ignore the second one. Unfortunately I did
not find a way to disable the second event type, so it is filtered in software.
That is we need to
CNTregister register is resetARR(auto-reload register)Testing procedure
Verify that
tests/periph_timer&periph_timer_periodicstill work.tested on
nucleo-l031k6,weact-f411ce&stm32f429i-discoIssues/PRs references