Skip to content

Fix static initialization order fiasco for psram_allocator globals #837

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robertlipe wants to merge 1 commit into
base: main
Choose a base branch
from
+64 −48
Open

Fix static initialization order fiasco for psram_allocator globals #837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 28 additions & 20 deletions include/effects/matrix/PatternAnimatedGIF.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,18 +103,22 @@ struct GIFInfo : public EmbeddedFile
{}
};

static const std::map<GIFIdentifier, const GIFInfo, std::less<GIFIdentifier>, psram_allocator<std::pair<const GIFIdentifier, const GIFInfo>>> AnimatedGIFs =
static const std::map<GIFIdentifier, const GIFInfo, std::less<GIFIdentifier>, psram_allocator<std::pair<const GIFIdentifier, const GIFInfo>>>& GetAnimatedGIFs()
{
// Banana has 8 frames. Most music is around 120BPM, so we need to play each frame for 1/15th of a second to somewhat align with a typical beat
{ GIFIdentifier::Banana, GIFInfo(banana_start, banana_end, 32, 32, 10 ) }, // 4 KB
{ GIFIdentifier::Nyancat, GIFInfo(nyancat_start, nyancat_end, 64, 32, 18 ) }, // 20 KB
{ GIFIdentifier::Pacman, GIFInfo(pacman_start, pacman_end, 64, 12, 20 ) }, // 36 KB
{ GIFIdentifier::Atomic, GIFInfo(atomic_start, atomic_end, 32, 32, 60 ) }, // 21 KB
{ GIFIdentifier::ColorSphere, GIFInfo(colorsphere_start, colorsphere_end, 32, 32, 16 ) }, // 52 KB
{ GIFIdentifier::ThreeRings, GIFInfo(threerings_start, threerings_end, 64, 32, 24 ) }, // 9 KB
{ GIFIdentifier::Tesseract, GIFInfo(tesseract_start, tesseract_end, 40, 32, 40 ) }, // 24 KB
{ GIFIdentifier::Firelog, GIFInfo(firelog_start, firelog_end, 64, 32, 16 ) }, // 24 KB
};
static const std::map<GIFIdentifier, const GIFInfo, std::less<GIFIdentifier>, psram_allocator<std::pair<const GIFIdentifier, const GIFInfo>>> AnimatedGIFs =
{
// Banana has 8 frames. Most music is around 120BPM, so we need to play each frame for 1/15th of a second to somewhat align with a typical beat
{ GIFIdentifier::Banana, GIFInfo(banana_start, banana_end, 32, 32, 10 ) }, // 4 KB
{ GIFIdentifier::Nyancat, GIFInfo(nyancat_start, nyancat_end, 64, 32, 18 ) }, // 20 KB
{ GIFIdentifier::Pacman, GIFInfo(pacman_start, pacman_end, 64, 12, 20 ) }, // 36 KB
{ GIFIdentifier::Atomic, GIFInfo(atomic_start, atomic_end, 32, 32, 60 ) }, // 21 KB
{ GIFIdentifier::ColorSphere, GIFInfo(colorsphere_start, colorsphere_end, 32, 32, 16 ) }, // 52 KB
{ GIFIdentifier::ThreeRings, GIFInfo(threerings_start, threerings_end, 64, 32, 24 ) }, // 9 KB
{ GIFIdentifier::Tesseract, GIFInfo(tesseract_start, tesseract_end, 40, 32, 40 ) }, // 24 KB
{ GIFIdentifier::Firelog, GIFInfo(firelog_start, firelog_end, 64, 32, 16 ) }, // 24 KB
};
return AnimatedGIFs;
}

// The decoder needs us to track some state, but there's only one instance of the decoder, and
// we can't pass it a pointer to our state because the callback doesn't allow you to pass any
Expand All @@ -140,7 +144,11 @@ g_gifDecoderState;
// We dynamically allocate the GIF decoder because it's pretty big and we don't want to waste the base
// ram on it. This way it, and the GIFs it decodes, can live in PSRAM.

const std::unique_ptr<GifDecoder<MATRIX_WIDTH, MATRIX_HEIGHT, 16, true>> g_ptrGIFDecoder = make_unique_psram<GifDecoder<MATRIX_WIDTH, MATRIX_HEIGHT, 16, true>>();
static GifDecoder<MATRIX_WIDTH, MATRIX_HEIGHT, 16, true>& GetGIFDecoder()
{
static const std::unique_ptr<GifDecoder<MATRIX_WIDTH, MATRIX_HEIGHT, 16, true>> g_ptrGIFDecoder = make_unique_psram<GifDecoder<MATRIX_WIDTH, MATRIX_HEIGHT, 16, true>>();
return *g_ptrGIFDecoder;
}

