stm32-rs / stm32f0xx-hal

A Rust `embedded-hal` implementation for all MCUs in the STM32 F0 family
BSD Zero Clause License
134 stars 59 forks source link

USB: STM32F070: USB Clock source selection #83

Closed pigrew closed 4 years ago

pigrew commented 4 years ago

On the STM32F070RB, USB can only be used with the PLL source, not HSI48 (which doesn't exist). A method to select RCC.CFGR3.USBSW=1 should be provided.

Some implementation points to be considered (with my tentative answers in parenthesis):

  1. What should the default be? (always set to HSI48?)
  2. Should there be a debug assertion in freeze() that the PLL is enabled if the PLL source is selected for USB? (Yes)
  3. Should there be a debug assertion in the USB HAL enable() that the selected clock's speed is 48 MHz? (Maybe not since we'd need to start also adding Rcc to the USB peripheral struct)
  4. What should the usage be?

Proposed usage:

    let mut rcc = dp.RCC
        .configure()
        .usbclksrc(stm32f0xx_hal::rcc::USBClkSource::HSI48)
        .sysclk(48.mhz())
        .hclk(48.mhz())
        .pclk(48.mhz())
        .freeze(&mut dp.FLASH);

ADDENDUM: The STM32F1xx HAL has some logic related assertions about the USB clock, so my plan is to duplicate its API as much as possible.

therealprof commented 4 years ago

Not a big fan of debug assertions (or anything that radically changes the behaviour between dev and release builds, really) but the rest of your proposal sounds good to me.

pigrew commented 4 years ago

I've been struggling with how much #[cfg(...)] to use in the code. On one hand, it makes mistakes (targeting features not on the particular MCU) create compile-time errors (good thing, IMHO). On the other hand, it will make the generated API documentation and source code harder to read (since they will have lots of conditional compile directives in them).

Currently, I have:

#[cfg(any(
    feature = "stm32f042",
    feature = "stm32f048",
    feature = "stm32f070", // Doesn't have HSI48
    feature = "stm32f072",
    feature = "stm32f078",
))]
pub enum USBClockSource {
    #[cfg(feature = "stm32f070")]
    /// USB peripheral tranceiver clock is disabled
    Disabled,
    /// HSI48 is used as USB peripheral tranceiver clock
    #[cfg(not(feature = "stm32f070"))]
    HSI48,
    /// PLL output is used as USB peripheral tranceiver clock
    PLL
}

Should I continue with these cfg statements, or just let the enum be defined for all MCUs but have assertions that they are valid in the freeze() function?