Fix static initialization order fiasco for psram_allocator globals

[robertlipe]

Description

Wrap global static objects that use psram_allocator in Meyer's singletons (Construct-On-First-Use) to prevent PSRAM allocation before the memory subsystem is initialized during app_main().

Affected globals:

• AnimatedGIFs map and g_ptrGIFDecoder (PatternAnimatedGIF.h)
• weatherIcons map (PatternWeather.h)
• CWebServer::mySettingSpecs (webserver.cpp/h)

This is particularly terrible because we call psramAlloc() before main() but psram isn't configured until after we're inside of setup().

Contributing requirements

• 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]

Two notes before the actual single comment I want to make:

• I will accept a "nitpicker" sneer on what I'm about to argue for
• I'll write the suggestion once, but it can be projected to all cases where a global variable was replaced by a function (call), and I do indeed mean to make it for all of those.

Now that the global variable has become a function that is called, can we call the function once within a single function context, and stick the result of the call in a local variable that we then use, if we use it more than once? I know the simple return of a static function variable will be inlined down to nothing, but I'd like to keep our improved code clean in terms of intent.

[robertlipe]

Accepted. I'll change it. (you can quit reading here if you like :-) )

It's funny because this particular instance is one I DID outsource to AI (once I recognized the problem well enough to describe it) and actually MADE it do that. Pointer-chasing has always been a pet peeve of mine. I know the OO die-hards love it, but I understand branch prediction, cache misses, pipeline flushes, the difficulty of proving that a function is pure and that nothing has changed since the last call to it (good luck with that in a threaded world), the need to have the register allocator tiptoe around callee registers to avoid spills, and all that. I thought this project LIKED that model, though, and I was pretty sure that you were the one pitched me on it.

I've not disassembled it, but I'm pretty sure I know that the number of overhea clock cycles in things like

256:        g()->setTextColor(g()->to16bit(CRGB::Black));
257:        g()->setCursor(x-1, y);   g()->print(pszText);
258:        g()->setCursor(x+1, y);   g()->print(pszText);
259:        g()->setCursor(x, y-1);   g()->print(pszText);
260:        g()->setCursor(x, y+1);   g()->print(pszText);
262:        g()->setTextColor(g()->to16bit(CRGB::White));
263:        g()->setCursor(x, y);

or

147:        g()->GetNoise().noise_y += dy * 4;
148:        g()->GetNoise().noise_x += dx * 4;
149:        g()->GetNoise().noise_z += dz * 4;

would make me queasy. Most of the things in deviceConfig resolve down to runtime constants anyway—effectively globals. It's not like we have multiple EffectManagers or JSONWriters or DrawSafeCircles or whatever. They're globals. So I actually rocked the boat here to make it more in line with my understanding of what's "NightDriver-y." :-)

I'll change this to allocate these things once and put them in a member or pass them around as arguments.

That said, if I'm cracking this PR open again (that's OK), I have a different fix in mind for all of this anyway...but that's another area where once I start chopping at that tree, its' going to be a project of its own. :-) At least one of these tables that we're carefully crafting ... sure seems to be an array of constants.

I would offer to move this back to 'draft' to show that I agree this needs some attention and I'll fix it, but, well...

[davepl]

I’m no rocket surgeon but as long as GetNoise() is const ref, it should decompose to strict assignment [EAX+n]. The compiler’s pretty smart if you’re willing to share all you know about the data. What matters is: g() must be cheap and visible to the compiler (it is) setCursor() / GetNoise() should be small, non-virtual, and ideally defined in the h file GetNoise() should return a reference, not a value (it does) we should compile with optimization (-O2/-Os) and ideally LTO (not sure on LTO) If those things are true, the code collapses to no worse than storing to global variables. - Dave

…

