Conversation
…re documentation Core technical changes: - Migrated 30+ core class implementations from headers to .cpp files to improve build times and follow IWYU best practices. - Pruned redundant implementations and redefinitions across the codebase (EffectManager, TaskManager, LEDBuffer, etc.). - Restored extensive technical documentation, historical 'lore', and debugX instrumentation lost during initial migration. - Kept performance-critical XY() mapping logic inline in headers (GFXBase, HUB75GFX, HexagonGFX) to minimize register pressure on ESP32. - Resolved build failures and link errors in Mesmerizer and other environments. - Corrected various dormant bugs: - Fixed dangling palette references in particle and spectrum effects by changing members from reference to value storage. - Fixed floating-point truncation bug in SimpleColorBeat::Draw. - Corrected visibility and redefinition of EffectTaskParams in NightDriverTaskManager. - Ensured IJSONSerializable resides exclusively in interfaces.h to decouple core logic from heavy JSON headers. - Cleaned up branch by removing all scratch files, build logs, and unintended artifacts from the PR.
|
Heads-up Rutger/Dave: Once this IWYU refactor is landed, I have a second submarine arriving immediately behind it that replaces the RemoteDebug library with a native socket-based implementation (TelnetServer and Console classes). I’ve kept that work separate to avoid complicating this IWYU review, but be aware that many of the files I just moved logic into (like network.cpp and main.cpp) will see logic changes in the very next PR to rip out the abandoned debugger. If you'd prefer to see those together (you really don't...), let me know, but my plan is to land the structural change first. That will make the second one look much more mechanical - plus, it'll make the inevetible collisions my problem and not yours, though it may be handy to zoom out and see future direction. Let's focus on this one now. (If you want to toss/edit any of this doc, feel free.) I'll get the abandonded RemoteDebug work found and uploaded next. It's comparatively small...but also in the "it'll only hurt for a little while" category. |
|
In case you care what Gemini says: |
rbergen
left a comment
rbergen
left a comment
There was a problem hiding this comment.
Well done! Sonnet agrees with Gemini, and my manual review (yes, I did one) only yielded a few things that I would like you to look at before we proceed.
I am duty-bound to ask if you actually ran this branch on a number of devices we're targeting, including Mesmerizer...?
And yes, thank you for isolating your next PR from this one.
Ultimately to make it run on compilers of the last ten years and on chips from the last six.
Haha, couldn't help yourself! Congrats on holding it in until the very end. ;)
| @@ -1,3 +1,6 @@ | |||
| #pragma once | |||
|
|
|||
| #include "random_utils.h" | |||
There was a problem hiding this comment.
I suggest we put this under the file header.
There was a problem hiding this comment.
I'm a bit surprised by this file showing up in the diff. Why would you downgrade specific packages in the package-lock.json for the on-board web app, while package.json apparently is unchanged?
| // SystemContainer | ||
| // | ||
| // Holds a number of system-wide objects that take care of core/supportive functions on the chip. | ||
| // The objects are added to this class in an "enriched property" style. This means that for each object, the class | ||
| // contains: | ||
| // - A declaration of the member variable (using SC_DECLARE) | ||
| // - A Setup method that creates and returns the object in question (mostly through SC_SIMPLE_SETUP_FOR or | ||
| // SC_FORWARDING_SETUP_FOR) | ||
| // - A Has method that returns true if the object has been Setup, and a property getter that returns a reference to | ||
| // the actual object (both using SC_GETTERS_FOR) | ||
| // | ||
| // The difference between SC_SIMPLE_SETUP_FOR and SC_FORWARDING_SETUP_FOR is that the former invokes a parameterless | ||
| // constructor when creating the object, and the latter forwards any arguments passed to it on to the constructor. | ||
| // | ||
| // SC_SIMPLE_PROPERTY and SC_FORWARDING_PROPERTY are provided for convenience; they combine a declaration, simple or | ||
| // forwarding Setup method, and the Has and getter methods. | ||
| // | ||
| // Most macros accept two parameters: | ||
| // - The name of the property, as used in the Setup, Has and getter methods | ||
| // - The type of the property, as held/returned by this class | ||
| // | ||
| // The actual composition of this class is largely driven by the macros mentioned, only irregular Setup methods are | ||
| // coded manually. |
There was a problem hiding this comment.
What you've done here is unroll the macros across systemcontainer.h and systemcontainer.cpp. As much as I hate repetitive code I can see how this is defendable; for one the macro solution was extremely ugly in itself.
However, please update the documentation as well, then. (I think a select of 23 lines & Delete will do, here).
| [env:demo_v2] | ||
| extends = env:demo | ||
| platform = platformio/espressif32 @ ^6.12.0 | ||
|
|
||
| [env:demo_v3] | ||
| extends = env:demo | ||
| platform = https://github.com/pioarduino/platform-espressif32/releases/download/55.03.30-2/platform-espressif32.zip | ||
| board_build.partitions = config/partitions_custom_noota_v3.csv |
There was a problem hiding this comment.
Please leave a comment in platformio.ini or the PR that explains the purpose of the new _v2 and _v3 envs.
2 participants
Description
I realize that seeing "+193 files" in a PR is usually a reason to go for a long walk and never come back. This note is intended to
explain why this was necessary, why it’s safe, and why some things (like GetEffectManager) had to change.
Previously, our headers were "anesthetized" by a lucky include order. Target A would build because Header B happened to include Header
C, which defined a type needed by Header D.
In Arduino v3 (GCC 15), this luck ran out. The include order changed just enough that demo would build, but ledstrip would
spontaneously fail. Our implicit dependencies were literal "build killers."
To fix this, I followed "Include What You Use" (IWYU) dogma:
.cppfiles. It is simply good taste to have code exist in one place. This separates the definition,declaration, and usage trees.
dragging in the whole world.
globals.hPrecedence. We now strictly insist that globals.h is the first local include. This is enforced via new audit tools in/tools.
EffectManager() → GetEffectManager()
This wasn't a stylistic choice; it was a compiler requirement for decoupling.
EffectManager(), we couldn't easily forward-declare the class without the compiler getting confused between the function and the
type.
the "recompile the world" effect when effect logic changes.
network.h → nd_network.h
Arduino v3 introduced a system <Network.h>. Our local network.h was causing "poisonous" collisions. I surrendered and renamed ours to
nd_network.h to break the circle.
byte_utils.h: Replaced ByteswapU64 and friends with more traditional versions. The previous versions were reading source memoryfar too many times; the new versions decay to std::memcpy on our LE architecture, which is significantly faster and handles
unaligned access better.
at runtime.
tables/filesystems will change), they prove the core logic is now v3-ready.
technical "lore" (like Stefan Petrick’s notes). If a comment fell between the floorboards, it was "sawdust" from the heavy
lifting—not intentional removal.
The vast majority of this PR is Code Motion. If you see a function disappear from a .h and appear in a .cpp, it is the same logic
you’ve already approved—just living in its proper home.
Van dik hout zaagt men planken. We had to saw some thick planks to get here, but the resulting frame is much stronger.
Ultimately to make it run on compilers of the last ten years and on chips from the last six.
Contributing requirements
mainas the target branch.