Skip to content

esp32/pwm: Use IDF functions to calculate resolution correctly. #16090

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
Merged
merged 1 commit into from
Oct 30, 2024
Merged

esp32/pwm: Use IDF functions to calculate resolution correctly. #16090

merged 1 commit into from
Oct 30, 2024

Conversation

andrewleech
Copy link
Contributor

Summary

This PR attempts to fix the esp32 pwm configuration across all chips, as reported in https://github.com/orgs/micropython/discussions/16086

This has been implemented wuth suggestions from @projectgus (made independantly from the above discussion) in #15862 (comment)

This PR also includes the SOC Capabilities changes made to the pwm driver in #15862

Testing

This has only been tested on ESP32 S3 and has been shown to fix the particular issue first reported in the above discussion:

>>> from machine import PWM,Pin
>>> pwm= PWM(6)
>>> pwm.duty_u16(int(50*655.35))
>>> pwm.freq(200)
>>> pwm.freq(5000)

image

Trade-offs and Alternatives

This code now appears to follow the IDF recommended method of determining resolution which simplifies our code, as well as removing a previously hardcoded (and often incorrect) src clk speed.

I don't think there would have been much value in attempting to just fix the original code rather than this replacement as the IDF functions now used are hopefully better tested (by espressif) across their range of chips.

@andrewleech andrewleech mentioned this pull request Oct 27, 2024
Merged
@robert-hh robert-hh mentioned this pull request Oct 29, 2024
Open
@projectgus
Copy link
Contributor

Thanks @andrewleech for this! I hadn't noticed when I commented in the other PR that this was a regression from adding ESP32-C6 support, I wish I'd noticed when I reviewed that PR. 😆

The approach looks sensible to me, and leveraging the ESP-IDF APIs here should make future SoCs easier to integrate.

I haven't tested this but wanted to note for reference that @robert-hh tested it on all supported SoCs (thanks robert!)

@dpgeorge
Copy link
Member

I have not tested this but am happy to merge it given the review and testing done by others.

Eventually it would be good to add tests for machine.PWM, eg to at least construct a PWM instance to show that it works, and then also to (self-) test the frequency and duty cycle, eg using machine.time_pulse_us()

@pi-anl @dpgeorge
esp32/machine_pwm: Use IDF functions to calculate resolution correctly.
548babf 
This commit fixes PWM configuration across C3, C6, S2 and S3 chips, which
was broken by 6d79937.  Without this fix
the PWM frequency is limited to a maximum of 2446Hz (on S2 at least).

Signed-off-by: Andrew Leech <andrew@alelec.net>
@dpgeorge
Copy link
Member

I did a quick test of this PR on ESP32 and ESP32-S2. ESP32 is fine before and after this PR. ESP32-S2 can only go up to freq=2446 without this PR, but can go up to freq=40_000_000 with this PR.

@dpgeorge dpgeorge merged commit 548babf into micropython:master Oct 30, 2024
8 checks passed
@dpgeorge
Copy link
Member

@andrewleech @projectgus do you think this is worth a patch release, v1.24.1? (That release could also include other patches, if we find any.)

@andrewleech
Copy link
Contributor Author

I wouldn't have necessarily thought this change alone would be enough to warrant an extra release, though it could be a confusing regression for users.

I reckon it's worth waiting a few more days before merging too many other big changes to see if more issues crop up.

@projectgus
Copy link
Contributor

Agree with @andrewleech I think, worth waiting to see if anything else crops up.

@dpgeorge
Copy link
Member

A patch release works like this:

  • create a v1.24-release branch, branched from v1.24.0 tag
  • cherry-pick only the fixes from master into that branch
  • tag v1.24.1 on that branch, and make a release from that

So, we can merge whatever we like to master now and it won't affect a potential patch release.

@dpgeorge
Copy link
Member

This doesn't build on any IDF version prior to 5.2, because ledc_find_suitable_duty_resolution() was only introduced in 5.2.

Options:

  • remove all support in MicroPython for IDF prior to 5.2
  • reinstate the old PWM code for IDF prior to 5.2 (even though it only really works on ESP32-OG)
  • write new code that works for all IDF versions

@projectgus
Copy link
Contributor

This doesn't build on any IDF version prior to 5.2, because ledc_find_suitable_duty_resolution() was only introduced in 5.2.

Ah, drat. Well, we were planning to drop support for pre-5.2 around about now.

However, if this is going into a patch release then it seems a bit unfair to support older IDF in v1.24 and drop support in v1.24.1. So maybe we should reinstate the old code behind an #ifdef check for now (can revert the regression from ESP32-C6 merge at least, so it works the same as it did in v1.23).

I can look into this if you'd like.

@dpgeorge
Copy link
Member

So maybe we should reinstate the old code behind an #ifdef check for now (can revert the regression from ESP32-C6 merge at least, so it works the same as it did in v1.23).

Yes, that sounds like a good idea. If you can look into it that would be great, thanks.

@projectgus projectgus mentioned this pull request Nov 1, 2024
Merged
@dpgeorge dpgeorge added this to the release-1.24.1 milestone Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
port-esp32
Projects
None yet
Milestone
release-1.24.1
Development

Successfully merging this pull request may close these issues.
5 participants
@andrewleech @projectgus @dpgeorge @ricksorensen @pi-anl