Adding Particle System (port from AC) #237
Conversation
- port from AC - also adding hardware random functions
WalkthroughThis update introduces a comprehensive particle system framework for both 2D and 1D LED matrix effects, adding new classes, memory management, and rendering utilities. The effect mode enumeration is expanded to include a wide range of particle and particle system modes, increasing the total mode count from 198 to 228. A new hardware-based random number generator API is added, providing fast, platform-specific random number functions in various bit widths and ranges. The utility file implements bounded random number functions using the hardware RNG. All changes are declarative, focusing on new features and expanded capabilities for effect rendering and randomness. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ParticleSystem2D
participant ParticleSystem1D
participant HardwareRNG
User->>ParticleSystem2D: Initialize & configure
User->>ParticleSystem2D: Emit particles (spray/flame/angle)
ParticleSystem2D->>ParticleSystem2D: Update particles (move, collide, render)
ParticleSystem2D->>HardwareRNG: Request random values (for emission, physics)
HardwareRNG-->>ParticleSystem2D: Return random numbers
User->>ParticleSystem1D: Initialize & configure
User->>ParticleSystem1D: Emit particles (spray)
ParticleSystem1D->>ParticleSystem1D: Update particles (move, collide, render)
ParticleSystem1D->>HardwareRNG: Request random values
HardwareRNG-->>ParticleSystem1D: Return random numbers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
wled00/FXparticleSystem.h (1)
82-96: Particle flag unions repeat the same pitfallThe same packing caveat applies to
PSparticleFlags,PSparticleFlags1D.
Consider the same remedy or at least a compile‑time size assertion.
🧹 Nitpick comments (5)
wled00/util.cpp (2)
657-663: Division‑by‑zero / degenerate upper‑limit
hw_random(uint32_t upperlimit)returns0whenupperlimit == 0.
While harmless, silently returning a constant defeats the caller’s entropy expectation.
A defensive early‑exit withreturn 0;(or even anassert(upperlimit)in debug builds) would make the behaviour explicit.
664-670: Inclusive vs exclusive upper bound – please document
hw_random(lower, upper)returns values in[lower, upper)(exclusive upper bound).
Several Arduino utilities (e.g.random()) follow the same rule, but WLED helpers sometimes use inclusive upper bounds.
Adding a short doxygen note will save developers from off‑by‑one errors.wled00/FXparticleSystem.h (2)
221-224:blur2D/blur1Dsignatures expose largeuint32_tparams – risk of accidental overflow
xblur,yblur,blurare compared with pixel counts (uint16_t).
Passing a too‑large value because of a mismatched type can allocate huge stack buffers.
Recommend narrowing touint16_tor clamping inside the function.
242-249: Magic constants deserve a comment
PS_P_MINHARDRADIUS_1D 32,PS_P_MINSURFACEHARDNESS_1D 120– the header notes “do not change or hourglass effect will be broken” but not why.Add a short rationale (collision bin size? fixed‑point maths?) so future maintainers don’t “clean up” the constants.
wled00/FXparticleSystem.cpp (1)
1844-1853: Modulo on negative numbers is implementation‑defined
checkBoundsAndWrap()usesposition % (max + 1)whilepositioncan be negative.
For signed operands the%result is implementation‑defined (§8.3.9/2 C++20), so behaviour may change between compilers/optimisations.A portable substitute:
auto wrap = [&](int32_t pos) { int32_t m = max + 1; pos %= m; return pos < 0 ? pos + m : pos; }; position = wrap(position);Not urgent for the currently supported GCC/XTensa toolchains, but worth hardening for future portability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
wled00/FX.h(1 hunks)wled00/FXparticleSystem.cpp(1 hunks)wled00/FXparticleSystem.h(1 hunks)wled00/fcn_declare.h(1 hunks)wled00/util.cpp(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
wled00/util.cpp (1)
wled00/fcn_declare.h (4)
hw_random(414-414)hw_random(414-414)hw_random(415-415)hw_random(416-416)
wled00/FXparticleSystem.cpp (3)
wled00/FXparticleSystem.h (31)
force(153-153)force(173-173)force(337-337)ParticleSystem2D(136-136)isadvanced(198-198)isadvanced(362-362)size(172-172)size(336-336)emitter(143-143)emitter(144-144)emitter(145-145)emitter(317-317)part(141-141)part(147-147)part(148-148)part(151-151)part(154-154)part(318-318)part(320-320)part(322-322)limitSpeed(34-36)limitSpeed(34-34)blur2D(222-222)calculateNumberOfParticles2D(225-225)calculateNumberOfSources2D(226-226)allocateParticleSystemMemory2D(227-227)ParticleSystem1D(312-312)blur1D(387-387)calculateNumberOfParticles1D(384-384)calculateNumberOfSources1D(385-385)allocateParticleSystemMemory1D(386-386)wled00/FX.cpp (2)
wrap(5401-5417)wrap(5401-5401)wled00/fcn_declare.h (13)
hw_random16(417-417)hw_random16(417-417)hw_random16(418-418)hw_random16(418-418)hw_random16(419-419)hw_random16(419-419)cos16_t(453-453)sin16_t(452-452)r(434-434)r(434-434)b(312-312)b(312-312)b(343-343)
🪛 Cppcheck (2.10-2)
wled00/FXparticleSystem.cpp
[warning] 445-445: Uninitialized variable
(uninitvar)
🔇 Additional comments (3)
wled00/fcn_declare.h (1)
401-407: Nice addition – clear documentation & sensible defaultsThe introductory comment block precisely documents timing, entropy caveats and inlining trade‑offs.
Helps future maintainers a lot – thanks!wled00/FX.h (1)
370-371: MODE_COUNT is consistent – good catchHighest ID is 227,
MODE_COUNTset to 228 → off‑by‑one avoided.wled00/FXparticleSystem.cpp (1)
441-446: Potential race onforcecounterinside loop
tempcounter = forcecounter; ... forcecounter = tempcounter;means that forusedParticles > 1the last particle processed dictates the new global counter.
If you intended each particle to keep its own delay counter you’ll need a per‑particle array (similar toadvPartProps[i].forcecounter) or at least incrementforcecounteroutside the loop once.Please confirm the desired behaviour; otherwise subtle timing differences occur when
usedParticleschanges.🧰 Tools
🪛 Cppcheck (2.10-2)
[warning] 445-445: Uninitialized variable
(uninitvar)
| #ifdef ESP8266 | ||
| #define HW_RND_REGISTER RANDOM_REG32 | ||
| #else // ESP32 family | ||
| #include "soc/wdev_reg.h" | ||
| #define HW_RND_REGISTER REG_READ(WDEV_RND_REG) | ||
| #endif | ||
| #define random hw_random // replace arduino random() | ||
| inline uint32_t hw_random() { return HW_RND_REGISTER; }; | ||
| uint32_t hw_random(uint32_t upperlimit); // not inlined for code size |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Risk of globally replacing random() with a macro
#define random hw_random silently overrides every free‑standing call to random() across the whole code‑base (and third‑party libraries that might be compiled together with WLED).
If any TU
- calls Arduino’s
randomSeed()but expects Arduino’srandom()implementation - has a namespace‑qualified
randomoverload - contains a class method called
random()
the pre‑processor substitution will break compilation or change semantics in hard‑to‑trace ways.
Consider limiting the override to the files that actually need it, e.g.
#ifdef WLED_REPLACE_RANDOM
#define random hw_random
#endifor wrap it in #ifndef ARDUINO_ARCH_AVR … #endif if only ESP chips are affected.
| inline uint32_t hw_random() { return HW_RND_REGISTER; }; | ||
| uint32_t hw_random(uint32_t upperlimit); // not inlined for code size | ||
| int32_t hw_random(int32_t lowerlimit, int32_t upperlimit); | ||
| inline uint16_t hw_random16() { return HW_RND_REGISTER; }; | ||
| inline uint16_t hw_random16(uint32_t upperlimit) { return (hw_random16() * upperlimit) >> 16; }; // input range 0-65535 (uint16_t) | ||
| inline int16_t hw_random16(int32_t lowerlimit, int32_t upperlimit) { int32_t range = upperlimit - lowerlimit; return lowerlimit + hw_random16(range); }; // signed limits, use int16_t ranges | ||
| inline uint8_t hw_random8() { return HW_RND_REGISTER; }; | ||
| inline uint8_t hw_random8(uint32_t upperlimit) { return (hw_random8() * upperlimit) >> 8; }; // input range 0-255 | ||
| inline uint8_t hw_random8(uint32_t lowerlimit, uint32_t upperlimit) { uint32_t range = upperlimit - lowerlimit; return lowerlimit + hw_random8(range); }; // input range 0-255 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Out‑of‑range handling for 8‑/16‑bit helpers
hw_random8(lower, upper) and hw_random16(lower, upper) assume lower < upper.
If the caller accidentally swaps the limits, upper - lower underflows (unsigned!) and the helper returns almost the full 0‑255/0‑65535 range – a silent logic bug.
A minimal guard costs two instructions and avoids hours of debugging:
-inline uint8_t hw_random8(uint32_t lowerlimit, uint32_t upperlimit) { uint32_t range = upperlimit - lowerlimit; return lowerlimit + hw_random8(range); };
+inline uint8_t hw_random8(uint32_t lowerlimit, uint32_t upperlimit) {
+ if (upperlimit <= lowerlimit) return lowerlimit;
+ uint32_t range = upperlimit - lowerlimit;
+ return lowerlimit + hw_random8(range);
+}(Analogous change for the 16‑bit overload.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| inline uint32_t hw_random() { return HW_RND_REGISTER; }; | |
| uint32_t hw_random(uint32_t upperlimit); // not inlined for code size | |
| int32_t hw_random(int32_t lowerlimit, int32_t upperlimit); | |
| inline uint16_t hw_random16() { return HW_RND_REGISTER; }; | |
| inline uint16_t hw_random16(uint32_t upperlimit) { return (hw_random16() * upperlimit) >> 16; }; // input range 0-65535 (uint16_t) | |
| inline int16_t hw_random16(int32_t lowerlimit, int32_t upperlimit) { int32_t range = upperlimit - lowerlimit; return lowerlimit + hw_random16(range); }; // signed limits, use int16_t ranges | |
| inline uint8_t hw_random8() { return HW_RND_REGISTER; }; | |
| inline uint8_t hw_random8(uint32_t upperlimit) { return (hw_random8() * upperlimit) >> 8; }; // input range 0-255 | |
| inline uint8_t hw_random8(uint32_t lowerlimit, uint32_t upperlimit) { uint32_t range = upperlimit - lowerlimit; return lowerlimit + hw_random8(range); }; // input range 0-255 | |
| inline uint8_t hw_random8(uint32_t lowerlimit, uint32_t upperlimit) { | |
| if (upperlimit <= lowerlimit) return lowerlimit; | |
| uint32_t range = upperlimit - lowerlimit; | |
| return lowerlimit + hw_random8(range); | |
| } |
| typedef union { | ||
| struct{ // one byte bit field for 2D settings | ||
| bool wrapX : 1; | ||
| bool wrapY : 1; | ||
| bool bounceX : 1; | ||
| bool bounceY : 1; | ||
| bool killoutofbounds : 1; // if set, out of bound particles are killed immediately | ||
| bool useGravity : 1; // set to 1 if gravity is used, disables bounceY at the top | ||
| bool useCollisions : 1; | ||
| bool colorByAge : 1; // if set, particle hue is set by ttl value in render function | ||
| }; | ||
| byte asByte; // access as a byte, order is: LSB is first entry in the list above | ||
| } PSsettings2D; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Bit‑field layout may differ between ESP8266 & ESP32
PSsettings2D and PSsettings1D rely on an 8‑bit bool bit‑field inside a union.
ABI differences (alignment, endian packing) can yield:
sizeof(PSsettings2D)== 2 instead of 1 on one platform- unexpected ordering of bits (
wrapX≠LSB)
When the struct is transmitted over the wire or compared as asByte, bugs appear.
Safer alternatives:
using PSsettings2D = uint8_t;
constexpr uint8_t PS_WRAP_X = 0x01;
constexpr uint8_t PS_WRAP_Y = 0x02;
// ...or std::bitset<8>.
At minimum, add static_assert(sizeof(PSsettings2D) == 1, "Unexpected packing");.
| public: | ||
| ParticleSystem2D(const uint32_t width, const uint32_t height, const uint32_t numberofparticles, const uint32_t numberofsources, const bool isadvanced = false, const bool sizecontrol = false); // constructor | ||
| // note: memory is allcated in the FX function, no deconstructor needed | ||
| void update(void); //update the particles according to set options and render to the matrix | ||
| void updateFire(const uint8_t intensity, const bool renderonly); // update function for fire, if renderonly is set, particles are not updated (required to fix transitions with frameskips) | ||
| void updateSystem(void); // call at the beginning of every FX, updates pointers and dimensions | ||
| void particleMoveUpdate(PSparticle &part, PSparticleFlags &partFlags, PSsettings2D *options = NULL, PSadvancedParticle *advancedproperties = NULL); // move function | ||
| // particle emitters | ||
| int32_t sprayEmit(const PSsource &emitter); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Constructor parameter mismatch guard
ParticleSystem2D(width, height, numParticles, numSources …) takes raw counts, but upstream macros (ESP32_MAXPARTICLES, …) cap them.
If the caller passes a bigger value the constructor silently proceeds, later allocators may overflow.
Add:
if (numberofparticles > MAX_ALLOWED_PARTICLES)
numberofparticles = MAX_ALLOWED_PARTICLES;(or an assert) to fail fast.
| uint16_t binIndices[maxBinParticles]; // creat array on stack for indices, 2kB max for 1024 particles (ESP32_MAXPARTICLES/2) | ||
| uint32_t binParticleCount; // number of particles in the current bin | ||
| uint16_t nextFrameStartIdx = hw_random16(usedParticles); // index of the first particle in the next frame (set to fixed value if bin overflow) | ||
| uint32_t pidx = collisionStartIdx; //start index in case a bin is full, process remaining particles next frame | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Large automatic array may overflow the tiny MCU stack
uint16_t binIndices[maxBinParticles]; is allocated on the stack.
With the default limits (usedParticles/2 for 2‑D, /4 for 1‑D) this can be several kilobytes (e.g. 2 kB for 1024 particles).
On ESP8266 the default task stack is only 4 kB, leaving little head‑room and causing random resets.
Consider:
- allocating the array once in
PSdataEndmemory, or - capping
maxBinParticlesto a conservative constant (e.g. 256), or - switching to a small
StaticVector/ring‑buffer allocated inIRAM_ATTR.
| setParticleSize(1); // 2x2 rendering size by default | ||
| motionBlur = 0; //no fading by default | ||
| smearBlur = 0; //no smearing by default | ||
| emitIndex = 0; | ||
| collisionStartIdx = 0; | ||
|
|
||
| //initialize some default non-zero values most FX use |
There was a problem hiding this comment.
Several internal counters start uninitialised
Members such as forcecounter, gforcecounter (and their 1‑D counterparts) are later read and incremented inside applyForce / applyGravity, but they are never initialised in the constructor, resulting in undefined behaviour.
Please add explicit zero‑initialisation in both constructors, e.g.:
smearBlur = 0; //no smearing by default
emitIndex = 0;
collisionStartIdx = 0;
+ forcecounter = 0;
+ gforcecounter = 0;Do the same for the 1‑D class.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setParticleSize(1); // 2x2 rendering size by default | |
| motionBlur = 0; //no fading by default | |
| smearBlur = 0; //no smearing by default | |
| emitIndex = 0; | |
| collisionStartIdx = 0; | |
| //initialize some default non-zero values most FX use | |
| setParticleSize(1); // 2x2 rendering size by default | |
| motionBlur = 0; // no fading by default | |
| smearBlur = 0; // no smearing by default | |
| emitIndex = 0; | |
| collisionStartIdx = 0; | |
| forcecounter = 0; | |
| gforcecounter = 0; | |
| // initialize some default non-zero values most FX use |
- removed local buffer for ESP8266 in 1D system to save on RAM - increased particle brightness in PS Impact - minor tweak in collision binning (might improve speed) - removed comments and some other unused stuff - fixed a few compiler wranings
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
wled00/FXparticleSystem.h (2)
57-69: Bit-field layout may differ between ESP8266 & ESP32
PSsettings2Drelies on an 8-bitboolbit-field inside a union. ABI differences (alignment, endian packing) can cause unexpected behavior when accessing asasByte.At minimum, add a static assertion to verify structure size:
byte asByte; // access as a byte, order is: LSB is first entry in the list above } PSsettings2D; +static_assert(sizeof(PSsettings2D) == 1, "PSsettings2D size mismatch");
135-143: Constructor parameter mismatch guard needed for safety
ParticleSystem2D(width, height, numParticles, numSources …)takes raw counts, but those values should be capped by platform-specific macros to prevent memory issues.In the constructor implementation, add:
if (numberofparticles > ESP32_MAXPARTICLES) numberofparticles = ESP32_MAXPARTICLES; if (numberofsources > ESP32_MAXSOURCES) numberofsources = ESP32_MAXSOURCES;wled00/FXparticleSystem.cpp (4)
25-38: Constructor initializes PS pointers before knowing matrix sizeIn the
ParticleSystem2Dconstructor, pointers are calculated byupdatePSpointers()beforesetMatrixSize()setsmaxXpixelandmaxYpixel. This causes incorrect memory layout and potential buffer overflows.Fix the ordering issue in both 2D and 1D constructors:
numParticles = numberofparticles; // number of particles allocated in init usedParticles = numParticles; // use all particles by default advPartProps = nullptr; //make sure we start out with null pointers (just in case memory was not cleared) advPartSize = nullptr; - updatePSpointers(isadvanced, sizecontrol); // set the particle and sources pointer - setMatrixSize(width, height); + setMatrixSize(width, height); // make size‑dependent members valid first + updatePSpointers(isadvanced, sizecontrol); // now compute the correct pointer layout
38-44: Several internal counters start uninitialisedMembers like
forcecounterandgforcecounterare used in methods likeapplyForceandapplyGravitybut are never initialized in the constructor.Add zero-initialization in both constructors:
motionBlur = 0; //no fading by default smearBlur = 0; //no smearing by default emitIndex = 0; collisionStartIdx = 0; + forcecounter = 0; + gforcecounter = 0;
787-790: Large automatic array may overflow the MCU stack
uint16_t binIndices[maxBinParticles]is allocated on the stack, which can be several kilobytes. On ESP8266 with a 4KB task stack, this could cause random resets.Consider one of these approaches:
- Allocate the array in
PSdataEndmemory- Cap
maxBinParticlesto a conservative constant (e.g., 256)- Use a static buffer to avoid stack allocation
1137-1148: Same constructor ordering issue in 1D particle systemThe ParticleSystem1D constructor has the same pointer initialization issue as the 2D version.
Apply the same ordering fix:
numParticles = numberofparticles; // number of particles allocated in init usedParticles = numParticles; // use all particles by default advPartProps = nullptr; //make sure we start out with null pointers (just in case memory was not cleared) //advPartSize = nullptr; - updatePSpointers(isadvanced); // set the particle and sources pointer - setSize(length); + setSize(length); // make size‑dependent members valid first + updatePSpointers(isadvanced); // pointers now calculated with correct size
🧹 Nitpick comments (5)
wled00/FXparticleSystem.h (3)
1-9: Copyright year might be incorrectThe copyright year is set to 2025, which is either a year ahead or might indicate a long-term copyright. Please verify if this is intentional.
- Copyright (c) 2025 Damian Schneider + Copyright (c) 2024 Damian Schneider
279-284: Remove unused flag declared as TODOThe
forcedirectionflag is marked with a TODO comment indicating it's not used anymore.bool outofbounds : 1; // out of bounds flag, set to true if particle is outside of display area bool collide : 1; // if set, particle takes part in collisions bool perpetual : 1; // if set, particle does not age (TTL is not decremented in move function, it still dies from killoutofbounds) bool reversegrav : 1; // if set, gravity is reversed on this particle - bool forcedirection : 1; // direction the force was applied, 1 is positive x-direction (used for collision stacking, similar to reversegrav) TODO: not used anymore, can be removed + bool reserved : 1; // reserved for future use bool fixed : 1; // if set, particle does not move (and collisions make other particles revert direction), bool custom1 : 1; // unused custom flags, can be used by FX to track particle states bool custom2 : 1;
50-55: Consider using static_cast for shifts to improve clarityMagic numbers in shift operations can be confusing. Consider using static_cast or explicit comments.
-#define PS_P_RADIUS_SHIFT 6 // shift for RADIUS +#define PS_P_RADIUS_SHIFT static_cast<int>(log2(PS_P_RADIUS)) // shift for RADIUS, equals 6wled00/FXparticleSystem.cpp (2)
1822-1842: Missing optimizer attribute on blur1D functionThe
blur1Dfunction could benefit from the same optimization attribute used on other performance-critical functions.-void blur1D(CRGB *colorbuffer, uint32_t size, uint32_t blur, uint32_t start) +__attribute__((optimize("O2"))) void blur1D(CRGB *colorbuffer, uint32_t size, uint32_t blur, uint32_t start)
7-8: Copyright year might be incorrect (same issue as in header)The copyright year is set to 2025, which is likely incorrect.
- Copyright (c) 2025 Damian Schneider + Copyright (c) 2024 Damian Schneider
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/FXparticleSystem.cpp(1 hunks)wled00/FXparticleSystem.h(1 hunks)
🧰 Additional context used
🪛 Cppcheck (2.10-2)
wled00/FXparticleSystem.cpp
[warning] 445-445: Uninitialized variable
(uninitvar)
🔇 Additional comments (1)
wled00/FXparticleSystem.cpp (1)
183-204:Details
✅ Verification successful
hw_random16 calls without checking for availability
The code calls
hw_random16()which is likely a hardware random function, but there's no guard to ensure this function exists or falls back to a software implementation if hardware RNG is unavailable.Check if there's a fallback mechanism defined elsewhere in the code or consider adding one:
🏁 Script executed:
#!/bin/bash # Check for declarations of hw_random16 and any fallback mechanisms grep -r "hw_random16" --include="*.h" --include="*.cpp" .Length of output: 12399
No additional fallback needed for hw_random16 – hardware register is wrapped with inline definitions
I’ve confirmed that
hw_random16is defined inwled00/fcn_declare.has three inline overloads (no‑arg, upper‑limit, and lower‑/upper‑limit) all ultimately reading fromHW_RND_REGISTER. The comment inwled00/util.cpprefers to a 32‑bit hardware RNG implementation (used by other APIs) and does not provide a software fallback forhw_random16. Since all calls tohw_random16already resolve to the inline functions and will fail to compile ifHW_RND_REGISTERisn’t defined for your target, no further guard or fallback exists at present.If you intend to support platforms without a hardware RNG, you’ll need to add a software fallback in one place—ideally in
fcn_declare.hor a new header—by #ifdef’ing on a target‑specific macro (e.g.#ifdef HAS_HW_RNG) and providing a PRNG for the else case. Otherwise, the existing inline definitions suffice.– No changes required unless software‐RNG fallback is desired.
| ParticleSystem1D(const uint32_t length, const uint32_t numberofparticles, const uint32_t numberofsources, const bool isadvanced = false); // constructor | ||
| // note: memory is allcated in the FX function, no deconstructor needed |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply same constructor parameter safety checks for 1D system
Similar to the 2D constructor, add parameter validation to the ParticleSystem1D constructor.
In the constructor implementation, add:
if (numberofparticles > ESP32_MAXPARTICLES_1D)
numberofparticles = ESP32_MAXPARTICLES_1D;
if (numberofsources > ESP32_MAXSOURCES_1D)
numberofsources = ESP32_MAXSOURCES_1D;| typedef union { | ||
| struct{ | ||
| // one byte bit field for 1D settings | ||
| bool wrap : 1; | ||
| bool bounce : 1; | ||
| bool killoutofbounds : 1; // if set, out of bound particles are killed immediately | ||
| bool useGravity : 1; // set to 1 if gravity is used, disables bounceY at the top | ||
| bool useCollisions : 1; | ||
| bool colorByAge : 1; // if set, particle hue is set by ttl value in render function | ||
| bool colorByPosition : 1; // if set, particle hue is set by its position in the strip segment | ||
| bool unused : 1; | ||
| }; | ||
| byte asByte; // access as a byte, order is: LSB is first entry in the list above | ||
| } PSsettings1D; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same bit-field layout issue affects PSsettings1D
Like PSsettings2D, the PSsettings1D union has the same potential for inconsistent bit layout across platforms.
Add a static assertion and consider using explicit bit flags:
byte asByte; // access as a byte, order is: LSB is first entry in the list above
} PSsettings1D;
+static_assert(sizeof(PSsettings1D) == 1, "PSsettings1D size mismatch");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| typedef union { | |
| struct{ | |
| // one byte bit field for 1D settings | |
| bool wrap : 1; | |
| bool bounce : 1; | |
| bool killoutofbounds : 1; // if set, out of bound particles are killed immediately | |
| bool useGravity : 1; // set to 1 if gravity is used, disables bounceY at the top | |
| bool useCollisions : 1; | |
| bool colorByAge : 1; // if set, particle hue is set by ttl value in render function | |
| bool colorByPosition : 1; // if set, particle hue is set by its position in the strip segment | |
| bool unused : 1; | |
| }; | |
| byte asByte; // access as a byte, order is: LSB is first entry in the list above | |
| } PSsettings1D; | |
| typedef union { | |
| struct{ | |
| // one byte bit field for 1D settings | |
| bool wrap : 1; | |
| bool bounce : 1; | |
| bool killoutofbounds : 1; // if set, out of bound particles are killed immediately | |
| bool useGravity : 1; // set to 1 if gravity is used, disables bounceY at the top | |
| bool useCollisions : 1; | |
| bool colorByAge : 1; // if set, particle hue is set by ttl value in render function | |
| bool colorByPosition : 1; // if set, particle hue is set by its position in the strip segment | |
| bool unused : 1; | |
| }; | |
| byte asByte; // access as a byte, order is: LSB is first entry in the list above | |
| } PSsettings1D; | |
| static_assert(sizeof(PSsettings1D) == 1, "PSsettings1D size mismatch"); |
| } | ||
| forcecounter = tempcounter; // save value back | ||
| } | ||
|
|
There was a problem hiding this comment.
Uninitialized variable warning on tempcounter
The variable tempcounter might be uninitialized if the loop is never entered (when usedParticles is 0), but is later assigned to forcecounter.
Initialize the variable before the loop:
- uint8_t tempcounter;
+ uint8_t tempcounter = forcecounter;
// note: this is not the most computationally efficient way to do this, but it saves on duplicate code and is fast enough
for (uint32_t i = 0; i < usedParticles; i++) {
tempcounter = forcecounter;
applyForce(particles[i], xforce, yforce, tempcounter);
}
forcecounter = tempcounter; // save value back📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } | |
| forcecounter = tempcounter; // save value back | |
| } | |
| uint8_t tempcounter = forcecounter; | |
| // note: this is not the most computationally efficient way to do this, but it saves on duplicate code and is fast enough | |
| for (uint32_t i = 0; i < usedParticles; i++) { | |
| tempcounter = forcecounter; | |
| applyForce(particles[i], xforce, yforce, tempcounter); | |
| } | |
| forcecounter = tempcounter; // save value back | |
| } |
🧰 Tools
🪛 Cppcheck (2.10-2)
[warning] 445-445: Uninitialized variable
(uninitvar)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
wled00/FXparticleSystem.cpp (4)
440-447:⚠️ Potential issueUninitialized variable issue with tempcounter
The
tempcountervariable might be used uninitialized if the loop is never entered (whenusedParticlesis 0), but is later assigned toforcecounter.Initialize the variable before the loop:
- uint8_t tempcounter; + uint8_t tempcounter = forcecounter; // note: this is not the most computationally efficient way to do this, but it saves on duplicate code and is fast enough for (uint32_t i = 0; i < usedParticles; i++) { tempcounter = forcecounter; applyForce(particles[i], xforce, yforce, tempcounter); } forcecounter = tempcounter; // save value back🧰 Tools
🪛 Cppcheck (2.10-2)
[warning] 445-445: Uninitialized variable
(uninitvar)
788-792:⚠️ Potential issueLarge automatic array may overflow the tiny MCU stack
uint16_t binIndices[maxBinParticles];is allocated on the stack. With the default limits (usedParticles/2for 2D,/4for 1D) this can be several kilobytes (e.g., 2 kB for 1024 particles).On ESP8266 the default task stack is only 4 kB, leaving little headroom and potentially causing random resets.
Consider:
- Allocating the array once in
PSdataEndmemory- Capping
maxBinParticlesto a conservative constant (e.g., 256)- Using a small
StaticVector/ring-buffer allocated inIRAM_ATTR
1148-1151:⚠️ Potential issueMissing initialization of counters in 1D system
Similar to the 2D system, internal counters in the 1D system constructor are not properly initialized:
motionBlur = 0; //no fading by default smearBlur = 0; //no smearing by default emitIndex = 0; collisionStartIdx = 0; + forcecounter = 0; + gforcecounter = 0;This could lead to undefined behavior when using
applyForce()andapplyGravity()methods.
38-44:⚠️ Potential issueSeveral internal counters remain uninitialized
In the
ParticleSystem2Dconstructor, members likeforcecounterandgforcecounterare not initialized before they're read and incremented in methods likeapplyForceandapplyGravity. This results in undefined behavior.Please add explicit zero-initialization in the constructor:
smearBlur = 0; //no smearing by default emitIndex = 0; collisionStartIdx = 0; + forcecounter = 0; + gforcecounter = 0;
🧹 Nitpick comments (3)
wled00/FXparticleSystem.cpp (3)
624-666: Optimize memory access pattern in particle rendering functionThe render function uses a nested structure
pixcowith x,y coordinates, but the memory access pattern isn't optimal.The current approach creates a struct of arrays, when an array of structs would be more cache-friendly:
- struct { - int32_t x,y; - } pixco[4]; // particle pixel coordinates + struct PixelCoord { + int32_t x, y; + }; + PixelCoord pixco[4]; // particle pixel coordinatesAlso, consider using constant indices when accessing the array elements for better compiler optimization.
1850-1870: Optimize force calculation for common caseThe
calcForce_dvfunction can be optimized for the common case where force is a power of 16:static int32_t calcForce_dv(const int8_t force, uint8_t &counter) { if (force == 0) return 0; // for small forces, need to use a delay counter int32_t force_abs = abs(force); // absolute value (faster than lots of if's only 7 instructions) int32_t dv = 0; + // Fast path for powers of 16 + if ((force_abs & 0xF) == 0) { + return force / 16; // Powers of 16 can skip counter logic + } + // for small forces, need to use a delay counter, apply force only if it overflows if (force_abs < 16) { counter += force_abs; if (counter > 15) { counter -= 16; dv = force < 0 ? -1 : 1; // force is either 1 or -1 if it is small (zero force is handled above) } } else dv = force / 16; // MSBs, note: cannot use bitshift as dv can be negative return dv; }This optimization would improve performance for common force values that are multiples of 16.
7-7: Copyright year is set to 2025The copyright year in the file header is set to 2025, which is in the future. Update it to reflect the actual year:
- Copyright (c) 2025 Damian Schneider + Copyright (c) 2024 Damian Schneider
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FXparticleSystem.cpp(1 hunks)
🧰 Additional context used
🪛 Cppcheck (2.10-2)
wled00/FXparticleSystem.cpp
[warning] 445-445: Uninitialized variable
(uninitvar)
🔇 Additional comments (1)
wled00/FXparticleSystem.cpp (1)
143-144:⚠️ Potential issueConstructor initializes PS pointers before knowing strip size – memory layout is wrong
In the 1D particle system constructor,
updatePSpointers()is called aftersetSize(), but the ordering should be reversed.advPartProps = nullptr; //make sure we start out with null pointers (just in case memory was not cleared) //advPartSize = nullptr; - setSize(length); - updatePSpointers(isadvanced); // set the particle and sources pointer (call this before accessing sprays or particles) + updatePSpointers(isadvanced); // set the particle and sources pointer + setSize(length);This ordering issue would cause memory offsets to be calculated incorrectly, potentially leading to memory corruption.
Likely an incorrect or invalid review comment.
| uint32_t binParticleCount; // number of particles in the current bin | ||
| uint16_t nextFrameStartIdx = hw_random16(usedParticles); // index of the first particle in the next frame (set to fixed value if bin overflow) | ||
| uint32_t pidx = collisionStartIdx; //start index in case a bin is full, process remaining particles next frame |
There was a problem hiding this comment.
Same stack overflow risk in 1D particle system
Like the 2D system, the 1D particle system also uses a large stack-allocated array:
uint16_t binIndices[maxBinParticles];With the default limit of usedParticles/4, this can still consume significant stack space, especially on memory-constrained devices like ESP8266.
Apply the same mitigation strategies recommended for the 2D system:
- Allocate the array in data memory rather than on the stack
- Cap
maxBinParticlesto a conservative constant - Consider a more memory-efficient data structure
| CRGB renderbuffer[100]; // 10x10 pixel buffer | ||
| memset(renderbuffer, 0, sizeof(renderbuffer)); // clear buffer | ||
| //particle size to pixels: < 64 is 4x4, < 128 is 6x6, < 192 is 8x8, bigger is 10x10 | ||
| //first, render the pixel to the center of the renderbuffer, then apply 2D blurring | ||
| fast_color_add(renderbuffer[4 + (4 * 10)], color, pxlbrightness[0]); // oCrder is: bottom left, bottom right, top right, top left | ||
| fast_color_add(renderbuffer[5 + (4 * 10)], color, pxlbrightness[1]); | ||
| fast_color_add(renderbuffer[5 + (5 * 10)], color, pxlbrightness[2]); | ||
| fast_color_add(renderbuffer[4 + (5 * 10)], color, pxlbrightness[3]); | ||
| uint32_t rendersize = 2; // initialize render size, minimum is 4x4 pixels, it is incremented int he loop below to start with 4 | ||
| uint32_t offset = 4; // offset to zero coordinate to write/read data in renderbuffer (actually needs to be 3, is decremented in the loop below) | ||
| uint32_t maxsize = advPartProps[particleindex].size; | ||
| uint32_t xsize = maxsize; | ||
| uint32_t ysize = maxsize; | ||
| if (advPartSize) { // use advanced size control | ||
| if (advPartSize[particleindex].asymmetry > 0) | ||
| getParticleXYsize(&advPartProps[particleindex], &advPartSize[particleindex], xsize, ysize); | ||
| maxsize = (xsize > ysize) ? xsize : ysize; // choose the bigger of the two | ||
| } | ||
| maxsize = maxsize/64 + 1; // number of blur passes depends on maxsize, four passes max | ||
| uint32_t bitshift = 0; | ||
| for (uint32_t i = 0; i < maxsize; i++) { | ||
| if (i == 2) //for the last two passes, use higher amount of blur (results in a nicer brightness gradient with soft edges) | ||
| bitshift = 1; | ||
| rendersize += 2; | ||
| offset--; | ||
| blur2D(renderbuffer, rendersize, rendersize, xsize << bitshift, ysize << bitshift, offset, offset, true); | ||
| xsize = xsize > 64 ? xsize - 64 : 0; | ||
| ysize = ysize > 64 ? ysize - 64 : 0; | ||
| } | ||
|
|
||
| // calculate origin coordinates to render the particle to in the framebuffer | ||
| uint32_t xfb_orig = x - (rendersize>>1) + 1 - offset; | ||
| uint32_t yfb_orig = y - (rendersize>>1) + 1 - offset; | ||
| uint32_t xfb, yfb; // coordinates in frame buffer to write to note: by making this uint, only overflow has to be checked (spits a warning though) | ||
|
|
||
| //note on y-axis flip: WLED has the y-axis defined from top to bottom, so y coordinates must be flipped. doing this in the buffer xfer clashes with 1D/2D combined rendering, which does not invert y | ||
| // transferring the 1D buffer in inverted fashion will flip the x-axis of overlaid 2D FX, so the y-axis flip is done here so the buffer is flipped in y, giving correct results | ||
|
|
||
| // transfer particle renderbuffer to framebuffer | ||
| for (uint32_t xrb = offset; xrb < rendersize + offset; xrb++) { | ||
| xfb = xfb_orig + xrb; | ||
| if (xfb > (uint32_t)maxXpixel) { | ||
| if (wrapX) { // wrap x to the other side if required | ||
| if (xfb > (uint32_t)maxXpixel << 1) // xfb is "negative", handle it | ||
| xfb = (maxXpixel + 1) + (int32_t)xfb; // this always overflows to within bounds | ||
| else | ||
| xfb = xfb % (maxXpixel + 1); // note: without the above "negative" check, this works only for powers of 2 | ||
| } | ||
| else | ||
| continue; | ||
| } | ||
|
|
||
| for (uint32_t yrb = offset; yrb < rendersize + offset; yrb++) { | ||
| yfb = yfb_orig + yrb; | ||
| if (yfb > (uint32_t)maxYpixel) { | ||
| if (wrapY) {// wrap y to the other side if required | ||
| if (yfb > (uint32_t)maxYpixel << 1) // yfb is "negative", handle it | ||
| yfb = (maxYpixel + 1) + (int32_t)yfb; // this always overflows to within bounds | ||
| else | ||
| yfb = yfb % (maxYpixel + 1); // note: without the above "negative" check, this works only for powers of 2 | ||
| } | ||
| else | ||
| continue; | ||
| } | ||
| fast_color_add(framebuffer[xfb + (maxYpixel - yfb) * (maxXpixel + 1)], renderbuffer[xrb + yrb * 10]); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Large temporary buffer allocation on stack in renderParticle
The function allocates a 100-element CRGB array on the stack:
CRGB renderbuffer[100]; // 10x10 pixel bufferEach CRGB is typically 3 bytes, resulting in a 300-byte stack allocation. For most platforms this is manageable, but on ESP8266 with limited stack space, this could contribute to stack overflows, especially when combined with other stack allocations.
Consider:
- Moving this buffer to heap allocation at initialization time
- Reducing buffer size if full 10x10 particles are not needed
- Using a smaller buffer and multi-pass rendering
There was a problem hiding this comment.
there is enough stack memory available, using heap causes heap fragmentation.
There was a problem hiding this comment.
@DedeHai I understand your reasoning. Stack allocation is indeed preferable in this case if you've verified there's sufficient stack space available. You're right that heap allocations can lead to fragmentation over time, especially in embedded systems with frequent allocations/deallocations in tight rendering loops.
Thanks for sharing this insight about your implementation decision.
✏️ Learnings added
Learnt from: DedeHai
PR: MoonModules/WLED-MM#237
File: wled00/FXparticleSystem.cpp:678-743
Timestamp: 2025-05-04T10:32:30.438Z
Learning: In the Particle System implementation for WLED, stack allocation is preferred over heap allocation for temporary rendering buffers because there's enough stack memory available and heap allocations can cause memory fragmentation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Here is what is still needed to make this PR work as intended:
Since I am not too familiar with MM I will not update this PR but leave these changes to someone with better code knowledge. |
- fixed inconsitencies in size rendering - fixed palette being wrapped in color by position and color by age modes - Fixed bug in memory layout: for some unknown reason, if flags come before particles, last flag is sometimes overwritten, changing memory laout seems to fix that - New color modes in PS Fireworks 1D: - custom3 slider < 16: lower saturation (check1: single color or multi-color explosions) - custom3 slider <= 23: full saturation (check1: single color or multi-color explosions) - custom3 slider > 23: color by speed (check 1 has not effect here) - custom slider = max: color by age or color by position (depends on check1)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
wled00/FXparticleSystem.h (1)
57-69: Bit-field packing still platform-dependent – risk of ABI mismatchPrevious feedback pointed out that the anonymous bit-fields inside
PSsettings2Dmay be packed differently between ESP8266 and ESP32, breakingasByte-style comparisons and transmissions.
The structure is still unchanged, so the problem remains. Consider switching to explicit bit-flags or add astatic_assert(sizeof(PSsettings2D)==1)at minimum.wled00/FXparticleSystem.cpp (6)
439-447:tempcountermay carry garbage on first use
tempcounteris declared but not initialised before being passed toapplyForce, reproducing the cppcheck warning.
Initialise it with the currentforcecountervalue.- uint8_t tempcounter; + uint8_t tempcounter = forcecounter;🧰 Tools
🪛 Cppcheck (2.10-2)
[warning] 446-446: Uninitialized variable
(uninitvar)
678-684: 100-element stack buffer still risks ESP8266 overflow
CRGB renderbuffer[100](~300 B) is allocated on the stack each call torenderParticle().
On ESP8266 the default task stack is only 4 kB and this function is called for every particle ‑ stack blow-ups and resets are still likely.Move the buffer to:
- a static
thread_localarray, or- heap /
PSdataEnd, or- reuse a shared scratch buffer.
798-803: LargebinIndices[]on stack can still exhaust MCU stack
uint16_t binIndices[maxBinParticles]may be >2 kB (ESP8266 stack is 4 kB). The earlier recommendation to pre-allocate this array inPSdataEndor cap its size has not been implemented.
1616-1617: Stack allocation ofbinIndicesrepeats the overflow hazardThe 1-D collision routine still allocates
binIndiceson the stack (up to ~1 kB). Reuse a pre-allocated buffer or capmaxBinParticlesto a safe constant.
38-43:⚠️ Potential issueCounters
forcecounter/gforcecounterstart uninitialisedBoth counters are read and incremented later (e.g.
applyForce,applyGravity) but are never initialised here, leading to undefined behaviour and sporadic velocity spikes.smearBlur = 0; //no smearing by default emitIndex = 0; collisionStartIdx = 0; + forcecounter = 0; + gforcecounter = 0;
1151-1160:⚠️ Potential issueSame uninitialised counters in 1-D constructor
ParticleSystem1Dalso misses zero-initialisation forforcecounterandgforcecounter.smearBlur = 0; //no smearing by default emitIndex = 0; collisionStartIdx = 0; + forcecounter = 0; + gforcecounter = 0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/FXparticleSystem.cpp(1 hunks)wled00/FXparticleSystem.h(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
wled00/FXparticleSystem.cpp (3)
wled00/FXparticleSystem.h (27)
force(153-153)force(173-173)force(337-337)ParticleSystem2D(136-136)hardness(159-159)hardness(160-160)hardness(326-326)x(162-162)x(327-327)size(172-172)size(336-336)part(141-141)part(147-147)part(148-148)part(151-151)part(154-154)part(318-318)part(320-320)part(322-322)limitSpeed(34-36)limitSpeed(34-34)blur2D(222-222)calculateNumberOfParticles2D(225-225)calculateNumberOfSources2D(226-226)allocateParticleSystemMemory2D(227-227)ParticleSystem1D(312-312)blur1D(389-389)wled00/FX.cpp (2)
wrap(5401-5417)wrap(5401-5401)wled00/fcn_declare.h (13)
hw_random16(417-417)hw_random16(417-417)hw_random16(418-418)hw_random16(418-418)hw_random16(419-419)hw_random16(419-419)cos16_t(453-453)sin16_t(452-452)r(434-434)r(434-434)b(312-312)b(312-312)b(343-343)
🪛 Cppcheck (2.10-2)
wled00/FXparticleSystem.cpp
[warning] 446-446: Uninitialized variable
(uninitvar)
| if (advancedproperties) { //using individual particle size? | ||
| setParticleSize(particlesize); // updates default particleHardRadius | ||
| if (advancedproperties->size > PS_P_MINHARDRADIUS) { | ||
| particleHardRadius += (advancedproperties->size - PS_P_MINHARDRADIUS); // update radius | ||
| renderradius = particleHardRadius; | ||
| } | ||
| } | ||
| // note: if wall collisions are enabled, bounce them before they reach the edge, it looks much nicer if the particle does not go half out of view |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Calling setParticleSize() inside the per-particle loop is expensive and global
setParticleSize() recalculates global members (particleHardRadius, motionBlur) every time it is invoked.
Invoking it for every particle in particleMoveUpdate() (hot path) causes:
- O(n²) overhead on size-controlled effects.
- Global state oscillation – later particles see a different
particleHardRadiusthan earlier ones.
Extract radius computation into a small inline helper that operates on local variables instead of mutating global settings, and call it once per frame.
There was a problem hiding this comment.
this is intentional, not using the function leads to unnecessary code-duplication
There was a problem hiding this comment.
@DedeHai I understand your reasoning. The call to setParticleSize() within the loop does keep the code DRY by reusing the logic for calculating the particle radius. While there is a theoretical performance cost from recalculating global state for each particle, you've made a conscious design decision prioritizing code maintainability over this potential overhead. Makes sense given the complexity of the particle system implementation.
✏️ Learnings added
Learnt from: DedeHai
PR: MoonModules/WLED-MM#237
File: wled00/FXparticleSystem.cpp:241-248
Timestamp: 2025-05-04T10:31:58.827Z
Learning: In the particle system (FXparticleSystem.cpp), calling `setParticleSize()` inside the particle update loop is intentional despite potential performance implications. This design decision prioritizes code reuse and maintainability over performance optimization.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
One blocker for merge is that PS isn't optional but merging in will break any builds that use animartrix as this would take us over 254 effects |
|
If I counted right, the currently used effect number is 220 (with PS effects). |
|
Given MoonModules focus on 2D and larger more complex setups Vs WLED, I'm much more comfortable ditching basic 1D legacy effects to free to resources for PS |
Adding Particle System (port from AC)
3 participants
Summary by CodeRabbit
New Features
Improvements