// PatternAnimatedGIF
//
Expand Down Expand Up @@ -260,8 +268,8 @@ class PatternAnimatedGIF : public EffectWithId<PatternAnimatedGIF>

// Open the GIF and start decoding

auto gif = AnimatedGIFs.find(_gifIndex);
if (gif == AnimatedGIFs.end())
auto gif = GetAnimatedGIFs().find(_gifIndex);
if (gif == GetAnimatedGIFs().end())
Comment on lines +271 to +272
Copy link
Copy Markdown
Collaborator

@rbergen rbergen Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

@robertlipe robertlipe Apr 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

throw std::runtime_error(str_sprintf("Unable to locate GIF by index %d in the map.", (int) _gifIndex).c_str());

// Set up the gifDecoderState with all of the context that it will need to decode and
Expand Down Expand Up @@ -310,12 +318,12 @@ class PatternAnimatedGIF : public EffectWithId<PatternAnimatedGIF>

// Set the GIF decoder callbacks to our static functions

g_ptrGIFDecoder->setScreenClearCallback( screenClearCallback );
g_ptrGIFDecoder->setUpdateScreenCallback( updateScreenCallback );
g_ptrGIFDecoder->setDrawPixelCallback( drawPixelCallback );
g_ptrGIFDecoder->setDrawLineCallback( drawLineCallback );
GetGIFDecoder().setScreenClearCallback( screenClearCallback );
GetGIFDecoder().setUpdateScreenCallback( updateScreenCallback );
GetGIFDecoder().setDrawPixelCallback( drawPixelCallback );
GetGIFDecoder().setDrawLineCallback( drawLineCallback );

_gifReadyToDraw = (ERROR_NONE == g_ptrGIFDecoder->startDecoding((uint8_t *) gif->second.contents, gif->second.length));
_gifReadyToDraw = (ERROR_NONE == GetGIFDecoder().startDecoding((uint8_t *) gif->second.contents, gif->second.length));
if (!_gifReadyToDraw)
debugW("Failed to start decoding GIF");
}
Expand All @@ -341,7 +349,7 @@ class PatternAnimatedGIF : public EffectWithId<PatternAnimatedGIF>
g()->Clear(_bkColor);

if (_gifReadyToDraw)
g_ptrGIFDecoder->decodeFrame(false);
GetGIFDecoder().decodeFrame(false);

}
};
Expand Down
52 changes: 28 additions & 24 deletions include/effects/matrix/PatternWeather.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,27 +100,31 @@ extern const uint8_t thunderstorm_night_end[] asm("_binary_assets_bmp_thun

static constexpr auto pszDaysOfWeek = to_array( { "SUN", "MON", "TUE", "WED", "THU", "FRI", "SAT" } );

static std::map<const String, EmbeddedFile, std::less<const String>, psram_allocator<std::pair<const String, EmbeddedFile>>> weatherIcons =
static std::map<const String, EmbeddedFile, std::less<const String>, psram_allocator<std::pair<const String, EmbeddedFile>>>& GetWeatherIcons()
{
{ "01d", EmbeddedFile(clearsky_start, clearsky_end) },
{ "02d", EmbeddedFile(fewclouds_start, fewclouds_end) },
{ "03d", EmbeddedFile(scatteredclouds_start, scatteredclouds_end) },
{ "04d", EmbeddedFile(brokenclouds_start, brokenclouds_end) },
{ "09d", EmbeddedFile(showerrain_start, showerrain_end) },
{ "10d", EmbeddedFile(rain_start, rain_end) },
{ "11d", EmbeddedFile(thunderstorm_start, thunderstorm_end) },
{ "13d", EmbeddedFile(snow_start, snow_end) },
{ "50d", EmbeddedFile(mist_start, mist_end) },
{ "01n", EmbeddedFile(clearsky_night_start, clearsky_night_end) },
{ "02n", EmbeddedFile(fewclouds_night_start, fewclouds_night_end) },
{ "03n", EmbeddedFile(scatteredclouds_night_start, scatteredclouds_night_end) },
{ "04n", EmbeddedFile(brokenclouds_night_start, brokenclouds_night_end) },
{ "09n", EmbeddedFile(showerrain_night_start, showerrain_night_end) },
{ "10n", EmbeddedFile(rain_night_start, rain_night_end) },
{ "11n", EmbeddedFile(thunderstorm_night_start, thunderstorm_night_end) },
{ "13n", EmbeddedFile(snow_night_start, snow_night_end) },
{ "50n", EmbeddedFile(mist_night_start, mist_night_end) }
};
static std::map<const String, EmbeddedFile, std::less<const String>, psram_allocator<std::pair<const String, EmbeddedFile>>> weatherIcons =
{
{ "01d", EmbeddedFile(clearsky_start, clearsky_end) },
{ "02d", EmbeddedFile(fewclouds_start, fewclouds_end) },
{ "03d", EmbeddedFile(scatteredclouds_start, scatteredclouds_end) },
{ "04d", EmbeddedFile(brokenclouds_start, brokenclouds_end) },
{ "09d", EmbeddedFile(showerrain_start, showerrain_end) },
{ "10d", EmbeddedFile(rain_start, rain_end) },
{ "11d", EmbeddedFile(thunderstorm_start, thunderstorm_end) },
{ "13d", EmbeddedFile(snow_start, snow_end) },
{ "50d", EmbeddedFile(mist_start, mist_end) },
{ "01n", EmbeddedFile(clearsky_night_start, clearsky_night_end) },
{ "02n", EmbeddedFile(fewclouds_night_start, fewclouds_night_end) },
{ "03n", EmbeddedFile(scatteredclouds_night_start, scatteredclouds_night_end) },
{ "04n", EmbeddedFile(brokenclouds_night_start, brokenclouds_night_end) },
{ "09n", EmbeddedFile(showerrain_night_start, showerrain_night_end) },
{ "10n", EmbeddedFile(rain_night_start, rain_night_end) },
{ "11n", EmbeddedFile(thunderstorm_night_start, thunderstorm_night_end) },
{ "13n", EmbeddedFile(snow_night_start, snow_night_end) },
{ "50n", EmbeddedFile(mist_night_start, mist_night_end) }
};
return weatherIcons;
}

