Low resolutions of pwm affect the frequency

[seanybaggins]

using this code

use cortex_m_semihosting::{dbg, hprintln};
use panic_semihosting as _;

use stm32f3xx_hal as hal;
use hal::prelude::*;
use hal::pwm;

use cortex_m::asm;

use cortex_m_rt::entry;

#[cfg_attr(not(test), entry)]
fn main() -> ! {
    let device_peripherals = hal::pac::Peripherals::take().unwrap();
    // Setting the clock frequency to the maximum supported value
    let mut flash = device_peripherals.FLASH.constrain();
    let mut reset_and_control_clock = device_peripherals.RCC.constrain();
    let clocks = reset_and_control_clock
        .cfgr
        .sysclk(16.mhz())
        .freeze(&mut flash.acr);

    // Configuring PWM
    let resolution = 250;
    let frequency = 1.hz();
    let pwm_channel_no_pins =
        pwm::tim16(device_peripherals.TIM16, resolution, frequency, &clocks);
    let mut gpiob = device_peripherals.GPIOB.split(&mut reset_and_control_clock.ahb);
    let pb8 = gpiob.pb8.into_af1(&mut gpiob.moder, &mut gpiob.afrh);
    let mut pwm_channel = pwm_channel_no_pins
        .output_to_pb8(pb8);
    pwm_channel.set_duty(pwm_channel.get_max_duty() / 2);
    pwm_channel.enable();

    loop{}

I get a pwm signal that I expect. A 50% duty cycle with a 1 second period.

If I change resolution = 200 then I get a signal that has a period of about 1/10 of the original.

[seanybaggins]

Are there any tradeoffs to setting the resolution to u16::MAX. If not perhaps the solution does not need to be exposed to the user of the library.

[IamfromSpace]

Hi @seanybaggins, original author of that particular module here. The issue you're running into is a variant of the one mentioned in #138. In that one, the resolution is set so high that the prescaler becomes too coarse to yield a reasonably close approximation to the desired frequency. In your case, the resolution is set so low that prescaler overflows.

We have to set the prescaler so that: timer_freq / prescaler == resolution * pwm_freq. Flipping that around to solve for the prescaler we get, prescaler = timer_freq / resolution / pwm freq. In your first example (250), the result is 64000, which is just inside our u16 maximum, giving us a valid prescaler value. In the second example, we get 80000 which overflows.

I also link #138, because it goes into details about why this is more challenging to abstract away that I was originally hoping. In fact, this interface was, I felt, the best hope for doing so, and you're not the first person to bump up against a leak.

As a tl;dr the issue is that you only have a limited number of bits, and must share them between the frequency accuracy and resolution fineness. By putting them all in resolution, certain frequencies become essentially unachievable.

It's also challenging to be strict here. This is configuration, so it's probably reasonable to panic if the result is obviously not going to make sense. This may make sense in the case of prescaler overflow, but can't be done if the resulting frequency is approximate, because many frequencies can only be approximately achieved!

I think the first thing to do is allow more flexibility in the ways that these values can be configured. As an example, a user should probably be able to set the values exactly if they so desire. I've got a rough sketch of that I'll open as a PR for more feedback shortly. Hopefully we can find something that moves the needle towards fewer gotchas :)

[David-OConnor]

There are 3 things that affect frequency (or period): PSC register value, ARR register value, and timer clock speed. The current implementation works well for some frequency and clock combos, but not others. The current code includes two parallel implementations: One in the timer module, the other in pwm. The intent is for timer to own the timer's reg block, and pwm to use raw pointers, in order to conform to the embedded-hal trait.

timer:

let psc = crate::unwrap!(u16::try_from((ticks - 1) / (1 << 16)).ok());

// NOTE(write): uses all bits in this register.
self.tim.psc.write(|w| w.psc().bits(psc));

let arr = crate::unwrap!(u16::try_from(ticks / u32::from(psc + 1)).ok());

pwm:

// Set the "resolution" of the duty cycle (ticks before restarting at 0)
// Oddly this is unsafe for some timers and not others
//
// NOTE(write): not all timers are documented in stm32f3, thus marked unsafe.
// This write uses all bits of this register so there are no unknown side effects.
#[allow(unused_unsafe)]
tim.arr.write(|w| unsafe {
    w.arr().bits(res)
});

// Set the pre-scaler
// TODO: This is repeated in the timer/pwm module.
// It might make sense to move into the clocks as a crate-only property.
// TODO: ppre1 is used in timer.rs (never ppre2), should this be dynamic?
let clock_freq = clocks.$pclkz().0 * if clocks.ppre1() == 1 { 1 } else { 2 };
let prescale_factor = clock_freq / res as u32 / freq.0;
// NOTE(write): uses all bits of this register.
tim.psc.write(|w| w.psc().bits(prescale_factor as u16 - 1));

Code from a fork I'm working on: It also is naive, and produces imprecise results at high frequencies. Check out the comments for a description of what's going on, and possible ways to address this.

