RP2040: Higher-level API for PWM

[cylewitruk]

Creating a draft PR for review/discussion as a WIP implementation for #2784.

These changes try to bring the Pwm module a little closer to some of the other implementations we see in embassy, though RP2040 is a little different in that it has eight slices with two channels each (A & B), where some registers are set for the slice and others for the channels.

I'm not 100% on the calculations so I'd love an extra eye on them. Based on the RP2040 Datasheet, section 4.5 and some other implementations I looked at.

[crabdancing]

Thank you so much for working on this! It'll be great for a number of my projects! :D

The frequency calculations seem to be spot-on from my testing. I only have a cheap Hantek to work with, but it seems to exactly match everything I've tested up to 50Mhz (after which, it strains the capabilities of my oscilloscope and causes a lot of impedance, so it's hard to tell what's going on).

It looks like set_duty_a and set_duty_b don't actually work independently in my tests -- at least, not if you're doing:

    let mut c: Config = Default::default();
    c.top = u16::MAX;
    let mut pwm = Pwm::new_output_ab(p.PWM_SLICE0, p.PIN_16, p.PIN_17, c.clone());

If I do:

    pwm.set_duty_a(0).unwrap();
    pwm.set_duty_b(100).unwrap();

Both A and B are on, according to my oscilloscope.

But if I do:

    pwm.set_duty_a(100).unwrap();
    pwm.set_duty_b(0).unwrap();

Both A and B are off.

I'm operating at 25% clock freq, if that helps reproduce:

    let freq = (clk_sys_freq() as f64 * 0.25) as u32;
    info!("{}", freq);
    pwm.set_freq(freq).unwrap();

And I haven't done anything else to the state of the struct.

[cylewitruk]

@crabdancing awesome you were able to try it out and that it seems to work-"ish" 😄 I'm waiting for my oscilloscope so I don't have anything to test with yet, so that's a huge help!

I'll see if I can find what might be going wrong with a/b duties tomorrow -- according to the datasheet a/b should be able to operate at different duties and I don't see any issues with the code at a glance..

[crabdancing]

Glad I can help! :)

I've been looking through it and I don't get it either. The mainline version of my program has no trouble independently controlling the PWM signals with compare_a and compare_b.

[crabdancing]

I have a cute little test program that plays an animation on oscilloscopes, alternately moving two channels back and forth past each other by smoothly changing the duty cycle. On the mainline embassy-rs, it seems to work fine, but on the fork it seems completely broken -- nothing even comes out. I would expect them to work about half the time as long as nothing is being set to 0, so my original hypothesis is suspect -- something else deeper is probably going on.

You can see the test version here, and the original version here.

Edit: trying things like:

    pwm.set_duty_a(100).unwrap();
    pwm.set_duty_a(0).unwrap();
    pwm.set_duty_a(100).unwrap();
    pwm.set_duty_b(100).unwrap();
    pwm.set_duty_b(0).unwrap();
    pwm.set_duty_b(100).unwrap();
    pwm.set_duty_a(100).unwrap();
    pwm.set_duty_a(0).unwrap();
    pwm.set_duty_a(100).unwrap();

Produces weird results. Currently, this seems to result in only duty_a being active -- even though, logically, both should be if my initial hypothesis was correct.

Doing:

    pwm.set_duty_a(100).unwrap();
    pwm.set_duty_b(100).unwrap();

seems to result in only B being on, and vice versa. I think the entire slice state is being wiped, maybe?

Also noteworthy that the signals show up as tiny spikes, meaning the on period is way too short for how high the off period is. E.g., a duty cycle of 100 ought to be full-on forever, and a duty cycle of 50 ought to be half on and half off with a clean square wave, but with some of these examples, it ends up with tiny spikes where it's mostly off at 100, and doesn't seem to work at all at 50?

[cylewitruk]

@crabdancing thanks for the detailed input! I'll see if I can try to reproduce some of this with tests today and figure out what's going on 😄

[cylewitruk]

You can see the test version here, and the original version here.

@crabdancing The second (original) link doesn't seem to work, I get a 404, would be good to be able to see the working vs. not working to compare

[crabdancing]

Oof, sorry. I accidentally only pushed testing branch. Forgot to do git push --all. Try now?

[cylewitruk]

@crabdancing I'm still looking at this, just so you know -- have been doing some reworking of the API etc. Got my oscilloscope today so I can play with it this weekend and try to test things myself 😄