On Apr 8, 2026, at 6:34 PM, Robert Lipe ***@***.***> wrote: @robertlipe commented on this pull request. In include/effects/matrix/PatternAnimatedGIF.h <#837 (comment)>: > + auto gif = GetAnimatedGIFs().find(_gifIndex); + if (gif == GetAnimatedGIFs().end()) Accepted. I'll change it. (you can quit reading here if you like :-) ) It's funny because this particular instance is one I DID outsource to AI (once I recognized the problem well enough to describe it) and actually MADE it do that. Pointer-chasing has always been a pet peeve of mine. I know the OO die-hards love it, but I understand branch prediction, cache misses, pipeline flushes, the difficulty of proving that a function is pure and that nothing has changed since the last call to it (good luck with that in a threaded world), the need to have the register allocator tiptoe around callee registers to avoid spills, and all that. I thought this project LIKED that model, though, and I was pretty sure that you were the one pitched me on it. I've not disassembled it, but I'm pretty sure I know that the number of overhea clock cycles in things like 256: g()->setTextColor(g()->to16bit(CRGB::Black)); 257: g()->setCursor(x-1, y); g()->print(pszText); 258: g()->setCursor(x+1, y); g()->print(pszText); 259: g()->setCursor(x, y-1); g()->print(pszText); 260: g()->setCursor(x, y+1); g()->print(pszText); 262: g()->setTextColor(g()->to16bit(CRGB::White)); 263: g()->setCursor(x, y); or 147: g()->GetNoise().noise_y += dy * 4; 148: g()->GetNoise().noise_x += dx * 4; 149: g()->GetNoise().noise_z += dz * 4; would make me queasy. Most of the things in deviceConfig resolve down to runtime constants anyway—effectively globals. It's not like we have multiple EffectManagers or JSONWriters or DrawSafeCircles or whatever. They're globals. So I actually rocked the boat here to make it more in line with my understanding of what's "NightDriver-y." :-) I'll change this to allocate these things once and put them in a member or pass them around as arguments. That said, if I'm cracking this PR open again (that's OK), I have a different fix in mind for all of this anyway...but that's another area where once I start chopping at that tree, its' going to be a project of its own. :-) At least one of these tables that we're carefully crafting ... sure seems to be an array of constants. I would offer to move this back to 'draft' to show that I agree this needs some attention and I'll fix it, but, well... — Reply to this email directly, view it on GitHub <#837 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCFYZNFCP7FGT6MDTQ634U34ZDAVCNFSM6AAAAACXPC3XVKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DANZZGI2DINRWGI>. You are receiving this because you are subscribed to this thread.

[davepl]

But in the spirit of “you made me look!”. LEDStripEffect::g() and EffectManager::g() currently return std::shared_ptr by value, so every g()->... site bumps refcounts even in tight draw loops. That’s not good. I’ll take it as an action item to look into it! - Dave

…

On Apr 8, 2026, at 6:34 PM, Robert Lipe ***@***.***> wrote: @robertlipe commented on this pull request. In include/effects/matrix/PatternAnimatedGIF.h <#837 (comment)>: > + auto gif = GetAnimatedGIFs().find(_gifIndex); + if (gif == GetAnimatedGIFs().end()) Accepted. I'll change it. (you can quit reading here if you like :-) ) It's funny because this particular instance is one I DID outsource to AI (once I recognized the problem well enough to describe it) and actually MADE it do that. Pointer-chasing has always been a pet peeve of mine. I know the OO die-hards love it, but I understand branch prediction, cache misses, pipeline flushes, the difficulty of proving that a function is pure and that nothing has changed since the last call to it (good luck with that in a threaded world), the need to have the register allocator tiptoe around callee registers to avoid spills, and all that. I thought this project LIKED that model, though, and I was pretty sure that you were the one pitched me on it. I've not disassembled it, but I'm pretty sure I know that the number of overhea clock cycles in things like 256: g()->setTextColor(g()->to16bit(CRGB::Black)); 257: g()->setCursor(x-1, y); g()->print(pszText); 258: g()->setCursor(x+1, y); g()->print(pszText); 259: g()->setCursor(x, y-1); g()->print(pszText); 260: g()->setCursor(x, y+1); g()->print(pszText); 262: g()->setTextColor(g()->to16bit(CRGB::White)); 263: g()->setCursor(x, y); or 147: g()->GetNoise().noise_y += dy * 4; 148: g()->GetNoise().noise_x += dx * 4; 149: g()->GetNoise().noise_z += dz * 4; would make me queasy. Most of the things in deviceConfig resolve down to runtime constants anyway—effectively globals. It's not like we have multiple EffectManagers or JSONWriters or DrawSafeCircles or whatever. They're globals. So I actually rocked the boat here to make it more in line with my understanding of what's "NightDriver-y." :-) I'll change this to allocate these things once and put them in a member or pass them around as arguments. That said, if I'm cracking this PR open again (that's OK), I have a different fix in mind for all of this anyway...but that's another area where once I start chopping at that tree, its' going to be a project of its own. :-) At least one of these tables that we're carefully crafting ... sure seems to be an array of constants. I would offer to move this back to 'draft' to show that I agree this needs some attention and I'll fix it, but, well... — Reply to this email directly, view it on GitHub <#837 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCFYZNFCP7FGT6MDTQ634U34ZDAVCNFSM6AAAAACXPC3XVKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DANZZGI2DINRWGI>. You are receiving this because you are subscribed to this thread.

[robertlipe]

If you'll take g() (which is actually not in the critical path here, but I know that's your baby), I'm on Network now and will then come back to this PR. For this PR, pretty much any time there is a global static (even if it's searching groucho glasses and doesn't LOOK like a global) there should be a giant danger sign nearby. Since I don't think I described how I got here, we have (at least) these four files that use PSRAM allocations for global statics, meaning they can potentially be called before setup executes. This potential has always existed. GCC 14 makes this a reality on at least RISC-V. These four cases in captivity of this PR happen to be in paths that don't run in strips or even EFFECTS_DEMO, so I hadn't noticed it until I turned on EFFECTS_FULLMATRIX on a GCC14 target. Why is this a big deal? They're manually shoved into our PSRAM allocator, which doesn't get initialized until after the PSRAM_Init() gets called by something in setup; ergo, those throw a bad alloc on boot. Oh, and we haven't initialized the serial port yet, either, so it just silently crash-loops. Fun times! On Wed, Apr 8, 2026 at 10:09 PM David W Plummer ***@***.***> wrote:

…

*davepl* left a comment (PlummersSoftwareLLC/NightDriverStrip#837) <#837 (comment)> But in the spirit of “you made me look!”. LEDStripEffect::g() and EffectManager::g() currently return std::shared_ptr by value, so every g()->... site bumps refcounts even in tight draw loops. That’s not good. I’ll take it as an action item to look into it! - Dave > On Apr 8, 2026, at 6:34 PM, Robert Lipe ***@***.***> wrote: > > > @robertlipe commented on this pull request. > > In include/effects/matrix/PatternAnimatedGIF.h < #837 (comment)>: > > > + auto gif = GetAnimatedGIFs().find(_gifIndex); > + if (gif == GetAnimatedGIFs().end()) > Accepted. I'll change it. (you can quit reading here if you like :-) ) > > It's funny because this particular instance is one I DID outsource to AI (once I recognized the problem well enough to describe it) and actually MADE it do that. Pointer-chasing has always been a pet peeve of mine. I know the OO die-hards love it, but I understand branch prediction, cache misses, pipeline flushes, the difficulty of proving that a function is pure and that nothing has changed since the last call to it (good luck with that in a threaded world), the need to have the register allocator tiptoe around callee registers to avoid spills, and all that. I thought this project LIKED that model, though, and I was pretty sure that you were the one pitched me on it. > > I've not disassembled it, but I'm pretty sure I know that the number of overhea clock cycles in things like > > 256: g()->setTextColor(g()->to16bit(CRGB::Black)); > 257: g()->setCursor(x-1, y); g()->print(pszText); > 258: g()->setCursor(x+1, y); g()->print(pszText); > 259: g()->setCursor(x, y-1); g()->print(pszText); > 260: g()->setCursor(x, y+1); g()->print(pszText); > 262: g()->setTextColor(g()->to16bit(CRGB::White)); > 263: g()->setCursor(x, y); > or > > 147: g()->GetNoise().noise_y += dy * 4; > 148: g()->GetNoise().noise_x += dx * 4; > 149: g()->GetNoise().noise_z += dz * 4; > would make me queasy. Most of the things in deviceConfig resolve down to runtime constants anyway—effectively globals. It's not like we have multiple EffectManagers or JSONWriters or DrawSafeCircles or whatever. They're globals. So I actually rocked the boat here to make it more in line with my understanding of what's "NightDriver-y." :-) > > I'll change this to allocate these things once and put them in a member or pass them around as arguments. > > That said, if I'm cracking this PR open again (that's OK), I have a different fix in mind for all of this anyway...but that's another area where once I start chopping at that tree, its' going to be a project of its own. :-) At least one of these tables that we're carefully crafting ... sure seems to be an array of constants. > > I would offer to move this back to 'draft' to show that I agree this needs some attention and I'll fix it, but, well... > > — > Reply to this email directly, view it on GitHub < #837 (comment)>, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AA4HCFYZNFCP7FGT6MDTQ634U34ZDAVCNFSM6AAAAACXPC3XVKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DANZZGI2DINRWGI>. > You are receiving this because you are subscribed to this thread. > — Reply to this email directly, view it on GitHub <#837 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD35TVV7TCLNEQNZ2J5T4U4H63AVCNFSM6AAAAACXPC3XVKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DEMJRGE3TCOBTHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[davepl]

The big change is that the hot rendering accessors now return references instead of copying shared_ptrs: they now expose GFXBase& / const GFXBase& from g(). That drove a broad automated update from g()->... to g(). ... and to cached local references in effect code and runtime code. Substantively, this removes refcount traffic from hot draw loops and gives the compiler a much simpler object model to optimize. Op-heavy effects should be a lot faster, particularly on the larger surfaces like the mesmerizer. I also fixed const-correctness around mutable renderer state. include/gfxbase.h (line 188) now has const and non-const GetNoise() overloads instead of a const method returning a mutable reference, and palette-mutating manager methods in include/effectmanager.h (line 223) / src/effectmanager.cpp (line 455) are no longer incorrectly marked const. In a few places that repeatedly chained through internal APIs, I collapsed that to local refs, for example in include/effects/matrix/PatternMandala.h (line 112), so the code reads cleaner and emits less repeated access code. On the build side, platformio.ini (line 557) no longer forces -Og for m5s3demo_v3, so that target now keeps the repo’s optimized codegen path (-Ofast). I tried enabling LTO, but the current Xtensa/PlatformIO linker path in this repo is not LTO-ready, so I intentionally backed that out rather than leave the build broken. That’s just how I roll :-) Why this helps... - Removes avoidable shared_ptr inc/dec work from effect/render hot paths. - Makes ownership explicit: g() is a borrowed accessor now, not an ownership-sharing API. - Improves const-correctness and surfaced a couple of real mutating-from-const issues. - Reduces repeated chained loads by caching graphics, noise, and palette refs where it mattered. - Keeps readability roughly the same or better; most call sites became simpler. I’ll submit a PR shortly! - Dave

…

