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 MonoTimer's frequency #56

Merged
merged 1 commit into from
Mar 7, 2020
Merged

Fix MonoTimer's frequency #56

merged 1 commit into from
Mar 7, 2020

Conversation

teskje
Copy link
Collaborator

@teskje teskje commented Jan 29, 2020

The MonoTimer internally counts the cycles of the Cortex core. So unless I am missing something, its frequency is HCLK, not SYSCLK, according to the reference manual. I also verified this as true on an STM32F303VC.

Without this fix MonoTimer is only usable for timing measurements if HCLK equals SYSCLK.

@teskje
Copy link
Collaborator Author

teskje commented Jan 30, 2020

Looks like the CI jobs got killed while trying to set up the build cache. Not sure what went wrong there.

@dfrankland
Copy link
Member

Sorry, I am totally behind on this. I am actually abroad right now, but I will take a look as soon as I return from my travels (about 1 week).

This commit sets `MonoTimer`'s frequency to HCLK.

The `MonoTimer` internally counts the cycles of the Cortex core.
The core's clock is HCLK, but SYSCLK was used for the timer's frequency
before this commit.
@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Mar 7, 2020

Looks like the CI jobs got killed while trying to set up the build cache. Not sure what went wrong there.

Yeah, I stumbled on this issue today as well. The cache on the travis instance is corrupted. The only way to fix it, is to disable the caching in cargo right now.

@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Mar 7, 2020

I rebased it, as the travis fix is already in master.

@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Mar 7, 2020

The MonoTimer internally counts the cycles of the Cortex core. So unless I am missing something, its frequency is HCLK, not SYSCLK, according to the reference manual. I also verified this as true on an STM32F303VC.

Without this fix MonoTimer is only usable for timing measurements if HCLK equals SYSCLK.

I have difficulties finding the reference about this in the manual. Can you point me to the direction?

The HCLK (Hardware Clock) is the clock, with which the microprocessor is initially clocked. The SYSCLK (system clock) should be the one, which does clock the core and which is generated via PLLs from the HCLK. As this implements a montonic timer of the core, it should use the system clock as is, if I understand it correctly.

@teskje
Copy link
Collaborator Author

teskje commented Mar 7, 2020

The reference I've been looking at is the RM0316. Section 9.2 has a couple clock trees for the different devices.

There you can see that the SYSCLK is indeed what comes out of the PLL. However, what goes into the PLL is not the HCLK, but the HSI (internal oscillator) or the HSE (external oscillator). The HCLK is the clock of the AHB (calculated from the SYSCLK via the AHB prescaler), to which, as the clock trees show, the core is connected.

I haven't looked into the references for other STM32F3 chips though. My assumption was that they would all behave similarly.

@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Mar 7, 2020

Oh I find these clock-trees hard to read. You are right, the sysclock goes through the AHB prescaler before getting converted to the HCLK. This is referenced as FHCLK in the clock tree, if i'm reading this correctly. Is this correct?

Looking at stm32f1xx-hal they use the sysclk as well.

This makes it pretty clear http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0337e/BEIEFGDB.html

@teskje
Copy link
Collaborator Author

teskje commented Mar 7, 2020

I agree, these graphs are hard to read, because the clocks on these boards are quite complex. In this case you can just search for the word "core". HCLK and FHCLK both go to the core. Looks like FHCLK is what's referred to as FCLK in your link. But they have the same frequency anyway.

Looking at the reference for STM32F103 devices, RM0008 the core also gets the HCLK there, so they probably have the same bug in the stm32f1xx-hal then. I assume the MonoTimer implementations in most hal crates have been more or less copied from early japaric code and nobody ever used it enough to find the bug.

@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Mar 7, 2020

I assume the MonoTimer implementations in most hal crates have been more or less copied from early japaric code and nobody ever used it enough to find the bug.

Yeah that's what i'm guessing as well. I did not use this yet, either. Just because I'm curious, do you have an example at hand, where it would be used? I have some ideas, but this would help me to understand it.

Either way, LGTM!

@Sh3Rm4n Sh3Rm4n self-requested a review March 7, 2020 21:55
@teskje
Copy link
Collaborator Author

teskje commented Mar 7, 2020

I use it from time to time for one-off performance measurements during development. For example, I used it to verify whether my timer was configured correctly for my target frequency. This was also how I stumbled over this bug.

@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Mar 7, 2020

This seems reasonable. Thank you for fixing this.

Before someone else stumbles upon this, and gets confused by the wrong feedback, ill merge this now.

@Sh3Rm4n Sh3Rm4n merged commit 403643c into stm32-rs:master Mar 7, 2020
@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Mar 7, 2020

Well I forgot to ask for a changelog entry 😅 If you want, you can do a follow up PR, if not I'll do it myself. However I think, that this fix is important to be visible.

@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Mar 7, 2020

BTW, currently there is a bug, where the HCLK is calculated wrong if you use a division above 16 (HPRE::DIV64 and higher). This is addressed in #43

@teskje
Copy link
Collaborator Author

teskje commented Mar 7, 2020

Oh, didn't think about the changelog. I can add that tomorrow, no problem!

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.

3 participants