From 3e4328140c96626312cae680d4d781acf61da7f9 Mon Sep 17 00:00:00 2001 From: Dan Halbert Date: Fri, 12 Feb 2021 15:34:36 -0500 Subject: [PATCH 1/2] fix off-by-one PWM top issue --- ports/raspberrypi/common-hal/pwmio/PWMOut.c | 17 +++++++++-------- ports/raspberrypi/common-hal/pwmio/PWMOut.h | 4 ++-- shared-bindings/adafruit_bus_device/SPIDevice.c | 1 - 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/ports/raspberrypi/common-hal/pwmio/PWMOut.c b/ports/raspberrypi/common-hal/pwmio/PWMOut.c index e861bab5a9..b4e7caa25a 100644 --- a/ports/raspberrypi/common-hal/pwmio/PWMOut.c +++ b/ports/raspberrypi/common-hal/pwmio/PWMOut.c @@ -164,7 +164,8 @@ void common_hal_pwmio_pwmout_deinit(pwmio_pwmout_obj_t* self) { extern void common_hal_pwmio_pwmout_set_duty_cycle(pwmio_pwmout_obj_t* self, uint16_t duty) { self->duty_cycle = duty; - uint16_t actual_duty = duty * self->top / ((1 << 16) - 1); + // Do arithmetic in 32 bits to prevent overflow. + uint16_t actual_duty = (uint32_t) duty * self->top / ((1 << 16) - 1); pwm_set_chan_level(self->slice, self->channel, actual_duty); } @@ -172,11 +173,11 @@ uint16_t common_hal_pwmio_pwmout_get_duty_cycle(pwmio_pwmout_obj_t* self) { return self->duty_cycle; } -void pwmio_pwmout_set_top(pwmio_pwmout_obj_t* self, uint32_t top) { +void pwmio_pwmout_set_top(pwmio_pwmout_obj_t* self, uint16_t top) { self->actual_frequency = common_hal_mcu_processor_get_frequency() / top; self->top = top; pwm_set_clkdiv_int_frac(self->slice, 1, 0); - pwm_set_wrap(self->slice, self->top - 1); + pwm_set_wrap(self->slice, self->top); } void common_hal_pwmio_pwmout_set_frequency(pwmio_pwmout_obj_t* self, uint32_t frequency) { @@ -187,7 +188,7 @@ void common_hal_pwmio_pwmout_set_frequency(pwmio_pwmout_obj_t* self, uint32_t fr target_slice_frequencies[self->slice] = frequency; // For low frequencies use the divider to give us full resolution duty_cycle. - if (frequency < (common_hal_mcu_processor_get_frequency() / (1 << 16))) { + if (frequency <= (common_hal_mcu_processor_get_frequency() / (1 << 16))) { // Compute the divisor. It's an 8 bit integer and 4 bit fraction. Therefore, // we compute everything * 16 for the fractional part. // This is 1 << 12 because 4 bits are the * 16. @@ -202,15 +203,15 @@ void common_hal_pwmio_pwmout_set_frequency(pwmio_pwmout_obj_t* self, uint32_t fr div16 = (1 << 12) - 1; } self->actual_frequency = frequency16 / div16; - self->top = 1 << 16; + self->top = (1 << 16) - 1; pwm_set_clkdiv_int_frac(self->slice, div16 / 16, div16 % 16); - pwm_set_wrap(self->slice, self->top - 1); + pwm_set_wrap(self->slice, self->top); } else { uint32_t top = common_hal_mcu_processor_get_frequency() / frequency; self->actual_frequency = common_hal_mcu_processor_get_frequency() / top; - self->top = top; + self->top = MIN(UINT16_MAX, top - 1); pwm_set_clkdiv_int_frac(self->slice, 1, 0); - pwm_set_wrap(self->slice, self->top - 1); + pwm_set_wrap(self->slice, self->top); } common_hal_pwmio_pwmout_set_duty_cycle(self, self->duty_cycle); } diff --git a/ports/raspberrypi/common-hal/pwmio/PWMOut.h b/ports/raspberrypi/common-hal/pwmio/PWMOut.h index cbb0b707a3..0070e188b5 100644 --- a/ports/raspberrypi/common-hal/pwmio/PWMOut.h +++ b/ports/raspberrypi/common-hal/pwmio/PWMOut.h @@ -39,11 +39,11 @@ typedef struct { bool variable_frequency; uint16_t duty_cycle; uint32_t actual_frequency; - uint32_t top; + uint16_t top; } pwmio_pwmout_obj_t; void pwmout_reset(void); // Private API for AudioPWMOut. -void pwmio_pwmout_set_top(pwmio_pwmout_obj_t* self, uint32_t top); +void pwmio_pwmout_set_top(pwmio_pwmout_obj_t* self, uint16_t top); #endif // MICROPY_INCLUDED_ATMEL_SAMD_COMMON_HAL_PWMIO_PWMOUT_H diff --git a/shared-bindings/adafruit_bus_device/SPIDevice.c b/shared-bindings/adafruit_bus_device/SPIDevice.c index ec1f53c858..74fa28e4d5 100644 --- a/shared-bindings/adafruit_bus_device/SPIDevice.c +++ b/shared-bindings/adafruit_bus_device/SPIDevice.c @@ -34,7 +34,6 @@ #include "lib/utils/buffer_helper.h" #include "lib/utils/context_manager_helpers.h" -#include "py/objproperty.h" #include "py/runtime.h" #include "supervisor/shared/translate.h" From 0ec99b37e0cb906d86590687ed06013516823cbe Mon Sep 17 00:00:00 2001 From: Dan Halbert Date: Sat, 13 Feb 2021 13:16:49 -0500 Subject: [PATCH 2/2] handle 100% duty cycle; improve actual_frequency calc --- ports/raspberrypi/common-hal/pwmio/PWMOut.c | 31 +++++++++++++++++---- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/ports/raspberrypi/common-hal/pwmio/PWMOut.c b/ports/raspberrypi/common-hal/pwmio/PWMOut.c index b4e7caa25a..00b461880c 100644 --- a/ports/raspberrypi/common-hal/pwmio/PWMOut.c +++ b/ports/raspberrypi/common-hal/pwmio/PWMOut.c @@ -45,6 +45,18 @@ uint32_t slice_variable_frequency; static uint32_t channel_use; static uint32_t never_reset_channel; +// Per the RP2040 datasheet: +// +// "A CC value of 0 will produce a 0% output, i.e. the output signal +// is always low. A CC value of TOP + 1 (i.e. equal to the period, in +// non-phase-correct mode) will produce a 100% output. For example, if +// TOP is programmed to 254, the counter will have a period of 255 +// cycles, and CC values in the range of 0 to 255 inclusive will +// produce duty cycles in the range 0% to 100% inclusive." +// +// So 65534 should be the maximum top value, and we'll set CC to be TOP+1 as appropriate. +#define MAX_TOP 65534 + static uint32_t _mask(uint8_t slice, uint8_t channel) { return 1 << (slice * CHANNELS_PER_SLICE + channel); } @@ -165,8 +177,16 @@ void common_hal_pwmio_pwmout_deinit(pwmio_pwmout_obj_t* self) { extern void common_hal_pwmio_pwmout_set_duty_cycle(pwmio_pwmout_obj_t* self, uint16_t duty) { self->duty_cycle = duty; // Do arithmetic in 32 bits to prevent overflow. - uint16_t actual_duty = (uint32_t) duty * self->top / ((1 << 16) - 1); - pwm_set_chan_level(self->slice, self->channel, actual_duty); + uint16_t compare_count; + if (duty == 65535) { + // Ensure that 100% duty cycle is 100% full on and not rounded down, + // but do MIN() to keep value in range, just in case. + compare_count = MIN(UINT16_MAX, (uint32_t) self->top + 1); + } else { + compare_count= ((uint32_t) duty * self->top + MAX_TOP / 2) / MAX_TOP; + } + // compare_count is the CC register value, which should be TOP+1 for 100% duty cycle. + pwm_set_chan_level(self->slice, self->channel, compare_count); } uint16_t common_hal_pwmio_pwmout_get_duty_cycle(pwmio_pwmout_obj_t* self) { @@ -202,15 +222,16 @@ void common_hal_pwmio_pwmout_set_frequency(pwmio_pwmout_obj_t* self, uint32_t fr if (div16 >= (1 << 12)) { div16 = (1 << 12) - 1; } - self->actual_frequency = frequency16 / div16; - self->top = (1 << 16) - 1; + self->actual_frequency = (frequency16 + (div16 / 2)) / div16; + self->top = MAX_TOP; pwm_set_clkdiv_int_frac(self->slice, div16 / 16, div16 % 16); pwm_set_wrap(self->slice, self->top); } else { uint32_t top = common_hal_mcu_processor_get_frequency() / frequency; self->actual_frequency = common_hal_mcu_processor_get_frequency() / top; - self->top = MIN(UINT16_MAX, top - 1); + self->top = MIN(MAX_TOP, top); pwm_set_clkdiv_int_frac(self->slice, 1, 0); + // Set TOP register. For 100% duty cycle, CC must be set to TOP+1. pwm_set_wrap(self->slice, self->top); } common_hal_pwmio_pwmout_set_duty_cycle(self, self->duty_cycle);