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.

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

Fix delays and rounding in samd PWM #9343

Merged
merged 7 commits into from
Jun 21, 2024
Merged

Conversation

timchinowsky
Copy link

Blocking delays are observed when setting duty_cycle in the samd port of CircuitPython (see #7653), behavior which interferes with some applications of PWM, and which differs from from that of other ports. Investigation of this issue revealed another problem: In some cases, the API will round a non-zero duty cycle down to zero, causing the PWM signal to stop cycling at all. The changes proposed in this PR address both of these issues.

Detailed description of how these fixes were tested contains many plots and can be found in (https://github.com/timchinowsky/circuitpython/blob/fix-samd-pwm/tools/pwm/pr.md) and (https://github.com/timchinowsky/circuitpython/blob/fix-samd-pwm/tools/pwm/README.md).

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thanks for the thorough testing and fixes.

I think the tools/pwm contents would be better hooked up in docs/ so that it ends up on docs.circuitpython.org OR as an Adafruit Playground writeup. In tools, it won't be very discoverable.

You can make the docs.cp.org contents with make html at the top level. It ends up in _build IIRC.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Another place to put the tests is in tests/circuitpython-manual. I'm not sure we should put the very detailed testing screenshots and discussion in the repo. I'd suggest putting it in this PR and putting a link to it somewhere with the tests.

@timchinowsky
Copy link
Author

Another place to put the tests is in tests/circuitpython-manual. I'm not sure we should put the very detailed testing screenshots and discussion in the repo. I'd suggest putting it in this PR and putting a link to it somewhere with the tests.

tests/circuitpython-manual is certainly on-point. I'd want to put a couple images there to illustrate what "good" and "bad" look like. I put the test data in the repo bc it's hard to wrangle a lot of images if you just "attach" them. Probably I should just put them in a PDF, like this: pr9343.pdf.

@timchinowsky
Copy link
Author

For test documentation, seems like the repo issues and PRs are the defacto places for this, but I'll add some lines to the online docs for PWMOut to clarify the rounding behavior.

@timchinowsky timchinowsky requested a review from dhalbert June 20, 2024 19:52
tannewt
tannewt previously approved these changes Jun 21, 2024
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you!

@dhalbert
Copy link
Collaborator

I shrank the PNG images by about 70% using https://tinypng.com, and pushed them. I see little if any visible change.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

I shrank the images, and will merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants