Fix missing sync of timer ENABLE bit

[fko-kuptec]

Summary

I stumbled across some unexpected behaviour when changing the period of a TC-based PWM. When changing the PWM frequency in a loop, only every other change of the frequency was accepted. After some digging, I found that the setting and clearing of the ENABLE bit of the peripheral was not synchronized in many cases.

Adding the synchronization fixes the issue, at least on my hardware (ATSAMD51P20A). I inserted the sync to all places where the peripherals get enabled or disabled, for both D5x and D11 chips. Hope, I haven't overlooked something.

It would be nice, if someone could test it on the D11/D21 platforms, since I haven't done that.

[fko-kuptec]

Okay, seems like the D11 tests fail. Should have checked that before

[fko-kuptec]

Okay, I didn't expect the synchronization to work slightly different on D11 and D21 for TC peripherals. TCC work as for D5x chips...

I don't think, though, that I introduced the remaining errors with my changes 😅

[jbeaurivage]

Thanks, do you have an example to test this out on D11/D21 platforms?

Also, could you make the syncing a method? Would make it just a little easier to read. :)

[fko-kuptec]

Thanks, do you have an example to test this out on D11/D21 platforms?

I came up with the following example for an ItsyBitsy M0 Express. It lets the red LED blink between max and half power using different frequencies. But to be honest: The D21 does not seem to care about this patch. It works no matter using the current crates-io version or my patched one, if I didn't mess anything up...

#![no_std]
#![no_main]

use panic_reset as _;

use cortex_m_rt::entry;
use hal::{
    clock::GenericClockController,
    delay::Delay,
    ehal::delay::DelayNs,
    fugit::RateExtU32 as _,
    gpio,
    pac::{CorePeripherals, Peripherals},
    prelude::_embedded_hal_Pwm,
    pwm,
};

#[entry]
fn main() -> ! {
    let mut peripherals = Peripherals::take().unwrap();
    let core = CorePeripherals::take().unwrap();

    let mut clocks = GenericClockController::with_internal_32kosc(
        peripherals.gclk,
        &mut peripherals.pm,
        &mut peripherals.sysctrl,
        &mut peripherals.nvmctrl,
    );
    let mut delay = Delay::new(core.SYST, &mut clocks);
    let pins = gpio::Pins::new(peripherals.port);

    let _pin: gpio::Pin<_, gpio::AlternateF> = pins.pa17.into();

    let gclk0 = clocks.gclk0();
    let mut pwm = pwm::Pwm0::new(
        &clocks.tcc0_tcc1(&gclk0).unwrap(),
        1_000.Hz(),
        peripherals.tcc0,
        &mut peripherals.pm,
    );
    let channel = pwm::Channel::_3;

    loop {
        for i in 2..6 {
            pwm.set_period(i.kHz());
            let max = pwm.get_max_duty();
            pwm.set_duty(channel, max);
            delay.delay_ms(500);

            pwm.set_period(500.Hz());
            let max = pwm.get_max_duty();
            pwm.set_duty(channel, max / 4);
            delay.delay_ms(500);
        }
    }
}

---

Also, could you make the syncing a method? Would make it just a little easier to read. :)

I definetly see why you would like to put the syncing into a method. However, there are also other registers that need syncing, which are also not inside a separate method. That would make it a bit inconsitent, wouldn't it?

[jbeaurivage]

Right, after reading the module's code, I didn't expect to see so much syncing all across the implementation. Ideally all of these little tasks would be broken into their own methods (ie, enable() -> set ENABLE bit, then wait for syncing, and so on). I get what you're saying as far as consistency.

I'll let you decide whether you'd like to make that little refactor happen, or if you'd rather I just merge this as is.

[fko-kuptec]

At the moment, to be honest, I would prefer if you could merge it as is. Maybe creating an issue for refactoring would be an option, so that it doesn't get lost?

[jbeaurivage]

No worries. It's really not a big deal, just something we'll want to consider if that module eventually gets refactored.