PWM support

[therealprof]

I started implementing PWM for this crate in two slightly different ways:

Approach one

https://github.com/stm32-rs/stm32f0xx-hal/tree/features/pwm

This uses the typical approach of initialising the PWM peripheral with the timer and pins to use for PWM. The code is loosely based on https://github.com/stm32-rs/stm32f1xx-hal/blob/master/src/pwm.rs and I can see a number of advantages and disadvantages with this.

Advantages:

• Prevents misconfigurations of timer, channel and GPIO combinations

Disadvantages:

• Does not allow using one channel for multiple GPIOs at once
• Earth shattering number of timer, channel and GPIO combinations need to be encoded
• Someone might want to use PWM without assigned GPIO which is not possible in the current design

Approach two

https://github.com/stm32-rs/stm32f0xx-hal/tree/features/pwm-alternate

This approach only binds the timer to provide 4 PWM channels without any association to a GPIO.

Advantages:

• Allows for much more flexible configuration of zero or more GPIOs per channel

Disadvantages:

• No sanity checking for correctly configured GPIO pins and channel relations

---

I'm a bit on the fence which variant we should go for. Approach one is rather limiting, especially if we want to support advanced uses of PWM (and maybe other nice timer features). It is also very complex to implement for all chips and all possible combinations at once. On the other hand it would be great to have a way of telling a user that some timer/channel/GPIO combination will not work or is incorrectly configured.

Ideas?

CC @zklapow

[HarkonenBade]

I'd be very interested to take a crack at doing some trait witchcraft in the hope of getting the flexibility of the second solution while maintaining compile time verification. I'll see what I can do, though im not certain how much free time I will have to look at it immediately.

[david-sawatzke]

I think we should do the first variant. Being able to use multiple is also possible with variant one (by just configuring a pin to the af, without doing anything else with it), but that isn't the most used variant.

I'd also propose adding an Unused pin, implementing all the special function traits, if some pin is not going to be used. For example, having an Rx only UART, or not needing MISO with SPI. I've implemented a WIP Ws2812 driver https://github.com/david-sawatzke/ws2812-spi-rs using spi, that only needs the mosi pin, but with the current design sck and miso also need to be reserved.

Implementing PWM this way would make using it the most common way (one pin per channel) really obvious and safe, while also allowing the other variants with obvious patterns in the code.

As an aside:

I think having a type safe interface is important, even though it is a lot of overhead. Maybe we could consolidate all the different bindings for gpios in the future in a bindings.rs file, with all the afs implemented in one spot, this would also make comparing it with the data sheet easier.

[zklapow]

I agree with everything @david-sawatzke said.

I prefer the first approach because it gives us more compile time guarantees which is one of the main things I appreciate about doing embedded dev with Rust. I definitely like the idea of having an Unused pin.

[therealprof]

I think we should do the first variant. Being able to use multiple is also possible with variant one (by just configuring a pin to the af, without doing anything else with it), but that isn't the most used variant.

Unlike most other peripherals which are really only useful if they're actually connected to something this is not the case for the PWM (or other timer functions). While the PwmPin traits specifically target GPIO pins, the PWM mode of the timers can do a whole lot more which is completely unrelated from those traits, like trigger interrupt handlers or other timers. As I mentioned due to the way the timers work, defining all Timer/Channel/Pin combinations is going to be a ton of work. I'm not convinced defining Unused pins is the best way to go here.

But you're right we should have this for other peripherals...

NB: We already have those filler types here https://github.com/stm32-rs/stm32f4xx-hal/ and could reuse that approach.

[therealprof]

@zklapow

I prefer the first approach because it gives us more compile time guarantees which is one of the main things I appreciate about doing embedded dev with Rust. I definitely like the idea of having an Unused pin.

This issue is not about about sacrificing compile time guarantees but about usability and flexibility. The Unused pin approach is not only more cumbersome to implement and use but also limits the usability because (as mentioned) you can actually drive multiple pins with one channel (and sometimes this is an important feature) which this model can not support.

[david-sawatzke]

It does support it by virtue of just being able to configure other pins to the right af. This is (afaict) also what you'd need to do in approach 2, so no usability is lost, and it does make the more unusual and more often wrong approach of driving multiple pins with the same pwm channel possible, but hard to do accidentally

[therealprof]

It does support it by virtue of just being able to configure other pins to the right af.

And how is that a safe interface?

This is (afaict) also what you'd need to do in approach 2,

