Fix static initialization order fiasco for psram_allocator globals#837
Closed
robertlipe wants to merge 5 commits into
Closed
Fix static initialization order fiasco for psram_allocator globals#837robertlipe wants to merge 5 commits into
robertlipe wants to merge 5 commits into
Conversation
rbergen
reviewed
Apr 8, 2026
Contributor
|
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.
|
Contributor
|
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.
|
Collaborator
Author
|
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:
***@***.***>
|
Contributor
|
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.
|
Collaborator
Author
|
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:
***@***.***>
|
Collaborator
|
@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. |
Collaborator
|
Ah, there are pending changes to this PR, which means only the other two I mentioned remain. Onwards and upwards. |
Collaborator
Author
|
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:
***@***.***>
|
Collaborator
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. |
Collaborator
|
Converting this to draft after all as the influx of new PRs has increased, and then clarity of status matters. |
Collaborator
Author
|
Thanx. This problem is here and it's real (I "rediscovered" it last night I
tried to say "oh, tinyled is new and it's 'mine'; let's make it Arduino
V3....and it blew up gloriously.) but I'm still unhappy with the
shallow cut here and have been trying a deeper one. It's just been
preempted.
Draft is appropriate as this is on ice right now.
…On Sun, Apr 26, 2026 at 2:52 AM Rutger van Bergen ***@***.***> wrote:
*rbergen* left a comment (PlummersSoftwareLLC/NightDriverStrip#837)
<#837 (comment)>
Converting this to draft after all as the influx of new PRs has increased,
and then clarity of status matters.
—
Reply to this email directly, view it on GitHub
<#837 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCSD33GOBPNCY5YRFZDT2L4XW55LAVCNFSM6AAAAACXPC3XVKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DGMRRGU3DMOJUGQ>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/ACCSD3YXMQXROK64OWYUIYT4XW55LA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMZSGE2TMNRZGQ2KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/ACCSD32UNBJRRMY6HJTDCGT4XW55LA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMZSGE2TMNRZGQ2KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
robertlipe
added a commit
to robertlipe/NightDriverStrip
that referenced
this pull request
May 16, 2026
…ersSoftwareLLC#837 feedback) - In PatternWeather.h and PatternAnimatedGIF.h, store results of singleton getters in local variables. - In src/webserver.cpp, use local variable for GetMySettingSpecs() in LoadDeviceSettingSpecs(). - Add AGENTS.md and GEMINI.md for AI agent context.
…ather to use class member data. This commit moves problematic static/global variables into class instance members, providing a cleaner architecture and resolving Static Initialization Order Fiasco (SIOF) issues without relying on Meyers Singletons. Key changes: - CWebServer: Converted static members and methods to instance members/methods; updated route handlers to capture 'this'. - PatternAnimatedGIF: Moved decoder state and GIF maps to instance data; implemented an 'ActiveInstance' tracker for decoder callbacks. - PatternWeather: Migrated 'weatherIcons' to an instance member. - Verified build and syntax via 'tab5_demo' environment.
This commit cleans up outdated comments and explicit allocator logic that are no longer needed now that the platform-default allocator policy handles PSRAM routing automatically. - include/interfaces.h: Removed broken references to non-existent 'make_unique_internal' helpers. - src/jsonserializer.cpp: Removed explicit 'JsonPsramAllocator' in favor of default allocation. - src/main.cpp, include/effects/strip/fireeffect.h, src/gfxbase.cpp: Updated comments to remove references to legacy project-side PSRAM helpers.
This commit adds a pre-build script 'tools/pio_audit.py' that executes 'audit_globals_order.py' and 'audit_include_rules.py'. These audits are now a mandatory gate in the build process; any violation will terminate the build with an error, ensuring project standards for header inclusion and ordering are maintained.
robertlipe
force-pushed
the
fix/global-static-order
branch
2 times, most recently
from
ba37153 to
234331d
Compare
…udits. This commit resolves numerous header inclusion and ordering issues to satisfy the new project-wide build gate: - Added missing 'globals.h' to 'iservice.h', 'crgbw.h', 'pixelformat.h'. - Alphabetized system and local includes in 'ledstripeffect.h', 'gfxbase.h', 'ws281xgfx.h', 'webserver.h', 'crgbw.h', 'pixelformat.h', 'ws281xoutputmanager.cpp', 'deviceconfig.cpp', 'webserver.cpp', 'ledstripeffect.cpp', and 'systemcontainer.cpp'. - Enforced tiering by moving system headers before local headers where necessary.
robertlipe
force-pushed
the
fix/global-static-order
branch
from
234331d to
5aa8f68
Compare
This commit refactors GFXBase::getPolarMap to replace manual double-checked locking and std::unique_ptr with a thread-safe C++11 magic static. The large PolarMapArray is still allocated on the heap to ensure it lands in PSRAM (lazy initialization during setup), but the code is now more idiomatic and readable. Preserved the detailed comments on geometric calculations.
Collaborator
Author
|
PTAL. This special little flower needed some time to grow and bloom. It uncovered out all kinds of interesting language issues, like our Noise stuff and our Gfx accessors, and it leapfrogged a couple of other PRs that were rooted in this one, but I'm NOW officially happy (enough) with this. |
Collaborator
|
Thank you @robertlipe. Not to cause even more clashes between intended changes, I'm going to wait with merging this until issue #877 has been resolved. The reason is that quite a few of the files this PR is touching are also mentioned in an analysis of possible causes for what's described in that issue. |
4 tasks
rbergen
added a commit
that referenced
this pull request
Jun 6, 2026
Adapt core #837 refactors to current main with safe SIOF fixes, audit integration, and build-flag cleanup
Collaborator
|
Superseded by #891. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
This is particularly terrible because we call psramAlloc() before main() but psram isn't configured until after we're inside of setup().
Contributing requirements
mainas the target branch.