PWM is not atomic/sync with WFI and cleared `clk_sys_pwm`

[kolbma]

When sleeping with disabled clk_sys_pwm there is some undefined result for PWM-channels.
I set_duty(0) or disable() the channel before going to sleep, but sometimes the LEDs stay on the old duty, rare it is lighting with full power instead going off.
If I do about 2000 or more asm::nop() before wfi() the chance of occurrence is lower, but not gone.
With clk_sys_pwm-bit set, there is no problem.
When I interpret the datasheet correctly, there should be no problem with disabling pwm during sleep and reusing it afterwards.

[jannic]

I guess this is just the way the hardware works: If you disable clk_sys_pwm, that doesn't magically the value sent by the pwm peripheral to the i/o pin circuit. And without a clock, the pin will stay high or low, depending on the current value the moment the clock stopped.
Also, setting set_duty(0) internally just sets the CC register, which doesn't take effect immediately. The value is double-buffered and only takes effect when the PWM counter wraps around. (See datasheet section 4.5.2.3. Double Buffering for a better explanation.)
Maybe your 2000 cycle delay is often, but not always, long enough so the 0 value got effective before you turn of the clock? To be sure you'd have to wait one full PWM cycle. How long that takes depends on your PWM config.

[kolbma]

Yes this is the case, but the API provides a disable() which is the same like duty(0) and it doesn't provide something to disable PWM before sleep without invalidating the settings, because it is owned memory. Per register you could disable PWM but in Rust this stuff is already owned, so you can not modify.

[jannic]

Slice::disable() does set csr.en to false. Does that set the pin to low? In the datasheet, I can't find a clear description what that bit does, exactly.

[ithinuel]

@kolbma Disabling the PWM does not mean setting the pin to 0.

Say you are disabling the the PWM in the middle of a high-phase, if the PIN were brought down immediately, it would be creating low dutycycle glitches.

What you need is a way to wait for the end of the current cycle once you set the duty to 0.
I believe that could be done by polling has_overflown if it were reading intr instead of ints.

[jannic]

I believe that could be done by polling has_overflown if it were reading intr instead of ints.

Interesting observation. Is there any use case where reading ints makes sense for implementing has_overflown? I tend to think this is a bug and should be changed.

[jannic]

I tend to think this is a bug and should be changed.

#562

[kolbma]

Slice::disable() does set csr.en to false. Does that set the pin to low? In the datasheet, I can't find a clear description what that bit does, exactly.

Thanks for reactions of all of you.

Well, could not try, I think there is no chance to disable the slice, while there are channel assignments:

cannot borrow `*pwm3` as mutable more than once at a time

Or without using refs:

borrow of partially moved value: `pwm3`
partial move occurs because `pwm3.channel_a` has type `rp_pico::rp2040_hal::pwm::Channel<rp_pico::rp2040_hal::pwm::Pwm3, rp_pico::rp2040_hal::pwm::FreeRunning, rp_pico::rp2040_hal::pwm::A>`, which does not implement the `Copy` trait

Would work only if I drop all this before disable() and reassign and re-configure after enabling it again. But this is overhead and if I do dropping, in this situation I am in no need to disable it any longer, because then there is nothing left which needs to be disabled when calling wfi().

What happens to PWM, if I disable the used pin?

Compiling is possible:
Channel::output_to() returns e.g. Pin<Gpio25, Function<Pwm>> and on this pin I can call Pin::into_pull_down_disabled().
After wakeup I can call e.g. Channel::output_to(Pin<Gpio25, PullDownDisabled>).

Maybe this is the way to go, and let the PWM part do whatever it likes to do without a clock and without output pin.

But not sure if you should name it disable when there is nothing disabled ;-)

Also when it is not possible to disable a setup and used slice, how do you want to sync the slices (called channels in datasheet) like it is described in PWM: EN Register...?

This register aliases the CSR_EN bits for all channels.
Writing to this register allows multiple channels to be enabled
or disabled simultaneously, so they can run in perfect sync.
For each channel, there is only one physical EN register bit,
which can be accessed through here or CHx_CSR.

The has_overflown() might be interesting if you really want to wait for a defined state, but in which state PWM is at the interrupt, I guess, depends on the SliceMode.
I want it to stop powering immediately and any signal glitches are of no interest.

[ithinuel]

What happens to PWM, if I disable the used pin?

It continues running, unaware that the pin is no longer following it's commands.

Channel::output_to() returns e.g. Pin<Gpio25, Function> and on this pin I can call Pin::into_pull_down_disabled().
After wakeup I can call e.g. Channel::output_to(Pin<Gpio25, PullDownDisabled>).

Indeed, TBH output_to only does an into_mode so you could skip this call completly and do the let p: Pin<_, FunctionPwm> = p.into_mode(); yourself and get the same result.

But not sure if you should name it disable when there is nothing disabled ;-)

Indeed Channel::disable may be a bit misleading. On the other hand, the doc explicitly says it's not actually disabling anything.
Note that Slice::disable effectively disable the slice as expected.

I want it to stop powering immediately and any signal glitches are of no interest.

I think disabling the pin in a given state is your way to go then :)

[jannic]

But not sure if you should name it disable when there is nothing disabled ;-)

I fully agree! Unfortunately, we don't really have a choice here: Note that this is inside impl embedded_hal::PwmPin for Channel, so the name of the function is defined by embedded-hal.
The only alternative would be to not implement that trait at all.

rust-embedded/embedded-hal#430
→ The new trait in embedded-hal 1.0 probably won't have that disable function, for reasons related to this issue. "I think they were underspecified (for example, is the pin high or low when disabled?), and not very useful in practice."

borrow of partially moved value: pwm3

It might not fit well to your code layout, but it may be possible to avoid destructuring.
Instead of this:

let mut channel = pwm.channel_a;
channel.set_duty(6000);
[...]
pwm.disable(); // <-- doesn't work

you could do this:

pwm.channel_a.set_duty(6000);
[...]
pwm.disable(); // <-- still works!

[kolbma]

you could do this:

pwm.channel_a.set_duty(6000);
[...]
pwm.disable(); // <-- still works!

Oh, yeah ;-) I oversaw that. There is no need to have the channel-struct all the time available, once registers are set.
And the setup is staying in registers and not the struct... there are only methods with bit-banging.

Thank you very much.