/**
* @brief This class implements the Weather Data effect
Expand Down Expand Up @@ -510,16 +514,16 @@ class PatternWeather : public EffectWithId<PatternWeather>
}

// Draw the graphics
auto iconEntry = weatherIcons.find(iconToday);
if (iconEntry != weatherIcons.end())
auto iconEntry = GetWeatherIcons().find(iconToday);
if (iconEntry != GetWeatherIcons().end())
{
auto icon = iconEntry->second;
if (JDR_OK != TJpgDec.drawJpg(0, 10, icon.contents, icon.length)) // Draw the image
debugW("Could not display icon %s", iconToday.c_str());
}

iconEntry = weatherIcons.find(iconTomorrow);
if (iconEntry != weatherIcons.end())
iconEntry = GetWeatherIcons().find(iconTomorrow);
if (iconEntry != GetWeatherIcons().end())
{
auto icon = iconEntry->second;
if (JDR_OK != TJpgDec.drawJpg(xHalf+1, 10, icon.contents, icon.length)) // Draw the image
Expand Down
2 changes: 1 addition & 1 deletion include/webserver.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ class CWebServer
}
};

static std::vector<SettingSpec, psram_allocator<SettingSpec>> mySettingSpecs;
static std::vector<SettingSpec, psram_allocator<SettingSpec>>& GetMySettingSpecs();
static std::vector<std::reference_wrapper<SettingSpec>> deviceSettingSpecs;
static const std::map<String, ValueValidator> settingValidators;

Expand Down
10 changes: 7 additions & 3 deletions src/webserver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,11 @@ const std::map<String, CWebServer::ValueValidator> CWebServer::settingValidators
{ DeviceConfig::BrightnessTag, [](const String& value) { return g_ptrSystem->GetDeviceConfig().ValidateBrightness(value); } }
};

std::vector<SettingSpec, psram_allocator<SettingSpec>> CWebServer::mySettingSpecs = {};
std::vector<SettingSpec, psram_allocator<SettingSpec>>& CWebServer::GetMySettingSpecs()
{
static std::vector<SettingSpec, psram_allocator<SettingSpec>> instance;
return instance;
}
std::vector<std::reference_wrapper<SettingSpec>> CWebServer::deviceSettingSpecs{};

// Member function template specializations
Expand Down Expand Up @@ -479,13 +483,13 @@ const std::vector<std::reference_wrapper<SettingSpec>> & CWebServer::LoadDeviceS
{
if (deviceSettingSpecs.empty())
{
mySettingSpecs.emplace_back(
GetMySettingSpecs().emplace_back(
"effectInterval",
"Effect interval",
"The duration in milliseconds that an individual effect runs, before the next effect is activated.",
SettingSpec::SettingType::PositiveBigInteger
);
deviceSettingSpecs.insert(deviceSettingSpecs.end(), mySettingSpecs.begin(), mySettingSpecs.end());
deviceSettingSpecs.insert(deviceSettingSpecs.end(), GetMySettingSpecs().begin(), GetMySettingSpecs().end());

auto deviceConfigSpecs = g_ptrSystem->GetDeviceConfig().GetSettingSpecs();
deviceSettingSpecs.insert(deviceSettingSpecs.end(), deviceConfigSpecs.begin(), deviceConfigSpecs.end());
Expand Down
Loading