Imprecise PLL frequency calculation

[t123yh]

trafficstars

Consider this example for STM32F407:

    let mut config = Config::default();
    {
        config.rcc.hse = Some(Hse {
            freq: Hertz(25_000_000),
            mode: HseMode::Oscillator,
        });
        config.rcc.pll_src = PllSource::HSE;
        config.rcc.pll = Some(Pll {
            prediv: PllPreDiv::DIV15,
            mul: PllMul::MUL192,
            divp: Some(PllPDiv::DIV2),
            divq: None,
            divr: None,
        });
        config.rcc.ahb_pre = AHBPrescaler::DIV1;
        config.rcc.apb1_pre = APBPrescaler::DIV4;
        config.rcc.apb2_pre = APBPrescaler::DIV2;
        config.rcc.sys = Sysclk::PLL1_P;
    }

    let p = embassy_stm32::init(config);

This first divides input frequency (25MHz) by 15, multipliy it by 192, then divide it by 2. This would result in an actual sysclk of 25000000/15*192/2=160000000 (160MHz), which is a nice-looking frequency.

However, the PLL code in f4's rcc first divides the frequency, then multiplies. This would make sysclk = Hertz(159999936), rather than 160MHz, due to rounding issue (because 25 is not divisible by 15).

We cannot simply change the code to multiply first and then divide, because 25000000*192=4800000000, which is out of bound for u32 (Hertz).

The impact of this is somewhat huge. If you do something like this:

    let mut can1 = Can::new(p.CAN1, p.PD0, p.PD1, Irqs);

    can1.modify_filters()
        .enable_bank(0, Fifo::Fifo0, Mask32::accept_all());

    can1.modify_config()
        .set_loopback(false) // Receive own frames
        .set_silent(false)
        .set_bitrate(500_000);

set_bitrate will panic, because it considers APB1 to have 39999984Hz, thus cannot have 500000Hz baud rate.

[t123yh]

I think there might be 3 possible things here:

1. Most obviously, fix the PLL frequency calculation code, possibly by using u64;
2. Warn user about rounding issues when calculating frequency;
3. Make the peripherals tolerant the imprecise frequency by setting an error margin.

[Dirbaio]

I'd try using u64 and check if code size increases too much or not. I think it shouldn't, if you use LTO then RCC code usually gets optimized very well because the input is all compile-time-known constants.

I think in some cases we might want some error margin in the drivers anyway? even with u64 you can end up in situations where freq is something like 39999999Hz maybe?

[t123yh]

I'd try using u64 and check if code size increases too much or not. I think it shouldn't, if you use LTO then RCC code usually gets optimized very well because the input is all compile-time-known constants.

Yes, agree. Maybe I can try to implement it. Although it may cause the code to look slightly uglier.

Maybe Hertz should be changed to Hertz<T> where T can be u64? Or, have Hertz and BigHertz and make it possible to convert between?

I think in some cases we might want some error margin in the drivers anyway? even with u64 you can end up in situations where freq is something like 39999999Hz maybe?

Yes, agree

[Dirbaio]

Hertz is only to make the public api typesafe, it's fine to internally just use u32/u64. I wouldn't do BigHertz or Hertz<u64> since it doesn't appear in the public api at all.

[EliteTK]

Would it not be an option to allow explicitly (optionally) specifying the frequencies as part of the config? If someone makes a mistake it's not really much different than claiming you have a 8MHz HSE with a prediv of 4 and actually having a 25MHz HSE thereby probably way above the VCO input limit.

(Maybe another option is to use fixed-point arithmetic but really it's my personal opinion that this stuff shouldn't be getting calculated at runtime anyway.)

[EliteTK]

Regarding my fixed-point comment, I'm not sure anymore what was actually meant earlier in the discussion by suggesting upgrading to u64 and if it didn't already mean using fixed-point arithmetic.

Regardless, I have made a horrible proof of concept here:

https://github.com/embassy-rs/embassy/compare/main...EliteTK:embassy:horrible-fixed-point-rcc-fix

(also backported to 0.1.0: https://github.com/embassy-rs/embassy/compare/main...EliteTK:embassy:horrible-fixed-point-rcc-fix-0.1.0)

And on real hardware:

DEBUG rcc: Clocks { sys: Hertz(100000000), pclk1: Hertz(50000000), pclk1_tim: Hertz(100000000), pclk2: Hertz(100000000), pclk2_tim: Hertz(100000000), hclk1: Hertz(100000000), hclk2: Hertz(100000000), hclk3: Hertz(100000000), plli2s1_q: None, plli2s1_r: None, pll1_q: None, rtc: Some(Hertz(32000)) }

With the following clock config on an f411:

config.rcc.hsi = false;
config.rcc.hse = Some(Hse {
    freq: mhz(25),
    mode: HseMode::Oscillator,
});
config.rcc.pll_src = PllSource::HSE;
config.rcc.pll = Some(Pll {
    prediv: PllPreDiv::DIV13,
    mul: PllMul::MUL104,
    divp: Some(PllPDiv::DIV2),
    divq: None,
    divr: None,
});
config.rcc.ahb_pre = AHBPrescaler::DIV1;
config.rcc.apb1_pre = APBPrescaler::DIV2;
config.rcc.apb2_pre = APBPrescaler::DIV1;
config.rcc.sys = Sysclk::PLL1_P;

Seems like this could be an option but maybe done with a bit more thought than the 10 minutes of writing and 5 minutes of troubleshooting that I've done.