Here's a little sneak-peek at the API I'm thinking of. There's a fair bit of conditionals which are easily confusing (hence this issue 😅 ), so my thinking is to provide a builder-DSL that helps ensure that only valid values can be set depending on the DIVMODE etc. Feel free to give your thoughts:

// Initialize peripherals
let peripherals = embassy_rp::init(Default::default());

// Initialize PWM slice 0 as a free-running PWM with a frequency of 100 kHz.
let mut slice0 = peripherals.pwm_0()
    .free_running()
    .frequency(Frequency::KHz(100.0))
    .with_channel_a(&peripherals.PIN_0, |a| a
        .duty_cycle(100.0)
        .invert(true)
    )
    .with_channel_b(&peripherals.PIN_1, |b| b
        .duty_cycle(50.0)
    )
    .build();

// Alternative syntax:
// let mut slice0 = PwmSlice::builder(peripherals.PWM_SLICE_0)
//    .free_running()
//    ...

// Initialize PWM slice 1 as a level-sensitive PWM with a divider of 5.
let mut slice1 = peripherals.pwm_1()
    .level_sensitive()
    .divider(5, 0)
    .with_input(&peripherals.PIN_3)
    .with_output(&peripherals.PIN_2)
    .build();

// Enable multiple slices simultaneously...
enable_pwm_slices(|slices| slices
    .slice_0()
    .slice_1()
);

slice1.phase_advance();
slice1.phase_retard();

// Disable slices one-by-one
slice0.disable();
slice1.disable();

[crabdancing]

It looks great! :) The alternative syntax seems like it might be better for more easily iterating over slices to avoid duplicate code. That was a frustration I had with a lot of embedded HALs in Rust, when I'm using a lot of pins. I get what they're trying to do with typestate to prevent programmer mistakes, tho!

The builder design pattern seems nice! I generally know what all the options are and what they mean at a glance. I would be totally happy to use this in my code :D

[cylewitruk]

It looks great! :) The alternative syntax seems like it might be better for more easily iterating over slices to avoid duplicate code. That was a frustration I had with a lot of embedded HALs in Rust, when I'm using a lot of pins. I get what they're trying to do with typestate to prevent programmer mistakes, tho!

The builder design pattern seems nice! I generally know what all the options are and what they mean at a glance. I would be totally happy to use this in my code :D

Thanks for the feedback!

The alternative syntax seems like it might be better for more easily iterating over slices to avoid duplicate code. That was a frustration I had with a lot of embedded HALs in Rust, when I'm using a lot of pins.

Would you mind elaborating on your use-case a little, maybe a brief example of your desired usage? The peripherals.slice_x() methods are just an extension trait on Peripherals providing some convenience methods over Peripherals, internally using the "alternative syntax" (still using generics).

It sounds like you'd like to do something like a for slice_id in [1u8, 2u8, 5u8, 7u8]? If so, then you'd probably also want a get_slice_pins(n: u8) -> PwmPins or similar so that generics (type safety) can still be used but you can still loop in a slice-agnostic manner? Where PwmPins is something like:

struct PwmPins {
    // Pins 0-20
    pin_a_left: PeripheralRef<'static, AnyPin>,
    pin_b_left: PeripheralRef<'static, AnyPin>,
    // Pins 21-34
    // Slice 7 "right" pins are unavailable for PWM
    pin_a_right: Option<PeripheralRef<'static, AnyPin>>,
    pin_b_right: Option<PeripheralRef<'static, AnyPin>>
}

[cylewitruk]

Branch rename killed this PR, so it's continued over at #2882

[crabdancing]

@cylewitruk

It sounds like you'd like to do something like a for slice_id in [1u8, 2u8, 5u8, 7u8]? If so, then you'd probably also want a get_slice_pins(n: u8) -> PwmPins or similar so that generics (type safety) can still be used but you can still loop in a slice-agnostic manner? Where PwmPins is something like:

Yes, that's exactly right! This can be resolved with macros, but it is nice to be able to do conventional, easy-to-follow Rust programming to handle lots of slices/pins/whatever, instead of being stuck with macros or boilerplate or -- often -- both at once. Generics would also be a solution for this, but the extra heap allocations may be unacceptable for some environments -- which I think just leaves the user with transmuting and not much else. Theoretically safe if every type is of the same size, but definitely not ideal to introduce unsafe just to iterate over slices.

Anyways, it's not a huge deal -- and if it doesn't fit with the rest of embassy-rs, it's totally reasonable to just do it in a way that matches the precedent in this library if you want. Just wanted to list everything I thought of!

Anyways, I'll subscribe to the other branch, so feel free to ping me over there if you'd like to continue the discussion that way. :)