Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rp2040 SetDutyCycle is not implemented for pwm #2784

Open
mattiekat opened this issue Apr 6, 2024 · 8 comments
Open

rp2040 SetDutyCycle is not implemented for pwm #2784

mattiekat opened this issue Apr 6, 2024 · 8 comments

Comments

@mattiekat
Copy link

mattiekat commented Apr 6, 2024

Found a bit of a rough edge when trying to use a motor controller library and found that embedded_hal::pwm::SetDutyCycle is not implemented for any of the pwm types.

Looking at the rp2040-hal crate I see it was implemented for rp2040_hal::pwm::Channel maybe I am misunderstanding and this should be wrapped instead?

@crabdancing
Copy link

Yeah, I'm super confused how to actually use PWM since we don't have a set_duty :c

@cylewitruk
Copy link

cylewitruk commented Apr 21, 2024

Just ran into this as well trying to use an RGB LED module... It looks like according to the example that the Config's compare_b / top (for channel b) is the duty-cycle?

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

    let mut c: Config = Default::default();
    c.top = 0x8000;
    c.compare_b = 8;
    let mut pwm = Pwm::new_output_b(p.PWM_SLICE4, p.PIN_25, c.clone());

    loop {
        info!("current LED duty cycle: {}/32768", c.compare_b);
        Timer::after_secs(1).await;
        c.compare_b = c.compare_b.rotate_left(4);
        pwm.set_config(&c);
    }
}

@crabdancing
Copy link

crabdancing commented Apr 22, 2024

Yeah, it's weird. The docs have this to say about compare:

    /// The output on channel A goes high when `compare_a` is higher than the
    /// counter. A compare of 0 will produce an always low output, while a
    /// compare of `top + 1` will produce an always high output.
    pub compare_a: u16,
    /// The output on channel B goes high when `compare_b` is higher than the
    /// counter. A compare of 0 will produce an always low output, while a
    /// compare of `top + 1` will produce an always high output.
    pub compare_b: u16,

It also says at the top:

/// Note the period in clock cycles of a slice can be computed as:
/// `(top + 1) * (phase_correct ? 1 : 2) * divider`

This was super confusing to me, as it seems from the latter comment that compare_a & compare_b ought not to come into the period at all. I think what's happening is we're adjusting one clock (the one representing PWM duty cycle), and if that clock value is higher than another clock (system clock?), the pin will rise? I don't exactly get it, and unfortunately the actual code seems to be making use of low-level MCU features with a lot of magic numbers -- so it's hard to infer from that too.

@cylewitruk
Copy link

cylewitruk commented Apr 22, 2024

Yeah I would have expected an implementation more like the NRF & STM32 impls...

I can't seem to get the example for RP to work regardless (Pico W) 🤷

Have a look at the datasheet section 4.5 for more PWM details which help explain a bit of the above... but there is no straightforward conversion from MicroPython examples to this. For example, I don't see how to set the channel frequency... The input_pwm example uses counter() as the frequency (in Hz), but looking at other implementations set_counter() wouldn't be the correct mapping to the missing freq() method.

@cylewitruk
Copy link

@lulf @Dirbaio tagged you guys as you seem to have been the most recently active in issues..
It would be great to get some sort of response on this issue as we're three waiting for some kind of input...

@Dirbaio
Copy link
Member

Dirbaio commented Apr 22, 2024

current status is the API is low-level, you pass in the values that are directly configured in the hardware. The datasheet explains the meaning of those values.

I agree it's not the most intuitive thing, if someone wants to work on adding a higher-level API where you set the frequency and duty cycle and it does the calculations for you, a pull request would be welcome.

@cylewitruk
Copy link

current status is the API is low-level, you pass in the values that are directly configured in the hardware. The datasheet explains the meaning of those values.

I agree it's not the most intuitive thing, if someone wants to work on adding a higher-level API where you set the frequency and duty cycle and it does the calculations for you, a pull request would be welcome.

Thanks for the quick confirmation 🙏

@cylewitruk
Copy link

@mattiekat @crabdancing have a peek at #2855 -- it's a start anyway..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants