esp32/machine_pwm: Keep duty constant when changing frequency.

IhorNehrutsa · dpgeorge · commit 09fe80d0912b · 2021-12-22T00:05:58.000+11:00
Save and restore the same duty cycle when the frequency (or frequency
resolution) is changed.  This allows a smooth frequency change.

Also update the esp32 PWM quickref to be clearer.
diff --git a/docs/esp32/quickref.rst b/docs/esp32/quickref.rst
@@ -224,14 +224,18 @@ Use the :ref:`machine.PWM <machine.PWM>` class::
     from machine import Pin, PWM
 
     pwm0 = PWM(Pin(0))         # create PWM object from a pin
-    pwm0.freq()                # get current frequency (default 5kHz)
+    freq = pwm0.freq()         # get current frequency (default 5kHz)
     pwm0.freq(1000)            # set PWM frequency from 1Hz to 40MHz
-    pwm0.duty()                # get current duty cycle, range 0-1023 (default 512, 50%)
+
+    duty = pwm0.duty()         # get current duty cycle, range 0-1023 (default 512, 50%)
     pwm0.duty(256)             # set duty cycle from 0 to 1023 as a ratio duty/1023, (now 25%)
+
+    duty_u16 = pwm0.duty_u16() # get current duty cycle, range 0-65535
     pwm0.duty_u16(2**16*3//4)  # set duty cycle from 0 to 65535 as a ratio duty_u16/65535, (now 75%)
-    pwm0.duty_u16()            # get current duty cycle, range 0-65535
+
+    duty_ns = pwm0.duty_ns()   # get current pulse width in ns
     pwm0.duty_ns(250_000)      # set pulse width in nanoseconds from 0 to 1_000_000_000/freq, (now 25%)
-    pwm0.duty_ns()             # get current pulse width in ns
+
     pwm0.deinit()              # turn off PWM on the pin
 
     pwm2 = PWM(Pin(2), freq=20000, duty=512)  # create and configure in one go
@@ -246,7 +250,7 @@ Number of groups (speed modes)                                2         1
 Number of timers per group                                    4         4         4
 Number of channels per group                                  8         8         6
 -----------------------------------------------------  --------  --------  --------
-Different of PWM frequencies (groups * timers)                8         4         4
+Different PWM frequencies (groups * timers)                   8         4         4
 Total PWM channels (Pins, duties) (groups * channels)        16         8         6
 =====================================================  ========  ========  ========
 
diff --git a/ports/esp32/machine_pwm.c b/ports/esp32/machine_pwm.c
@@ -85,17 +85,17 @@ STATIC ledc_timer_config_t timers[PWM_TIMER_MAX];
 // duty_u16() and duty_ns() use 16-bit resolution or less
 
 // Possible highest resolution in device
-#if CONFIG_IDF_TARGET_ESP32
-#define HIGHEST_PWM_RES (LEDC_TIMER_16_BIT) // 20 bit in fact, but 16 bit is used
+#if (LEDC_TIMER_BIT_MAX - 1) < LEDC_TIMER_16_BIT
+#define HIGHEST_PWM_RES (LEDC_TIMER_BIT_MAX - 1)
 #else
-#define HIGHEST_PWM_RES (LEDC_TIMER_14_BIT)
+#define HIGHEST_PWM_RES (LEDC_TIMER_16_BIT) // 20 bit for ESP32, but 16 bit is used
 #endif
 // Duty resolution of user interface in `duty_u16()` and `duty_u16` parameter in constructor/initializer
 #define UI_RES_16_BIT (16)
 // Maximum duty value on highest user interface resolution
 #define UI_MAX_DUTY ((1 << UI_RES_16_BIT) - 1)
 // How much to shift from the HIGHEST_PWM_RES duty resolution to the user interface duty resolution UI_RES_16_BIT
-#define UI_RES_SHIFT (16 - HIGHEST_PWM_RES) // 0 for ESP32, 2 for S2, S3, C3
+#define UI_RES_SHIFT (UI_RES_16_BIT - HIGHEST_PWM_RES) // 0 for ESP32, 2 for S2, S3, C3
 
 // If the PWM frequency is less than EMPIRIC_FREQ, then LEDC_REF_CLK_HZ(1 MHz) source is used, else LEDC_APB_CLK_HZ(80 MHz) source is used
 #define EMPIRIC_FREQ (10) // Hz
@@ -205,24 +205,19 @@ STATIC void configure_channel(machine_pwm_obj_t *self) {
 }
 
 STATIC void set_freq(machine_pwm_obj_t *self, unsigned int freq, ledc_timer_config_t *timer) {
-    // Even if the timer frequency is already set,
-    // the set_duty_x() is required to reconfigure the channel duty anyway
     if (freq != timer->freq_hz) {
-        PWM_DBG("set_freq(%d)", freq)
-
         // Find the highest bit resolution for the requested frequency
         unsigned int i = LEDC_APB_CLK_HZ; // 80 MHz
         if (freq < EMPIRIC_FREQ) {
             i = LEDC_REF_CLK_HZ; // 1 MHz
         }
 
-        #if 1
+        #if ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)
         // original code
         i /= freq;
         #else
         // See https://github.com/espressif/esp-idf/issues/7722
-        unsigned int divider = i / freq; // truncated
-        // int divider = (i + freq / 2) / freq; // rounded
+        int divider = (i + freq / 2) / freq; // rounded
         if (divider == 0) {
             divider = 1;
         }
@@ -245,6 +240,7 @@ STATIC void set_freq(machine_pwm_obj_t *self, unsigned int freq, ledc_timer_conf
         }
 
         // Configure the new resolution and frequency
+        unsigned int save_duty_resolution = timer->duty_resolution;
         timer->duty_resolution = res;
         timer->freq_hz = freq;
         timer->clk_cfg = LEDC_USE_APB_CLK;
@@ -256,7 +252,6 @@ STATIC void set_freq(machine_pwm_obj_t *self, unsigned int freq, ledc_timer_conf
         esp_err_t err = ledc_timer_config(timer);
         if (err != ESP_OK) {
             if (err == ESP_FAIL) {
-                PWM_DBG(" (timer timer->speed_mode %d, timer->timer_num %d, timer->clk_cfg %d, timer->freq_hz  %d, timer->duty_resolution %d) ", timer->speed_mode, timer->timer_num, timer->clk_cfg, timer->freq_hz, timer->duty_resolution);
                 mp_raise_msg_varg(&mp_type_ValueError, MP_ERROR_TEXT("unreachable frequency %d"), freq);
             } else {
                 check_esp_err(err);
@@ -266,15 +261,17 @@ STATIC void set_freq(machine_pwm_obj_t *self, unsigned int freq, ledc_timer_conf
         if (self->mode == LEDC_LOW_SPEED_MODE) {
             check_esp_err(ledc_timer_rst(self->mode, self->timer));
         }
-    }
 
-    // Save the same duty cycle when frequency or channel are changed
-    if (self->duty_x == HIGHEST_PWM_RES) {
-        set_duty_u16(self, self->duty_u16);
-    } else if (self->duty_x == PWRES) {
-        set_duty_u10(self, self->duty_u10);
-    } else if (self->duty_x == -HIGHEST_PWM_RES) {
-        set_duty_ns(self, self->duty_ns);
+        // Save the same duty cycle when frequency is changed
+        if (save_duty_resolution != timer->duty_resolution) {
+            if (self->duty_x == HIGHEST_PWM_RES) {
+                set_duty_u16(self, self->duty_u16);
+            } else if (self->duty_x == PWRES) {
+                set_duty_u10(self, self->duty_u10);
+            } else if (self->duty_x == -HIGHEST_PWM_RES) {
+                set_duty_ns(self, self->duty_ns);
+            }
+        }
     }
 }
 
@@ -287,14 +284,12 @@ STATIC int ns_to_duty(machine_pwm_obj_t *self, int ns) {
     } else if (duty > UI_MAX_DUTY) {
         duty = UI_MAX_DUTY;
     }
-    // PWM_DBG(" ns_to_duty(UI_MAX_DUTY=%d freq_hz=%d duty=%d=%f <- ns=%d) ", UI_MAX_DUTY, timer.freq_hz, duty, (float)ns * UI_MAX_DUTY * timer.freq_hz / 1000000000.0, ns);
     return duty;
 }
 
 STATIC int duty_to_ns(machine_pwm_obj_t *self, int duty) {
     ledc_timer_config_t timer = timers[TIMER_IDX(self->mode, self->timer)];
     int64_t ns = ((int64_t)duty * 1000000000LL + (int64_t)timer.freq_hz * UI_MAX_DUTY / 2) / ((int64_t)timer.freq_hz * UI_MAX_DUTY);
-    // PWM_DBG(" duty_to_ns(UI_MAX_DUTY=%d freq_hz=%d duty=%d -> ns=%f=%d) ", UI_MAX_DUTY, timer.freq_hz, duty, (float)duty * 1000000000.0 / ((float)timer.freq_hz * UI_MAX_DUTY), ns);
     return ns;
 }
 
@@ -316,23 +311,27 @@ STATIC void set_duty_u16(machine_pwm_obj_t *self, int duty) {
     if ((duty < 0) || (duty > UI_MAX_DUTY)) {
         mp_raise_msg_varg(&mp_type_ValueError, MP_ERROR_TEXT("duty_u16 must be from 0 to %d"), UI_MAX_DUTY);
     }
-    duty >>= HIGHEST_PWM_RES + UI_RES_SHIFT - timers[TIMER_IDX(self->mode, self->timer)].duty_resolution;
-    int max_duty = (1 << timers[TIMER_IDX(self->mode, self->timer)].duty_resolution) - 1;
-    if (duty < 0) {
-        duty = 0;
-    } else if (duty > max_duty) {
-        duty = max_duty;
-    }
-    check_esp_err(ledc_set_duty(self->mode, self->channel, duty));
+    ledc_timer_config_t timer = timers[TIMER_IDX(self->mode, self->timer)];
+    int channel_duty = duty >> (HIGHEST_PWM_RES + UI_RES_SHIFT - timer.duty_resolution);
+    int max_duty = (1 << timer.duty_resolution) - 1;
+    if (channel_duty < 0) {
+        channel_duty = 0;
+    } else if (channel_duty > max_duty) {
+        channel_duty = max_duty;
+    }
+    check_esp_err(ledc_set_duty(self->mode, self->channel, channel_duty));
     check_esp_err(ledc_update_duty(self->mode, self->channel));
 
     /*
     // Bug: Sometimes duty is not set right now.
+    // Not a bug. It's a feature. The duty is applied at the beginning of the next signal period.
+    // Bug: It has been experimentally established that the duty is setted during 2 signal periods, but 1 period is expected.
     // See https://github.com/espressif/esp-idf/issues/7288
     if (duty != get_duty_u16(self)) {
-        ets_delay_us(100);
+        PWM_DBG("set_duty_u16(%u), get_duty_u16():%u, channel_duty:%d, duty_resolution:%d, freq_hz:%d", duty, get_duty_u16(self), channel_duty, timer.duty_resolution, timer.freq_hz);
+        ets_delay_us(2 * 1000000 / timer.freq_hz);
         if (duty != get_duty_u16(self)) {
-            PWM_DBG(" (set_duty_u16(%u) get_duty_u16()=%u duty_resolution=%d) ", duty, get_duty_u16(self), timers[TIMER_IDX(self->mode, self->timer)].duty_resolution);
+            PWM_DBG("set_duty_u16(%u), get_duty_u16():%u, channel_duty:%d, duty_resolution:%d, freq_hz:%d", duty, get_duty_u16(self), channel_duty, timer.duty_resolution, timer.freq_hz);
         }
     }
     */
