Skip to content

drivers: Optimized periph PWM interfaces [adopted]#4638

Merged
jnohlgard merged 11 commits intoRIOT-OS:masterfrom
jnohlgard:pr/periph-pwm-api-change
Feb 14, 2016
Merged

drivers: Optimized periph PWM interfaces [adopted]#4638
jnohlgard merged 11 commits intoRIOT-OS:masterfrom
jnohlgard:pr/periph-pwm-api-change

Conversation

@jnohlgard
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard commented Jan 14, 2016

This is @haukepetersen's #3644, rebased on latest master, squashed and additionally updated the servo API to return void on servo_set in order to match the API change for pwm_set.

There was a merge conflict in the STM32F4 driver stemming from #4454. The change in #4454 was copied with only the change rename the frequency variable freq

@jnohlgard jnohlgard 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. CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 14, 2016
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Jan 14, 2016

Reviewing this API change is somewhat low-hanging fruit compared to some of the other periph API changes (e.g. #4430, #3216, #3929, #4040) , since PWM is not widely implemented by the supported CPUs and not widely used either.

@jnohlgard jnohlgard force-pushed the pr/periph-pwm-api-change branch from 4b286c1 to 348c1b2 Compare January 15, 2016 14:24
@jnohlgard jnohlgard added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Jan 15, 2016
@A-Paul A-Paul mentioned this pull request Jan 15, 2016
Closed
@haukepetersen
Copy link
Copy Markdown
Contributor

haukepetersen commented Jan 20, 2016

+1 one for the PR, but I can't really ACK it, since most of the changes I did myself...

@kYc0o
Copy link
Copy Markdown
Contributor

kYc0o commented Jan 25, 2016

Can I help to ACK?

jnohlgard
jnohlgard reviewed Jan 26, 2016
View reviewed changes
drivers/include/periph/pwm.h
Copy link
Copy Markdown
Member Author

@jnohlgard jnohlgard Jan 26, 2016

Choose a reason for hiding this comment

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

Any opinions on limiting value to 16 bits?

Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen Jan 26, 2016

Choose a reason for hiding this comment

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

yes, I think its good!

Copy link
Copy Markdown
Member

@A-Paul A-Paul Jan 28, 2016

Choose a reason for hiding this comment

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

What are the advantages?

@A-Paul
Copy link
Copy Markdown
Member

A-Paul commented Jan 26, 2016

The trailing underscores in the periph_conf.h's guards are addressed in onother PR right?

@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Jan 26, 2016

@A-Paul There are some PRs open for include guards, but I don't know if these files are fixed there specifically. Regardless, it is better to do a separate PR for the include guards rather than extending the life of this PR..

@A-Paul
Copy link
Copy Markdown
Member

A-Paul commented Jan 26, 2016

Has someone already run the test on any HW?

@A-Paul
Copy link
Copy Markdown
Member

A-Paul commented Jan 26, 2016

Test on arduino-due was successful. STM32F{3,4} -according to @haukepetersen- is O.K. too.

@A-Paul
Copy link
Copy Markdown
Member

A-Paul commented Jan 28, 2016

Can I help to ACK?

@kYc0o, yes sure.

@jnohlgard jnohlgard force-pushed the pr/periph-pwm-api-change branch from 348c1b2 to 5cae0e5 Compare February 4, 2016 09:32
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Feb 4, 2016

Rebased, replaced F_CPU occurrences by CLOCK_CORECLOCK

@jnohlgard jnohlgard removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Feb 4, 2016
A-Paul
A-Paul reviewed Feb 4, 2016
View reviewed changes
cpu/lpc11u34/periph/pwm.c Outdated
/* Check if the frequency and resolution is applicable */
if (CLOCK_CORECLOCK/(resolution*frequency) <= 0) {
if (CLOCK_CORECLOCK / (res * freq) <= 0) {
return -2;
Copy link
Copy Markdown
Member

@A-Paul A-Paul Feb 4, 2016

Choose a reason for hiding this comment

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

Outdated return value.

@jnohlgard jnohlgard force-pushed the pr/periph-pwm-api-change branch from 5cae0e5 to 8cb4995 Compare February 9, 2016 17:38
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Feb 9, 2016

Rebased. Fixed the return values and added a missing default: return 0 case for the lpc2387.

OK to squash?

@haukepetersen
Copy link
Copy Markdown
Contributor

haukepetersen commented Feb 9, 2016

yes and ACK

@haukepetersen
Copy link
Copy Markdown
Contributor

haukepetersen commented Feb 12, 2016

@gebart, have you time to rebase/squash? I could use this PR later today :-)

@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Feb 12, 2016

@haukepetersen will do right now.

@jnohlgard jnohlgard force-pushed the pr/periph-pwm-api-change branch from 8cb4995 to c0de4e4 Compare February 12, 2016 14:33
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Feb 12, 2016

@haukepetersen I got no conflicts but a 3-way merge in kinetis_common that was automatically resolved by git, I'll re-test with a servo on mulle. Did you test any other platforms?

% git rebase master
First, rewinding head to replay your work on top of it...
Applying: tests/periph_pwm: adjusted to interface changes
Applying: drivers/periph/pwm: modernized PWM driver interface
Applying: cpu/lpc11u34: adapted to PWM interface changes
Applying: cpu/lpc2387: adapted to PWM interface changes
Applying: cpu/sam3: adapted to PWM interface changes
Applying: cpu/samd21: adapted to PWM interface changes
Applying: cpu/stm32f3: adapted to PWM interface changes
Applying: cpu/stm32f4: adapted to PWM interface changes
Applying: cpu/kinetis_common: adapted to PWM interface changes
Using index info to reconstruct a base tree...
A   cpu/kinetis_common/pwm.c
Falling back to patching base and 3-way merge...
Auto-merging cpu/kinetis_common/periph/pwm.c
Applying: boards: fixed some PWM definitions
Applying: drivers/servo: adapted to PWM interface changes
Applying: squash cpu/lpc11u34 returns
Applying: squash cpu/lpc2387 returns
Applying: squash cpu/stm32f3 returns

@jnohlgard jnohlgard force-pushed the pr/periph-pwm-api-change branch from c0de4e4 to f4564d6 Compare February 12, 2016 14:52
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Feb 12, 2016

Works on mulle.
@haukepetersen OK to squash? will squash

@jnohlgard jnohlgard force-pushed the pr/periph-pwm-api-change branch from f4564d6 to f6bd9ca Compare February 12, 2016 15:10
@haukepetersen
Copy link
Copy Markdown
Contributor

haukepetersen commented Feb 12, 2016

nice, thanks, ACK holds

@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Feb 14, 2016

Travis is green -> GO!

jnohlgard pushed a commit that referenced this pull request Feb 14, 2016
Merge pull request #4638 from gebart/pr/periph-pwm-api-change
d1a57d0 
drivers: Optimized periph PWM interfaces
@jnohlgard jnohlgard merged commit d1a57d0 into RIOT-OS:master Feb 14, 2016
@jnohlgard jnohlgard deleted the pr/periph-pwm-api-change branch February 14, 2016 07:54
This was referenced Feb 14, 2016
Closed
Merged
@A-Paul A-Paul mentioned this pull request Feb 19, 2016
Closed
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: 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.

5 participants

@jnohlgard @haukepetersen @kYc0o @A-Paul @BytesGalore