On Apr 8, 2026, at 9:06 PM, Robert Lipe ***@***.***> wrote: robertlipe left a comment (PlummersSoftwareLLC/NightDriverStrip#837) <#837 (comment)>If you'll take g() (which is actually not in the critical path here, but I know that's your baby), I'm on Network now and will then come back to this PR. For this PR, pretty much any time there is a global static (even if it's searching groucho glasses and doesn't LOOK like a global) there should be a giant danger sign nearby. Since I don't think I described how I got here, we have (at least) these four files that use PSRAM allocations for global statics, meaning they can potentially be called before setup executes. This potential has always existed. GCC 14 makes this a reality on at least RISC-V. These four cases in captivity of this PR happen to be in paths that don't run in strips or even EFFECTS_DEMO, so I hadn't noticed it until I turned on EFFECTS_FULLMATRIX on a GCC14 target. Why is this a big deal? They're manually shoved into our PSRAM allocator, which doesn't get initialized until after the PSRAM_Init() gets called by something in setup; ergo, those throw a bad alloc on boot. Oh, and we haven't initialized the serial port yet, either, so it just silently crash-loops. Fun times! On Wed, Apr 8, 2026 at 10:09 PM David W Plummer ***@***.***> wrote: > *davepl* left a comment (PlummersSoftwareLLC/NightDriverStrip#837) > <#837 (comment)> > But in the spirit of “you made me look!”. LEDStripEffect::g() and > EffectManager::g() currently return std::shared_ptr by value, so every > g()->... site bumps refcounts even in tight draw loops. That’s not good. > > I’ll take it as an action item to look into it! > > - Dave > > > > On Apr 8, 2026, at 6:34 PM, Robert Lipe ***@***.***> wrote: > > > > > > @robertlipe commented on this pull request. > > > > In include/effects/matrix/PatternAnimatedGIF.h < > #837 (comment)>: > > > > > > + auto gif = GetAnimatedGIFs().find(_gifIndex); > > + if (gif == GetAnimatedGIFs().end()) > > Accepted. I'll change it. (you can quit reading here if you like :-) ) > > > > It's funny because this particular instance is one I DID outsource to AI > (once I recognized the problem well enough to describe it) and actually > MADE it do that. Pointer-chasing has always been a pet peeve of mine. I > know the OO die-hards love it, but I understand branch prediction, cache > misses, pipeline flushes, the difficulty of proving that a function is pure > and that nothing has changed since the last call to it (good luck with that > in a threaded world), the need to have the register allocator tiptoe around > callee registers to avoid spills, and all that. I thought this project > LIKED that model, though, and I was pretty sure that you were the one > pitched me on it. > > > > I've not disassembled it, but I'm pretty sure I know that the number of > overhea clock cycles in things like > > > > 256: g()->setTextColor(g()->to16bit(CRGB::Black)); > > 257: g()->setCursor(x-1, y); g()->print(pszText); > > 258: g()->setCursor(x+1, y); g()->print(pszText); > > 259: g()->setCursor(x, y-1); g()->print(pszText); > > 260: g()->setCursor(x, y+1); g()->print(pszText); > > 262: g()->setTextColor(g()->to16bit(CRGB::White)); > > 263: g()->setCursor(x, y); > > or > > > > 147: g()->GetNoise().noise_y += dy * 4; > > 148: g()->GetNoise().noise_x += dx * 4; > > 149: g()->GetNoise().noise_z += dz * 4; > > would make me queasy. Most of the things in deviceConfig resolve down to > runtime constants anyway—effectively globals. It's not like we have > multiple EffectManagers or JSONWriters or DrawSafeCircles or whatever. > They're globals. So I actually rocked the boat here to make it more in line > with my understanding of what's "NightDriver-y." :-) > > > > I'll change this to allocate these things once and put them in a member > or pass them around as arguments. > > > > That said, if I'm cracking this PR open again (that's OK), I have a > different fix in mind for all of this anyway...but that's another area > where once I start chopping at that tree, its' going to be a project of its > own. :-) At least one of these tables that we're carefully crafting ... > sure seems to be an array of constants. > > > > I would offer to move this back to 'draft' to show that I agree this > needs some attention and I'll fix it, but, well... > > > > — > > Reply to this email directly, view it on GitHub < > #837 (comment)>, > or unsubscribe < > https://github.com/notifications/unsubscribe-auth/AA4HCFYZNFCP7FGT6MDTQ634U34ZDAVCNFSM6AAAAACXPC3XVKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DANZZGI2DINRWGI>. > > > You are receiving this because you are subscribed to this thread. > > > > — > Reply to this email directly, view it on GitHub > <#837 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ACCSD35TVV7TCLNEQNZ2J5T4U4H63AVCNFSM6AAAAACXPC3XVKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DEMJRGE3TCOBTHA> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> > — Reply to this email directly, view it on GitHub <#837 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCF4VXLROYCCMIAIQKED4U4OTTAVCNFSM6AAAAACXPC3XVKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DEMJRGM4DENJRG4>. You are receiving this because you commented.

[robertlipe]

Cool. Thanx. (More later.) For you specifically, let me call out that the _v3 versions are **not** "Dave ladder compatible" (that's actually the name of a state in a PR I've not submitted yet...) with the ones currently under your eve. If you attempt to OTA from a v2 (which is everything) to a v3 build, you're in for a bad time. The partition table had to change. You, probably uniquely, know that means that IPU just doesn't work. I spent hours in denal of that, but the solution is to say in giant scary words "it won't work" instead of dealing with the 1% failure that makes you get out the ladder. I have another breaking change in the queue for the v3 builds. That's my corresponding strategy for rolling. "Break it as infrequently as you can, but be forthright when you." On Thu, Apr 9, 2026 at 10:43 AM David W Plummer ***@***.***> wrote:

…

*davepl* left a comment (PlummersSoftwareLLC/NightDriverStrip#837) <#837 (comment)> The big change is that the hot rendering accessors now return references instead of copying shared_ptrs: they now expose GFXBase& / const GFXBase& from g(). That drove a broad automated update from g()->... to g(). ... and to cached local references in effect code and runtime code. Substantively, this removes refcount traffic from hot draw loops and gives the compiler a much simpler object model to optimize. Op-heavy effects should be a lot faster, particularly on the larger surfaces like the mesmerizer. I also fixed const-correctness around mutable renderer state. include/gfxbase.h (line 188) now has const and non-const GetNoise() overloads instead of a const method returning a mutable reference, and palette-mutating manager methods in include/effectmanager.h (line 223) / src/effectmanager.cpp (line 455) are no longer incorrectly marked const. In a few places that repeatedly chained through internal APIs, I collapsed that to local refs, for example in include/effects/matrix/PatternMandala.h (line 112), so the code reads cleaner and emits less repeated access code. On the build side, platformio.ini (line 557) no longer forces -Og for m5s3demo_v3, so that target now keeps the repo’s optimized codegen path (-Ofast). I tried enabling LTO, but the current Xtensa/PlatformIO linker path in this repo is not LTO-ready, so I intentionally backed that out rather than leave the build broken. That’s just how I roll :-) Why this helps... - Removes avoidable shared_ptr inc/dec work from effect/render hot paths. - Makes ownership explicit: g() is a borrowed accessor now, not an ownership-sharing API. - Improves const-correctness and surfaced a couple of real mutating-from-const issues. - Reduces repeated chained loads by caching graphics, noise, and palette refs where it mattered. - Keeps readability roughly the same or better; most call sites became simpler. I’ll submit a PR shortly! - Dave > On Apr 8, 2026, at 9:06 PM, Robert Lipe ***@***.***> wrote: > > > robertlipe > left a comment > (PlummersSoftwareLLC/NightDriverStrip#837) > < #837 (comment)>If you'll take g() (which is actually not in the critical path here, but I > know that's your baby), I'm on Network now and will then come back to this > PR. > > For this PR, pretty much any time there is a global static (even if it's > searching groucho glasses and doesn't LOOK like a global) there should be a > giant danger sign nearby. > > Since I don't think I described how I got here, we have (at least) these > four files that use PSRAM allocations for global statics, meaning they can > potentially be called before setup executes. This potential has always > existed. GCC 14 makes this a reality on at least RISC-V. These four cases > in captivity of this PR happen to be in paths that don't run in strips or > even EFFECTS_DEMO, so I hadn't noticed it until I turned on > EFFECTS_FULLMATRIX on a GCC14 target. > > Why is this a big deal? They're manually shoved into our PSRAM allocator, > which doesn't get initialized until after the PSRAM_Init() gets called by > something in setup; ergo, those throw a bad alloc on boot. Oh, and we > haven't initialized the serial port yet, either, so it just silently > crash-loops. Fun times! > > > > On Wed, Apr 8, 2026 at 10:09 PM David W Plummer ***@***.***> > wrote: > > > *davepl* left a comment (PlummersSoftwareLLC/NightDriverStrip#837) > > < #837 (comment)> > > But in the spirit of “you made me look!”. LEDStripEffect::g() and > > EffectManager::g() currently return std::shared_ptr by value, so every > > g()->... site bumps refcounts even in tight draw loops. That’s not good. > > > > I’ll take it as an action item to look into it! > > > > - Dave > > > > > > > On Apr 8, 2026, at 6:34 PM, Robert Lipe ***@***.***> wrote: > > > > > > > > > @robertlipe commented on this pull request. > > > > > > In include/effects/matrix/PatternAnimatedGIF.h < > > #837 (comment)>: > > > > > > > > > + auto gif = GetAnimatedGIFs().find(_gifIndex); > > > + if (gif == GetAnimatedGIFs().end()) > > > Accepted. I'll change it. (you can quit reading here if you like :-) ) > > > > > > It's funny because this particular instance is one I DID outsource to AI > > (once I recognized the problem well enough to describe it) and actually > > MADE it do that. Pointer-chasing has always been a pet peeve of mine. I > > know the OO die-hards love it, but I understand branch prediction, cache > > misses, pipeline flushes, the difficulty of proving that a function is pure > > and that nothing has changed since the last call to it (good luck with that > > in a threaded world), the need to have the register allocator tiptoe around > > callee registers to avoid spills, and all that. I thought this project > > LIKED that model, though, and I was pretty sure that you were the one > > pitched me on it. > > > > > > I've not disassembled it, but I'm pretty sure I know that the number of > > overhea clock cycles in things like > > > > > > 256: g()->setTextColor(g()->to16bit(CRGB::Black)); > > > 257: g()->setCursor(x-1, y); g()->print(pszText); > > > 258: g()->setCursor(x+1, y); g()->print(pszText); > > > 259: g()->setCursor(x, y-1); g()->print(pszText); > > > 260: g()->setCursor(x, y+1); g()->print(pszText); > > > 262: g()->setTextColor(g()->to16bit(CRGB::White)); > > > 263: g()->setCursor(x, y); > > > or > > > > > > 147: g()->GetNoise().noise_y += dy * 4; > > > 148: g()->GetNoise().noise_x += dx * 4; > > > 149: g()->GetNoise().noise_z += dz * 4; > > > would make me queasy. Most of the things in deviceConfig resolve down to > > runtime constants anyway—effectively globals. It's not like we have > > multiple EffectManagers or JSONWriters or DrawSafeCircles or whatever. > > They're globals. So I actually rocked the boat here to make it more in line > > with my understanding of what's "NightDriver-y." :-) > > > > > > I'll change this to allocate these things once and put them in a member > > or pass them around as arguments. > > > > > > That said, if I'm cracking this PR open again (that's OK), I have a > > different fix in mind for all of this anyway...but that's another area > > where once I start chopping at that tree, its' going to be a project of its > > own. :-) At least one of these tables that we're carefully crafting ... > > sure seems to be an array of constants. > > > > > > I would offer to move this back to 'draft' to show that I agree this > > needs some attention and I'll fix it, but, well... > > > > > > — > > > Reply to this email directly, view it on GitHub < > > #837 (comment)>, > > or unsubscribe < > > https://github.com/notifications/unsubscribe-auth/AA4HCFYZNFCP7FGT6MDTQ634U34ZDAVCNFSM6AAAAACXPC3XVKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DANZZGI2DINRWGI>. > > > > > You are receiving this because you are subscribed to this thread. > > > > > > > — > > Reply to this email directly, view it on GitHub > > < #837 (comment)>, > > or unsubscribe > > < https://github.com/notifications/unsubscribe-auth/ACCSD35TVV7TCLNEQNZ2J5T4U4H63AVCNFSM6AAAAACXPC3XVKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DEMJRGE3TCOBTHA> > > . > > You are receiving this because you were mentioned.Message ID: > > ***@***.***> > > > — > Reply to this email directly, view it on GitHub < #837 (comment)>, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AA4HCF4VXLROYCCMIAIQKED4U4OTTAVCNFSM6AAAAACXPC3XVKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DEMJRGM4DENJRG4>. > You are receiving this because you commented. > — Reply to this email directly, view it on GitHub <#837 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD34FIULGQGO3USINBED4U7ALVAVCNFSM6AAAAACXPC3XVKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DEMJVGUZTOMRXHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[rbergen]

@robertlipe @davepl I'll make the comment here because both of you seem to be watching this PR: I'm mildly confused about what the discussion you're having means for this and subsequent PRs. As in, there is a mention of a future PR from Dave, but I don't currently know if that's supposed to be a PR that alters this one, or a follow-up PR, assuming that the one I'm adding this comment to is merged.

The facts being that I now have the time to review things and I like empty PR lists :), I will proceed with reviewing and merging this PR, #801 and #836, and in that order for reasons of review volume - unless I encounter merge conflicts on that path, in which case I will skip the PRs that turn red.

[rbergen]

Ah, there are pending changes to this PR, which means only the other two I mentioned remain. Onwards and upwards.

[robertlipe]

To clarify, we strayed slightly off topic of this PR specifically. I was showing that I rocked this PR to do this weird (sorrynotsorry - but we've all three said it now) thing because it was done in so many other places when Dave stepped in and agreed to fix the examples I offerd. (Thank you for the callout and thanks, Dave, for making the deep cut on that.) You asked for changes here that I haven't made yet. (If Dave wants to make similar changes in these four places simply obsoleting this PR wouldn't make me sad.) When I return to this one, and it's pretty high in my own queue of strategic ones, I'll do one of two things. 1) Do the somewhat mechanical thing literally asked above. 2) Restructure them to avoid the goofy fake static initializer and make them member data that is then initialized directly in the effect constructors. I might do #1 on the step to #2. I just had to go fix network so my device would boot again so I could come back to it. (Or, I guess I could debug this in any of the 57 environments that aren't fatally affected...) I"m specifically NOT going to go on my long-fantasized rampage and kill our wrapper for PreferPSRAMAlloc in this PR. I have three in the review queue, I think. I'll go look. I can make this draft to show that I have ownership until then (I know you have mixed feelings about that) if it improves your day in some way.

…

On Thu, Apr 9, 2026 at 11:26 AM Rutger van Bergen ***@***.***> wrote: *rbergen* left a comment (PlummersSoftwareLLC/NightDriverStrip#837) <#837 (comment)> Ah, there are *pending* changes to this PR, which means only the other two I mentioned remain. Onwards and upwards. — Reply to this email directly, view it on GitHub <#837 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD3ZEII6J2SVBYOA7JFT4U7FMRAVCNFSM6AAAAACXPC3XVKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DEMJVG44TSNZUHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[rbergen]

I can make this draft to show that I have ownership until then (I know you
have mixed feelings about that) if it improves your day in some way.

Actually, I don't like PRs being opened in draft status, but I don't mind them entering that state if the review cycle yields that it should be ignored for a while. I know, that's a very specific Rutgerian distinction, but sorrynotsorry - yeah, I actually did that.

All that being as it may, it doesn't matter anymore, because I have now already established there's nothing to look at. Which makes I will not look at it again until I'm informed (by GitHub or otherwise) that things here have changed.