PWM implementation

[thalesfragoso]

I still have to test this on hardware and add some documentation and examples. I will try to test this on the weekend but it would be nice to get some feedback already w.r.t the implementation.

#124 should be merged first.
Closes #67.

PS: Inspired by the implementation in the F1 HAL.

[thalesfragoso]

Ok, this got a bit out of hand. There are some differences between the available timers, because of that different macros were necessary. The way it's now it should work with all timers, but it looks a bit ugly and maybe there is a better way to do it.

I would like some feedback and I would like to know if there's a better approach to this.

PS: For some reason TIM5 interface on stm32f410 is different of any other timer in any supported chip, maybe there's a problem in the svd. For now I had to add one more macro to take care of this case.

[danclive]

Looking forward to this feature！

[thalesfragoso]

@danclive This seems to be working ok already, but I didn't have the opportunity to check everything on the oscilloscope, I tried on the stm32f401. If you can do some tests that would be helpful.

[cjblocker]

I've tested this on an stm32f415 using most of the channels from TIM1,2,3,4,8 and everything seems to be working great.
I'm happy to run any hardware tests if needed, though I only have the f415 on hand.

[thalesfragoso]

Thanks @cjblocker , have you tested if the frequency/period is correct ?
I will see if I can get this PR ready for review this week (probably by the weekend).

[therealprof]

@thalesfragoso Could you rebase this?

[thalesfragoso]

Sure, I just want to polish some stuff and then I will do the rebase. I will see if I can find the time in the next few days.

[cjblocker]

So I looked at the frequency on the scope. It's accurate for low-frequencies (500Hz read at about 502Hz, 20khz read about 20.01khz). But for higher frequencies, say 1 MHz, where my returned max duty is only 16, I get about 950khz. Putting a slightly higher frequency so that my max duty cycle is 15, then I actually get 1.001 Mhz on the scope, which is closer to what I wanted.

So maybe there is an off-by-one error, and it doesn't affect the lower frequencies/larger counts as much? I don't think this API is meant to guarantee any arbitrary frequency anyway though, as integer division and integer counters can only do so much.

[TeXitoi]

@cjbloqker did you use_hse in your test? Without that, you'll use the ISE clock that is precise at about +-1-5%

[cjblocker]

Oh good call, I was not. That will probably explain my errors. I can rerun my tests later.

[cjblocker]

That was the problem. Frequencies look perfect now.

[thalesfragoso]

I guess this is ready for review. I'm not quite delighted by this implementation but its quirks seemed necessary to be able to support all these different timers in all these different parts. I'm all open to ideas to improve this.

[jkboyce]

@thalesfragoso thanks for the pwm driver! It works on my Nucleo-F446RE board and output is as expected on the scope. I haven't tested many timers, frequencies, and duty cycles though. A couple of questions:

1. How does one configure PWM for just a single output channel on a given timer? I.e., the first two lines compile but the third does not:

    let tim3_channels = pwm::tim3(dp.TIM3, (pa6, pa7, pb0, pb1,), clocks, 50.hz()); // 4 channels, OK
    let tim3_channels = pwm::tim3(dp.TIM3, (pa6, pa7,), clocks, 50.hz()); // 2 channels, OK
    let tim3_channels = pwm::tim3(dp.TIM3, (pa6,), clocks, 50.hz()); // 1 channel, fails

Error is:
the trait `stm32f4xx_hal::pwm::Pins<stm32f4::stm32f446::TIM3, _>` is not implemented for `(stm32f4xx_hal::gpio::gpioa::PA6<stm32f4xx_hal::gpio::Alternate<stm32f4xx_hal::gpio::AF2>>,)`

I'm a Rust learner so I can't discern why your pins_impl! macro doesn't handle this case.

2. Is it possible to send a given channel to multiple pins? The stm32f3xx-hal pwm module allows this, but here the Pins<TIM, P> marker trait seems to be enforcing a constraint that only one output pin is allowed per channel. I'm just wondering if this is as intended. :)

[thalesfragoso]

Hey @jkboyce , thanks for testing.

• 1, Try:

let tim3_channel = pwm::tim3(dp.TIM3, pa6, clocks, 50.hz());

• 2, Hmm, I think if you just put the other pin in the correct alternate function it would just work, thinking about it now, it seems kinda bad, but I don't see how we could prevent that. But I think I can add an implementation accepting more than one pin per channel. Something like this:

let tim1_channels = pwm::tim1(dp.TIM1, ((pa8, pe9), pa9), 50.hz());

[thalesfragoso]

Ok @jkboyce , now you should be able to do something like this:

let channels = (
    (pa6, pb4),
    pa7,
);
let tim3_channels = pwm::tim3(dp.TIM3, channels, clocks, 50.hz());

Assuming all the pins are in the right configuration. Could you test this on the hardware for me ? Thanks.

[jkboyce]

Hi @thalesfragoso , thanks for the tips. Nice work! Confirming that your solution works for me for 1 channel. Also you are correct, with your previous version I was able to add more pins to the output by calling tim() with one of the channel pins, then putting the other pin into the correct AF. I think this is a perfectly good solution.

Your update works as well, specifically I tried this snippet and verified it on the scope:

    let tim3_channels = pwm::tim3(
        dp.TIM3,
        ((pa6, pb4), pa7, pb0, pb1),  // changing pb4 to pb5 gives a compile error
        clocks,
        50.hz(),
    );

I see that the pins themselves are unused in the call to pwm::tim() but their marker traits are used to decide which channels to enable. Given this, it's a judgment call whether it's worth the extra complexity to support nested tuples, for I guess the bit of extra type-checking it gives. The other potential gotcha is that there may not be a way to enable two output pins for a single channel? (E.g., what if above I'd wanted only pins pa6 and pb4 for TIM3, both corresponding to channel 1.) Maybe it's good to keep it simple?

[thalesfragoso]

I see that the pins themselves are unused in the call to pwm::tim() but their marker traits are used to decide which channels to enable. Given this, it's a judgment call whether it's worth the extra complexity to support nested tuples, for I guess the bit of extra type-checking it gives. The other potential gotcha is that there may not be a way to enable two output pins for a single channel? (E.g., what if above I'd wanted only pins pa6 and pb4 for TIM3, both corresponding to channel 1.) Maybe it's good to keep it simple?

Hmm, I don't quite understand, do you mean this ?

let tim3_channel = pwm::tim3(
    dp.TIM3,
    (pa6, pb4),
    clocks,
    50.hz(),
);

I think this should just work and give you a pwm channel (not a tuple of channels).
Also, the complexity added is quite minimum, I didn't have to change any logic of the implementation itself, I just added a couple of lines to say that if you have a tuple of types and all of them implements the PinCx trait for a TIMx then the tuple itself also implements the trait for the timer. I think it's worth keeping since it gives more type checking without any overhead.

[therealprof]

@thalesfragoso Looks good to me. Mind fixing the merge conflict?

[thalesfragoso]

@therealprof Done.

[therealprof]

Excellent, thanks a lot!