Should Timer::new() make sure clocks are initialized?

[jannic]

Currently, Timer::new(...) can be called without initializing clocks and watchdog. This will most probably cause the timer to return nonsensical values.

One way to avoid this would be to change the signature of the function:

   pub fn new(_clocks: ClocksManager, timer: TIMER, resets: &mut RESETS) -> Self

While this still doesn't guarantee that the clocks are configured to correct values, it at least proves that initializing the clocks wasn't simply forgotten.

[ithinuel]

I don't think there's a fool proof way to guaranty that. There'll always be some error margin that leave enough room for mistake to go unnoticed.

I think this solution makes sense, at least it forces the user to be aware of this.

[jannic]

Do we need to support cases where, for some reason, the clocks are initialized manually without using the HAL methods, so ClocksManager is not available - but Timer might be usable?

/// Create a new instance of `Timer` without providing `ClocksManager`.
/// Safety: The caller must make sure that the clocks are properly initialized and
/// 1MHz timer ticks are generated.
unsafe fn new_without_clock_manager(timer: TIMER, resets: &mut RESETS) -> Timer

Or am I over-thinking this?

[jannic]

I just noticed that even the docs at https://docs.rs/rp2040-hal/latest/rp2040_hal/timer/struct.CountDown.html#usage get it wrong and forget to initialize the clocks.

[KizzyCode]

Hm, I'm not sure if this is really necessary – I have multiple projects where I do this "wrong", and I've also done some tests without initializing the clocks manager or the watchdog, and for me(!) this has worked every time.

Of course this might be an happy accident or I'm relying on the goodwill of my hardware, but is there any documentation which confirms this? I'm not an expert w. regards to ARM bare-metal, so please forgive me if this is common knowledge 😅

---

Do we need to support cases where, for some reason, the clocks are initialized manually without using the HAL methods, so ClocksManager is not available - but Timer might be usable?

This might be necessary when using USB: UsbBus::new needs ClocksManager::usb_clock, so I could either destructure ClocksManager (then I cannot use Timer::new anymore), or I could create some timers (and thus cannot use UsbBus::new anymore).

[ithinuel]

@KizzyCode by default the chip relies on an internal clock generator that's less accurate than an external crystal. Further more, that generator's spec vary with temperature so you actual delay will change.

This may not be a huge deal for your application (in which case you could've saved a few penny per device). But if you're interfacing with external components over UART, then your clock may deviate by more than the common 3% tolerance and you comm will be unreliable.

[KizzyCode]

@ithinuel That makes perfect sense, thank you very much for the explanation! 😊

[ithinuel]

My pleasure.

To give a bit more context, you can check the rp2040's datasheet's clock source chapter.

The startup frequency is typically 6MHz but varies with PVT (Process, Voltage and Temperature). The frequency is likely to be in the range 4-8MHz and is guaranteed to be in the range 1.8-12MHz.

It's also worth nothing that your core is running at that frequency instead of the 125Mhz you were probably expecting :D