Skip to content

drivers: Optimized periph PWM interfaces#3644

Closed
haukepetersen wants to merge 16 commits intoRIOT-OS:masterfrom
haukepetersen:opt_periph_pwm
Closed

drivers: Optimized periph PWM interfaces#3644
haukepetersen wants to merge 16 commits intoRIOT-OS:masterfrom
haukepetersen:opt_periph_pwm

Conversation

@haukepetersen
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen commented Aug 17, 2015

Updated the PWM interface slightly:

  • changed some types to explicit width
  • removed return value from pwm_set
  • made types definable by CPUs
  • EDIT: simplified error return code of pwm_init

I did however not yet optimize any of the existing PWM drivers with the new possibilities...

@haukepetersen haukepetersen added Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Aug 17, 2015
@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Aug 18, 2015

@A-Paul might be interested

A-Paul
A-Paul reviewed Aug 19, 2015
View reviewed changes
drivers/include/periph/pwm.h Outdated
Copy link
Copy Markdown
Member

@A-Paul A-Paul Aug 19, 2015

Choose a reason for hiding this comment

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

I don't understand this.
Also found no place in your changes where it is used.

Copy link
Copy Markdown
Contributor Author

@haukepetersen haukepetersen Aug 24, 2015

Choose a reason for hiding this comment

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

It is not used (yet). This define provides a universal means of accessing the different PWM devices in a more programmable fashion, while still providing the possibility for each CPU to override this default behavior. Have a look at the GPIO define and it's usage in drivers/include/periph/gpio.h and how some CPUs use it (e.g. cpu/stm32f4/include/periph_cpu.h)...

@A-Paul
Copy link
Copy Markdown
Member

A-Paul commented Aug 19, 2015

In your change list you forgot to mention the interface change for pwm_init() regarding error codes.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Aug 24, 2015

@A-Paul: added it to the list of changes.

rebased.

@OlegHahm OlegHahm modified the milestone: Release NEXT MAJOR Sep 2, 2015
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Oct 20, 2015

@A-Paul ping?!

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Oct 20, 2015

rebased

@haukepetersen haukepetersen added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 20, 2015
@A-Paul
Copy link
Copy Markdown
Member

A-Paul commented Oct 20, 2015

@A-Paul ping?!

Huh??
Gimme a hint. What did I miss?

@LudwigKnuepfer
Copy link
Copy Markdown
Member

LudwigKnuepfer commented Oct 21, 2015

@A-Paul I think @haukepetersen wants your approval.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Oct 21, 2015

Yap, the all reoccurring question about the ack :-)

But I just see, that Travis is complaining, I look into it quickly

@A-Paul
Copy link
Copy Markdown
Member

A-Paul commented Oct 21, 2015

I'm kind of sorry for the latency ...
However, I just wanted to assist by commenting some things that attracted my attention. I didn't realize that I'm suddenly in charge for the final ack, particularly without being assigned.
Furthermore I'm not shure if I have the needed insight to check all the consequences of that API change.

@haukepetersen haukepetersen assigned A-Paul and unassigned kaspar030 Oct 21, 2015
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Oct 21, 2015

No problem. Now you are assigned :-) But nevertheless, not being assigned doesn't mean you can't ack something...

I reworked this PR again slightly by adding a pwm_channels() function that is very useful for testing and run-time channel assignments. Also Travis should now hopefully be happy...

@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Oct 21, 2015

@A-Paul I think you have the needed insights and also Travis will be your helping hand

@LudwigKnuepfer
Copy link
Copy Markdown
Member

LudwigKnuepfer commented Oct 22, 2015

@A-Paul Don't Panic!

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Oct 23, 2015

Travis is not happy -> I will introduce a fix for that separately, so lets put this PR on hold until the fix is merged. I will update this PR then accordingly...

@A-Paul A-Paul added the State: waiting for other PR State: The PR requires another PR to be merged first label Oct 26, 2015
@miri64
Copy link
Copy Markdown
Member

miri64 commented Nov 27, 2015

What's the fix? Which PR is it waiting on?

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Nov 27, 2015

#3420

@haukepetersen haukepetersen added CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Dec 9, 2015
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Dec 9, 2015

Ok, added some guards to the old style PWM driver implementations, now Travis should be happy with this PR. I would propose to merge this if anyone is ok with the API changes and than introduce updated PWM drivers for the affected CPUs separately...

So this PR is not waiting on any other PR anymore, all it needs is an ACK and squashing...

@haukepetersen haukepetersen mentioned this pull request Dec 9, 2015
Closed
@DipSwitch
Copy link
Copy Markdown
Member

DipSwitch commented Dec 9, 2015 via email

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Dec 10, 2015

Not sure I understand you problem correctly. Do you mean mapping the (timerdev, cc-channel) tuple to logical lines as done in the ADC and DAC interfaces? This would IMHO not be feasible here, as the programmed resolution and freq is the same for all channels of a timer device.

If you mean simple remapping of MCU timer cc-channels to logical RIOT channels (e.g. tim4-ch3 -> PWM_0-CH0), this is possible out of the box.

@DipSwitch
Copy link
Copy Markdown
Member

DipSwitch commented Dec 10, 2015 via email

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Dec 10, 2015

yep.

DipSwitch added a commit to DipSwitch/RIOT that referenced this pull request Dec 13, 2015
@DipSwitch
Update PWM driver to RIOT-OS#3644
085fe34
@jnohlgard jnohlgard mentioned this pull request Jan 14, 2016
Merged
@A-Paul
Copy link
Copy Markdown
Member

A-Paul commented Jan 15, 2016

@haukepetersen should this be closed in favor of #4638?

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Jan 20, 2016

yes.

DipSwitch added a commit to DipSwitch/RIOT that referenced this pull request Mar 16, 2016
@DipSwitch
Update PWM driver to RIOT-OS#3644
3152e35
DipSwitch added a commit to DipSwitch/RIOT that referenced this pull request Mar 29, 2016
@DipSwitch
Update PWM driver to RIOT-OS#3644
fc81e99
@haukepetersen haukepetersen deleted the opt_periph_pwm branch March 8, 2017 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@A-Paul A-Paul

Labels

CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@haukepetersen @PeterKietzmann @A-Paul @LudwigKnuepfer @miri64 @DipSwitch @OlegHahm @kaspar030