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

Fix: use correct clock division factor in DelayMs() / GetMs() #652

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ndonald2
Copy link
Contributor

Summary

The denominator for dividing a timer's frequency for DelayMs()/GetMs() is completely wrong and in fact currently produces tick counts much shorter than DelayUs()/GetUs().

Example with previous code before change:

GetFreq()  == 240000000 (i.e. 2 x PCLK1 freq)

DelayUs(100)
ticks to delay = 100 * (240000000 / 1000000) 
               = 100 * 240
               = 24000 ticks

DelayMs(100)
ticks to delay = 100 * (240000000 / 100000000) 
               = 100 * 2.4 
               = 100 * 2 (floored due to int division)
               = 200 ticks

This should make the problem obvious: DelayMs(100) should not result in a delay of fewer timer ticks than DelayUs(100) - the denominator is 5 orders of magnitude too big!

The Easy Fix (included here)

Easy fix: change the denominator to the correct value (1000)

The "Better" Fix (for future consideration)

There is still misleading/incorrect code here in some cases, as a given timer configured by the developer may be using a different prescaler and/or may overflow at 16-bits making requested delays impossible and/or making the ms/us getter methods rather useless/inaccurate.

It may instead be a better choice to move these calculations/methods out of TimerHandle and directly into System since there we at least know that the TIM2 instance is using the full PCLK speed with no prescaler.

However removing these methods completely from the interface would be a breaking change.

Copy link

Test Results

150 tests  ±0   150 ✅ ±0   0s ⏱️ ±0s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 201235f. ± Comparison against base commit bd13385.

@ndonald2 ndonald2 changed the title Fix: use correct clock division factor in DelayMs() / GetMs() (totally broken right now) Fix: use correct clock division factor in DelayMs() / GetMs() Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant