Templates by davepl · Pull Request #747 · PlummersSoftwareLLC/NightDriverStrip

davepl · 2025-08-17T22:22:14Z

Replace macro stucture with typed templates

I read the contribution guidelines in CONTRIBUTING.md.
I understand the BlinkenPerBit metric, and maximized it in this PR.
I selected main as the target branch.
All code herein is subjected to the license terms in COPYING.txt.

rbergen · 2025-08-19T06:31:49Z

I noticed this PR (#747) is the same as PR #748, except this one aims to merge the templates branch into ttgo, and the other into main. I will focus my review on #748 and leave this PR alone.

robertlipe · 2025-08-19T07:16:07Z

Even #748 is a tough journey, as noted in: #748 (review) #748 (comment) #748 (comment) (I'm calling this out since we've BOTH fallen out of sync in comments by the other this week.) I think this is actually a tangled-up version of three large PRs (rework audio, rework config, and rework effects instantiation) plus a raft of minor ones unrelated to the primary topic of any of those. I mean, we "all" know the code in question, and the changes are all recent enough that we can somewhat mentally triage such things, but it's pretty painful for the Harries on the project that may not share that grasp and are attempting due diligence in analyzing a problem that affects them and then slam into big mixing bowls of code like that. I also never really realized how badly GitHub's review tools just fall down on PRs of this magnitude. Is that really how Microsoft runs code reviews internally? Is there a better (free/cheap) tool for reviewing such CLs beyond the GH web links in these messages? I'm probably spoiled by Google's internal tools that even years ago handled things like code motion, overlapping (stacked) changesets, marking reviews as 'finished' while the CL itself is still changing, and tracking whitespace changes better than this. I've probably just not kept up with the state of the art in open-source-ville, so I welcome being introduced to better tooling, though my budget is a wee smaller than Google's. :-) When a substantial percentage of the varioius ISO committee members, GCC, and Clang developers hanging out at the company, we had an embarrassment of riches internally and it's one of the few things I miss deeply. So, yes, this PR is messy. I'd welcome tips on making the best of reviewing code like this, though, because life is sometimes messy. RJL

…

On Tue, Aug 19, 2025 at 1:32 AM Rutger van Bergen ***@***.***> wrote: *rbergen* left a comment (PlummersSoftwareLLC/NightDriverStrip#747) <#747 (comment)> I noticed this PR (#747 <#747>) is the same as PR #748 <#748>, except this one aims to merge the templates branch into ttgo, and the other into main. I will focus my review on #748 <#748> and leave this PR alone. — Reply to this email directly, view it on GitHub <#747 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD346LNFGXFWTVXXTDR33OLAG5AVCNFSM6AAAAACEDOUCI2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTCOJZGQYDENBUG4> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

davepl · 2025-08-19T16:04:56Z

It was a lot of work to write, too :-)

I'd say one approach would be to review the deltas as I submitted them - first the audio, then the templates, etc. Each is relative to the one before it was I careful NOT to make it just one big combined PR (or that was my intent).

Honestly, for soundanalyzer and effects.cpp I'd review the final file, not the diff.

Copilot

Pull Request Overview

This PR converts the LED effects system from a macro-based structure to a type-safe template system, improving code maintainability and type safety. The changes remove preprocessor macros in favor of C++ templates and enum-based effect identifiers, while providing backward compatibility through transitional constructors.

Key changes:

Replaces macro-based effect definitions with template-based registration system
Converts effect IDs from preprocessor defines to strongly-typed enums
Updates memory allocation utilities to use proper template overloading

Reviewed Changes

Copilot reviewed 70 out of 71 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
src/gfxbase.cpp	Updates `make_unique_psram_array` calls and removes blank lines
src/effects.cpp	Major refactoring from macro-based to template-based effect registration
src/effectmanager.cpp	Updates to use new enum-based effect IDs
src/deviceconfig.cpp	Updates memory allocation function call
platformio.ini	Build configuration updates and removed obsolete environment
include/types.h	Template overload improvements for PSRAM allocation
include/soundanalyzer.h	Conditional compilation and namespace fixes
include/ledstripeffect.h	Effect ID system overhaul and macro replacement
include/globals.h	Namespace cleanup for `accumulate` function
include/effectsupport.h	Template-based effect registration helpers
include/effects/strip/*.h	Effect ID updates across all strip effects
include/effects/matrix/*.h	Effect ID updates across all matrix effects
include/effects.h	Conversion from preprocessor defines to enums
include/effectfactories.h	Factory function type updates for template compatibility

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-08-19T19:54:09Z

 #include "effects/strip/meteoreffect.h"         // meteor blend effect
 #include "effects/strip/stareffect.h"           // star effects
-#include "effects/strip/bouncingballeffect.h"   // bouncing ball effectsenable+
+#include "effects/strip/bouncingballeffect.h"   // bouncing ball effects


Correct the typo in the comment. Change 'effectsenable+' to 'effects'.

Copilot · 2025-08-19T19:54:09Z

  public:

-    ColorBeatWithFlash(const String & strName) : BeatEffectBase(), ParticleSystem<RingParticle>(), LEDStripEffect(EFFECT_STRIP_COLOR_BEAT_WITH_FLASH, strName)
+    ColorBeatWithFlash(const String & strName) : BeatEffectBase(), ParticleSystem<RingParticle>(), LEDStripEffect(idStripColorBeatWithFlash, strName)


Missing static constexpr EffectId kId and effectId() override in ColorBeatWithFlash class. This is inconsistent with the pattern used in other effect classes throughout the codebase.

Copilot · 2025-08-19T19:54:10Z


    ColorBeatOverRed(const String & strName)
-      : LEDStripEffect(EFFECT_STRIP_COLOR_BEAT_OVER_RED, strName),
+  : LEDStripEffect(idStripColorBeatOverRed, strName),


Missing static constexpr EffectId kId and effectId() override in ColorBeatOverRed class. This is inconsistent with the pattern used in other effect classes throughout the codebase.

Copilot · 2025-08-19T19:54:10Z


    MoltenGlassOnVioletBkgnd(const String & strName, const CRGBPalette16 & Palette)
-      : LEDStripEffect(EFFECT_STRIP_MOLTEN_GLASS_ON_VIOLET_BKGND, strName),
+  : LEDStripEffect(idStripMoltenGlassOnVioletBkgnd, strName),


Missing static constexpr EffectId kId and effectId() override in MoltenGlassOnVioletBkgnd class. This is inconsistent with the pattern used in other effect classes throughout the codebase.

Copilot · 2025-08-19T19:54:10Z


    NewMoltenGlassOnVioletBkgnd(const String & strName, const CRGBPalette16 & Palette)
-      : LEDStripEffect(EFFECT_STRIP_NEW_MOLTEN_GLASS_ON_VIOLET_BKGND, strName),
+  : LEDStripEffect(idStripNewMoltenGlassOnVioletBkgnd, strName),


Missing static constexpr EffectId kId and effectId() override in NewMoltenGlassOnVioletBkgnd class. This is inconsistent with the pattern used in other effect classes throughout the codebase.

Copilot · 2025-08-19T19:54:11Z

+  FanBeatEffect(const String & strName) : LEDStripEffect(idStripFanBeat, strName)
  {
  }



There are two consecutive 'public:' access specifiers. Remove the duplicate on line 385.

Copilot · 2025-08-19T19:54:11Z

 public:

-    PatternQR() : LEDStripEffect(EFFECT_MATRIX_QR, "QR")
+    PatternQR() : LEDStripEffect(idMatrixQR, "QR")


Missing static constexpr EffectId kId and effectId() override in PatternQR class. This is inconsistent with the pattern used in other effect classes throughout the codebase.

Copilot · 2025-08-19T19:54:11Z

  public:

-    PatternPulse() : LEDStripEffect(EFFECT_MATRIX_PULSE, "Pulse")
+    PatternPulse() : LEDStripEffect(idMatrixPulse, "Pulse")


Missing static constexpr EffectId kId and effectId() override in PatternPulse class. This is inconsistent with the pattern used in other effect classes throughout the codebase.

Copilot · 2025-08-19T19:54:12Z

-  PatternSpiro() : LEDStripEffect(EFFECT_MATRIX_SPIRO, "Spiro")
+  PatternSpiro() : LEDStripEffect(idMatrixSpiro, "Spiro")
  {
  }


Missing static constexpr EffectId kId and effectId() override in PatternSpiro class. This is inconsistent with the pattern used in other effect classes throughout the codebase.

Copilot · 2025-08-19T19:54:12Z

-  PatternRainbowFlag() : LEDStripEffect(EFFECT_MATRIX_RAINBOW_FLAG, "RainbowFlag")
+  PatternRainbowFlag() : LEDStripEffect(idMatrixRainbowFlag, "RainbowFlag")
  {
  }


Missing static constexpr EffectId kId and effectId() override in PatternRainbowFlag class. This is inconsistent with the pattern used in other effect classes throughout the codebase.

rbergen · 2025-08-19T20:16:16Z

It was a lot of work to write, too :-)

Trust me, you're talking about PR #746. :)

I think we should move towards merging that first, and then focus on #748.

This PR (#747) is really irrelevant if we follow that approach; #748 will merge the changes since #746 straight into main.

Make SoundAnalyzer a template, remove Mic type; Use proper TFT_eSPI includes, fines;

davepl added 8 commits August 17, 2025 13:13

Change from macros to templates

2b4deb8

Add kId everywhere

f593b46

Make radial fire dynamically update, fix psram array allocator

9b11683

DEFINE to kConst

e0ce809

Move accumulate out of std, various tweaks

b807b67

ELECROW use I2S_AUDIO, trim dead code

b4987d9

Finish adding effectId() to effects, removes belt project

e51a8c4

Fix Init base class call

3417e4f

davepl requested review from Copilot and rbergen August 19, 2025 19:50

Copilot AI reviewed Aug 19, 2025

View reviewed changes

davepl and others added 3 commits August 20, 2025 13:03

Merge pull request #746 from PlummersSoftwareLLC/ttgo

7fed301

Make SoundAnalyzer a template, remove Mic type; Use proper TFT_eSPI includes, fines;

Merge remote-tracking branch 'upstream/main' into templates

3d2c7ba

Remove empty soundanalyzer.cpp

48a31a2

davepl merged commit b4120b8 into ttgo Aug 21, 2025
141 checks passed

Uh oh!

Conversation

davepl commented Aug 17, 2025

Uh oh!

rbergen commented Aug 19, 2025

Uh oh!

robertlipe commented Aug 19, 2025 via email

Uh oh!

davepl commented Aug 19, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

rbergen commented Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rbergen commented Aug 19, 2025 •

edited

Loading