RP2040 PWM higher-level API

[cylewitruk]

Renamed the branch on my end which automatically closed PR #2855 - this PR is the continuation. Proposes a solution for issue #2784 as well as some additional improvements.

This PWM PR:

• Provides a higher-level API using the builder pattern, attempting to cover the most common use-cases.
• Adds support for edge timing using 32-bit DMA which allows measuring frequencies down to ~1Hz.
• Provides simple APIs for specifying duty cycle etc.

Outstanding:

• Ensure all code docs are in place and thorough.
• Finish the unchecked versions of apply (to pass uint "numbers" instead of peripherals, for those who wish to evade the typestate checks).

Remaining Questions:

• Is this API something that people like? Could we think of expanding the builder pattern into other areas?
• How should this be structured? Currently I have things in a v2.rs to not invade the current implementation.
• Are there any other common use-cases which should be covered?

[cylewitruk]

@crabdancing

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.

Couldn't agree more about trying to be as Rust-idiomatic as possible 😊

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.

In this case everything is static dispatch and expanded at compile-time (and thus you can also enforce constraints at compile-time as well) - I don't think dynamic dispatch (trait objects a'la dyn = heap allocs) are used anywhere -- anyway, I agree that avoiding them is paramount in embedded.

The unfortunate downside so that is that you can't iterate over trait impl's of different types, so this kind of API would need to be a sort of abstraction which works directly with the lower-level API's.

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!

That's what I was looking for 😊 In general I think that three layers of indirection would be good for a suite like embassy to enable all forms of :

1. The PAC (very-low-level, direct access to HW)
2. Intermediate HAL API's (like the current state of the PWM API's)
3. Self-documenting builders/configurators on top of pt. 2 (and pt. 1 where necessary).
   That would enable clear code for many cases but A) not break compatibility for current users, and B) be beginner-friendly without hindering advanced usage where needed.

Nothing that's been discussed with the embassy maintainers yet, but hoping they'll chime in here soon enough 😅 I'm personally new to embassy so I'm writing mostly out of my own "what I would have liked" perspective. Having to refer so much to the datasheet, which in itself can be found lacking/confusing, isn't super productive haha...

[cylewitruk]

@crabdancing give it a try now :) Here's what I get for the pwm.rs example:

Note I added your "bouncy" program to examples as well.

[crabdancing]

The example looks fine to me so far :)

Looks like it keeps working if I change it up to 41 MHz, or down to 8 Hz. I'm not sure why I can't get it above 41Mhz, but that seems ok for most use cases.

The only potential significant issue I see is that it's showing 508Hz for both of us, instead, of 500.

If I do:

.frequency(Frequency::Hz(500 - 8))

I finally get exactly 500 Hz. Also if I change it to 600, I end up getting 612. Not sure if the oscilloscopes are inaccurate or if there's something slightly wrong with the math. Regardless, it's working way better than before!

[cylewitruk]

Looks like it keeps working if I change it up to 41 MHz, or down to 8 Hz. I'm not sure why I can't get it above 41Mhz, but that seems ok for most use cases.

Can you get a better representation if you use 10x?

The only potential significant issue I see is that it's showing 508Hz for both of us, instead, of 500.

If I do:

.frequency(Frequency::Hz(500 - 8))

I finally get exactly 500 Hz. Also if I change it to 600, I end up getting 612. Not sure if the oscilloscopes are inaccurate or if there's something slightly wrong with the math.

Aha, I thought it was just my scope that might have been off, but if you're getting it too then there could definitely be something slightly off with the fractional divider... seems it's a bigger problem down in Hz, if I set 30KHz then I get 30KHz spot-on on the scope. Here's the math code - do you see anything that could be improved? This has been the hair-puller of all of this, there's basically no "definitive example" (that I could find) on how to do this so that it's accurate across the entire frequency range 🤯 For each frequency there's a number of div/top combinations that work.

[cylewitruk]

