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

Use max frequency and round down when calculating PCLK1 #365

Merged
merged 1 commit into from
Oct 24, 2021

Conversation

jasLogic
Copy link
Contributor

When pclk1 is not set use hclk or 36 MHz to stay below the max frequency of PCLK1.
This is needed if you set the hclk to a value > 36 MHz and don't specify a value for pclk1. Then the prescaler would be set to one automatically and the maximum value would be exceeded.

Also round up when calculating the prescaler so that the given pclk1 value is not exceeded.
This is important because you could have configurations where the prescaler would still be too small to stay below 36 MHz. For example when hclk = 48 MHz and pclk1 = 36 Mhz than hclk / pclk1 = 1 (integer division is rounded down) and the prescaler would be set to one.

@TheZoq2
Copy link
Member

TheZoq2 commented Oct 13, 2021

Thanks for the PR, seems like a great change that should prevent some weird behaviour!

Since #337 is still open, I think there will be a conflict with it, @burrbull does it make sense to move this change over there and merge the whole thing in one go? (I guess I also still need to have a look at that PR :D)

@TheZoq2 TheZoq2 added Breaking change hacktoberfest-accepted Not spam and submitted in october labels Oct 13, 2021
@burrbull
Copy link
Member

Since #337 is still open, I think there will be a conflict with it, @burrbull does it make sense to move this change over there and merge the whole thing in one go? (I guess I also still need to have a look at that PR :D)

I can update my PR after this be merged or do as it suits you best.

@jasLogic
Copy link
Contributor Author

Btw: Should I also add this to the changelog? The commit doesn't change the API but I just thought that it might round specified frequency down instead of up (like it did before) which could break a user program.

@TheZoq2
Copy link
Member

TheZoq2 commented Oct 14, 2021

I can update my PR after this be merged or do as it suits you best.
Let's merge this now then :)

Btw: Should I also add this to the changelog? The commit doesn't change the API but I just thought that it might round specified frequency down instead of up (like it did before) which could break a user program.

It should certainly go in the changelog in my opinion. The question is if it is a breaking change. I suppose technically it is since it changes the way the code behaves, but I don't think anyone (/enough people) would be relying on the previous behavior for it to warrant its own release

When pclk1 is not set use hclk or 36 MHz to stay below the max
frequency of PCLK1.
Also round up when calculating the prescaler so that the given pclk1
value is not exceeded.
@jasLogic
Copy link
Contributor Author

Done.

Also maybe rounding down when calculating frequencies should be done more generally on other clocks too? I think most user specified frequencies are probably maximum frequencies and not minimum.

@TheZoq2
Copy link
Member

TheZoq2 commented Oct 24, 2021

Also maybe rounding down when calculating frequencies should be done more generally on other clocks too? I think most user specified frequencies are probably maximum frequencies and not minimum.

I'm not sure, I think my personal assumption is that you set a minimum frequency because you want to hit some performance target but maybe not

@TheZoq2 TheZoq2 merged commit 25702c0 into stm32-rs:master Oct 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Not spam and submitted in october
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants