-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
esp32/machine_pwm.c: Handle multiple timers: Bugfix PWM.deinit() critical error and fix pwm_print() #6654
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
esp32/machine_pwm.c: Handle multiple timers: Bugfix PWM.deinit() critical error and fix pwm_print() #6654
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,6 +32,8 @@ | |
#include "modmachine.h" | ||
#include "mphalport.h" | ||
|
||
#include "esp_error.h" | ||
|
||
// Forward dec'l | ||
extern const mp_obj_type_t machine_pwm_type; | ||
|
||
|
@@ -46,6 +48,13 @@ typedef struct _esp32_pwm_obj_t { | |
// (-1 if not assigned) | ||
STATIC int chan_gpio[LEDC_CHANNEL_MAX]; | ||
|
||
// Which channel has which timer assigned? | ||
// (-1 if not assigned) | ||
STATIC int chan_timer[LEDC_CHANNEL_MAX]; | ||
|
||
// List of timer configs | ||
STATIC ledc_timer_config_t timers[LEDC_TIMER_MAX]; | ||
|
||
// Params for PW operation | ||
// 5khz | ||
#define PWFREQ (5000) | ||
|
@@ -57,32 +66,36 @@ STATIC int chan_gpio[LEDC_CHANNEL_MAX]; | |
#endif | ||
// 10-bit resolution (compatible with esp8266 PWM) | ||
#define PWRES (LEDC_TIMER_10_BIT) | ||
// Timer 1 | ||
#define PWTIMER (LEDC_TIMER_1) | ||
|
||
// Config of timer upon which we run all PWM'ed GPIO pins | ||
STATIC bool pwm_inited = false; | ||
STATIC ledc_timer_config_t timer_cfg = { | ||
.duty_resolution = PWRES, | ||
.freq_hz = PWFREQ, | ||
.speed_mode = PWMODE, | ||
.timer_num = PWTIMER | ||
}; | ||
|
||
STATIC void pwm_init(void) { | ||
|
||
// Initial condition: no channels assigned | ||
for (int x = 0; x < LEDC_CHANNEL_MAX; ++x) { | ||
chan_gpio[x] = -1; | ||
chan_timer[x] = -1; | ||
} | ||
|
||
// Init with default timer params | ||
ledc_timer_config(&timer_cfg); | ||
// Initial condition: no timers assigned | ||
for (int x = 0; x < LEDC_TIMER_MAX; ++x) { | ||
timers[x].duty_resolution = PWRES; | ||
// unset timer is -1 | ||
timers[x].freq_hz = -1; | ||
timers[x].speed_mode = PWMODE; | ||
timers[x].timer_num = x; | ||
} | ||
} | ||
|
||
STATIC int set_freq(int newval) { | ||
int ores = timer_cfg.duty_resolution; | ||
int oval = timer_cfg.freq_hz; | ||
STATIC int set_freq(int newval, ledc_timer_config_t *timer) { | ||
int ores = timer->duty_resolution; | ||
int oval = timer->freq_hz; | ||
|
||
// If already set, do nothing | ||
if (newval == oval) { | ||
return 1; | ||
} | ||
|
||
// Find the highest bit resolution for the requested frequency | ||
if (newval <= 0) { | ||
|
@@ -99,16 +112,64 @@ STATIC int set_freq(int newval) { | |
} | ||
|
||
// Configure the new resolution and frequency | ||
timer_cfg.duty_resolution = res; | ||
timer_cfg.freq_hz = newval; | ||
if (ledc_timer_config(&timer_cfg) != ESP_OK) { | ||
timer_cfg.duty_resolution = ores; | ||
timer_cfg.freq_hz = oval; | ||
timer->duty_resolution = res; | ||
timer->freq_hz = newval; | ||
if (ledc_timer_config(timer) != ESP_OK) { | ||
timer->duty_resolution = ores; | ||
timer->freq_hz = oval; | ||
return 0; | ||
} | ||
return 1; | ||
} | ||
|
||
STATIC int found_timer(int freq, bool same_freq_only) { | ||
int free_timer_found = -1; | ||
int timer; | ||
// Find a free PWM Timer using the same freq | ||
for (timer = 0; timer < LEDC_TIMER_MAX; ++timer) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. change to |
||
if (timers[timer].freq_hz == freq) { | ||
// A timer already use the same freq. Use it now. | ||
return timer; | ||
} | ||
if (!same_freq_only && (free_timer_found == -1) && (timers[timer].freq_hz == -1)) { | ||
free_timer_found = timer; | ||
// Continue to check if a channel with the same freq is in used. | ||
} | ||
} | ||
|
||
return free_timer_found; | ||
} | ||
|
||
// 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. change to |
||
if ((i != curr_channel) && (chan_timer[i] == timer)) { | ||
return true; | ||
} | ||
} | ||
return false; | ||
} | ||
|
||
/******************************************************************************/ | ||
STATIC uint32_t get_resolution(esp32_pwm_obj_t *self) { | ||
return timers[chan_timer[self->channel]].duty_resolution; | ||
} | ||
|
||
STATIC uint32_t get_duty(esp32_pwm_obj_t *self) { | ||
uint32_t duty; | ||
duty = ledc_get_duty(PWMODE, self->channel); | ||
duty <<= PWRES - timers[chan_timer[self->channel]].duty_resolution; | ||
return duty; | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should use |
||
ESP_EXCEPTIONS(ledc_update_duty(PWMODE, self->channel)); | ||
} | ||
|
||
/******************************************************************************/ | ||
|
||
// MicroPython bindings for PWM | ||
|
@@ -117,8 +178,10 @@ STATIC void esp32_pwm_print(const mp_print_t *print, mp_obj_t self_in, mp_print_ | |
esp32_pwm_obj_t *self = MP_OBJ_TO_PTR(self_in); | ||
mp_printf(print, "PWM(%u", self->pin); | ||
if (self->active) { | ||
mp_printf(print, ", freq=%u, duty=%u", timer_cfg.freq_hz, | ||
ledc_get_duty(PWMODE, self->channel)); | ||
uint32_t duty; | ||
duty = ledc_get_duty(PWMODE, self->channel); | ||
mp_printf(print, ", freq=%u, duty=%u(%u/%u)", timers[chan_timer[self->channel]].freq_hz, | ||
get_duty(self), duty, 1<<get_resolution(self)); | ||
} | ||
mp_printf(print, ")"); | ||
} | ||
|
@@ -155,40 +218,48 @@ STATIC void esp32_pwm_init_helper(esp32_pwm_obj_t *self, | |
} | ||
self->channel = channel; | ||
|
||
int timer; | ||
int freq = args[ARG_freq].u_int; | ||
|
||
// Check if freq wasn't passed as an argument | ||
if (freq == -1) { | ||
// Check if already set, otherwise use the defaut freq | ||
freq = chan_timer[self->channel] != -1 ? timers[chan_timer[self->channel]].freq_hz : PWFREQ; | ||
} | ||
|
||
timer = found_timer(freq, false); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. change to |
||
|
||
if (timer == -1) { | ||
mp_raise_ValueError(MP_ERROR_TEXT("out of PWM timers")); | ||
} | ||
chan_timer[channel] = timer; | ||
|
||
// New PWM assignment | ||
self->active = 1; | ||
if (chan_gpio[channel] == -1) { | ||
ledc_channel_config_t cfg = { | ||
.channel = channel, | ||
.duty = (1 << timer_cfg.duty_resolution) / 2, | ||
.duty = (1 << timers[timer].duty_resolution) / 2, | ||
.gpio_num = self->pin, | ||
.intr_type = LEDC_INTR_DISABLE, | ||
.speed_mode = PWMODE, | ||
.timer_sel = PWTIMER, | ||
.timer_sel = timer, | ||
}; | ||
if (ledc_channel_config(&cfg) != ESP_OK) { | ||
mp_raise_msg_varg(&mp_type_ValueError, MP_ERROR_TEXT("PWM not supported on pin %d"), self->pin); | ||
} | ||
chan_gpio[channel] = self->pin; | ||
} | ||
|
||
// Maybe change PWM timer | ||
int tval = args[ARG_freq].u_int; | ||
if (tval != -1) { | ||
if (tval != timer_cfg.freq_hz) { | ||
if (!set_freq(tval)) { | ||
mp_raise_msg_varg(&mp_type_ValueError, MP_ERROR_TEXT("bad frequency %d"), tval); | ||
} | } | |
// Set timer frequency | ||
if (!set_freq(freq, &timers[timer])) { | ||
mp_raise_msg_varg(&mp_type_ValueError, MP_ERROR_TEXT("bad frequency %d"), freq); | ||
} | ||
|
||
// Set duty cycle? | ||
int dval = args[ARG_duty].u_int; | ||
if (dval != -1) { | ||
dval &= ((1 << PWRES) - 1); | ||
dval >>= PWRES - timer_cfg.duty_resolution; | ||
ledc_set_duty(PWMODE, channel, dval); | ||
ledc_update_duty(PWMODE, channel); | ||
set_duty(self, dval); | ||
} | ||
} | ||
|
||
|
@@ -231,6 +302,14 @@ STATIC mp_obj_t esp32_pwm_deinit(mp_obj_t self_in) { | |
|
||
// Valid channel? | ||
if ((chan >= 0) && (chan < LEDC_CHANNEL_MAX)) { | ||
// Clean up timer if necessary | ||
if (!is_timer_in_use(chan, chan_timer[chan])) { | ||
ledc_timer_rst(PWMODE, chan_timer[chan]); | ||
// Flag it unused | ||
timers[chan_timer[chan]].freq_hz = -1; | ||
} | ||
chan_timer[chan] = -1; // channel now don't use a timer!!! | ||
|
||
// Mark it unused, and tell the hardware to stop routing | ||
chan_gpio[chan] = -1; | ||
ledc_stop(PWMODE, chan, 0); | ||
|
@@ -243,38 +322,73 @@ STATIC mp_obj_t esp32_pwm_deinit(mp_obj_t self_in) { | |
STATIC MP_DEFINE_CONST_FUN_OBJ_1(esp32_pwm_deinit_obj, esp32_pwm_deinit); | ||
|
||
STATIC mp_obj_t esp32_pwm_freq(size_t n_args, const mp_obj_t *args) { | ||
esp32_pwm_obj_t *self = MP_OBJ_TO_PTR(args[0]); | ||
if (n_args == 1) { | ||
// get | ||
return MP_OBJ_NEW_SMALL_INT(timer_cfg.freq_hz); | ||
return MP_OBJ_NEW_SMALL_INT(timers[chan_timer[self->channel]].freq_hz); | ||
} | ||
|
||
// set | ||
int tval = mp_obj_get_int(args[1]); | ||
if (!set_freq(tval)) { | ||
|
||
if (tval == timers[chan_timer[self->channel]].freq_hz) { | ||
return mp_const_none; | ||
} | ||
|
||
int current_timer = chan_timer[self->channel]; | ||
int new_timer = -1; | ||
bool current_in_use = is_timer_in_use(self->channel, current_timer); | ||
|
||
// Check if an already running with the same freq is running | ||
new_timer = found_timer(tval, true); | ||
|
||
// If no existing timer was found, and the current one is in use, then find a new one | ||
if (new_timer == -1 && current_in_use) { | ||
// Have to found a new timer | ||
new_timer = found_timer(tval, false); | ||
|
||
if (new_timer == -1) { | ||
mp_raise_ValueError(MP_ERROR_TEXT("out of PWM timers")); | ||
} | ||
} | ||
|
||
if (new_timer != -1 && new_timer != current_timer) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add comment: |
||
// Bind the channel to the new timer | ||
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")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you use |
||
} | ||
|
||
if (!current_in_use) { | ||
// Free the old timer | ||
ledc_timer_rst(PWMODE, current_timer); | ||
|
||
// Flag it unused | ||
timers[current_timer].freq_hz = -1; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this can return here. |
||
|
||
current_timer = new_timer; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The cases here are very confusing and difficult to follow. It would be nice to find a better way to structure this. |
||
// Set the freq | ||
if (!set_freq(tval, &timers[current_timer])) { | ||
mp_raise_msg_varg(&mp_type_ValueError, MP_ERROR_TEXT("bad frequency %d"), tval); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use |
||
} | ||
return mp_const_none; | ||
} | ||
|
||
STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(esp32_pwm_freq_obj, 1, 2, esp32_pwm_freq); | ||
|
||
STATIC mp_obj_t esp32_pwm_duty(size_t n_args, const mp_obj_t *args) { | ||
esp32_pwm_obj_t *self = MP_OBJ_TO_PTR(args[0]); | ||
int duty; | ||
|
||
if (n_args == 1) { | ||
// get | ||
duty = ledc_get_duty(PWMODE, self->channel); | ||
duty <<= PWRES - timer_cfg.duty_resolution; | ||
return MP_OBJ_NEW_SMALL_INT(duty); | ||
return MP_OBJ_NEW_SMALL_INT(get_duty(self)); | ||
} | ||
|
||
// set | ||
duty = mp_obj_get_int(args[1]); | ||
duty &= ((1 << PWRES) - 1); | ||
duty >>= PWRES - timer_cfg.duty_resolution; | ||
4D88 ledc_set_duty(PWMODE, self->channel, duty); | ||
ledc_update_duty(PWMODE, self->channel); | ||
set_duty(self, mp_obj_get_int(args[1])); | ||
|
||
return mp_const_none; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be called
find_timer