Merge pull request #4192 from dhalbert/pico-pwmout-top-fix-4189

RP2040: fix off-by-one PWM top issue

3 files changed, 34 insertions, 13 deletions

diff --git a/ports/raspberrypi/common-hal/pwmio/PWMOut.c b/ports/raspberrypi/common-hal/pwmio/PWMOut.c
index e861bab5a..00b461880 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);
 }
@@ -164,19 +176,28 @@ 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);
-    pwm_set_chan_level(self->slice, self->channel, actual_duty);
+    // Do arithmetic in 32 bits to prevent overflow.
+    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) {
     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 +208,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.
@@ -201,16 +222,17 @@ 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;
+        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 - 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(MAX_TOP, top);
         pwm_set_clkdiv_int_frac(self->slice, 1, 0);
-        pwm_set_wrap(self->slice, self->top - 1);
+        // 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);
 }
diff --git a/ports/raspberrypi/common-hal/pwmio/PWMOut.h b/ports/raspberrypi/common-hal/pwmio/PWMOut.h
index cbb0b707a..0070e188b 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 ec1f53c85..74fa28e4d 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"