Fix broken rtic-monotonic stm32 behavior #958
Conversation
|
Would you care to explain? I think this does not work; you are now using cc1 twice. cc1 is currently used for the timeout interrupt, and cc2 is used for the half period overflow interrupt. I don't think you can use cc1 for both purposes. |
|
The way this timer works requires two compare units, one for scheduled interrupts, and one for the half way overflow, to get rid of a race condition. I'm not sure what you mean with cc3, this implementation only uses cc1 and cc2. |
|
Are you 100% sure? Zero doubts? |
|
80%. As chiptool use macros a lot instead of generating pure Rust you can't be always sure. |
Yes, the math for calculating the offsets of the ccr registers is 0 based even though it starts at ccr1, it's rather annoying. And stm32-pac doesn't differentiate between the timers that have 4 ccr's and and the ones that have 2, so the panic to catch an invalid index doesn't work. |
|
Then the proper fix is to change the ccr(2) to ccr(0). The rest is important and should not be touched. |
| @@ -294,21 +294,13 @@ macro_rules! make_timer { | |||
| } | |||
|
|
|||
| fn clear_compare_flag() { | |||
| $timer.sr().modify(|r| r.set_ccif(1, false)); | |||
| // Do nothing, because the flags are cleared below in on_interrupt | |||
There was a problem hiding this comment.
This is incorrect, this flag is for the user triggered interrupt, the other one is for the half-overflow interrupt. Please do not remove this line.
rtic-monotonics/src/stm32.rs
Outdated
| $timer.ccr(2).write(|r| r.set_ccr(($bits::MAX - ($bits::MAX >> 1)).into())); | ||
| $timer.dier().modify(|r| r.set_ccie(2, true)); | ||
| $timer.ccr(1).write(|r| r.set_ccr(($bits::MAX - ($bits::MAX >> 1)).into())); | ||
| $timer.dier().modify(|r| r.set_ccie(1, true)); |
There was a problem hiding this comment.
Modify to ccr(0). ccr(1) is already occupied.
| fn disable_timer() { | ||
| $timer.dier().modify(|r| r.set_ccie(1, false)); | ||
| } | ||
|
|
There was a problem hiding this comment.
Please leave this. It is used by ccr(1). It isn't absolutely crucial, but it does serve its purpose. It reduces the amount of interrupts fired in idle.
rtic-monotonics/src/stm32.rs
Outdated
| if $timer.sr().read().ccif(2) { | ||
| $timer.sr().modify(|r| r.set_ccif(2, false)); | ||
| if $timer.sr().read().ccif(1) { | ||
| $timer.sr().modify(|r| r.set_ccif(1, false)); |
There was a problem hiding this comment.
Use ccr(0) instead. ccr(1) is already used.
I can confirm that it works, I have it running on an STM32F072CB right in front of me. From what I understand, the way the code works currently is that there are 2 interrupts fired per period, one for the halfway point (the CCIF flags this), and one for the overflow (the UIF flags this, it doesn't require a compare unit). I don't see CCR1 being used anywhere. Unless I'm misunderstanding there's only one compare unit being used in the original code, and it's the same deal with the code I updated. On the removal of disable timer, as it was implemented on_interrupt could randomly panic if it was paused before a half period and unpaused after both interrupts had fired, because the full period if statement comes first in the on_interrupt method. Also, from reading the doc comments on that function it seems like the timer is still supposed to run, just not give interrupts? Which isn't possible in our case, as we rely on the interrupts to track overflows since it's only a 16-bit timer. I tried making that function change cen to enable and disable the timer itself, but that made my defmt timestamps stop counting after a couple ticks. What do you mean by user triggered interrupt? I'm more than happy to add the code back in for clearing the flag, but we clear both flags used for time keeping in the on_interrupt function so as far as I can tell it would only hinder us to clear them there, as it's called before on_interrupt is. I agree that using CCR0 makes the most sense, so I'll change that. |
|
Also, what is the set_compare function supposed to do? Right now it just changes the half period interrupt's timing, which doesn't seem like a useful function. Is it supposed to change the period of the timer? In which case that should be changed to update the prescale value instead of a CCR register. |
| @@ -290,25 +290,17 @@ macro_rules! make_timer { | |||
| 0 | |||
| }; | |||
|
|
|||
| $timer.ccr(1).write(|r| r.set_ccr(val.into())); | |||
| $timer.ccr(0).write(|r| r.set_ccr(val.into())); | |||
There was a problem hiding this comment.
Don't change this one, this is the user triggered interrupt I'm talking about. It gets set whenever the user says "wait 3 ms", so that an interrupt wakes up the program when 3ms are over.
It does not. It only changes the half way interrupt because you made it do that :) it operates the other comparator. The function tells the timer to wake up at a specific time. The half way interrupt should never get changed, and didn't, bevor your changes. |
|
We need that second compare because we support async waiting; in async, you cannot busy wait. You need a wakeup interrupt. And the other two interrupts are not enough; they trigger roughly every 50 ms or so, but we support microsecond waits. |
|
Hi. From the issue outlined it sounds to me that the only change needed it |
Correct. That is also my understanding. Should we continue this branch or should I create a change myself? |
|
@korken89 I believe this can be closed, then |
This fixes #956, and also another seemingly longstanding bug where stm32 the monotonic timer would use compare unit 3, even when it didn't exist.