Add support for HSE and USB clocks

[mvirkkunen]

Here's an initial idea for supporting the HSE clock input (which is needed for a within-spec USB clock) as well as configuring the USB clock (or more like validating, as there's only one bit to configure, and it's only valid when the other clocks are configured correctly).

I've manually tested this on hardware using the clock output function and a scope with various clock combinations and it seems to work in practice.

I'm not quite sure about the API though. Here's potential issues I'd like to get feedback on:

• Is just having one method to set the oscillator frequency a good idea? In theory it follows the existing API in that the user only specifies frequencies, and the HAL sets the clock divisors and sources accordingly.
• Setting the clocks now has much more potential for failing, as the external oscillator can fail to start. Should the method attempt to detect it and panic or fallback if the oscillator fails, or should it be changed to return Result? (That'll introduce warnings everywhere though, so I'd consider it a breaking change)
  • The master version can also get stuck if the PLL fails to lock, but that's pretty uncommon when using the internal oscillator.
• Would it be better if the user had to request the USB clock (something like .require_usbclk() as there's no frequency to configure) and the method paniced if the rest of the clock configuration wasn't correct? Currently it just returns a usbclk_valid value in the Clocks struct that the user or a USB HAL could check with asserts.

I also changed the calculation for the PLL multiplier a bit in order to calculate it correctly with HSE, which may have changed behavior in cases where there was rounding involved. I'm not 100% sure how the original math worked.

Additionally this fixes an off-by-one error in the HCLK/PCLK2 frequency asserts, which only manifested when I added HSE support as the maximum frequency can only be reached with it.

[TeXitoi]

Is just having one method to set the oscillator frequency a good idea?

It seems natural to me. Do you have any other idea?

Setting the clocks now has much more potential for failing, as the external oscillator can fail to start.

This version is only more functionality without breaking change. Thus I vote for that for now, and possibly doing something more complex in a future incompatible version.

An expanded solution might be having a CFGR::nb_freeze(self) that return an object with a fn freeze(&mut self, acr: ACR) -> nb::Result<ACR, !>. It can be added later.

Would it be better if the user had to request the USB clock (something like .require_usbclk() as there's no frequency to configure) and the method paniced if the rest of the clock configuration wasn't correct?

No, it will not be better as the developer can't handle the failing case. Panicking is great when you want to provide a simpler API, but you should always have a non panicking version. But maybe getting the USB clock can return an option?

[mvirkkunen]

Another possibility for a clock source setting API could be something like a clock_source method and an enum to go with it:

enum ClockSource<F: Into<Hertz>> {
    HSI,
    HSE(F),
}

That might more clearly indicate that you're choosing between two things instead of just enabling something. If It's more wordy though, and requires another "use". If there were more than two oscillators available an enum would definitely be better in my opinion.

The USB clock has only one valid frequency, so I don't think using an option for it would be that useful.

[TeXitoi]

The USB clock has only one valid frequency, so I don't think using an option for it would be that useful.

No, I mean you have a token in Clocks that you would need to give to the constructor of the usb driver, something like:

pub struct UsbClock { _0: () }
pub struct Clocks {
    // ...
    pub usb_clock: Option<UsbClock>,
}
fn usb_driver(_: UsbClock) -> UsbDriver {
    // ...
}

But it means Clocks no more Copy, that may be problematic (and a big breaking change).

[TeXitoi]

Another possibility for a clock source setting API could be something like a clock_source method and an enum to go with it

The actual solution is more ergonomic in most case I think. But the 2 solutions can be present at the same time.

Maybe renaming the setter fn use_hse(self, hse_freq: impl Into<Hertz>) -> Self would be more explicit?

[mvirkkunen]

That could be better. If it's OK to some extra related cleanup I could also add documentation comments to all the methods which makes it even easier to tell what they do, and change them all to use impl Trait syntax now that that exists.

[TeXitoi]

+1 for doc comments. impl Trait in argument position is a bit controversial. I personally don't care, but I am of no authority here 😉

[TeXitoi]

@japaric any feedback? That's really an important feature to use the blue pill at full speed.

[TeXitoi]

can you

fn use_hse<H: Into<Hertz>>(self, hse_freq: H) -> Self

and add doc comments? I will then be OK to merge if @therealprof have no objection.

[mvirkkunen]

OK, looks good (I presume "mut self"?), it'll have to wait for a while though as I only have my phone with me while traveling for a couple of weeks. Thanks for taking a look!

[TeXitoi]

Yes, mut self. No hurry, take your time 😄

[mvirkkunen]

I finally got around to this! Does it look good to you?

[therealprof]

Suggested change

[mvirkkunen]

Better? :)

[therealprof]

LGTM