Skip to content

cpu/stm32x: unified PWM driver implementations#6186

Merged
aabadie merged 4 commits intoRIOT-OS:masterfrom
haukepetersen:opt_stm32_pwm
Dec 16, 2016
Merged

cpu/stm32x: unified PWM driver implementations#6186
aabadie merged 4 commits intoRIOT-OS:masterfrom
haukepetersen:opt_stm32_pwm

Conversation

@haukepetersen
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen commented Dec 7, 2016

(re)based on #6184

Saves some ROM and code size while vastly improving the maintainability of the STM CPUs...

@haukepetersen haukepetersen added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation State: waiting for other PR State: The PR requires another PR to be merged first labels Dec 7, 2016
@haukepetersen haukepetersen added this to the Release 2017.01 milestone Dec 7, 2016
@haukepetersen haukepetersen changed the title Opt stm32 pwm cpu/stm32x: unified PWM driver implementations Dec 7, 2016
Copy link
Copy Markdown
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is super useful.
I recently noticed that pwm was missing on most nucleo boards supported by RIOT. Thanks !

@OlegHahm OlegHahm added the Area: drivers Area: Device drivers label Dec 8, 2016
@temmihoo
Copy link
Copy Markdown

As I commented on #6192 the issue of PWM touches other entities as well.

All in all, PWM is not a specific peripheral in any mcu but is generally done utilizing hardware timer output compare functionality. Thus I think it could be better to abstract the pwm driver as utilizing underlying timer abstraction, which in turn would have relatively simple driver support for the real peripheral.

As to STM32 family, I personally think that supporting timers or pwm at BOARD level is outright wrong as it is the MCU that have the peripheral hardware on SoC die instead of boards. The MCU in turn are arranged so that the whole family of STM32* has four different timer types, of which individual MCU models have some combination of. The subfamily (F0...F7 etc etc) doesn't have much to do with what timers are on a specific model of MCU.

This in turn means that there is no such thing as "the timers in F3 series" as individual models inside the series differ quite a lot in how many which timers are in what register address. Notably the F3x4 has a high resolution timer for 10 channels, which the others do not have.

Now, I'm not opposing merging this PR but more like calling forth discussion on the topic of timer support in general. Most MCU families from all vendors have roughly similar timers, which usually do input capture and output compare in similar fashion. Some of these have functionality that aims at producing certain kinds of pwm but the timers themselves aren't actually pwm devices but counter registers connected to binary compare and clock along with dividers.

In #6192 I linked to a decent article on STM32 timers, which link I include here as well.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

All in all, PWM is not a specific peripheral in any mcu but is generally done utilizing hardware timer output compare functionality

Not in any case -> e.g. the NRF52 has a dedicated PWM unit

As to STM32 family, I personally think that supporting timers or pwm at BOARD level is outright wrong

Yes, but that is why we do not do that -> the implementation is done for the CPU, the board merely configures it (e.g. pin configuration, which is board specific).

In general: I agree that the current form of the interface does not cover all the possible timer/PWM setups out there, BUT the interface is not meant to! The aim is, to create a slim, portable, straight-forward, and easy to use access to a CPU's major peripherals. The interface was never meant to cover all the possible configuration options one has -> this would be close to impossible looking at all the different features offered by all the vendors. So if one wants to use (some vendor specific) advanced features (like the advanced 10 channel timer for the STM32F334), one can always write a specialized driver (and define a specialized API) for exactly that functional unit - of course by loosing some means of portability then.

@temmihoo
Copy link
Copy Markdown

@haukepetersen So in this STM32 case is the PWM implemented on top of stm32 timer abstraction which is on the other pull request? If so, just adding more timer types as supported would immediately make them available for pwm generation as well.

I didn't know Nordic had directly pwm generating hardware. Apparently its register api is geared to modulating the PWM with some calculated waveform. Interesting.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

@temmihoo not quite, the PWM implementation is impelemented independently from the low-level timer driver directly on register level. But it is still very simple to enable more PWM devices by adapting the straight-forward configuration in the periph_conf.h

@haukepetersen haukepetersen added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Dec 13, 2016
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Dec 13, 2016

rebased as #6184 was merged

as we do not build the periph drivers selectable currently,
the guard is triggered even for boards that do not have any
PWM defined and don't want to use it...
@haukepetersen
Copy link
Copy Markdown
Contributor Author

rebased and fixed issue with tests/driver_servo

Copy link
Copy Markdown
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Dec 16, 2016

and go !

@aabadie aabadie merged commit 79043d2 into RIOT-OS:master Dec 16, 2016
@haukepetersen haukepetersen deleted the opt_stm32_pwm branch December 16, 2016 15:00
@haukepetersen
Copy link
Copy Markdown
Contributor Author

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants