add pwm fixes

Author: timchinowsky

ports/atmel-samd/common-hal/pwmio/PWMOut.c

@@ -249,8 +249,8 @@ extern void common_hal_pwmio_pwmout_set_duty_cycle(pwmio_pwmout_obj_t *self, uin
     // 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;
 
+    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;
@@ -271,8 +271,6 @@ extern void common_hal_pwmio_pwmout_set_duty_cycle(pwmio_pwmout_obj_t *self, uin
         }
         uint8_t channel = tcc_channel(t);
         Tcc *tcc = tcc_insts[t->index];
-        // Enable double-buffering
-        tcc->CTRLBCLR.bit.LUPD = 1;
         #ifdef SAMD21
         tcc->CCB[channel].reg = adjusted_duty;
         #endif

tools/pwm/README.md

@@ -7,8 +7,6 @@
 import sys
 import random
 
-cr10 = 10 ** (1 / 3)
-
 exponents = [
     2,
     2.333,
@@ -27,15 +25,15 @@
     6.667,
     7,
 ]
+
 freqs = [int(10**f) for f in exponents]
 
 top = 65536
 den = 10
 duties = [int(top * num / den) for num in range(1, den)]
 duties = [1, 65534, 1] + duties
-
 freq_duration = 1.0
-duty_duration = 0.00000001
+duty_duration = 0.000000001
 
 print("\n\n")
 board_name = sys.implementation[2]
@@ -45,6 +43,7 @@
     "Grand Central": (("D51", "TCC"), ("A15", "TC")),
     "Metro M0": (("A2", "TC"), ("A3", "TCC")),
     "ESP32-S3-DevKit": (("IO6", ""),),  # marked D5 on board for XIAO-ESP32-s3
+    "Feather ESP32-S2": (("D9", ""),),
     "XIAO nRF52840": (("D9", ""),),
 }
 

tools/pwm/code.py

@@ -1,16 +1,135 @@
-def duty(filename):
+import math
+import matplotlib.pyplot as plt
+import numpy as np
+from PIL import Image
+from PIL import Image
+from PIL import ImageFont
+from PIL import ImageDraw
+
+
+def read(
+    filename,
+    image_filename=None,
+    height=480,
+    width=640,
+    f_min=10,
+    f_max=1e8,
+    title="",
+    subtitle="",
+    version="",
+    duty_labels=(0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9),
+    freq_calls=tuple(),
+    margin=0.01,
+    duty_color=(255, 0, 0),
+    freq_color=(0, 255, 0),
+    calls_color=(0, 255, 255),
+    title_color=(255, 255, 255),
+):
+    """Read a one channel logic analyzer raw csv data file and generate a plot visualizing the PWM signal
+    captured in the file.  Each line of the file is a <time, level> pair indicating the times (in seconds)
+    at which the signal transitioned to that level.  For example:
+                           1.726313020,0
+                           1.726313052,1
+                           1.726313068,0
+                           1.727328804,1
+    """
+    left = 80
+    right = 80
+    bottom = 20
+    top = 60
+    x0 = left
+    y0 = top
+    y1 = height - bottom
+    x1 = width - right
     rising_edge = None
     falling_edge = None
+    pixels = np.zeros((height, width, 3), dtype=np.uint8) * 255
+    t0 = None
+    t1 = None
+    val = None
     with open(filename, "r") as f:
