panic attempting to setup PWM

[astraw]

I tried to use the new PWM functionality (by @IamfromSpace I think) and so far failed.

Using a stm32f303, I have the following code: stm32f3xx_hal::pwm::tim3(device.TIM3, 16000, Hertz(500), &clocks);

In a debug build, this panics with "attempt to subtract with overflow" in the following line in pwm.rs (using current git master, 609af0a91c2030ff2c3e35705e5394d66ec3cb81:

tim.psc.write(|w| w.psc().bits(prescale_factor as u16 - 1));

Indeed, debugging this a bit, I see that prescale_factor is set to 0. (Going into the logic where this is set, clock_freq is set to 4000000, res to 16000, and freq.0 to 500. These values thus cause prescale_factor to be set to 0.)

The thing is, I think this resolution and rate do work on this hardware since I am updating some old code where this works.

[dfrankland]

Correct me if I am wrong, but it seems like the order of operations in the old code is different than the new one.

Old code:

prescale_factor = ((clock_freq / freq) - 1) / res

New code:

prescale_factor = ((clock_freq / res) / freq) - 1

These both seem to be slightly incorrect though. According to Section 2.2 Time base generator of the STM32 cross-series timer overview the following should be used:

Update_event = TIM_CLK/((PSC + 1)*(ARR + 1)*(RCR + 1))

The above roughly translates to:

freq = clock_freq / ((prescale_factor + 1) * res)

Which can be flipped around to represent:

prescale_factor = ((clock_freq / freq) / res) - 1

[IamfromSpace]

@astraw I do think the old code was incorrect. @dfrankland, good call on checking the timer overview--which is equivalent to the new code that's there now (since freq and res can be swapped).

The value written to the register is the prescale factor - 1 (so that the hardware cannot represent divide by 0, and the entire range of the register can be used), so it must be the last step.

The values 4M, 16k, and 500 would result in a prescale factor of 0.5 (250/500), and that would evaluate to 0 due to the integer division. Then the subtraction would cause underflow. The old code wrote 0 to the register, but got there because the final math was 249 / 500.

[astraw]

Due to the integer prescale factor, one can not, in general, have both the exact resolution and exact frequency one desires. I can imagine users wanting to do one or the other, depending on the application. The code proposed by @dfrankland favors an exact resolution at the expense of an exact frequency. I propose thinking about a Timer constructor that itself takes a TimerConfig struct. This TimerConfig would have two alternative constructors.

Pseudo-code (I could imagine making generic over the exact types to deal with different resolution timers):

struct TimerConfig {
    prescaler_factor: u16,
    autoreload_register: u16,
}

impl TimerConfig {
    fn exact_resolution_and_approx_frequency(res: u16, freq: Hertz) -> Self {
        ...
    }
    fn approx_resolution_and_exact_frequency(res: u16, freq: Hertz) -> Self {
        ...
    }
}

This, though, seems to have a wider scope than just PWMs in stm32f3 hardware. I feel like there must already be good prior art even within the rust stm32 world. Do we know of any?

(By the way, linking to my old code was not to say that it is ideal - far from it! Just that it is possible to get the PWM running with these values and no panic.)

[IamfromSpace]

I completely agree that there will be a lot of approaches to and ways of thinking about the timer setup. A lot of people will want a more abstract representation (like frequency + percent duty cycle), while others will want simple control (like frequency + 256 divisions), and others will want a frequency and the maximum possible resolution.

Ideally we hide just enough so that if you get error, it's because you never cared, or if you do care then you never get any error. That's mostly why I deferred and picked this as a "default" interface, because that's hopefully what most people wanted most of the time.

I've been thinking as well that offering more constructors or helpers (I hadn't considered typed configurations, but that's also an viable way to go) could maybe help each user find their preferred interface.

I do think that exact freq and exact resolution is a reasonable desire. Use cases might be if you wanted to maximize resolution, or if you wanted it set to some helpful value (like 256, or 100, or 1000). In this case, some users might expect a panic if their chosen configuration isn't feasible. We can assert that the resulting calculation multiplies back out to the original clock frequency in that case (no precision lost).

Some of me now thinks that the simplest single interface is the "maximize resolution" interface, because only one input value is required, there's no possibility for error, and the user never loses duty cycle precision. The downside is that its not very flexible (which is why I didn't choose it).