Merge pull request #2240 from theacodes/fix-2086

Track unadjusted PWM duty cycle to avoid accumulating conversion errors

3 files changed, 21 insertions, 2 deletions

diff --git a/ports/atmel-samd/common-hal/pulseio/PWMOut.c b/ports/atmel-samd/common-hal/pulseio/PWMOut.c
index 6dcace21f..0adb23fc5 100644
--- a/ports/atmel-samd/common-hal/pulseio/PWMOut.c
+++ b/ports/atmel-samd/common-hal/pulseio/PWMOut.c
@@ -135,6 +135,7 @@ pwmout_result_t common_hal_pulseio_pwmout_construct(pulseio_pwmout_obj_t* self,
                                                     bool variable_frequency) {
     self->pin = pin;
     self->variable_frequency = variable_frequency;
+    self->duty_cycle = duty;
 
     if (pin->timer[0].index >= TC_INST_NUM &&
         pin->timer[1].index >= TCC_INST_NUM
@@ -322,6 +323,13 @@ void common_hal_pulseio_pwmout_deinit(pulseio_pwmout_obj_t* self) {
 }
 
 extern void common_hal_pulseio_pwmout_set_duty_cycle(pulseio_pwmout_obj_t* self, uint16_t duty) {
+    // Store the unadjusted duty cycle. It turns out the the process of adjusting and calculating
+    // the duty cycle here and reading it back is lossy - the value will decay over time.
+    // Track it here so that if frequency is changed we can use this value to recalculate the
+    // proper duty cycle.
+    // See https://github.com/adafruit/circuitpython/issues/2086 for more details
+    self->duty_cycle = duty;
+
     const pin_timer_t* t = self->timer;
     if (t->is_tc) {
         uint16_t adjusted_duty = tc_periods[t->index] * duty / 0xffff;
@@ -415,7 +423,6 @@ void common_hal_pulseio_pwmout_set_frequency(pulseio_pwmout_obj_t* self,
             break;
         }
     }
-    uint16_t old_duty = common_hal_pulseio_pwmout_get_duty_cycle(self);
     if (t->is_tc) {
         Tc* tc = tc_insts[t->index];
         uint8_t old_divisor = tc->COUNT16.CTRLA.bit.PRESCALER;
@@ -450,7 +457,7 @@ void common_hal_pulseio_pwmout_set_frequency(pulseio_pwmout_obj_t* self,
         #endif
     }
 
-    common_hal_pulseio_pwmout_set_duty_cycle(self, old_duty);
+    common_hal_pulseio_pwmout_set_duty_cycle(self, self->duty_cycle);
 }
 
 uint32_t common_hal_pulseio_pwmout_get_frequency(pulseio_pwmout_obj_t* self) {
diff --git a/ports/atmel-samd/common-hal/pulseio/PWMOut.h b/ports/atmel-samd/common-hal/pulseio/PWMOut.h
index eef653bcb..09abda819 100644
--- a/ports/atmel-samd/common-hal/pulseio/PWMOut.h
+++ b/ports/atmel-samd/common-hal/pulseio/PWMOut.h
@@ -36,6 +36,7 @@ typedef struct {
     const mcu_pin_obj_t *pin;
     const pin_timer_t* timer;
     bool variable_frequency;
+    uint16_t duty_cycle;
 } pulseio_pwmout_obj_t;
 
 void pwmout_reset(void);
diff --git a/shared-bindings/pulseio/PWMOut.c b/shared-bindings/pulseio/PWMOut.c
index 40981e0a8..53b88c61a 100644
--- a/shared-bindings/pulseio/PWMOut.c
+++ b/shared-bindings/pulseio/PWMOut.c
@@ -162,6 +162,11 @@ STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(pulseio_pwmout___exit___obj, 4, 4, pu
 //|      16 bit value that dictates how much of one cycle is high (1) versus low
 //|      (0). 0xffff will always be high, 0 will always be low and 0x7fff will
 //|      be half high and then half low.
+//|
+//|      Depending on how PWM is implemented on a specific board, the internal
+//|      representation for duty cycle might have less than 16 bits of resolution.
+//|      Reading this property will return the value from the internal representation,
+//|      so it may differ from the value set.
 STATIC mp_obj_t pulseio_pwmout_obj_get_duty_cycle(mp_obj_t self_in) {
     pulseio_pwmout_obj_t *self = MP_OBJ_TO_PTR(self_in);
     check_for_deinit(self);
@@ -193,6 +198,12 @@ const mp_obj_property_t pulseio_pwmout_duty_cycle_obj = {
 //|     32 bit value that dictates the PWM frequency in Hertz (cycles per
 //|     second). Only writeable when constructed with ``variable_frequency=True``.
 //|
+//|     Depending on how PWM is implemented on a specific board, the internal value
+//|     for the PWM's duty cycle may need to be recalculated when the frequency
+//|     changes. In these cases, the duty cycle is automatically recalculated
+//|     from the original duty cycle value. This should happen without any need
+//|     to manually re-set the duty cycle.
+//|
 STATIC mp_obj_t pulseio_pwmout_obj_get_frequency(mp_obj_t self_in) {
     pulseio_pwmout_obj_t *self = MP_OBJ_TO_PTR(self_in);
     check_for_deinit(self);