-        line = f.readline()
-        t, val = line.split(",")
-        t = float(t)
-        val = int(val)
-        if val == 0:
-            if falling_edge and rising_edge is not None:
-                period = t - falling_edge
-                duty_cycle = (t - rising_edge) / period
-                print(t, val, period, duty_cycle)
-            falling_edge = t
-        else:
-            rising_edge = t
+        first = True
+        for line in f:  # find min and max t, excluding first and last values
+            if val is not None:
+                if not first:
+                    if t0 is None or t < t0:
+                        t0 = t
+                    if t1 is None or t > t1:
+                        t1 = t
+                else:
+                    first = False
+            t, val = line.split(",")
+            try:
+                t = float(t)
+                val = int(val)
+            except ValueError:
+                val = None
+    print("plotting", t1 - t0, "seconds")
+
+    with open(filename, "r") as f:
+        pts = 0
+        f_log_max = int(math.log10(f_max))
+        f_log_min = int(math.log10(f_min))
+        f_log_span = f_log_max - f_log_min
+        for line in f:
+            t, val = line.split(",")
+            try:
+                t = float(t)
+                val = int(val)
+            except ValueError:
+                val = None
+            if val == 1:
+                if falling_edge is not None and rising_edge is not None:
+                    period = t - rising_edge
+                    frequency = 1 / period
+                    duty_cycle = (falling_edge - rising_edge) / period
+                    x = int((x1 - x0) * (t - t0) / (t1 - t0)) + x0
+                    y_duty = int((1 - duty_cycle) * (y1 - y0)) + y0
+                    y_freq = (
+                        int((y1 - y0) * (1 - (math.log10(frequency) - f_log_min) / f_log_span))
+                        + y0
+                    )
+                    x = max(x0, min(x, x1 - 1))
+                    y_duty = max(y0, min(y_duty, y1 - 1))
+                    y_freq = max(y0, min(y_freq, y1 - 1))
+                    pixels[y_duty, x] = duty_color
+                    pixels[y_freq, x] = freq_color
+                    pts += 1
+                rising_edge = t
+            elif val == 0:
+                falling_edge = t
+        image = Image.fromarray(pixels)
+        draw = ImageDraw.Draw(image)
+        draw.text((left - 10, top), "Duty", duty_color, anchor="rt")
+        draw.text((0, top), "Calls", calls_color, anchor="lt")
+        draw.text((width - right / 2, top), "Freq", freq_color, anchor="mt")
+
+        for duty in duty_labels:
+            draw.text(
+                (left - 10, y0 + (y1 - y0) * (1 - duty)),
+                f"{int(100*duty):d}%",
+                duty_color,
+                anchor="rm",
+            )
+        for exponent in range(f_log_min + 1, f_log_max):
+            draw.text(
+                (width - right / 2, y0 + (y1 - y0) * (1 - (exponent - f_log_min) / (f_log_span))),
+                str(10**exponent) + " Hz",
+                freq_color,
+                anchor="mm",
+            )
+        for freq, count in freq_calls:
+            draw.text(
+                (0, y0 + (y1 - y0) * (1 - (math.log10(freq) - f_log_min) / (f_log_span))),
+                f"{count} Hz",
+                calls_color,
+                anchor="lm",
+            )
+        subtitle += f", showing {pts} PWM cycles"
+        draw.text((width * 0.5, height * margin), title, title_color, anchor="mm")
+        draw.text((width * 0.5, height * 4 * margin), version, title_color, anchor="mm")
+        draw.text((width * 0.5, height * 8 * margin), subtitle, title_color, anchor="mm")
+        image.show()
+        if image_filename is not None:
+            image.save(image_filename)
+    return image

tools/pwm/duty.py

