esp32: Fix machine.PWM on IDF <5.2. #16127
Conversation
4b85c6a to
6e00c52
Compare
|
Features from this PR are implemented in #10854 |
| static void set_freq(machine_pwm_obj_t *self, unsigned int freq, ledc_timer_config_t *timer) { | ||
| esp_err_t err; |
There was a problem hiding this comment.
Any reason this was moved here, instead of just keeping it as it was, defined at the place of use below?
There was a problem hiding this comment.
Yes, the first place of use is now inside an #ifdef block.
There was a problem hiding this comment.
Oh, I see, I missed that it's also used for ledc_timer_config().
I would have put this within the if (freq != timer->freq_hz) block to keep its scope limited, but here is also fine considering it's a generic error return variable.
Oh, that's a shame! It would have been great to be able to test PWM on all ports without any hardware connections. But I guess it's too much to ask of all ports to support reading a pin in PWM mode. In that case we could do it with a single jumper wire, and still use Anyway, that can be done separately to this PR. |
I think we should fix this for a bugfix release. |
|
See #16147 for a test for PWM. |
I appreciate you've been working steadily at that PR for a while without a lot of feedback, however I don't quite understand what you mean. AFAIK this PR doesn't add any features, all the new code here is copy-paste of code that was removed in #16090 (but now with #ifdefs around it for older IDF). What features do you mean? |
I mean that ledc_find_suitable_duty_resolution is used like in this PR. |
Oh, I see. This function was already added in #16090 actually, but I see that if we'd merged #10854 earlier then we wouldn't have needed that one.
|
The cleanup in 548babf relies on some functions not available in older ESP-IDF. Temporarily restore them, until we drop support for ESP-IDF <5.2. PWM functionality should end up the same regardless of ESP-IDF version, and also no different from MicroPython V1.23. Signed-off-by: Angus Gratton <angus@redyak.com.au>
6e00c52 to
594670e
Compare
|
I tested this with the test from #16147 using IDF 5.0.4 and IDF 5.2.2, on both ESP32_GENERIC and ESP32_GENERIC_S2. The test passes (tests between 50Hz and 10kHz PWM frequency) except for some glitches for 100% duty cycle, which were already present on master. |
|
timer->duty_resolution must be in |
Summary
Restore PWM support for ESP-IDF v5.0.x and v5.1.x. The cleanup in #16090 relies on some functions not available in older ESP-IDF. Temporarily restore support for a possible V1.24.1 bugfix release, before we actually drop support for ESP-IDF <5.2.
(Edit: Dropped the other commit which was in this PR as it wasn't necessary.)
Testing
machine.time_pulse_us()as suggested by @dpgeorge here. Unfortunately it looks like readback of the same pin that's outputting PWM doesn't currently work - need to either fix that or require a jumper between two pins to run the test.Trade-offs and Alternatives