Swap PWM channel argument to pass by reference

[ryankurte]

Swap PWM channel arguments to be passed by reference to alleviate need for cloning / reconstructing channel objects per #244

[rust-highfive]

r? @therealprof

(rust_highfive has picked a reviewer for you, use r? to override)

[therealprof]

Makes sense to me but I'd second @eldruin comment (#244 (comment)) that it would be great to see a converted example.

[ryankurte]

yeah absolutely, just figured i'd get the ball rolling on this one

[hannobraun]

This PR changes the channel arguments into shared references, not mutable ones. I think this is a mistake.

The only method where taking a shared reference makes sense, in my opinion, is try_get_duty. All others make changes to the channel, at least semantically speaking (enabling/disabling it or setting its duty cycle). I think accepting a shared reference for these is wrong for the following reasons:

• Only having a shared reference might be inconvenient when implementing this method. An implementation might want to call methods on the PwmPin implementation that require a mutable reference.
• It could restrict how implementations can use borrowing to express valid/invalid use of the API. Usually having a shared reference to something means that something can't be modified elsewhere. If I want my HAL API to work like that, there's no way to enforce this, if my Pwm implementation allows users to do everything to a PWM channel using a shared reference.
• It is not in line with other traits in this crate, which use mutable references to express what semantically changes the referenced type, and what doesn't.

[ryankurte]

The only method where taking a shared reference makes sense, in my opinion, is try_get_duty. All others make changes to the channel, at least semantically speaking (enabling/disabling it or setting its duty cycle). I think accepting a shared reference for these is wrong for the following reasons:

i would pose the argument that the associated channel types are only index into the pwm peripheral, and that it's not the channel that is being mutated but the peripheral. This is where the state resides, the peripheral is a singleton where the channel indexes are not, and this is already specified in the API with &self and &mut self on the peripheral?

fn try_disable(&mut self, _: &Channel) -> Result<(), Self::Error> { unimplemented!() }
/// #     fn try_enable(&mut self, _: Channel) -> Result<(), Self::Error> { unimplemented!() }
/// #     fn try_enable(&mut self, _: &Channel) -> Result<(), Self::Error> { unimplemented!() }
/// #     fn try_get_duty(&self, _: Channel) -> Result<u16, Self::Error> { unimplemented!() }
/// #     fn try_get_duty(&self, _: &Channel) -> Result<u16, Self::Error> { unimplemented!() }

[hannobraun]

i would pose the argument that the associated channel types are only index into the pwm peripheral, and that it's not the channel that is being mutated but the peripheral.

I would totally agree with that reasoning, if Pwm was the only trait here. But we also have PwmPin, and while the relationship is not explicit, it makes sense to implement PwmPin for the type that is also used for Pwm::Channel. If that's the case, then you'd need a mutable reference to the PwmPin implementation to set the duty cycle via PwmPin::try_set_duty, but only a shared reference to the PwmPin impl to set it via Pwm::set_duty_cycle.

You don't have to do it that way, of course. Pwm::Channel and PwmPin could be totally different types. But that doesn't make a whole lot of sense to me. My interpretation from the previous paragraph is the only one that does make full sense to me right now: Pwm is a timer that can generate PWM signals. It can have multiple channels with different duty cycles, that all run on the same period (as defined by the timer). HALs can decide to implement PwmPin for these channels, to make it possible to set the duty cycle for each channel from different locations.

---

Now that I've written all of this, I'm thinking that the whole design may be sub-optimal. It's likely I'll provide a more detailed proposal later, but for now, how about this idea:

• Remove PwmPin
• Split Pwm into separate traits: pwm::EnableChannel, pwm::Period, pwm::DutyCycle
• Keep the Pwm trait as a supertrait that only defines Self::Channel

Then implementations can mix and match, to suit the hardware capabilities and their own design. They can implement all those traits for a single timer type, or additionally decide to provide the applicable implementations for each channel type too (with Self::Channel being set to () in that case).

[ryankurte]

You don't have to do it that way, of course. Pwm::Channel and PwmPin could be totally different types.

all good points! i think the split PwmPin case is interesting, it seems to me that to get PwmPin objects you would need to .split() a Pwm into types that contain a reference back to the initial Pwm (which the Channels cannot really contain)? otherwise you have both a Pwm implementation that can set outputs, and non-primitive Channel objects that can also do this but cannot easily be constructed or thrown around (something like Pwm::set_duty_cycle(Channel::A, ...) becomes, difficult, as does genericising over Channel = u8 or similar).

i am unsure about dropping PwmPin but your suggestions seem reasonable to me. i do wonder whether the Pwm case would be better represented with a channel index and an associated constant for the number of available channels because, the arbitrary channel type is going to make abstracting over a PWM instance much more difficult for a hypothetical PWM consumer.

