Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

drivers: pwm: add functions for capturing pwm pulse width and period #26025

Merged
merged 6 commits into from
Jan 12, 2021
Merged

drivers: pwm: add functions for capturing pwm pulse width and period #26025

merged 6 commits into from
Jan 12, 2021

Conversation

henrikbrixandersen
Copy link
Member

@henrikbrixandersen henrikbrixandersen commented Jun 6, 2020 •
edited
Loading

Extend the PWM API with optional functions for capturing the PWM pulse width and period time.

Fixes #26026.

Signed-off-by: Henrik Brix Andersen hebad@vestas.com

@henrikbrixandersen henrikbrixandersen added area: PWM Pulse Width Modulation RFC Request For Comments: want input from the community area: API Changes to public APIs labels Jun 6, 2020
@henrikbrixandersen henrikbrixandersen mentioned this pull request Jun 6, 2020
Closed
@henrikbrixandersen henrikbrixandersen force-pushed the pwm_capture branch 2 times, most recently from 5cf6119 to 444b114 Compare June 8, 2020 13:48
pabigot
pabigot reviewed Jun 9, 2020
View reviewed changes
Copy link
Collaborator

@pabigot pabigot left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Suggestion to make the API extension conditional on a Kconfig enabling it. Example is in 48dc2fd ("drivers: flash: add API to access SFDP region of serial flash devices") of #25851 where CONFIG_FLASH_JESD216_API enables the special case.

There're actually design considerations on how that should be implemented. In that approach the API functions are only defined if the feature is enabled. An alternative would be to always define them, but have them return -ENOTSUP if the feature is not enabled (that actually makes more sense at the moment).

@henrikbrixandersen
Copy link
Member Author

Suggestion to make the API extension conditional on a Kconfig enabling it. Example is in 48dc2fd ("drivers: flash: add API to access SFDP region of serial flash devices") of #25851 where CONFIG_FLASH_JESD216_API enables the special case.

Thank you. I have added a Kconfig option for enabling this. Example implementation still pending.

pabigot
pabigot reviewed Jul 27, 2020
View reviewed changes
Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

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

I see this is still draft, but since I was looking at it here're the initial comments. Will check again when there's an implementation.

include/drivers/pwm.h Outdated Show resolved Hide resolved
include/drivers/pwm.h Outdated Show resolved Hide resolved
@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Sep 26, 2020
@henrikbrixandersen henrikbrixandersen force-pushed the pwm_capture branch 2 times, most recently from 2962d5e to af63d9b Compare October 7, 2020 13:57
@henrikbrixandersen henrikbrixandersen force-pushed the pwm_capture branch 3 times, most recently from 49c163c to 6a2f6c6 Compare November 21, 2020 19:42
pabigot
pabigot reviewed Nov 21, 2020
View reviewed changes
Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

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

It doesn't seem quite right to have both a synchronous and an asynchronous API implementation. Seems like that would produce duplication. How about just the async as the implemented API, and have the sync wrapper use a stack signal structure and k_poll()?

include/drivers/pwm.h Outdated Show resolved Hide resolved
@coderkalyan
Copy link
Contributor

@henrikbrixandersen What is the state of this? Does the API need any more work? Or is the wait because implementations haven't been finished yet?

@henrikbrixandersen
Copy link
Member Author

@henrikbrixandersen What is the state of this? Does the API need any more work? Or is the wait because implementations haven't been finished yet?

I have had to rework the proposed API to meet real-life application needs (support for continuous capture and filtering through a callback mechanism). I have a couple of implementations almost ready. I will update this PR once the final issues have been worked out.

@anangl anangl dismissed their stale review January 7, 2021 10:22

Comments addressed, at least the essential ones.

@henrikbrixandersen henrikbrixandersen force-pushed the pwm_capture branch 2 times, most recently from 9262ffe to 9d775c6 Compare January 7, 2021 14:32
@MaureenHelm
Copy link
Member

@henrikbrixandersen By any chance are you planning to write an implementation for STM32 devices? I would really like to use this API on an STM32 device to replace my current GPIO interrupt "stopwatch" implementation, but I don't know very much at all about the low level STM32 HALs in order to be able to implement this driver ☹️

@coderkalyan Sorry, no. I have no plans for adding PWM capture support on STM32. Others can submit PRs for that, though, once this PR (hopefully) goes in.

@erwango can you take a look?

@erwango
Copy link
Member

erwango commented Jan 7, 2021

@henrikbrixandersen By any chance are you planning to write an implementation for STM32 devices? I would really like to use this API on an STM32 device to replace my current GPIO interrupt "stopwatch" implementation, but I don't know very much at all about the low level STM32 HALs in order to be able to implement this driver frowning_face

@coderkalyan Sorry, no. I have no plans for adding PWM capture support on STM32. Others can submit PRs for that, though, once this PR (hopefully) goes in.

@erwango can you take a look?

No plans on our side for now, @gmarull maybe?

@henrikbrixandersen henrikbrixandersen mentioned this pull request Jan 7, 2021
Merged
3 tasks
@henrikbrixandersen
Copy link
Member Author

To demonstrate the PWM capture API on more hardware, I have added another driver implementation in #31165. I will not add it here since it currently has a few dependencies on other PRs to be merged.

@coderkalyan
Copy link
Contributor

Ah, I guess I'll have to learn the LL framework and implement the driver myself 😢 😂 In the meantime, if anyone is kind enough to do it, it would be very much appreciated by me!

@zephyrbot zephyrbot requested review from MaureenHelm and utzig January 8, 2021 22:51
@henrikbrixandersen
Copy link
Member Author

@MaureenHelm Could you please re-approve? @zephyrbot cancelled your approval 👎

@henrikbrixandersen
drivers: pwm: add API for capturing pwm pulse width and period
ddbb2da 
Extend the PWM API with optional API functions for capturing PWM pulse
width and period cycles.

Fixes zephyrproject-rtos#26026.

Signed-off-by: Henrik Brix Andersen <hebad@vestas.com>
@henrikbrixandersen
CODEOWNERS: take ownership of PWM capture helper functions
dfd4dc4 
Take ownership of the PWM capture helper functions.

Signed-off-by: Henrik Brix Andersen <hebad@vestas.com>
@henrikbrixandersen
MAINTAINERS: add myself as collaborator on PWM
0b83398 
Add myself as collaborator on the PWM code.

Signed-off-by: Henrik Brix Andersen <hebad@vestas.com>
@henrikbrixandersen
drivers: pwm: mcux_ftm: add PWM capure support
e175064 
Add PWM capture support to the NXP MCUX FlexTimer driver.

Signed-off-by: Henrik Brix Andersen <hebad@vestas.com>
@henrikbrixandersen
boards: arm: frdm_k64f: enable FlexTimer 0 as PWM
08ed975 
Enable FlexTimer 0 (FTM0) as PWM and setup PTC1 as FTM0 channel 0 for
use in PWM loopback test.

Signed-off-by: Henrik Brix Andersen <hebad@vestas.com>
@henrikbrixandersen
tests: drivers: pwm: add PWM loopback test
c673eeb 
Add test cases for the PWM capture API using PWM signal loopback.

Signed-off-by: Henrik Brix Andersen <hebad@vestas.com>
@henrikbrixandersen
Copy link
Member Author

@anangl Please re-review.

@carlescufi carlescufi merged commit 90825a8 into zephyrproject-rtos:master Jan 12, 2021
@henrikbrixandersen henrikbrixandersen mentioned this pull request Jan 12, 2021
Merged
@henrikbrixandersen henrikbrixandersen deleted the pwm_capture branch January 14, 2021 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pabigot pabigot pabigot approved these changes

@anangl anangl anangl approved these changes

@mnkp mnkp Awaiting requested review from mnkp

@tbursztyka tbursztyka Awaiting requested review from tbursztyka

@galak galak Awaiting requested review from galak

@carlescufi carlescufi Awaiting requested review from carlescufi

@ioannisg ioannisg Awaiting requested review from ioannisg

@nashif nashif Awaiting requested review from nashif

@gmarull gmarull Awaiting requested review from gmarull

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

@utzig utzig Awaiting requested review from utzig

@Thalley Thalley Awaiting requested review from Thalley

Assignees

@anangl anangl

Labels
area: API Changes to public APIs area: Boards area: Devicetree Binding PR modifies or adds a Device Tree binding area: Devicetree area: Documentation area: Process area: PWM Pulse Width Modulation area: Tests Issues related to a particular existing or missing test platform: NXP NXP
Projects
None yet
Milestone
v2.5.0
Development

Successfully merging this pull request may close these issues.

RFC: drivers: pwm: add functions for capturing pwm pulse width and period
9 participants
@henrikbrixandersen @coderkalyan @MaureenHelm @erwango @pabigot @Thalley @carlescufi @anangl @zephyrbot