Merge pull request #63 from adafruit/pb-m4-timing

M4 Timing Adjustments for Broader Matrix Compatibility

README.md

@@ -115,3 +115,18 @@ compile-time).
 Most macros and functions begin with the prefix **\_PM\_** in order to
 avoid naming collisions with other code (exception being static functions,
 which can't be seen outside their source file).
+
+# Pull Requests
+
+If you encounter artifacts (noise, sparkles, dropouts and other issues) and
+it seems to resolve by adjusting the NOP counts, please do not submit this
+as a PR claiming a fix. Quite often what improves stability for one matrix
+type can make things worse for other types. Instead, open an issue and
+describe the hardware (both microcontroller and RGB matrix) and what worked
+for you. A general solution working across all matrix types typically
+involves monitoring the signals on a logic analyzer and aiming for a 50%
+duty cycle on the CLK signal, 20 MHz or less, and then testing across a
+wide variety of different matrix types to confirm; trial and error on just
+a single matrix type is problematic. Maintainers: this goes for you too.
+Don't merge a "fix" unless you've checked it out on a 'scope and on tested
+across a broad range of matrices.

library.properties

@@ -1,5 +1,5 @@
 name=Adafruit Protomatter
-version=1.5.9
+version=1.5.10
 author=Adafruit
 maintainer=Adafruit <info@adafruit.com>
 sentence=A library for Adafruit RGB LED matrices.

src/arch/samd51.h

@@ -196,19 +196,29 @@ uint32_t _PM_timerStop(Protomatter_core *core) {
   return count;
 }
 
-// See notes in core.c before the "blast" functions
+// See notes in core.c before the "blast" functions.
+// The NOP counts here were derived by monitoring on a fast logic analyzer,
+// aiming for 20 MHz clock at 50% duty cycle (for the unrolled parts of the
+// 'blast' loop...every Nth bit is somewhat longer), and tested for each
+// F_CPU setting. This seems to have the broadest compatibility across many
+// matrix varieties. Only one, a 64x32 flex matrix, showed some artifacts at
+// the end of a 4-matrix chain at 120-150 MHz F_CPU -- either use in shorter
+// chains, or can kludge it by running at 180-200 MHz or by moving one NOP
+// from the Low to High section (but which then causes issues with other
+// matrix types, so it's not done here), unfortunately no means of run-time
+// configuration for this.
 #if F_CPU >= 200000000
-#define _PM_clockHoldHigh asm("nop; nop; nop; nop; nop");
-#define _PM_clockHoldLow asm("nop; nop");
+#define _PM_clockHoldHigh asm("nop; nop");
+#define _PM_clockHoldLow asm("nop; nop; nop; nop; nop");
 #elif F_CPU >= 180000000
-#define _PM_clockHoldHigh asm("nop; nop; nop; nop");
-#define _PM_clockHoldLow asm("nop");
+#define _PM_clockHoldHigh asm("nop; nop");
+#define _PM_clockHoldLow asm("nop; nop; nop; nop");
 #elif F_CPU >= 150000000
-#define _PM_clockHoldHigh asm("nop; nop; nop");
-#define _PM_clockHoldLow asm("nop");
+#define _PM_clockHoldHigh asm("nop");
+#define _PM_clockHoldLow asm("nop; nop; nop");
 #else
-#define _PM_clockHoldHigh asm("nop; nop; nop");
-#define _PM_clockHoldLow asm("nop");
+#define _PM_clockHoldHigh asm("nop");
+#define _PM_clockHoldLow asm("nop; nop");
 #endif
 
 #define _PM_minMinPeriod 160

src/core.c

@@ -726,7 +726,7 @@ IRAM_ATTR static void blast_byte(Protomatter_core *core, uint8_t *data) {
   _PM_PORT_TYPE rgbclock = core->rgbAndClockMask; // RGB + clock bit
 #endif
   _PM_PORT_TYPE clock = core->clockMask; // Clock bit
-  uint8_t chunks = (core->chainBits + (_PM_chunkSize - 1)) / _PM_chunkSize;
+  uint16_t chunks = (core->chainBits + (_PM_chunkSize - 1)) / _PM_chunkSize;
 
   // PORT has already been initialized with RGB data + clock bits
   // all LOW, so we don't need to initialize that state here.