esp32/machine_pwm.c: Handle multiple timers: Bugfix PWM.deinit() critical error and fix pwm_print() #6654
Conversation
pwm_print() show real (duty/resolution) in brackets. Use ESP_EXCEPTIONS() macro.
|
Several questions on the forum |
There was a problem hiding this comment.
I've been using this PR for a few days. In general "it works" :-)
There is also #3608, which is a slightly different, more explicit approach.
A big issue I'm having is that this PR doesn't work with soft-reset. After a soft-reset the PWM unit continues running and when the python code creates a fresh PWM object of the same pin as an existing PWM unit then the outcome is somewhat undefined. At least, during interactive testing I was running into situations where a pin was stuck on an old PWM and despite creating a new one for it in python nothing would change. At the very least, there needs to be an init method that is called from main.c where the soft-reset comes in that stops all PWM activity.
| return 0; | ||
| } | ||
| return 1; | ||
| } | ||
|
|
||
| STATIC int found_timer(int freq, bool same_freq_only) { |
There was a problem hiding this comment.
This should be called find_timer
| int free_timer_found = -1; | ||
| int timer; | ||
| // Find a free PWM Timer using the same freq | ||
| for (timer = 0; timer < LEDC_TIMER_MAX; ++timer) { |
| // return true if the timer is in use in addition to curr_channel | ||
| STATIC bool is_timer_in_use(int curr_channel, int timer) { | ||
| int i; | ||
| for (i = 0; i < LEDC_CHANNEL_MAX; ++i) { |
| STATIC void set_duty(esp32_pwm_obj_t *self, uint32_t duty) { | ||
| duty &= ((1 << PWRES) - 1); | ||
| duty >>= PWRES - timers[chan_timer[self->channel]].duty_resolution; | ||
| ESP_EXCEPTIONS(ledc_set_duty(PWMODE, self->channel, duty)); |
There was a problem hiding this comment.
This should use check_esp_err, found in mphalport.c
| freq = chan_timer[self->channel] != -1 ? timers[chan_timer[self->channel]].freq_hz : PWFREQ; | ||
| } | ||
|
|
||
| timer = found_timer(freq, false); |
| } | ||
| } | ||
|
|
||
| if (new_timer != -1 && new_timer != current_timer) { |
There was a problem hiding this comment.
Add comment: // If another timer already has the right freq then just switch over to it
How could new_timer == current_timer here? I believe this whole if statement would be clearer if placed right after the call to found_timer.
|
|
||
| current_timer = new_timer; | ||
| } | ||
|
|
There was a problem hiding this comment.
The cases here are very confusing and difficult to follow. It would be nice to find a better way to structure this.
| chan_timer[self->channel] = new_timer; | ||
|
|
||
| if (ledc_bind_channel_timer(PWMODE, self->channel, new_timer) != ESP_OK) { | ||
| mp_raise_msg_varg(&mp_type_ValueError, MP_ERROR_TEXT("failed to bind timer to channel")); |
| } | ||
|
|
||
| // Set the freq | ||
| if (!set_freq(tval, &timers[current_timer])) { | ||
| mp_raise_msg_varg(&mp_type_ValueError, MP_ERROR_TEXT("bad frequency %d"), tval); |
|
|
||
| // Flag it unused | ||
| timers[current_timer].freq_hz = -1; | ||
| } |
There was a problem hiding this comment.
I believe this can return here.
|
FIXED in esp32/machine_pwm: handle multiple timers. #6276 |
Fixes micropython#6654 and fixes micropython#6689
This PR is inherited from esp32/machine_pwm: handle multiple timers. #6276 and replace it.
This PR uses PR ESP32: New feature: EspError exception #6638
This PR was tested together with PR ESP32: New features PCNT() and QUAD() #6639
It is tested with 4 different frequencies at 8 different channels with different duties.
Frequencies and duties are right on proper pins.
Output is