Removed PS memory manager and some minor improvements #4651
Conversation
- reverted all changes related to memory manager - moved local buffer into effect data memory - some RAM issues may occur on larger setups: tested on S3 it works fine up to 32x32 but runs into memory issues at 64x32
… speed - added struct for x and y coordinates, thx to @blazoncek
- 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
WalkthroughThis change substantially refactors the internal implementation of the particle system used in the project. It eliminates the custom memory management layer, removes global and per-segment buffers, and consolidates memory allocation into contiguous blocks with explicit alignment. Rendering logic is simplified, with local framebuffers used directly for drawing particles, and transition and overlay handling is removed. The code now uses unsigned types for loop indices and collision calculations, and redundant or outdated comments are cleaned up. The maximum particle counts for ESP8266 are reduced, and all related logic and declarations are updated accordingly. No public API signatures are changed. Changes
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.31.1)wled00/FX.cppTip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ 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:
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
wled00/FXparticleSystem.cpp (1)
1061-1069:⚠️ Potential issueDivision by 8 can drop the particle count to 0
When
sizecontrol == trueand the matrix is tiny (e.g. 2×2 → 4 pixels),
numberofParticlesis 4 before the division, becomes 0 afterwards and stays 0
because the “multiple of 4” alignment is applied after the division:numberofParticles = max(4, …); // => 4 numberofParticles /= 8; // => 0 numberofParticles = (numberofParticles+3) & ~0x03; // => 0A zero‑particle system will later dereference null‑sized arrays and crash.
Proposed fix (keeps alignment):
- if (sizecontrol) - numberofParticles /= 8; + if (sizecontrol) { + numberofParticles = max(4U, numberofParticles / 8); + }Apply an analogous guard in
calculateNumberOfParticles1D()(≈ line 1780).
🧹 Nitpick comments (4)
wled00/FXparticleSystem.h (2)
366-368: ESP8266 code path still owns aframebufferpointer
#ifndef ESP8266correctly removes the definition offramebufferfor ESP8266 builds, but:1.
allocateParticleSystemMemory1D()(see.cpp) actually excludes the memory block only under#ifndef ESP8266, so that part is safe.2. In contrast, the 2‑D system allocates a framebuffer unconditionally, meaning an ESP8266 running a 2‑D effect will still reserve
virtualWidth*virtualHeight*3bytes.
Given the PR goal is to keep ESP8266 memory consumption as low as possible, consider applying the same#ifndef ESP8266guard to the 2‑D member as well (and gate the render logic appropriately).This will avoid surprises when users switch segments from 1‑D to 2‑D on constrained devices.
190-206:numParticles&usedParticlesnaming collisionWith the new design,
numParticlesis the allocated count whileusedParticlesis the active subset.
Their purposes are distinct but the very similar names have already led to bugs (seesetUsedParticles()shiftingpercentage+1).Consider renaming one of them (
allocatedParticlesvsactiveParticles) or adding short doc‑comments to avoid future misunderstandings and off‑by‑one errors.wled00/FXparticleSystem.cpp (2)
563-574: O(N²) clear when motion‑blur disabledThe loop that zeroes or scales the 2‑D framebuffer iterates row‑by‑row:
for (y) // height for (x) // width fast_color_scale(...) // or memsetFor large matrices this is noticeably slower than a single
memset
(ormemset32, etc.) whenmotionBlur == 0.Suggestion (pseudo‑diff):
if (motionBlur) { // keep current double‑loop } else { - memset(framebuffer, 0, (maxXpixel+1)*(maxYpixel+1)*sizeof(CRGB)); + memset(framebuffer, 0, (maxXpixel+1)*(maxYpixel+1)*sizeof(CRGB)); // already O(N) }The scaling branch is fine; the zero‑case could just keep the
memsetand skip
the outer loop entirely, shaving a few hundred CPU cycles per frame on ESP32‑S2.
670-676: Per‑particle 300‑byte stack buffer risks overflow on deep call stacks
CRGB renderbuffer[100](≈ 300 bytes) is created for every call to
renderParticle()when advanced‑size particles are in use.
While the function returns quickly, on some tool‑chains the compiler may decide
to keep several frames inlined, leading to >1 kB additional stack usage.Consider:
1. A single static scratch buffer protected by
noInterrupts()/interrupts(), or
2. Moving the buffer toSEGENV.dataand re‑using it, or
3. LimitingadvPartProps[].sizeso that the 10×10 path is less frequent.This will leave more head‑room for WiFi and file‑system tasks, especially on
ESP8266.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wled00/FX.cpp(23 hunks)wled00/FX_fcn.cpp(0 hunks)wled00/FXparticleSystem.cpp(28 hunks)wled00/FXparticleSystem.h(4 hunks)
💤 Files with no reviewable changes (1)
- wled00/FX_fcn.cpp
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
🔇 Additional comments (9)
wled00/FX.cpp (9)
8093-8093: Parameter changes in particle system initialization align with memory management redesignThe change from requesting specific source counts to using '0' parameter for requested sources is consistent with the PR objective of removing the PS memory manager. This simplifies memory management by letting the particle system determine appropriate allocations.
Also applies to: 8895-8895
8377-8377: Type standardization and improved boundary checkingVariable
numMeteorsis now consistently defined asuint32_tand properly bounded by usingmin(PartSys->numSources, (uint32_t)NUMBEROFSOURCES)to ensure iterations don't exceed available resources.Also applies to: 8388-8389, 8408-8408
8413-8413: Enhanced particle emission logicThe explicit comparison (
vy < 0) improves readability, and the revised emission calculation now dynamically scales based on segment intensity and available particles, which is more efficient.Also applies to: 8419-8419
8440-8446: Platform-specific optimizations for particle lifetimesThe changes adjust particle lifetimes with separate values for ESP8266 (lower values: 900/100) and other platforms (higher values: 1250/250), aligned with the PR's goal of optimizing for ESP8266's memory constraints.
8468-8470: Enhanced particle brightness effectThis new code segment reduces TTL for particles, effectively creating a shorter but brighter spark lifespan, which aligns with the PR objective to enhance particle brightness in the Impact effect.
9564-9564: Consistent type standardization for loop indicesAll loop indices are now standardized to use unsigned types (
uint32_toruint), which is good practice for iteration counts and indices.Also applies to: 9827-9827, 9999-9999, 10281-10281, 10302-10302, 10319-10319, 10369-10369
9769-9769: Improved change detection for particle system settingsThe calculation for
settingssumnow comprehensively includes all relevant segment parameters, ensuring that any parameter change will properly trigger updates to the particle system.Also applies to: 10258-10258
9516-9517: Optimized initialization based on setting changesThe code now only reinitializes particles when the intensity setting has changed, which improves performance by avoiding unnecessary resets.
10020-10020: Improved particle lifetime calculationThe minLife calculation now directly relates to the source TTL plus intensity, creating a more consistent and adjustable particle behavior based on user settings.
| PSPRINTLN("PS 2D alloc"); | ||
| PSPRINTLN("numparticles:" + String(numparticles) + " numsources:" + String(numsources) + " additionalbytes:" + String(additionalbytes)); | ||
| uint32_t requiredmemory = sizeof(ParticleSystem2D); | ||
| uint32_t dummy; // dummy variable | ||
| if ((particleMemoryManager(numparticles, sizeof(PSparticle), dummy, dummy, SEGMENT.mode)) == nullptr) // allocate memory for particles | ||
| return false; // not enough memory, function ensures a minimum of numparticles are available | ||
|
|
||
| // functions above make sure these are a multiple of 4 bytes (to avoid alignment issues) | ||
| // functions above make sure numparticles is a multiple of 4 bytes (to avoid alignment issues) | ||
| requiredmemory += sizeof(PSparticleFlags) * numparticles; | ||
| requiredmemory += sizeof(PSparticle) * numparticles; | ||
| if (isadvanced) | ||
| requiredmemory += sizeof(PSadvancedParticle) * numparticles; | ||
| if (sizecontrol) | ||
| requiredmemory += sizeof(PSsizeControl) * numparticles; | ||
| requiredmemory += sizeof(PSsource) * numsources; | ||
| requiredmemory += additionalbytes; | ||
| requiredmemory += sizeof(CRGB) * SEGMENT.virtualLength(); // virtualLength is witdh * height | ||
| requiredmemory += additionalbytes + 3; // add 3 to ensure there is room for stuffing bytes | ||
| //requiredmemory = (requiredmemory + 3) & ~0x03; // align memory block to next 4-byte boundary | ||
| PSPRINTLN("mem alloc: " + String(requiredmemory)); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Frame‑buffer always allocated even when not used on ESP8266
allocateParticleSystemMemory2D() adds:
requiredmemory += sizeof(CRGB) * SEGMENT.virtualLength();without an #ifndef ESP8266 guard.
If an ESP8266 user only needs simple (non‑blurred) rendering, this reserves
up to 768 bytes for 16×16 or much more for taller matrices – exactly what the
PR tries to avoid.
Either:
1. Wrap this addition in #ifndef ESP8266, or
2. Make the allocation conditional on a new runtime flag (e.g. useLocalFB).
Please double‑check the intended behaviour and memory footprint.
🏁 Script executed:
#!/bin/bash
# Display the allocateParticleSystemMemory2D function context
rg -n "allocateParticleSystemMemory2D" -A30 -B5 wled00/FXparticleSystem.cpp
# Show the CRGB allocation lines to confirm they are unconditional
rg -n "CRGB" -A5 -B5 wled00/FXparticleSystem.cpp
# Check for any ESP8266-specific preprocessor guards in the file
rg -n "ESP8266" -n -C3 wled00/FXparticleSystem.cpp
# Look for any runtime flags related to frame buffer allocation
rg -n "FB" -n -C3 wled00/FXparticleSystem.cpp || trueLength of output: 28622
Guard 2D frame‑buffer allocation on ESP8266 to avoid unnecessary RAM use
In allocateParticleSystemMemory2D (wled00/FXparticleSystem.cpp, lines 1099–1101), the local CRGB buffer is always reserved—even on ESP8266 where it’s unused for simple (non‑blur) rendering. The 1D allocator already skips this on ESP8266; the 2D version should do the same.
• Wrap the CRGB allocation in an ESP8266 guard:
requiredmemory += sizeof(PSsource) * numsources;
- requiredmemory += sizeof(CRGB) * SEGMENT.virtualLength();
+ #ifndef ESP8266
+ requiredmemory += sizeof(CRGB) * SEGMENT.virtualLength();
+ #endif
requiredmemory += additionalbytes + 3;• Or introduce a runtime flag (e.g. useLocalFB) and only add the CRGB term when true.
Please update and verify your intended behavior and memory footprint on ESP8266.
📝 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.
| PSPRINTLN("PS 2D alloc"); | |
| PSPRINTLN("numparticles:" + String(numparticles) + " numsources:" + String(numsources) + " additionalbytes:" + String(additionalbytes)); | |
| uint32_t requiredmemory = sizeof(ParticleSystem2D); | |
| uint32_t dummy; // dummy variable | |
| if ((particleMemoryManager(numparticles, sizeof(PSparticle), dummy, dummy, SEGMENT.mode)) == nullptr) // allocate memory for particles | |
| return false; // not enough memory, function ensures a minimum of numparticles are available | |
| // functions above make sure these are a multiple of 4 bytes (to avoid alignment issues) | |
| // functions above make sure numparticles is a multiple of 4 bytes (to avoid alignment issues) | |
| requiredmemory += sizeof(PSparticleFlags) * numparticles; | |
| requiredmemory += sizeof(PSparticle) * numparticles; | |
| if (isadvanced) | |
| requiredmemory += sizeof(PSadvancedParticle) * numparticles; | |
| if (sizecontrol) | |
| requiredmemory += sizeof(PSsizeControl) * numparticles; | |
| requiredmemory += sizeof(PSsource) * numsources; | |
| requiredmemory += additionalbytes; | |
| requiredmemory += sizeof(CRGB) * SEGMENT.virtualLength(); // virtualLength is witdh * height | |
| requiredmemory += additionalbytes + 3; // add 3 to ensure there is room for stuffing bytes | |
| //requiredmemory = (requiredmemory + 3) & ~0x03; // align memory block to next 4-byte boundary | |
| PSPRINTLN("mem alloc: " + String(requiredmemory)); | |
| PSPRINTLN("PS 2D alloc"); | |
| PSPRINTLN("numparticles:" + String(numparticles) + " numsources:" + String(numsources) + " additionalbytes:" + String(additionalbytes)); | |
| uint32_t requiredmemory = sizeof(ParticleSystem2D); | |
| // functions above make sure numparticles is a multiple of 4 bytes (to avoid alignment issues) | |
| requiredmemory += sizeof(PSparticleFlags) * numparticles; | |
| requiredmemory += sizeof(PSparticle) * numparticles; | |
| if (isadvanced) | |
| requiredmemory += sizeof(PSadvancedParticle) * numparticles; | |
| if (sizecontrol) | |
| requiredmemory += sizeof(PSsizeControl) * numparticles; | |
| requiredmemory += sizeof(PSsource) * numsources; | |
| #ifndef ESP8266 | |
| requiredmemory += sizeof(CRGB) * SEGMENT.virtualLength(); // virtualLength is width * height | |
| #endif | |
| requiredmemory += additionalbytes + 3; // add 3 to ensure there is room for stuffing bytes | |
| //requiredmemory = (requiredmemory + 3) & ~0x03; // align memory block to next 4-byte boundary | |
| PSPRINTLN("mem alloc: " + String(requiredmemory)); |
blazoncek
left a comment
There was a problem hiding this comment.
IMO these are good changes that will make merging segment blending/layering easier.
I am still not sold on motion blur being included within PS, though. 😉
|
I do get your point about motion blurring being somewhat reduntant and can be easily done in the FX itself. I see it as an "ease of use" feature plus there is the speed advantage when doing it in the PS. Same goes for smear blur. |
Removing the mem-manager should fix all PS crashes but comes at the expence of increased RAM usage.
This is in preparation to merging segment layering by @blazoncek
The local rendering buffer is now also added to segment data, particles are also stored in segment data, as it was prior to adding the memorymanager.
A more appropriate memory management can be added back in later.
Since the increased RAM usage is a problem for ESP8266, I removed the local rendering buffer for the 1D system on ESP8266, making rendering a lot slower but at least it works up to about 600 LEDs.
Summary by CodeRabbit