No, the idea is a different one: You configure a channel and then you can assign the correct pins in the correct configuration to the appropriate channel, turning them into a PwmPin and granting access to the channel control.

so no usability is lost, and it does make the more unusual and more often wrong approach of driving multiple pins with the same pwm channel possible, but hard to do accidentally

Sorry, I could not disagree more. It is very common to drive as many pins as possible with the least amount of timers (and potentially also use some extended features like safety mode) or to use complimentary PWM signals. This is a very important use case and should be just as safe and sane to use as the "fade this single LED" implementation.

[david-sawatzke]

You configure a channel and then you can assign the correct pins in the correct configuration to the appropriate channel, turning them into a PwmPin and granting access to the channel control.

I don't see it.

The basic api is

let (c1, c2, c3, c4) = tim4(TIM4, Hertz(4), clocks).split();
c1.enable();
c1.set_duty(10);

And the pins just implement the CHx traits, which aren't used anywhere, so there you'd just need to set them to correct af and leave them alone, like the multi pin approach with method 1.

If @HarkonenBade implements a method to attach multiple pins, we should distinguish complimentary and normal channels anyway.

I do agree that it would be nice if the additional pins where type-checked somewhere, but that's not what approach 2 is doing (afaict).

[HarkonenBade]

Mmm, Also part of me feels like an appropriate thing to do would be to handle the AF selection internally. It feels like a more pleasing interface to just require say an gpioa::PA0<MODE> and internally we select the right AF for that pin.

[therealprof]

@david-sawatzke

I don't see it.
I do agree that it would be nice if the additional pins where type-checked somewhere, but that's not what approach 2 is doing (afaict).

Yeah, because it's not implemented yet and the main reason why I put it up for discussion (and possibly someone to come up with a clever implementation, cf. #26 (comment))

If @HarkonenBade implements a method to attach multiple pins, we should distinguish complimentary and normal channels anyway.

Seems you're quite the complexity junkie. 😏

[david-sawatzke]

Yeah, because it's not implemented yet

Ah, sorry, forgot about that

Seems you're quite the complexity junkie.

I'd hate to have a long debugging session with the exact wrong values until i look in the data sheet and see that it's inverted, so i think that's somewhat necessary

[therealprof]

@david-sawatzke

I'd hate to have a long debugging session with the exact wrong values until i look in the data sheet and see that it's inverted, so i think that's somewhat necessary

There're so many ways to mess up the setup. An inverted output will mostly likely be the very least of your problems and is easily corrected, too.

[therealprof]

Mmm, Also part of me feels like an appropriate thing to do would be to handle the AF selection internally. It feels like a more pleasing interface to just require say an gpioa::PA0 and internally we select the right AF for that pin.

Not sure there's a lot of benefit in that approach. Sounds like a ton of complexity (if possible to implement at all) with little benefit. If the interface is decent then rustc is quite good at pointing problems with wrong mode selections...

error[E0277]: the trait bound `stm32f0xx_hal::gpio::gpioa::PA2<stm32f0xx_hal::gpio::Alternate<stm32f0xx_hal::gpio::AF2>>: stm32f0xx_hal::serial::TxPin<stm32f0::stm32f0x2::USART2>` is not satisfied
  --> examples/serial_echo.rs:26:22
   |
26 |         let serial = Serial::usart2(p.USART2, (gpioa.pa2.into_alternate_af2(), gpioa.pa15.into_alternate_af1()), 115_200.bps(), clocks);
   |                      ^^^^^^^^^^^^^^ the trait `stm32f0xx_hal::serial::TxPin<stm32f0::stm32f0x2::USART2>` is not implemented for `stm32f0xx_hal::gpio::gpioa::PA2<stm32f0xx_hal::gpio::Alternate<stm32f0xx_hal::gpio::AF2>>`
   |
   = help: the following implementations were found:
             <stm32f0xx_hal::gpio::gpioa::PA2<stm32f0xx_hal::gpio::Alternate<stm32f0xx_hal::gpio::AF1>> as stm32f0xx_hal::serial::TxPin<stm32f0::stm32f0x2::USART2>>
   = note: required by `<stm32f0xx_hal::serial::Serial<stm32f0::stm32f0x2::USART2, TXPIN, RXPIN>>::usart2`

Maybe it would more sense to implement the Into trait for all GPIOs and change the impls to anything that intos into a particular mode. I just did a quick check and rustc is not capable of inferring the type though so this might also require deeper changes. Still sounds like a good idea anyway...

[bentwire]

Anything ever get decided on this? Would love to have PWM support integrated in to the HAL!