Creating abstractions over Pwm trait is not ideal

[amiraeva]

I'm working on a project in which I've defined an ActuatorMotor which wraps a Pwm channel on my device. As I'm refactoring to make ActuatorMotor generic over the specific Pwm hardware, I've found it annoying that all Pwm functions take Pwm::Channel by move.

Here's an example:

struct ActuatorMotor<P: Pwm> {
    pwm: P,
    channel: P::Channel,
}

impl<P: Pwm> ActuatorMotor<P>
{
    fn set_velocity(&mut self, v: Velocity) {
        // do stuff with velocity or whatever to get duty
        let duty = todo!();
        self.pwm.set_duty(self.channel, duty);
        // error: can't move out of self.channel
    }
}

A workaround that seems to work for now is to make channel store a closure which spits out instances of Pwm::Channel i.e. Fn() -> P::Channel. At a first glance, requiring a Clone and/or Copy bound on Pwm::Channel would solve my problem, or altering the method signatures in Pwm to instead take &mut Pwm::Channel instead of by move.

[ryankurte]

huh this can be frustrating! while we don't currently require Clone or Copy bounds from embedded-hal, if these are currently implemented for the types you require could you partially resolve this with a where: P::Channel = Clone (I can't remember the exact syntax for this, sorry if it's not quite right) bound on your ActuatorMotor impl?

[eldruin]

Have you tried PwmPin?

[amiraeva]

Thanks to both of you for getting back to me. The closure approach got too noisy, so I gave up and settled on using PwmPin. I just figured I'd chime in on my experience using Pwm in its current state before it gets stabilized. Why can't it take Channel as an &mut argument just like adc::Oneshot does for the pin argument in its methods?

[eldruin]

Thank you @amiraeva.
I would be fine with changing the methods to accept a mutable reference channel but we should see how that fits for HAL implementations for MCUs.

[David-OConnor]

Possibly related: It's not always ideal to represent PWM as a pin. For example, in STM32, PWM is controlled by timer peripherals. In F3, the PWM module used raw pointers so it could make it fit with the channel paradigm. I ended up adding PWM functionality to the timer struct, and was unable to make it work with the channel-based EH traits.

[ryankurte]

I ended up adding PWM functionality to the timer struct, and was unable to make it work with the channel-based EH traits.

interesting experience, are you able to share this code? it's an interesting use case, in my experience with PWM / timers it is not uncommon to run 2+ channels of PWM from a single timer but, the configuration changes between them tend to be somewhat restricted.

[David-OConnor]

stm32-rs/stm32f3xx-hal#141

Discussion in there is also relevant; has comments from me and the original, EH/channel-based PWM module author. You'll see the crux of the issue is that the trait is channel-based, while most of the settings for STM32 MCUs are not.

Eg, compare the code in that PR to the pwm.rs module, which uses the EH trait: https://github.com/stm32-rs/stm32f3xx-hal/blob/master/src/pwm.rs

[eldruin]

Fixed in #246