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/stm32/periph/timer: don't stop counter #19263

Merged
merged 1 commit into from
Feb 9, 2023

Conversation

Enoch247
Copy link
Contributor

@Enoch247 Enoch247 commented Feb 8, 2023

Contribution description

From the git comment msg:

If a timer's channel was set with a really small realtive duration from now, such that it would be missed (underflowed), the driver would stop the timer, potentially causing missed ticks. It was stopped to ensure that the channel's output-compare register could be set to the current counter value, before re-enabling the timer's counter. This is a condition that will ensure that the underflow won't happen again and the interrupt will fire, at the cost of losing some ticks for very high speed clocks.

This patch replaces the logic that stopped the timer. Instead it uses a register provided by the timer hardware to trigger timer interrupts via software.

Testing procedure

  1. do
    $ cd tests/periph_timer_short_relative_set
    $ make BOARD=nucleo-f303ze flash term
  2. follow prompts to run test
  3. observe all tests pass
  4. apply patch below to break test
  5. rerun test
  6. observe test fails, so new method is doing its job
patch to intentionally break test
diff --git a/cpu/stm32/periph/timer.c b/cpu/stm32/periph/timer.c
index 64a6f3a656..7078c46ab4 100644
--- a/cpu/stm32/periph/timer.c
+++ b/cpu/stm32/periph/timer.c
@@ -177,7 +177,7 @@ int timer_set(tim_t tim, int channel, unsigned int timeout)
     if (value > timeout) {
         /* time till timeout is larger than requested --> timer already expired
          * ==> let's make sure we have an IRQ pending :) */
-        dev(tim)->EGR |= (TIM_EGR_CC1G << channel);
+        //dev(tim)->EGR |= (TIM_EGR_CC1G << channel);
     }

Issues/PRs references

  • none known

If a timer's channel was set with a really small realtive duration from
now, such that it would be missed (underflowed), the driver would stop
the timer, potentially causing missed ticks. It was stopped to ensure
that the channel's output-compare register could be set to the current
counter value, before re-enabling the timer's counter. This is a
condition that will ensure that the underflow won't happen again and the
interrupt will fire, at the cost of losing some ticks for very high
speed clocks.

This patch replaces the logic that stopped the timer. Instead it uses a
register provided by the timer hardware to trigger timer interrupts via
software.
@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Feb 8, 2023
@benpicco benpicco requested a review from maribu February 8, 2023 22:24
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 8, 2023
@riot-ci
Copy link

riot-ci commented Feb 8, 2023

Murdock results

✔️ PASSED

289814e cpu/stm32/periph/timer: don't stop counter

Success Failures Total Runtime
6851 0 6851 10m:49s

Artifacts

@maribu
Copy link
Member

maribu commented Feb 9, 2023

Nice one!

@maribu
Copy link
Member

maribu commented Feb 9, 2023

Both tests/periph_timer and tests/periph_timer_short_relative_set still pass on STM32F7 and STM32F4. tests/periph_timer_short_relative_set also still passes on STM32F1.

There is an issue (already in master) with the STM32F1 in tests/periph_timer; it appears that there is one hardware timer less available then configured. I'm not sure if the STM32F103C8T6 on my Bluepill is genuine. I bought it prior to the chip shortage when there were still few and extremely imperfect clones, and the Bluepill board otherwise works like a charm, so I think there may actually be a misconfiguration in the board setup. Let's see what the datasheet says.

In any case: All testing suggests that this indeed works and is a clearly superior solution to the stopping the clock workaround.

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.

bors merge

@maribu
Copy link
Member

maribu commented Feb 9, 2023

Out of curiosity: Did the briefly stopping the timer indeed cause any issues?

The nRF5x timer driver is using the same work around. The reasoning is that missing a timeout and waiting for a full period is worse than loosing a timer tick or two whenever one sets a timeout that is to short to work reliably. This is based on the assumption that short relative timeouts (like less than three ticks on a 1 MHz clock) are set rarely and clock drift on the high frequency clock is so high anyway, that for accurate time keeping periph_rtt is used anyway.

It would be interesting to know if this reasoning is not correct for all use cases.

@bors
Copy link
Contributor

bors bot commented Feb 9, 2023

Build succeeded:

@bors bors bot merged commit 0607806 into RIOT-OS:master Feb 9, 2023
@Enoch247
Copy link
Contributor Author

Enoch247 commented Feb 9, 2023

Out of curiosity: Did the briefly stopping the timer indeed cause any issues?

The nRF5x timer driver is using the same work around. The reasoning is that missing a timeout and waiting for a full period is worse than loosing a timer tick or two whenever one sets a timeout that is to short to work reliably. This is based on the assumption that short relative timeouts (like less than three ticks on a 1 MHz clock) are set rarely and clock drift on the high frequency clock is so high anyway, that for accurate time keeping periph_rtt is used anyway.

It would be interesting to know if this reasoning is not correct for all use cases.

No, I did not encounter any real world problem. I think your reasoning is in-line with what most users would expect too.

I am investigating adding more interrupt handling to the periph_qdec module. I want to add the equivalent of something like timer_set for qdec. So I was studying how the timer module handles small relative offsets and happened to notice the opportunity to improve it.

@maribu
Copy link
Member

maribu commented Feb 9, 2023

Good to know that the work around didn't cause immediate issues.

Thx for the PR :) This indeed way superior compared to the work around used before :)

@MrKevinWeiss MrKevinWeiss added this to the Release 2023.04 milestone Apr 25, 2023
@Enoch247 Enoch247 deleted the fix-stm32-timer-driver branch May 23, 2023 16:08
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants