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

tests: pwm: Add test suite with GPIO loopback #82258

Merged
merged 2 commits into from
Jan 6, 2025
Merged

tests: pwm: Add test suite with GPIO loopback #82258

merged 2 commits into from
Jan 6, 2025

Conversation

raffarost
Copy link
Collaborator

Add PWM test suite which uses GPIO as loopback input to check the programmed timings.
Used for devices which don't have CC available.

@raffarost raffarost added the area: PWM Pulse Width Modulation label Nov 28, 2024
@raffarost raffarost mentioned this pull request Nov 28, 2024
Closed
@raffarost raffarost force-pushed the tests/pwm_gpio branch 3 times, most recently from f0327be to 355ad35 Compare December 9, 2024 20:13
@bjarki-andreasen
Copy link
Collaborator

We should probably align here since we have two PRs implementing gpio loopback for pwms #82485 :)

@bjarki-andreasen bjarki-andreasen self-assigned this Dec 17, 2024
@raffarost
Copy link
Collaborator Author

We should probably align here since we have two PRs implementing gpio loopback for pwms #82485 :)

hi @bjarki-andreasen, looks like we have a duplicate indeed :)
I wouldn't mind favouring either of them, how should we proceed?
Looks like so far this PR has more coverage in the tests (duty cycle tests, including 0 and 100%).

@bjarki-andreasen
Copy link
Collaborator

We should probably align here since we have two PRs implementing gpio loopback for pwms #82485 :)

hi @bjarki-andreasen, looks like we have a duplicate indeed :) I wouldn't mind favouring either of them, how should we proceed? Looks like so far this PR has more coverage in the tests (duty cycle tests, including 0 and 100%).

Your PR was created first, so it will take precedence unless you decide otherwise, I have asked the author of #82485 to review this one, I'm sure we can get something great out of it :)

@sylvioalves
Copy link
Collaborator

We should probably align here since we have two PRs implementing gpio loopback for pwms #82485 :)

hi @bjarki-andreasen, looks like we have a duplicate indeed :) I wouldn't mind favouring either of them, how should we proceed? Looks like so far this PR has more coverage in the tests (duty cycle tests, including 0 and 100%).

Pinging @nordic-pikr as PR owner. Would you review this one?

@nordic-pikr
Copy link
Contributor

Hi, I'm actually quite busy, I'm afraid I won't be able to do a review until the new year, at first glance, looks better than mine :)

@sylvioalves sylvioalves requested a review from kartben December 18, 2024 14:00
@sylvioalves
Copy link
Collaborator

@raffarost I think we can make it ready then.

@raffarost raffarost marked this pull request as ready for review December 18, 2024 17:08
@zephyrbot zephyrbot requested a review from anangl December 18, 2024 17:09
bjarki-andreasen
bjarki-andreasen requested changes Dec 19, 2024
View reviewed changes
tests/drivers/pwm/pwm_gpio_loopback/dts/bindings/test-pwm-gpio-loopback.yaml Outdated Show resolved Hide resolved
tests/drivers/pwm/pwm_gpio_loopback/src/main.c Outdated Show resolved Hide resolved
tests/drivers/pwm/pwm_gpio_loopback/src/main.c Outdated Show resolved Hide resolved
tests/drivers/pwm/pwm_gpio_loopback/src/main.c Outdated Show resolved Hide resolved
tests/drivers/pwm/pwm_gpio_loopback/src/main.c Outdated Show resolved Hide resolved
tests/drivers/pwm/pwm_gpio_loopback/src/main.c Outdated Show resolved Hide resolved
tests/drivers/pwm/pwm_gpio_loopback/testcase.yaml Outdated Show resolved Hide resolved
tests/drivers/pwm/pwm_gpio_loopback/Kconfig Show resolved Hide resolved
@raffarost
Copy link
Collaborator Author

thank you for your valuable comments! I hope to have addressed them properly.

I removed test-cases related to duty 100%+ as the API already does the check.
I'm still unsure about DEFAULT_PWM_PORT. It could go to Kconfig but there's also pwms which could define channel, period and flags. It can get confusing because of the Kconfigs for cycle and nsec settings and the test case for inverted signal, so I'd lean to adding it to Kconfig instead.

@nordic-pikr nordic-pikr mentioned this pull request Jan 2, 2025
Closed
@nordic-pikr
Copy link
Contributor

I'm still unsure about DEFAULT_PWM_PORT. It could go to Kconfig but there's also pwms which could define channel, period and flags. It can get confusing because of the Kconfigs for cycle and nsec settings and the test case for inverted signal, so I'd lean to adding it to Kconfig instead.

I don't know which option will be better, but it is certain that it needs to be unhardcoded. In Nordic cases, we will need to provide the channel number as a parameter somehow.

nordic-pikr
nordic-pikr suggested changes Jan 2, 2025
View reviewed changes
Copy link
Contributor

@nordic-pikr nordic-pikr left a comment

Choose a reason for hiding this comment

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

parameterize it please

@raffarost
Copy link
Collaborator Author

@nordic-pikr @bjarki-andreasen PTAL to see if this is better

Raffael Rostagno added 2 commits January 3, 2025 17:30
tests: pwm: Add test suite with GPIO loopback
e4f9f70 
Add PWM test suite which uses GPIO as loopback input to check
the programmed timings.

Signed-off-by: Raffael Rostagno <[email protected]>
tests: pwm_gpio_loopback: esp32: Add test overlays
061e6f4 
Add test overlays for ESP32 devices.

Signed-off-by: Raffael Rostagno <[email protected]>
bjarki-andreasen
bjarki-andreasen approved these changes Jan 6, 2025
View reviewed changes
Copy link
Collaborator

@bjarki-andreasen bjarki-andreasen left a comment

Choose a reason for hiding this comment

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

Looks good

@kartben kartben merged commit 16d7bbf into zephyrproject-rtos:main Jan 6, 2025
18 checks passed
@raffarost raffarost deleted the tests/pwm_gpio branch January 6, 2025 13:42
@nordic-pikr
Copy link
Contributor

I didn't have a chance to check it yesterday because of a bankholiday. What do you say for changing the definition names to e.g. TEST_*, because the current names are used in our driver. Please take a look at 83640

@raffarost
Copy link
Collaborator Author

hi @nordic-pikr, sure we can change the names
do you mean PWM_COUNT, by instance?

@bjarki-andreasen
Copy link
Collaborator

hi @nordic-pikr, sure we can change the names do you mean PWM_COUNT, by instance?

More like TEST_PWM :)

@raffarost
Copy link
Collaborator Author

@nordic-pikr @bjarki-andreasen sure, LGTM

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

@nordic-pikr nordic-pikr nordic-pikr requested changes

@bjarki-andreasen bjarki-andreasen bjarki-andreasen approved these changes

@sylvioalves sylvioalves sylvioalves approved these changes

@henrikbrixandersen henrikbrixandersen Awaiting requested review from henrikbrixandersen

@LucasTambor LucasTambor Awaiting requested review from LucasTambor

@kartben kartben Awaiting requested review from kartben

@anangl anangl Awaiting requested review from anangl

Assignees

@bjarki-andreasen bjarki-andreasen

Labels
area: PWM Pulse Width Modulation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@raffarost @bjarki-andreasen @sylvioalves @nordic-pikr @kartben