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

Add support for HSE bypass and CSS #156

Merged
merged 3 commits into from
Oct 29, 2020
Merged

Conversation

Piroro-hs
Copy link
Contributor

Fix #142.

However, APIs for the same thing are vary in all stm32-rs HALs... Are there any suggestions?

  • this PR
let clocks = rcc.cfgr/* .use_hse_crystal(8.mhz()) */.use_hse_bypass(8.mhz()).enable_css().freeze();
  • f0xx
let rcc = dp.RCC.configure().hse(HSEClock::new(8.mhz(), HSEBypassMode::Bypassed)).freeze(&mut dp.FLASH);
  • f7xx
let clocks = rcc.cfgr.hse(HSEClock::new(25.mhz(), HSEClockMode::Bypass)).freeze();
  • g0xx, g4xx
let rcc = rcc.freeze(Config::pll().pll_cfg(PllConfig { max: PLLSrc::HSE_BYPASS(8.mhz()), ..Default::default() }));
  • h7xx
let ccdr = rcc.use_hse(25.mhz()).bypass_hse().freeze(pwrcfg, &dp.SYSCFG);
  • l4xx
let clocks = rcc.cfgr.hse(8.mhz(), CrystalBypass::Disable, ClockSecuritySystem::Disable).freeze(&mut flash.acr, &mut pwr);

@Sh3Rm4n Sh3Rm4n self-requested a review October 22, 2020 08:20
Copy link
Member

@Sh3Rm4n Sh3Rm4n left a comment

Choose a reason for hiding this comment

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

Hey thanks for stepping forward and trying to fix this issue.

I'm not sure if we should use a use_hse_crystal() and a use_hse_bypass(), which both accept a frequency. These options are mutually exclusive.

I would prefer use_hse().bypass_crystal() maybe? Or something similar.

Thank you for the comparison to the different stm32 APIs!
As an ad hoc solution I would prefer the one from 'h7xx'.


A bit off-topic for this PR:

But I think an API rework has to be done for the rcc pll configuration.
For example, wrong configurations are resulting into panics (which I'm trying to address in #153).

But this is not an ideal solution. I would also like to prefer an API, which lets us easily create / calculate the PLL configuration in a separate function, which potentially could be executed in a const context (when const fn has enough stable features available).


In conclusion:

For this PR, the h7xx-solution is sufficient.

But for a major API rework I would prefer the g0xx, g4xx approach, even though it looks ugly. Maybe the 'f7xx' approach as an alternative, which doesn't seem as flexible, but looks cleaner.
I'll have to investigate.

Thanks for the documentation improvements! They are much appreciated, as they are easily overlooked.

}

/// Enable CSS (Clock Security System).
/// No effect if HSE is not enabled.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a note, that a CSS Event can be catched via CSSI (RM0316 9.2.7)?

src/rcc.rs Outdated
Comment on lines 298 to 301
/// Enable HSE (external clock) in bypass mode.
/// Uses user provided clock instead of HSI (internal RC oscillator) as the clock source.
/// Will result in a hang if an external clock source is not connected.
pub fn use_hse_bypass<F>(mut self, freq: F) -> Self
Copy link
Member

Choose a reason for hiding this comment

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

Maybe make it more clear, what the purpose of the bypass mode is?
I had to google to understand, what hse_bypass is for.

HSE crystal oscillator bypassed with external clock

is used for the documentation of the bypass flag.
Maybe:

Suggested change
/// Enable HSE (external clock) in bypass mode.
/// Uses user provided clock instead of HSI (internal RC oscillator) as the clock source.
/// Will result in a hang if an external clock source is not connected.
pub fn use_hse_bypass<F>(mut self, freq: F) -> Self
/// Enable HSE (external clock) in bypass mode.
/// Uses user provided clock instead of a crystal oscillator.
/// Will result in a hang if an external clock source is not connected.
pub fn use_hse_bypass<F>(mut self, freq: F) -> Self

Which makes it more clear, that with bypass mode enabled, the microcontroller won't drive the XTAL pins anymore, as it would do, if an external crystal would be present and (bypass mode inactive).

@David-OConnor
Copy link
Contributor

David-OConnor commented Oct 23, 2020

I don't like the word crystal, since there are alternative oscillators like MEMS.

Given most people won't have bypass enabled, and things work fine if you're using a bypassable oscillator but don't set the bypass (Consequence is not freeing up the pin for GPIO, and extra power use), I don't think we should force an explicit specification. Eg I like a method with concise syntax like .hse_bypass(ByPass::Yes), or .hse_bypass(true), from whatever perhiph struct owns the RCC_CR reg.

@David-OConnor
Copy link
Contributor

See #159

@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Oct 26, 2020

I don't like the word crystal, since there are alternative oscillators like MEMS.

What alternatvie would you suggest to differentiate between oscillators, which have to be driven and active clocks (for which the bypass mode is used)?

@David-OConnor
Copy link
Contributor

David-OConnor commented Oct 26, 2020

bypass_hse or hse_bypass. This is consistent with the ref mans and most of the existing impls listed at the top of this PR.

@Piroro-hs
Copy link
Contributor Author

Updated to h7xx style as a temporal solution.

I agree that more explicit APIs (such as g0xx, g4xx approach and #159) would be better.

Copy link
Member

@Sh3Rm4n Sh3Rm4n left a comment

Choose a reason for hiding this comment

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

Will merge after CI is all green (rustfmt)

@Piroro-hs
Copy link
Contributor Author

Fixed.

However, IDK why rustfmt is complaining about

stm32f3xx-hal/src/rcc.rs

Lines 604 to 607 in d10a122

w
.hsebyp().bit(self.hse_bypass)
.csson().bit(self.css)
.hseon().on()

while

stm32f3xx-hal/src/i2c.rs

Lines 257 to 264 in 92d9357

w
.add10().bit7()
.sadd().bits((addr << 1) as u16)
.rd_wrn().write()
.nbytes().bits(0)
.reload().completed()
.autoend().automatic()
.start().start()

is allowed.

@strom-und-spiele
Copy link
Collaborator

stm32f3xx-hal/src/i2c.rs

Lines 257 to 264 in 92d9357

w
.add10().bit7()
.sadd().bits((addr << 1) as u16)
.rd_wrn().write()
.nbytes().bits(0)
.reload().completed()
.autoend().automatic()
.start().start()

is "allowed".
because it is inside a macro and rustfmt does not seem to care.

I'd suggest using an auto linter and just let it do its thing so one gets used to the default style.
And if you find yourself changing the same line over and over again - fighting the linter for readability - there is (as a last resort)
#[rustfmt::skip]

@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Oct 29, 2020

rustfmt is definitly not perfect. There are options, we could set in a config file. I have'nt played around with it yet.
More importantly is a consitent style.

@Sh3Rm4n Sh3Rm4n merged commit 56c743f into stm32-rs:master Oct 29, 2020
@Sh3Rm4n Sh3Rm4n mentioned this pull request Dec 9, 2020
12 tasks
@Piroro-hs Piroro-hs deleted the rcc-hse-bypass branch March 30, 2021 04:55
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.

Add clock HSE bypass
4 participants