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

Added new clocks approach #159

Closed
wants to merge 24 commits into from
Closed

Added new clocks approach #159

wants to merge 24 commits into from

Conversation

David-OConnor
Copy link
Contributor

@David-OConnor David-OConnor commented Oct 25, 2020

It seems like the current system fails silently, and gives the user too many degrees of freedom. Eg the illusion you can set whatever speeds you want. I think the STM32Cube IDE visual clock config is a good guide for what configs are valid.

Suggestion: Instead of setting speeds, set the various scalers. This alone reduces the degrees of freedom. Have a helper method to show what speeds these output. Calculate the speeds, and validate before setting. This deals with errors in 2 ways: Prevents requesting speed combinations that can't be set using the scalers and 2: Fails explicitly if the resulting speeds are out of range.

Example use:

    let mut clocks = clocks::Clocks::default();  // Initialize to 48Mhz sys clock, and 24Mhz periph clocks
    clocks.hse_bypass = true;
    clocks.pll_mul = clocks::PllMul::Mul9;
    clocks.usb_pre = clocks::UsbPrescaler::Div1_5;

    if clocks.setup(&mut dp.RCC, &mut dp.FLASH).is_err() {
        rprintln!("Unable to configure clocks due to a speed error.")
    };
    rprintln!("Speeds: {:#?}", clocks.calc_speeds());
    
    // `rcc.rs` clock config we pass to other fns that need the speeds. For compatibility.
    let clocks = clocks.make_rcc_clocks();

See examples/clocks.rs for more sytnax.

Stated another way: With this new approach, you only need to validate that the resulting speeds are within defined ranges. With the existing approach, you're solving a system of equations that may have 0, 1, or multiple solutions. The current API doesn't seem robust enough to handle the 0 and multiple cases, and I think it might be easier to change approaches, than create a robust solver.

We can mitigate the more abstract nature of setting scalers by including sensible defaults. I've added a default::Default impl, but we could add several alternatives.

In summary, this is a big departure from the existing approach. Here's why I think it's worth it:

  • Non-breaking addition, vice change.
  • Existing approach is easy to create invalid configs with
  • Existing approach is opaque about what's being set

Challenge: Can you successfully set a 72Mhz sysclk using the existing module? I've been unable, but can using the example above.

@David-OConnor David-OConnor marked this pull request as ready for review October 25, 2020 20:46
@Sh3Rm4n Sh3Rm4n mentioned this pull request Dec 9, 2020
12 tasks
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.

None yet

2 participants