Note: API has been updated to accept slice/pins in the apply method, so configurations are slice/pin agnostic. Also introduced typestate to help guide the API usage (i.e. apply will only accept one pin of type A if only with_output_a() is called, same for B, or require A+B if both with_output_a() & with_output_b() are called.

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

let slice0 = Pwm::builder()
    .free_running()
    .frequency(Frequency::Hz(30_000))
    .phase_correct(false)
    .with_output_a(|a| a.duty_cycle(50.0))
    .with_output_b(|b| b.duty_cycle(75.0).invert(true))
    .apply(peripherals.PWM_SLICE0, peripherals.PIN_16, peripherals.PIN_17)
    .expect("Failed to configure slice");

slice0.enable();

I'll add an apply_unchecked() method where you can pass in usize's for slice and pin numbers as well to meet your wishes for being able to iterate.

If we can get the math sorted out then I think this PR can be ready for review this week 😄

[crabdancing]

Awesome! I'll try to comb through it later and help, if I manage to get some free time. Are there specific formulas in the RP2040 datasheet I can reference? It's >600 pages, so it's kinda hard to sort through. :P

Edit: if that fails, might also be worth checking out the C++ SDK to see what logic they're doing. Or maybe open an issue with the Raspberry Pi people? 🤔

[cylewitruk]

Awesome! I'll try to comb through it later and help, if I manage to get some free time. Are there specific formulas in the RP2040 datasheet I can reference? It's >600 pages, so it's kinda hard to sort through. :P

Haha, tell me about it 😅 Everything PWM is in section 4.5 with some formulas on page 531.

Edit: if that fails, might also be worth checking out the C++ SDK to see what logic they're doing. Or maybe open an issue with the Raspberry Pi people? 🤔

Yeah I've looked there and at other implementations as well as related forum Q's -- if it was just trying to get a specific duty cycle then we could just use 10,000 as TOP and then compare is easy to calculate. It's when trying to account for both frequency and duty % where it gets a little hairy, because DIV and TOP are what set the slice's frequency, which needs to be calculated first and then the compare value can be calculated based on that TOP value... I think we're getting rounding errors in the current impl tbh.

EDIT: @crabdancing give the latest a try, I switched the math to floating point and now I get 500Hz exactly, all the way down to 8Hz 😄 🎉

It'll be software FP so I'm considering introducing some options for using less-precise-but-faster integer math and maybe an option to set duty cycle at sysclock frequency (i.e. use 10,000 as top) and avoid much math -- for those who need to update things rapidly.

[cylewitruk]

@Dirbaio Invite you to take a quick peek at this PR while I'm buttoning it up in-case you have any design/code comments before I convert it from draft 🙏 I'm working on polishing the level/edge-sensitive areas as well as introducing a high-accuracy u32 counter with help of DMA (for pulse/freq measuring), as well as additional examples until then.

Right now I've got a v2.rs just to keep my changes a bit separate.. I'm keen on hearing opinions on how you guys would like this to be integrated w/o breaking changes (should it go i to mod.rs or should it be separate?)

[crabdancing]

@cylewitruk Sorry about not testing this for awhile. My picoprobe got fried somehow, and I have to make another. I'll check it out sometime today if I can make the time to build another one.

[cylewitruk]

@cylewitruk Sorry about not testing this for awhile. My picoprobe got fried somehow, and I have to make another. I'll check it out sometime today if I can make the time to build another one.

Ouch! No worries, I'm wrestling with the input side and DMA atm and hoping to get some feedback from maintainers in the meantime 😊

[crabdancing]

Sorry, my job had to pivot to a bunch of frontend stuff and I couldn't find the time to build another picoprobe until just now. The PWM init code is working beautifully! Thanks so much for your work on this, @cylewitruk! I tested it all the way down to 8Hz, and it works all the way up to 40 Mhz! :) And the 500 Hz example is spot on, on my test rig, too. Awesome stuff.

[crabdancing]

After some further testing, there's some sort of bug in the calculations for set_duty_cycle, as my latest pull of feat/rp2040-pwm-api gives a panic with Frequency below is too high when trying to call set_duty_cycle -- even though the frequency is set is well within supported frequencies:

//! This example shows how to use PWM (Pulse Width Modulation) in the RP2040 chip.
//!
//! The LED on the RP Pico W board is connected differently. Add a LED and resistor to another pin.

#![no_std]
#![no_main]

use defmt::info;
use embassy_executor::Spawner;
use embassy_rp::pwm::prelude::*;
use embassy_time::{Duration, Timer};
use {defmt_rtt as _, panic_probe as _};

#[embassy_executor::main]
async fn main(_spawner: Spawner) {
    // Initialize peripherals
    let peripherals = embassy_rp::init(Default::default());

    let mut slice0 = Pwm::builder()
        .free_running()
        .frequency(Frequency::Hz(100_000))
        .phase_correct(false)
        .with_output_a(|a| a.duty_cycle(100.0))
        .with_output_b(|b| b.duty_cycle(75.0))
        .apply(peripherals.PWM_SLICE0, peripherals.PIN_16, peripherals.PIN_17)
        .unwrap();

    slice0.enable();

    slice0.set_duty_cycle(embassy_rp::pwm::v2::Channel::A, 50.0).unwrap();
    slice0.set_duty_cycle(embassy_rp::pwm::v2::Channel::B, 50.0).unwrap();
    loop {
        info!("tick!");
        // Wait for a second
        Timer::after(Duration::from_secs(5)).await;
    }
}

Note that setting the duty cycle directly with:

        .with_output_a(|a| a.duty_cycle(50.0))
        .with_output_b(|b| b.duty_cycle(50.0))

does not produce this result.

Here's the output:

0.008311 DEBUG Setting A pin function to PWM
└─ embassy_rp::pwm::builder::free_running::{impl#16}::new_from_config @ /home/ada/Projects/rs/embassy-cylwit/embassy-rp/src/fmt.rs:130
0.008904 DEBUG
Freq = 100000.0
Top = 1249
Div = 0x01.0
Out = 100000.0
└─ embassy_rp::pwm::v2::{impl#6}::calculate_div_and_top @ /home/ada/Projects/rs/embassy-cylwit/embassy-rp/src/fmt.rs:130
0.009268 DEBUG Changing frequency to 100000Hz (from Hz=125000000, TOP=65536, DIV=1)
└─ embassy_rp::pwm::v2::{impl#6}::reconfigure @ /home/ada/Projects/rs/embassy-cylwit/embassy-rp/src/fmt.rs:130
0.009566 DEBUG Frequency changed to 100000Hz (TOP=1249, DIV=16)
└─ embassy_rp::pwm::v2::{impl#6}::reconfigure @ /home/ada/Projects/rs/embassy-cylwit/embassy-rp/src/fmt.rs:130
0.009815 DEBUG Updating compare value for channel A to 1249
└─ embassy_rp::pwm::v2::{impl#6}::reconfigure @ /home/ada/Projects/rs/embassy-cylwit/embassy-rp/src/fmt.rs:130
0.010066 DEBUG Setting B pin function to PWM
└─ embassy_rp::pwm::builder::free_running::{impl#16}::new_from_config @ /home/ada/Projects/rs/embassy-cylwit/embassy-rp/src/fmt.rs:130
0.010352 DEBUG
Freq = 100000.0
Top = 1249
Div = 0x01.0
Out = 100000.0
└─ embassy_rp::pwm::v2::{impl#6}::calculate_div_and_top @ /home/ada/Projects/rs/embassy-cylwit/embassy-rp/src/fmt.rs:130
0.010669 DEBUG No changes have been made to the frequency (100000Hz).
└─ embassy_rp::pwm::v2::{impl#6}::reconfigure @ /home/ada/Projects/rs/embassy-cylwit/embassy-rp/src/fmt.rs:130
0.010822 DEBUG Updating compare value for channel B to 936
└─ embassy_rp::pwm::v2::{impl#6}::reconfigure @ /home/ada/Projects/rs/embassy-cylwit/embassy-rp/src/fmt.rs:130
0.011098 ERROR panicked at 'Frequency below is too high ...'
└─ embassy_rp::pwm::v2::{impl#6}::calculate_div_and_top @ /home/ada/Projects/rs/embassy-cylwit/embassy-rp/src/fmt.rs:106
0.011245 ERROR panicked at /home/ada/.cargo/registry/src/index.crates.io-6f17d22bba15001f/defmt-0.3.6/src/lib.rs:368:5:
explicit panic
└─ panic_probe::print_defmt::print @ /home/ada/.cargo/registry/src/index.crates.io-6f17d22bba15001f/panic-probe-0.3.1/src/lib.rs:104

That said, the init code is at least doing exactly what it's supposed to. I see set_duty_cycle and apply both call reconfigure (albeit with some intermediate funcs in the call stack) so the bug is probably caused by a step that apply/new_from_config do that is not happening with a simple reconfigure call by itself (as is done by set_duty_cycle).

Anyways, great job again! I think its almost perfect. :3

[cylewitruk]

@crabdancing I've also been a bit busy the last week so haven't spent much time here -- thanks for finding that! I'll try to look at it in the coming days 😄 Really happy that the init code seems to be working fine now though, that means that the math in that path is at least correct 😅