From 7bd6440c14121d702a3aa08cd78c780c27aa75d4 Mon Sep 17 00:00:00 2001 From: Phillip Burgess Date: Thu, 3 Aug 2023 15:02:09 -0700 Subject: [PATCH 1/5] Adjust NOPs on M4 for new matrices --- src/arch/samd51.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/arch/samd51.h b/src/arch/samd51.h index d070daf..1fd7fab 100644 --- a/src/arch/samd51.h +++ b/src/arch/samd51.h @@ -199,16 +199,16 @@ uint32_t _PM_timerStop(Protomatter_core *core) { // See notes in core.c before the "blast" functions #if F_CPU >= 200000000 #define _PM_clockHoldHigh asm("nop; nop; nop; nop; nop"); -#define _PM_clockHoldLow asm("nop; nop"); +#define _PM_clockHoldLow asm("nop; nop; nop; nop"); #elif F_CPU >= 180000000 #define _PM_clockHoldHigh asm("nop; nop; nop; nop"); -#define _PM_clockHoldLow asm("nop"); +#define _PM_clockHoldLow asm("nop; nop; nop"); #elif F_CPU >= 150000000 -#define _PM_clockHoldHigh asm("nop; nop; nop"); -#define _PM_clockHoldLow asm("nop"); +#define _PM_clockHoldHigh asm("nop; nop; nop; nop"); +#define _PM_clockHoldLow asm("nop; nop; nop"); #else #define _PM_clockHoldHigh asm("nop; nop; nop"); -#define _PM_clockHoldLow asm("nop"); +#define _PM_clockHoldLow asm("nop; nop"); #endif #define _PM_minMinPeriod 160 From b4788466dee7a8f0a70a50655c4d218260eb63a5 Mon Sep 17 00:00:00 2001 From: Phillip Burgess Date: Thu, 3 Aug 2023 16:22:48 -0700 Subject: [PATCH 2/5] M4: better CLK duty cycle --- src/arch/samd51.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/arch/samd51.h b/src/arch/samd51.h index 1fd7fab..20e4cee 100644 --- a/src/arch/samd51.h +++ b/src/arch/samd51.h @@ -198,16 +198,16 @@ uint32_t _PM_timerStop(Protomatter_core *core) { // See notes in core.c before the "blast" functions #if F_CPU >= 200000000 -#define _PM_clockHoldHigh asm("nop; nop; nop; nop; nop"); -#define _PM_clockHoldLow asm("nop; nop; 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; nop; 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; nop"); +#define _PM_clockHoldHigh asm("nop"); #define _PM_clockHoldLow asm("nop; nop; nop"); #else -#define _PM_clockHoldHigh asm("nop; nop; nop"); +#define _PM_clockHoldHigh asm("nop"); #define _PM_clockHoldLow asm("nop; nop"); #endif From 2c070c86408c802053b6587f3bca83e4373ef684 Mon Sep 17 00:00:00 2001 From: Phillip Burgess Date: Thu, 3 Aug 2023 18:21:27 -0700 Subject: [PATCH 3/5] Fix chunk counter type for long chains --- src/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core.c b/src/core.c index 1416284..3d38853 100644 --- a/src/core.c +++ b/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. From f1956d25064d3355d3993cbb2ce9bed3b2f01a44 Mon Sep 17 00:00:00 2001 From: Phillip Burgess Date: Fri, 4 Aug 2023 08:58:56 -0700 Subject: [PATCH 4/5] Add notes on SAMD51 NOPs --- src/arch/samd51.h | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/arch/samd51.h b/src/arch/samd51.h index 20e4cee..86abc92 100644 --- a/src/arch/samd51.h +++ b/src/arch/samd51.h @@ -196,7 +196,17 @@ 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"); #define _PM_clockHoldLow asm("nop; nop; nop; nop; nop"); From eca749b742eba54e1286b87de29ab30bc872b489 Mon Sep 17 00:00:00 2001 From: Phillip Burgess Date: Fri, 4 Aug 2023 09:28:23 -0700 Subject: [PATCH 5/5] Bump version # for M4 fixes, add PR note to README --- README.md | 15 +++++++++++++++ library.properties | 2 +- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 3518047..39a6012 100644 --- a/README.md +++ b/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. diff --git a/library.properties b/library.properties index dd46d1c..56d3243 100644 --- a/library.properties +++ b/library.properties @@ -1,5 +1,5 @@ name=Adafruit Protomatter -version=1.5.9 +version=1.5.10 author=Adafruit maintainer=Adafruit sentence=A library for Adafruit RGB LED matrices.