/// Calculate values required to set the timer period: `PSC` and `ARR`. This can be
/// used for initial timer setup, or changing the value later.
fn calc_period_vals(period: f32, clocks: &clocks::Clocks) -> Result<(u16, u16), ValueError> {
    // PSC and ARR range: 0 to 65535
    // (PSC+1)*(ARR+1) = TIMclk/Updatefrequency = TIMclk * period
    // APB1 (pclk1) is used by Tim2, 3, 4, 6, 7.
    // APB2 (pclk2) is used by Tim8, 15-20 etc.
    let tim_clk = clocks.calc_speeds().timer1 * 1_000_000.;

    // We need to factor the right-hand-side of the above equation (`rhs` variable)
    // into integers. There are likely clever algorithms available to do this.
    // Some examples: https://cp-algorithms.com/algebra/factorization.html
    // We've chosen something quick to write, and with sloppy precision;
    // should be good enough for most cases.

    // - If you work with pure floats, there are an infinite number of solutions: Ie for any value of PSC, you can find an ARR to solve the equation.
    // - The actual values are integers that must be between 0 and 65_536
    // - Different combinations will result in different amounts of rounding errors. Ideally, we pick the one with the lowest rounding error.
    // - The aboveapproach sets PSC and ARR always equal to each other.
    // This results in concise code, is computationally easy, and doesn't limit
    // the maximum period. There will usually be solutions that have a smaller rounding error.

    let max_val = 65_535;
    let rhs = tim_clk * period;

    let arr = (rhs.sqrt() - 1.).round() as u16;
    let psc = arr;

    if arr > max_val || psc > max_val {
        return Err(ValueError {})
    }

    Ok((psc, arr))
}

This is fixable; we need a suitable algorithm for solving the equation using integers, as closely as possible, without too much computation.

[David-OConnor]

Here's what's going on specifically, as @IamfromSpace pointed out re the overflow:

timer clock = 16,000,000. arr = 200 freq = 1 psc = clock / arr / freq = 80,000. This overflows the max value of ~65k. I don't know what happens from here, other than it silently setting the wrong PSC.

Related: There's a (usually small) error in the pwm code not present in the timer code:

Current: psc = clock / arr / freq

What is should be: psc = clock / (arr + 1) / freq - 1

If you used the freq setup in either the original timer module code, or the code I posted, it would generate appropriate values. I don't think failing silently is the answer. It's better to calculate an appropriate PSC and ARR from the input frequency or period. The only time it would overflow is if the period is longer than the maximum (ie maxxing out both PSC and ARR), at which point it should explicitly fail.

[IamfromSpace]

Yeah, looking back at it, I was fairly sure that the resolution is off by one :/. The frequency is correct though I think.

And it does seem like there must be a simple/cheap alg that maximizes resolution against a target frequency. Period freqs that are co-prime with the timer freq are troublesome in that the naive maximum resolution will be too small to be useful (whenever PSC is large enough, ARR will be 1). In this case we need to somehow determine what the appropriate level of frequency accuracy is.

[David-OConnor]

Could you clarify what problems being co-prime causes? I've been treating PSC and ARR as interchangeable, which may not be right. (Although it seems to work in practice, so far)

[IamfromSpace]

Ah, yes, and treating them the same makes perfect sense in many contexts—because often only frequency matters.

PWM a place where that breaks down, because resolution is either somewhat or very important. For something like a servo, you need a high enough resolution that you can express the positions. If you range 180 across 5-10% duty cycle, then 3600 is going to allow you to express down to the degree. If you need to hit exact marks (say 10 degrees), then just 360 does the trick, but 577 misses them, even though it’s higher resolution.

Also, when we only care about frequency, it still makes sense to prioritize a larger PSC, as ARR is just soaking up bits, it doesn’t make it more accurate.

~So, if we use PSC to maximize accuracy of a co-prime frequency we’ll get a very low arr:~

~timer_freq: 72_000_000~ ~target_pwm_freq: 1_171~ ~prescale_factor: 61_486~ ~resolution: 1~

~We can never hit this frequency, so we use our entire prescaler value to get as close as possible. That leaves no remaining clock divisions to share with ARR.~

edit: In thinking about this more, this case is actually the easy one--we can just ignore the prescale_factor and only set the resolution to the appropriate divider. We get a large resolution and as accurate a result as we possibly could. There are other more challenging cases though.

[David-OConnor]

Thank you for the detailed explanation. I've been using timers to fire interrupts and use 50%-duty PWM; this explains why I didn't notice a problem in my tests - and why the PWM module lets you specify ARR, but not the timer.

Lets say you're using that clock and target pwm freq. What trouble could you run into from setting arr and psc both to 247? This would yield a frequency of 1170.7Hz. Would the problem be when setting a small, or precise duty cycle?

I'm showing the values for PSC and ARR in your post result in a freq of 585.5Hz. Is this to demonstrate that when needing a low ARR (Resolution?), we can't set PSC high enough to get the right freq? Thank you.

[IamfromSpace]

Right, so 247 for 5-10% only gives you 12 useful values, and for 0-180 your jumps are about 15deg, which isn’t a lot of control.

And it just becomes hard to say what’s better, because more resolution creates more freq drift.

A 1171 frequency is certainly a worst case—I’m not sure there’s any reasonable value. But it does a good job of illustrating the trade-off.

[IamfromSpace]

Just edited my previous comment, realized that my previous statement wasn't quite correct: only the prescaler determines the accuracy of the frequency when the resolution is fixed. If ARR has no constraints (like, be exactly x or be a multiple of y), then we get down to the original problem that you, @David-OConnor, had mentioned, how do we quickly and accurately factor our two dividers? And instead of an even split, we likely want to have a lop-sided one, where resolution is prioritized.

Probably best to continue this discussion on #196, but I wanted to have this correction here :)