8000 esp32/machine_pwm.c: Handle multiple timers: Bugfix PWM.deinit() critical error and fix pwm_print() by IhorNehrutsa · Pull Request #6654 · micropython/micropython · GitHub
[go: up one dir, main page]
Skip to content

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.

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

Closed
wants to merge 3 commits into from
Closed

esp32/machine_pwm.c: Handle multiple timers: Bugfix PWM.deinit() critical error and fix pwm_print() #6654

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b723f56
esp32/machine_pwm.c: Fix duty value printed in esp32_pwm_print().
IhorNehrutsa Nov 19, 2020
8c0ad27
esp32/machine_pwm.c: Fix PWM.deinit() critical error.
IhorNehrutsa Nov 27, 2020
5f9a0ee
Merge branch 'master' into bugfix/esp32_pwm_print
IhorNehrutsa Jun 8, 2021
8000
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
206 changes: 160 additions & 46 deletions ports/esp32/machine_pwm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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)
Expand All @@ -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) {
Expand All @@ -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) {
Copy link
Contributor

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

int free_timer_found = -1;
int timer;
// Find a free PWM Timer using the same freq
for (timer = 0; timer < LEDC_TIMER_MAX; ++timer) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to for (int timer = 0;

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to for (int i = 0;

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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use check_esp_err, found in mphalport.c

ESP_EXCEPTIONS(ledc_update_duty(PWMODE, self->channel));
}

/******************************************************************************/

// MicroPython bindings for PWM
Expand All @@ -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, ")");
}
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to int timer =


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);
}
}

Expand Down Expand Up @@ -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);
Expand All @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// 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"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you use check_esp_err?

}

if (!current_in_use) {
// Free the old timer
ledc_timer_rst(PWMODE, current_timer);

// Flag it unused
timers[current_timer].freq_hz = -1;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this can return here.


current_timer = new_timer;
}

Copy link
Contributor

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use check_esp_err?

}
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;
}
Expand Down
0