Added new effect usermod - Diffusion Fire #4667
Conversation
WalkthroughThis update introduces a new usermod named "user_fx" designed to provide custom LED effects for a 2D LED matrix. The changes add a new module directory containing a README, a library descriptor, source and header files implementing the usermod, and an updated constant definition for the usermod ID. The usermod implements two effect modes: a static mode and a "Diffusion Fire" animation, with logic for effect parameters, palette usage, and memory management. The module is integrated into the system through setup and registration mechanisms, and its unique ID is defined in the global constants header. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
usermods/user_fx/README.md (1)
1-4: Consider enhancing documentation.While the README provides a basic explanation of the usermod's purpose, it would be more helpful for users if you included:
- Description of the included effects (e.g., Diffusion Fire)
- Configuration parameters
- Example usage
- Any dependencies or requirements
usermods/user_fx/user_fx.h (1)
1-12: Class structure looks good, but consider adding documentation.The
UserFxUsermodclass follows standard WLED usermod structure with the required methods. Consider:
- Adding comments to describe what each method does
- Including documentation for the "Diffusion Fire" effect mentioned in the summary
- Declaring any private member variables needed for the effects
This will improve maintainability and make it easier for others to contribute to or use your code.
usermods/user_fx/user_fx.cpp (5)
13-14: Remove the unusedcallcounter
callis written but never read, so it produces dead code and wasted RAM.- static uint32_t call = 0; ... - call = 0; ... - call++;Also applies to: 34-41
42-48: Replace byte-wise scroll withmemmoveto cut copy time by ~3×Nested loops copy one byte at a time. A single
memmoveper row is both clearer and faster, especially on ESP32 wherememcpyis optimised.- for (unsigned y = 1; y < rows; y++) - for (unsigned x = 0; x < cols; x++) { - unsigned src = XY(x, y); - unsigned dst = XY(x, y - 1); - SEGMENT.data[dst] = SEGMENT.data[src]; - } + for (unsigned y = 1; y < rows; y++) { + memmove(&SEGMENT.data[XY(0, y - 1)], + &SEGMENT.data[XY(0, y)], + cols); // copy whole row in one shot + }
71-71: Scratch buffer is wider (uint16_t) than necessary
tmp_rowstores values clamped to 0-255, yet each cell consumes 2 bytes.
Switching touint8_thalves RAM consumption and removes the alignment headache mentioned earlier.
86-87: Consider returningrefresh_msfor power-savingThe function always returns
FRAMETIME(~16 ms) even when the effect is configured for a slowerrefresh_ms(e.g. 50 ms).
Returning the actual period lets WLED sleep longer and reduces CPU-load on constrained boards.
IfFRAMETIMEis required by the engine, a brief comment would help future maintainers understand the choice.
5-5: Out-of-bounds safety forXY()macro
XY()blindly computesx + SEG_W * y. A stray index can corrupt memory.
Adding a debug assertion (or at least documenting the expectation) prevents painful pixel-buffer overruns in user presets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
usermods/user_fx/README.md(1 hunks)usermods/user_fx/library.json(1 hunks)usermods/user_fx/user_fx.cpp(1 hunks)usermods/user_fx/user_fx.h(1 hunks)wled00/const.h(1 hunks)
🔇 Additional comments (2)
wled00/const.h (1)
213-213: Properly registered new usermod ID.The
USERMOD_ID_USER_FXconstant with value 58 is correctly defined, following the sequential numbering pattern of existing usermod IDs.usermods/user_fx/user_fx.cpp (1)
50-56: Turbulence parameter appears inverted – please confirm intentThe current logic only spawns sparks when
random8() > turbulence.
That means raising turbulence actually decreases spark probability, which feels counter-intuitive for a “turbulence” control.
Verify whether the comparison should be<instead.
usermods/user_fx/user_fx.cpp
Outdated
| unsigned dataSize = SEGMENT.length() + (cols * sizeof(uint16_t)); | ||
| if (!SEGENV.allocateData(dataSize)) | ||
| return mode_static(); // allocation failed | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid re-allocating the scratch buffer every frame
SEGENV.allocateData() is invoked on every call even after memory has been assigned, which can lead to heap fragmentation and skips a precious few µs of CPU time per frame.
- if (!SEGENV.allocateData(dataSize))
+ if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))
return mode_static(); // allocation failed📝 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.
| unsigned dataSize = SEGMENT.length() + (cols * sizeof(uint16_t)); | |
| if (!SEGENV.allocateData(dataSize)) | |
| return mode_static(); // allocation failed | |
| unsigned dataSize = SEGMENT.length() + (cols * sizeof(uint16_t)); | |
| - if (!SEGENV.allocateData(dataSize)) | |
| + if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize)) | |
| return mode_static(); // allocation failed |
There was a problem hiding this comment.
That is ok as allocateData() does its checks.
Even though SEGENV.data is not null it may point to a memory not allocated by the effect and may not be of correct size.
There was a problem hiding this comment.
No description provided.
|
thanks for updating, can you close the other PR? Can you add a short description of your effect in the readme? if more FX are added, it will give a good overview of what these effect do and are intended for. |
Just followed the other usermods. Also, the new system won't pick up header files. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
usermods/user_fx/user_fx.cpp (1)
27-29: Avoid re-allocating the scratch buffer on every frameThe current implementation calls
SEGENV.allocateData()on every frame, even after memory has been successfully allocated. This can lead to heap fragmentation and unnecessary CPU overhead.- if (!SEGENV.allocateData(dataSize)) + if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize)) return mode_static(); // allocation failed
🧹 Nitpick comments (5)
usermods/user_fx/user_fx.cpp (5)
12-14: Remove unused static counterThe
static uint32_t call = 0variable is declared and incremented (line 43) but never used meaningfully in the function. Consider removing it to improve code clarity.static uint16_t mode_diffusionfire(void) { - static uint32_t call = 0;Also remove the related initialization in line 34:
if (SEGENV.call == 0) { SEGMENT.fill(BLACK); SEGENV.step = 0; - call = 0; }And the increment in line 43:
uint16_t *tmp_row = reinterpret_cast<uint16_t *>(aligned); SEGENV.step = strip.now; - call++;
65-75: Add clarifying comments for the diffusion algorithmThe diffusion algorithm's mathematical model (particularly the formula
v * 100 / (300 + diffusion)) lacks explanatory comments. Adding comments would make the code more maintainable for future developers.// diffuse for (unsigned y = 0; y < rows; y++) { for (unsigned x = 0; x < cols; x++) { + // Sum the current pixel with its horizontal neighbors unsigned v = SEGMENT.data[XY(x, y)]; if (x > 0) { v += SEGMENT.data[XY(x - 1, y)]; } if (x < (cols - 1)) { v += SEGMENT.data[XY(x + 1, y)]; } + // Apply diffusion factor - higher values cause faster cooling/fading + // The 300 base value ensures some persistence, while diffusion (0-100) controls decay rate tmp_row[x] = min(255, (int)(v * 100 / (300 + diffusion))); }
91-94: Add comment clarifying the default palette selectionThe effect defaults to palette 35, but it's not clear what this palette represents. A comment would help future developers understand this choice.
static const char _data_FX_MODE_DIFFUSIONFIRE[] PROGMEM = "Diffusion Fire@!,Spark rate,Diffusion Speed,Turbulence,,Use " - "palette;;Color;;2;pal=35"; + "palette;;Color;;2;pal=35"; // Default to Fire palette (35)
59-60: Consider variable spark intensityCurrently, all sparks are set to maximum intensity (255). For more visual variety, consider randomizing the spark intensity.
if (p < spark_rate) { unsigned dst = XY(x, rows - 1); - SEGMENT.data[dst] = 255; + // Randomize spark intensity between 192-255 for more natural variation + SEGMENT.data[dst] = random8(192, 256); }
95-97: Potential magic number in effect registrationThe code uses the value 255 when registering the effect. Consider using a named constant or adding a comment explaining what this value represents.
void UserFxUsermod::setup() { - strip.addEffect(255, &mode_diffusionfire, _data_FX_MODE_DIFFUSIONFIRE); + // 255 means the effect will be added at the end of effects list + strip.addEffect(255, &mode_diffusionfire, _data_FX_MODE_DIFFUSIONFIRE); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/user_fx/user_fx.cpp(1 hunks)
🔇 Additional comments (2)
usermods/user_fx/user_fx.cpp (2)
53-62: Consider checking turbulence parameter range/validationThe
turbulenceparameter is directly compared with a random value without validation. Consider adding bounds checking or normalization to ensure consistent behavior.Random values from
hw_random8()range from 0-255. Ifturbulenceis somehow set to a value higher than 255, the conditionhw_random8() > turbulencewould always be false, preventing spark generation entirely. WhileSEGMENT.custom2(which providesturbulence) is likely limited to 0-255 in the UI, it's good practice to ensure this in the code:- if (hw_random8() > turbulence) { + // Ensure turbulence is within valid range (0-255) + if (hw_random8() > min(255, turbulence)) {
39-41: Good implementation of aligned memory accessThe code properly handles memory alignment for the temporary buffer to avoid hardware faults on architectures that don't support unaligned memory access (like ESP8266). This is a solid implementation that follows best practices.
usermods/sht/library.json
Outdated
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name:": "sht", | |||
| "name": "sht", | |||
There was a problem hiding this comment.
revert, it will be fixed with this pending PR: #4623
usermods/EXAMPLE/library.json
Outdated
| @@ -1,4 +1,4 @@ | |||
| { | |||
| "name:": "EXAMPLE", | |||
| "name": "EXAMPLE", | |||
usermods/user_fx/library.json
Outdated
| @@ -0,0 +1,3 @@ | |||
| { | |||
| "name:": "user_fx" | |||
There was a problem hiding this comment.
this is what you want to fix
usermods/user_fx/user_fx.cpp
Outdated
| } | ||
|
|
||
| if ((strip.now - SEGENV.step) >= refresh_ms) { | ||
| // Reserve one extra byte and align to 2-byte boundary to avoid hard-faults |
There was a problem hiding this comment.
you did not reserve the extra byte...
also this is much more readable:
uintptr_t addr = reinterpret_cast<uintptr_t>(original_pointer);
uintptr_t aligned = (addr + 1) & ~1;
|
please do not force push on PR's in review, it screws up file comparison and comments. |
This reverts commit dc136bf.
Okay, sorry. |
Updated PR #4473
Summary by CodeRabbit