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

RP2040: fix off-by-one PWM top issue #4192

Merged
merged 2 commits into from
Feb 17, 2021

Conversation

dhalbert
Copy link
Collaborator

Fixes #4189.

  • The pwmio_pwmout_obj_t top was 32 bits, but it's passed to pico-sdk as 16 bits. Make it 16 bits so the compiler can catch values that are too large. Then needed to force some arithmetic to be 32 bit.

  • Maintain top as a 0 to n-1 value. It was getting set to 65536 in one case. Remove self-> top -1 computations.

  • Comparison check for low vs high frequency had a boundary problem at .frequency = 1907. Fix by using <= comparison instead of <.

  • Add some bounds checking to make sure values fit in 16 bits.

  • Unrelated, missed getting into a previous PR: Remove extraneous include in shared-bindings/adafruit_bus_device/SPIDevice.c.

@jedgarpark Could you test this after the artifact builds? It's more complicated than the version I gave you earlier to test.

@dhalbert dhalbert requested a review from tannewt February 12, 2021 21:00
@jedgarpark
Copy link

Tested it, this works perfectly now, thank you @dhalbert

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.

I've gotta look at this further when I'm fresh. See section 4.5.2.2 in the datasheet. My understanding was that CC needs to be TOP+1 in order to not go low at all.

@dhalbert
Copy link
Collaborator Author

I see what you mean in the datasheet. I will look at the output with the Saleae. We may need to use 65534 as the TOP to set instead of 65535.

@dhalbert dhalbert requested a review from tannewt February 13, 2021 18:21
@dhalbert
Copy link
Collaborator Author

@tannewt I read the datasheet and did a lot of boundary testing with the Saleae, checking both the low and high frequency cases at the boundary , and much higher and lower frequencies. I made sure duty_cycle = 65535 result in high all the time, no glitches output. Just rounding properly did not always result in no glitches. I also improved the calculations of actual_frequency by rounding to the nearest integer.

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.

Ok great! Thanks @dhalbert

@tannewt tannewt merged commit 261b077 into adafruit:main Feb 17, 2021
@dhalbert dhalbert deleted the pico-pwmout-top-fix-4189 branch February 17, 2021 04:26
@dhalbert dhalbert restored the pico-pwmout-top-fix-4189 branch February 25, 2021 02:23
@dhalbert dhalbert deleted the pico-pwmout-top-fix-4189 branch February 25, 2021 02:27
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.

PWMOut on RPi Pico not able to run at full duty cycle
3 participants