@@ -0,0 +1,95 @@
+# Fixing API delays and rounding in samd PWM
+
+Blocking delays are observed when setting `duty_cycle` in the samd port of CircuitPython (see https://github.com/adafruit/circuitpython/issues/7653), behavior which interferes with some applications of PWM, and which differs from from that of other ports.  Here is a comparison of execution times of several basic operations on three different ports, showing the off-the-charts slowness of samd in setting PWM duty cycle:
+
+![](samd51_benchmarks.png)
+
+Investigation of this issue revealed another problem.  In some cases, the API will round a non-zero duty cycle down to zero, causing the PWM signal to stop cycling at all.  The cartoon below shows how for instance rounding can transform three PWM cycles, with the first almost 100% duty and the second two close to 0% duty, into one PWM cycle with three times the period and ~33% duty:
+
+![](pwm_rounding.png)
+
+The changes proposed in this PR address both of these issues.
+
+## Implementation
+
+The CP samd21/51 port serves two different processor families, each of which have two different types of PWM peripherals (TC and TCC).  Moreover, the TC peripheral for the samd21 is not identical to that of the samd51, as is hinted at by the datasheet excerpts below.  The description of TC for the samd51 notably adds mention of double-buffering, while the samd21/51 descriptions for TCC are identical:
+
+<details>
+<summary>Datasheet summaries of samd21 and samd51 TC and TCC peripherals</summary>
+
+![](samd_tc_tcc_datasheet_blurbs.png)
+
+</details>
+
+Conditional compilations and `if` statements in `PWMOUT.c` provide separate handling for each of the four combinations of samd21/samd51 and TC/TCC.  The code modifications consist of (1) removing synchronization delays and PWM configuration which were found to do more harm than good, and (2) adding `if` statements to disallow rounding down to zero.
+
+## Testing
+
+All combinations of samd21/samd51 and TC/TCC were tested separately by selecting board pins which support only one or the other of TC or TCC.  The boards and pins used (outlined in red) are shown in the diagram below:
+
+<details>
+<summary>Pins implementing TC and TCC on samd21/51</summary>
+
+![](samd21_51_board_pinouts.png)
+
+</details>
+
+Data gathered from each of the four types of PWM peripheral before and after code modifications are shown below.  Details of the test tools and procedure are [here](/tools/pwm/README.md).
+
+### samd51 TCC
+
+| Before | After |
+| ----------- | ---------- |
+| ![](ramps_samd51_tcc_910.png) | ![](ramps_samd51_tcc_910_fixed.png)  |
+
+In the "before" plot, the variation with frequency of call throughput (shown in cyan) and PWM ramp slope points out the frequency-dependent delay found when setting duty cycle.  The clutter in duty cycle and frequency measurements results from zero-rounding.  Neither is seen in the "after" plot - the PWM ramp slope is nearly constant, and duty cycle and frequency measurements contain no outliers other than at frequency transitions.
+
+### samd51 TC
+
+| Before | After |
+| ----------- | ---------- |
+| ![](ramps_samd51_tc_910.png) | ![](ramps_samd51_tc_910_fixed.png)  |
+
+These results are very similar to those for TCC.  Both delay and rounding artifacts are seen in the "before" plot and not in the "after" plot.
+
+### samd21 TCC
+
+| Before | After |
+| ----------- | ---------- |
+| ![](ramps_samd21_tcc_910.png) | ![](ramps_samd21_tcc_910_fixed.png)  |
+
+No frequency-dependent delay is seen for samd21 TCC, only rounding artifacts.  Compared to samd51, call throughput is ~4x slower (as would be expected for samd21's lower clock rate).  No rounding artifacts are seen in the "after" plot.
+
+### samd21 TC
+
+| Before | After |
+| ----------- | ---------- |
+| ![](ramps_samd21_tc_910.png) | ![](ramps_samd21_tc_910_fixed.png)  |
+
+So, this is the problem child.  samd21 TC does not provide double buffering of duty cycle registers, so writing to registers may or may not induce duty cycle artifacts depending on when exactly the write happens.  My takeaway regarding samd21 TC is, don't expect too much of it, but it's fine for less demanding applications.  Here is samd21 tc updating once a second just fine:
+
+![](slow_samd21_tc_910_fixed.png)
+
+High-rate samd21 TC should work well if updated in a DMA/ISR framework like that implemented in `audiopwmio` in other ports.  A logical next step for samd21/51 is to implement `audiopwmio`.
+
+## Other ports
+
+This PR describes PWM fix-ups for samd21/51, but a quick look indicates that other ports could use some attention as well.  Here is some test data for three other ports.
+
+### CircuitPython 9.04 on RP2040
+
+<img src="ramps_rp2040_904.png" />
+
+This looks pretty good except it shows the same rounding down to zero issue as samd51.  Probably all ports have this.
+
+### CircuitPython 9.01 on nRF52840
+
+<img src="ramps_nrf52840_901.png" />
+
+Similar to above, looks pretty good except for rounding to zero.  Tops out below 5 MHz due to lower clock speed.
+
+### CircuitPython 9.05 on ESP32
+
+<img src="ramps_esp32s2_905.png" />
+
+This is more complicated - looks like frequency tops out at 10 kHz, but duty cycle continues to be adjusted downward as if frequency has increased.  Needs investigation.