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

Fixes hclk miscalculation #85

Merged
merged 1 commit into from
Jul 17, 2019
Merged

Conversation

thalesfragoso
Copy link
Member

Closes #84 .

@kyrias
Copy link

kyrias commented Jul 16, 2019

This looks correct to me.

Though I would sort of rather use some kind of abstraction on top of HPREW instead, so instead of calculating hpre_bits, we would make a HPREW value and then have some function that converts a HPREW to a divisor which is then used to calculate the hclk. Advantage of that would be that it's much easier to write the correct code that way, and make the correct code be obviously correct, than when manually calculating the bit patterns.

This should be possible to do with const functions, which would allow more compile-time validation of the values even, hm.

@thalesfragoso
Copy link
Member Author

I don't think const-fn is there yet. I like the idea of improving readability to find mistakes more easily, but I am not sure if creating a function here is the best for now, since it took only 4 lines to fix the issue. Maybe when we get more flexibility in stable const-fn we can make some refactoring.

@TheZoq2, @therealprof, what do you think ? Can we merge this for now or try to improve ?

@therealprof
Copy link
Member

@thalesfragoso I think this is good for now.

One question out of curiosity, any reason why you didn't move out the common parts from the branches?

like

      let hclk = sysclk / (1 << hpre_bits - if hpre_bits >= 0b1100 { 0b0110 } else { 0b0111 });

@thalesfragoso
Copy link
Member Author

@thalesfragoso I think this is good for now.

One question out of curiosity, any reason why you didn't move out the common parts from the branches?

like

      let hclk = sysclk / (1 << hpre_bits - if hpre_bits >= 0b1100 { 0b0110 } else { 0b0111 });

Not really, I guess I prefer the former aesthetics, but no strong feelings about that, I can change it to the oneliner if you prefer.

@therealprof
Copy link
Member

@thalesfragoso I don't care, just curious. ;)

Copy link
Member

@therealprof therealprof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@therealprof therealprof merged commit 0588013 into stm32-rs:master Jul 17, 2019
@thalesfragoso thalesfragoso deleted the hclk-fix branch July 19, 2020 02:51
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.

hclk calculated incorrectly
3 participants