From 4fdf518251dd0751cf4e9b06b7ffb2b944e7d15c Mon Sep 17 00:00:00 2001 From: Thea Flowers Date: Tue, 22 Oct 2019 10:37:36 -0700 Subject: Track unadjusted PWM duty cycle to avoid accumulating conversion errors Fixes #2086 When the frequency of a `PWMOut` is change it re-sets the PWM's duty cycle as well, since the registers have to be re-calculated based on the new frequency. Unfortunately, `common_hal_pulseio_pwmout_get_duty_cycle` will return a value very close to, but not exactly, the value passed to `common_hal_pulseio_pwmout_set_duty_cycle`. If the frequency is modified without the calling code also re-setting the duty cycle then the duty cycle will decay over time. This fixes that problem by tracking the unadjusted duty cycle and re-setting the duty cycle to that value when the frequency is changed. --- ports/atmel-samd/common-hal/pulseio/PWMOut.c | 11 +++++++++-- ports/atmel-samd/common-hal/pulseio/PWMOut.h | 1 + 2 files changed, 10 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..92305161f 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 calucating + // 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 recalcuate 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); -- cgit v1.2.3 From 4b742f88d9d7b56a81de8559f3d55bf38bf03922 Mon Sep 17 00:00:00 2001 From: Thea Flowers Date: Fri, 25 Oct 2019 10:50:27 -0700 Subject: Fix spelling --- ports/atmel-samd/common-hal/pulseio/PWMOut.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ports/atmel-samd/common-hal/pulseio/PWMOut.c b/ports/atmel-samd/common-hal/pulseio/PWMOut.c index 92305161f..0adb23fc5 100644 --- a/ports/atmel-samd/common-hal/pulseio/PWMOut.c +++ b/ports/atmel-samd/common-hal/pulseio/PWMOut.c @@ -323,9 +323,9 @@ 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 calucating + // 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 recalcuate the + // 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; -- cgit v1.2.3 From 6782948751d5ca711213564f59b28dc3c4ca85bb Mon Sep 17 00:00:00 2001 From: Thea Flowers Date: Fri, 25 Oct 2019 10:56:41 -0700 Subject: Note behavior in documentation --- shared-bindings/pulseio/PWMOut.c | 11 +++++++++++ 1 file changed, 11 insertions(+) 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); -- cgit v1.2.3