[hannobraun]

all good points! i think the split PwmPin case is interesting, it seems to me that to get PwmPin objects you would need to .split() a Pwm into types that contain a reference back to the initial Pwm (which the Channels cannot really contain)? otherwise you have both a Pwm implementation that can set outputs, and non-primitive Channel objects that can also do this but cannot easily be constructed or thrown around (something like Pwm::set_duty_cycle(Channel::A, ...) becomes, difficult, as does genericising over Channel = u8 or similar).

First, a quick note: You'd typically want to implement such splitting by consuming the PwmPin impl and using unsafe register access internally in the Pwm impl (protected by critical sections, if necessary). A reference will make you very unhappy as soon as you want to move anything to another context (whether through RTIC or a plain static), so I strongly recommend against it.

Otherwise, I do agree with that paragraph, except that it made me realize another weirdness: If acquiring the PwmPin impls consumes the Pwm impl (or makes it inaccessible through references), then you can no longer use its methods (of course). But you also can't use its methods before that, because you don't have the PwmPin implementations yet.

Maybe I'm just not seeing how this API is meant to be implemented, but the more I think about it, the more I believe it needs a thorough redesign.

i am unsure about dropping PwmPin but your suggestions seem reasonable to me. i do wonder whether the Pwm case would be better represented with a channel index and an associated constant for the number of available channels because, the arbitrary channel type is going to make abstracting over a PWM instance much more difficult for a hypothetical PWM consumer.

Yes, good point. Maybe it would be best to just drop Channel completely. That would remove some flexibility in how to implement these traits, but that would probably a good thing.

I have an ongoing project that will need PWM traits soon (soon being relative; it's a side project and I'm busy with paying work). I'm currently thinking that, if I can't make the current traits work for my needs, it might be best if I create an embedded-pwm crate to replace the traits in embedded-hal. That would allow for some rapid iteration and eventually, everything could be merged back into embedded-hal.

[hannobraun]

I now believe that most of what I said has been wrong. I've summarized my insights for everyone who's interested, but as far as this PR is concerned, I now believe this should be merged as-is (and ideally released as alpha.3 soon).

@therealprof To be clear, by "converted example" you're referring to a HAL implementation that is updated to use these modified traits?

[ryankurte]

i had a quick go at modifying @eldruin's pwm-pca9685-rs but, this doesn't currently use the pwm traits, it's p difficult to map them to the operation of this particular device, and patching to e-h 1.0.0-alpha.2 causes a bug in cargo 🙃

[eldruin]

Yeah, sorry about that. I have been meaning to implement the pwm traits in the pwm-pca9685-rs crate but did not find time yet.

[hannobraun]

I implemented the modified trait in LPC8xx HAL: lpc-rs/lpc8xx-hal#292

Implementations (very ugly, for reasons unrelated to this PR): https://github.com/braun-embedded/lpc8xx-hal/blob/pwm-ryankurte/src/ctimer/peripheral.rs#L569-L768

Usage example: https://github.com/braun-embedded/lpc8xx-hal/blob/pwm-ryankurte/examples/ctimer_blink.rs#L55-L84

I don't have a generic driver that uses these traits yet, and won't have for at least a few weeks.

[ryankurte]

ahhh also i wonder whether it important that we're eliding phase at the moment? often you setup a PWM with a specific on and off count (across multiple channels), rather than just a period, for things like BLDC control

[eldruin]

Setting the phase is actually the way to configure the PWM for the PCA9685. There you just configure for each channel channel when it goes on and when it goes off with numbers in the range of [0-4096] (4095 + full on, actually).

[hannobraun]

Good point about the phase. I suggest handling that in a follow-up PR though. This one already is a clear improvement as-is and I'd like to see it merged rather sooner than later.

[ryankurte]

@therealprof are you happy with @hannobraun's implementations of this? worth note that we've gone for a shared reference rather than a mutable one (following a bunch of discussion on #256)

[therealprof]

I haven't checked. I trust @eldruin expertise here.

[eldruin]

I believe this is fine as-is. Sorry for the delay! I would be happy to approve it but it seems this needs to be rebased first.

[adamgreig]

Just a ping from this week's meeting, it looks like this PR could probably be merged after a rebase, @ryankurte ?

[eldruin]

Thanks!

bors r+

[eldruin]

Ach, @ryankurte could you run rustfmt?

[bors]

Build succeeded:

• ci-linux (1.35.0, x86_64-unknown-linux-gnu)
• ci-linux (stable, thumbv6m-none-eabi)
• ci-linux (stable, thumbv7m-none-eabi)
• ci-linux (stable, x86_64-unknown-linux-gnu)