Add new effects: music strobe and flame jet #4819
Conversation
WalkthroughAdded two new audio-reactive effects (mode_music_strobe, mode_flame_jet) implemented in FX.cpp, registered with metadata; added corresponding FX mode constants and updated MODE_COUNT in FX.h; and updated PlatformIO defaults and common settings in platformio.ini. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thanks. A video would be beneficial. |
The video shows an example of the work. |
Sorry, the music from the previous video may not be available in some countries. |
|
I like the flame jet effect, but how is it 2D only? looks 1D to me, did not check the code yet. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
wled00/FX.cpp (4)
10657-10659: Fade mapping almost wipes the frame every tick
map(..., 0, 255, 0, 20);followed byfadeBy = 255 - mappedSpeedconstrainsfadeByto 235–255, meaning nearly-full fade-out each frame. The strobe will be invisible except during the exact peak frame.Consider mapping speed directly to a wider fade range or inverting the relation:
- long mappedSpeed = map(SEGMENT.speed, 0, 255, 0, 20); - uint8_t fadeBy = 255 - mappedSpeed; + // Higher speed → faster fade-out (larger fadeBy) + uint8_t fadeBy = map(SEGMENT.speed, 0, 255, 20, 235);
10647-10650: Unify spelling: “strob” → “strobe”Both the comment and metadata string use “Strob”, while the common term (and likely user expectation) is “Strobe”. Rename for consistency and discoverability.
Also applies to: 10670-10671
10704-10727: Clamp loop upper bound to save MCU cyclesWhen
SEGENV.aux0exceedsSEG_H, theforloop still iteratesaux0times, immediately skipping out-of-range rows. On large matrices this is wasted work.- for (uint16_t y_offset = 0; y_offset < SEGENV.aux0; y_offset++) { + const uint16_t limit = MIN(SEGENV.aux0, SEG_H); + for (uint16_t y_offset = 0; y_offset < limit; y_offset++) {Repeat the same optimisation in the 1 D branch.
10759-10760: Non-English inline commentProject comments are English-only; please translate or remove the Russian note to stay consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.cpp(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: DedeHai
PR: wled/WLED#4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.652Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-06-27T06:04:44.652Z
Learnt from: DedeHai
PR: wled/WLED#4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.652Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
PR: wled/WLED#4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
wled00/FX.cpp
🔇 Additional comments (1)
wled00/FX.cpp (1)
10896-10896: Verify ID / MODE_COUNT alignmentEnsure
FX_MODE_MUSIC_STROBandFX_MODE_FLAME_JETare defined in FX.h and thatMODE_COUNTwas bumped to the highest ID + 1 (219 → 220) as per WLED convention.Also applies to: 10970-10971
|
Thanks for the quick feedback, @DedeHai Regarding Flame Jet: Regarding Music Strobe:
My I've pushed the changes for |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
wled00/FX.cpp (3)
10647-10649: Correct the spelling of “strobe”.The in-code comment and effect name use “strob”, which looks like a typo. Standardising on “strobe” avoids future search/consistency issues.
10657-10660:fadeByrange is too narrow to give a visible speed difference.Mapping speed 0-255 → 0-20 means
fadeByvaries only between 255 and 235, i.e. ~8 % of the usable range. Consider mapping to the full 0-255 to obtain a noticeably slower fade at low speed values:- long mappedSpeed = map(SEGMENT.speed, 0, 255, 0, 20); - uint8_t fadeBy = 255 - mappedSpeed; + uint8_t fadeBy = 255 - map(SEGMENT.speed, 0, 255, 0, 255);
10682-10685: Same narrow‐range fade issue as above.
map(..., 0, 64)limitsfade_byto 255–191. Expanding to the full range would make “Trail Length” more intuitive for users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.cpp(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: DedeHai
PR: wled/WLED#4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.652Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-06-27T06:04:44.652Z
Learnt from: DedeHai
PR: wled/WLED#4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.652Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
PR: wled/WLED#4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
wled00/FX.cpp
🧬 Code Graph Analysis (1)
wled00/FX.cpp (2)
usermods/mpu6050_imu/mpu6050_imu.cpp (1)
um_data(124-124)wled00/fcn_declare.h (3)
hw_random8(549-549)hw_random8(550-550)hw_random8(551-551)
🔇 Additional comments (2)
wled00/FX.cpp (2)
10893-10894: Verify header constants andMODE_COUNT.Adding
FX_MODE_MUSIC_STROBrequires :
- A unique, gap-free numeric ID in FX.h
- Updating
MODE_COUNTtohighest_ID + 1Please cross-check to prevent array-bounds errors (see team learning about
MODE_COUNT).
10731-10734: Same underflow risk for 1-D branch.Use a signed check (
if (current_index >= SEGLEN) break;) or break whenstart_index + i_offsetexceeds length to avoid needless looping.⛔ Skipped due to learnings
Learnt from: blazoncek PR: wled/WLED#4658 File: wled00/led.cpp:90-91 Timestamp: 2025-04-24T09:31:06.879Z Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs by falling back to the main segment ID, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.Learnt from: blazoncek PR: wled/WLED#4658 File: wled00/led.cpp:90-91 Timestamp: 2025-04-24T09:31:06.879Z Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.Learnt from: blazoncek PR: wled/WLED#4658 File: wled00/led.cpp:90-91 Timestamp: 2025-04-24T09:31:06.879Z Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.Learnt from: blazoncek PR: wled/WLED#4667 File: usermods/user_fx/user_fx.cpp:27-30 Timestamp: 2025-04-30T05:41:03.633Z Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
| bool samplePeak = *(bool*)um_data->u_data[3]; | ||
| int volumeRaw = *(int16_t*)um_data->u_data[1]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid raw pointer casts for audio flags.
*(bool*)um_data->u_data[3] assumes the pointer is valid and correctly aligned for a bool, which isn’t guaranteed across platforms.
If um_data already provides strongly-typed accessors (e.g. um_data->samplePeak or similar), use those instead; otherwise add a helper that hides the cast in one place.
🤖 Prompt for AI Agents
In wled00/FX.cpp around lines 10654 to 10655, avoid using raw pointer casts like
*(bool*)um_data->u_data[3] to access audio flags because it assumes pointer
validity and alignment that may not hold. Instead, check if um_data offers a
strongly-typed accessor for samplePeak and volumeRaw and use those. If not,
create a helper function that safely encapsulates the cast and access in one
place to improve safety and readability.
There was a problem hiding this comment.
um_data is not strongly typed as different usermods can send different data. Sadly we can't return a strongly typed result from getAudioData as the AudioReactive usermod might not be included in the build and so dummy data is returned for effects that expect audio
There was a problem hiding this comment.
@netmindz Thanks for the context. Agreed — since um_data is intentionally untyped (and can be simulated), we can’t introduce a strongly typed accessor.
Small, safe tweak we can still do to avoid aliasing through a bool pointer: read the documented byte and then convert to bool. That keeps the loose typing but eliminates potential alignment/aliasing pitfalls and matches the mapping comment in FX.cpp (samplePeak is uint8_t at u_data[3]).
Example minimal change in the new effects:
// before
bool samplePeak = *(bool*)um_data->u_data[3];
// after (safer and aligned with documented mapping)
bool samplePeak = (*(uint8_t*)um_data->u_data[3]) != 0;Optionally, we can centralize this in tiny helpers near getAudioData() to keep casts in one place (still generic, no strong typing):
static inline bool ar_samplePeak(const um_data_t* d) {
return (*(uint8_t*)d->u_data[3]) != 0;
}
static inline int16_t ar_volumeRaw(const um_data_t* d) {
return *(int16_t*)d->u_data[1];
}
// usage
bool samplePeak = ar_samplePeak(um_data);
int volumeRaw = ar_volumeRaw(um_data);If you’re OK with it, I’ll suggest the minimal bool change across mode_music_strobe and mode_flame_jet to stay consistent with the file’s mapping.
✏️ Learnings added
Learnt from: netmindz
PR: wled/WLED#4819
File: wled00/FX.cpp:10654-10655
Timestamp: 2025-08-08T17:22:37.352Z
Learning: In WLED FX.cpp, um_data returned by getAudioData() is intentionally untyped because different usermods provide different payloads, and simulateSound() may supply dummy data when the AudioReactive usermod isn’t built. Strongly typed accessors are not feasible; prefer explicit casts (e.g., samplePeak as uint8_t at u_data[3]) or small helpers that encapsulate these casts.
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.
🧠 Learnings used
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: DedeHai
PR: wled/WLED#4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.652Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
Learnt from: blazoncek
PR: wled/WLED#4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
There was a problem hiding this comment.
those helper methods would certainly help code readability, but it does mean adding ar_ functions into the main codebase that relate to a usermod, so you might as well at that stage add strongly typed return of getAudioData as both is adding AR specific code into the general codebase
There was a problem hiding this comment.
@netmindz Totally fair. Let’s keep core clean and avoid AR-specific helpers.
I’ll switch the samplePeak read to the safer inline form (aligned with the documented mapping) and leave everything else as-is:
// mode_music_strobe
um_data_t *um_data = getAudioData();
if (!um_data) return FRAMETIME;
bool samplePeak = (*(uint8_t*)um_data->u_data[3]) != 0;
int volumeRaw = *(int16_t*)um_data->u_data[1];// mode_flame_jet
um_data_t *um_data = getAudioData();
if (!um_data) return FRAMETIME;
bool samplePeak = (*(uint8_t*)um_data->u_data[3]) != 0;
int volumeRaw = *(int16_t*)um_data->u_data[1];This avoids aliasing through a bool pointer, preserves the untyped contract of um_data, and doesn’t introduce any AR-specific helpers into the codebase.
✏️ Learnings added
Learnt from: netmindz
PR: wled/WLED#4819
File: wled00/FX.cpp:10654-10655
Timestamp: 2025-08-08T17:31:47.296Z
Learning: In WLED core, avoid introducing AR-specific helper wrappers for um_data access. Keep um_data untyped and, when reading samplePeak, prefer `(*(uint8_t*)um_data->u_data[3]) != 0` over `*(bool*)` to avoid alignment/aliasing issues, while staying decoupled from the AudioReactive usermod.
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.
🧠 Learnings used
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
PR: wled/WLED#4819
File: wled00/FX.cpp:10654-10655
Timestamp: 2025-08-08T17:22:37.352Z
Learning: In WLED FX.cpp, um_data returned by getAudioData() is intentionally untyped because different usermods provide different payloads, and simulateSound() may supply dummy data when the AudioReactive usermod isn’t built. Strongly typed accessors are not feasible; prefer explicit casts (e.g., samplePeak as uint8_t at u_data[3]) or small helpers that encapsulate these casts.
Learnt from: DedeHai
PR: wled/WLED#4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.652Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
Learnt from: blazoncek
PR: wled/WLED#4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Learnt from: KrX3D
PR: wled/WLED#4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-136
Timestamp: 2025-04-27T09:37:28.415Z
Learning: Using PROGMEM for the seven-segment font array (umSSDRNumbers) in the WLED SSDR usermod causes compilation problems, so it should be left as a regular array.
Learnt from: willmmiles
PR: wled/WLED#4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
| for (uint16_t y_offset = 0; y_offset < SEGENV.aux0; y_offset++) { | ||
| uint16_t current_y = start_y - y_offset; | ||
| if (current_y >= SEG_H) continue; // Boundary check | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential unsigned underflow when current_y wraps.
When y_offset > start_y, current_y = start_y - y_offset wraps to 65 535, the subsequent >= SEG_H check skips the pixel but the loop keeps iterating. Break early to avoid wasted iterations:
if (y_offset > start_y) break; // prevents under-flow🤖 Prompt for AI Agents
In wled00/FX.cpp around lines 10702 to 10705, the calculation of current_y as
start_y minus y_offset can cause unsigned underflow when y_offset exceeds
start_y, resulting in a large value that passes the boundary check but wastes
iterations. To fix this, add a condition to break the loop early if y_offset is
greater than start_y, preventing the underflow and unnecessary loop iterations.
netmindz
changed the title
|
I think I follow the basic idea of the effects, but at the moment they feel like they might need a little more refinement, especially the strobe. You might be better looking at only certain frequencies rather than total volume, plus you might need to do things like compare against a running average, you might also possibly find the zero crossing data useful (think this is possibly only in WLED-MM at the moment) |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
wled00/FX.cpp (3)
10654-10655: Avoid aliasing through bool pointer when reading samplePeak.Casting
u_data[3]tobool*risks alignment/aliasing issues. The file’s AR mapping treats samplePeak as a byte; use a uint8_t read and compare to zero.- bool samplePeak = *(bool*)um_data->u_data[3]; + bool samplePeak = (*(uint8_t*)um_data->u_data[3]) != 0;Rationale: matches the documented mapping at the top of this file and avoids strict-aliasing pitfalls while keeping um_data untyped.
10679-10681: Avoid aliasing through bool pointer when reading samplePeak.Same concern as in Music Strobe. Please read the byte and compare to zero.
- bool samplePeak = *(bool*)um_data->u_data[3]; + bool samplePeak = (*(uint8_t*)um_data->u_data[3]) != 0;
10702-10705: Unsigned underflow when y_offset > start_y (wastes iterations).
current_y = start_y - y_offsetunderflows once y_offset exceeds start_y, thencurrent_y >= SEG_His true and the loop keeps iterating doing nothing.- for (uint16_t y_offset = 0; y_offset < SEGENV.aux0; y_offset++) { - uint16_t current_y = start_y - y_offset; - if (current_y >= SEG_H) continue; // Boundary check + for (uint16_t y_offset = 0; y_offset < SEGENV.aux0; y_offset++) { + if (y_offset > start_y) break; // prevent underflow and wasted iterations + uint16_t current_y = start_y - y_offset;This avoids wraparound and unnecessary loop work once the bottom row is exceeded.
🧹 Nitpick comments (7)
platformio.ini (1)
17-17: Use a portable path for build_cache_dir or move to overrideUsing "~" in paths is not reliably portable across all OS/CI shells. Prefer ${sysenv.HOME}/.buildcache (PlatformIO expands it) or keep this setting in platformio_override.ini so it remains local to developers.
Proposed change:
-build_cache_dir = ~/.buildcache +build_cache_dir = ${sysenv.HOME}/.buildcachewled00/FX.cpp (6)
10648-10648: Nit: fix typo in comment (“beat” not “bit”).“Simple bit strobe” → “Simple beat strobe”.
- Simple bit strobe + Simple beat strobe
10657-10660: Fade mapping may be too coarse (0..20 → 255..235).Mapping speed to 21 fade steps is quite coarse and makes high speeds only subtly different. Consider widening the range (e.g., 0..96) for more perceptible control.
- long mappedSpeed = map(SEGMENT.speed, 0, 255, 0, 20); + long mappedSpeed = map(SEGMENT.speed, 0, 255, 0, 96); uint8_t fadeBy = 255 - mappedSpeed;
10661-10666: Optional: normalize threshold against recent loudness.Using a fixed threshold (intensity) against
volumeRawcan be brittle across setups. Consider a running average (and/or band-limited loudness) to trigger on “above average” peaks rather than absolute values.Implementation direction:
- Maintain a small EMA of
volumeRawin SEGENV.aux fields (or SEGMENT.step).- Trigger when
samplePeak && volumeRaw > avg + offset.- Optionally support band selection via
SEGMENT.custom1/custom2in future.
10682-10685: Potentially confusing “Trail Length” mapping to fade strength.Metadata labels intensity as “Trail Length” while code maps intensity directly to fade strength. That’s OK, but trail length grows non-linearly with fade. If users expect roughly linear trail changes, consider a small LUT or a perceptual remap of intensity to fade.
10757-10758: Metadata palette default unused.The effect derives color from SEGCOLOR(0);
pal=35likely won’t affect output. Either:
- Drop
pal=35from metadata, or- Optionally derive base color from palette when no explicit color is set (consistency with other fire-like effects).
10967-10968: Placement: Flame Jet supports 1D and 2D.You registered Flame Jet in the 2D section. That’s fine since the effect detects 1D/2D at runtime. For discoverability, consider a short note or consistent grouping near other audio modes; functionally OK as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
platformio.ini(1 hunks)wled00/FX.cpp(3 hunks)wled00/FX.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/FX.h
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
📚 Learning: 2025-08-08T17:22:37.374Z
Learnt from: netmindz
PR: wled/WLED#4819
File: wled00/FX.cpp:10654-10655
Timestamp: 2025-08-08T17:22:37.374Z
Learning: In WLED FX.cpp, um_data returned by getAudioData() is intentionally untyped because different usermods provide different payloads, and simulateSound() may supply dummy data when the AudioReactive usermod isn’t built. Strongly typed accessors are not feasible; prefer explicit casts (e.g., samplePeak as uint8_t at u_data[3]) or small helpers that encapsulate these casts.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-08-08T17:31:47.330Z
Learnt from: netmindz
PR: wled/WLED#4819
File: wled00/FX.cpp:10654-10655
Timestamp: 2025-08-08T17:31:47.330Z
Learning: In WLED core, avoid introducing AR-specific helper wrappers for um_data access. Keep um_data untyped and, when reading samplePeak, prefer `(*(uint8_t*)um_data->u_data[3]) != 0` over `*(bool*)` to avoid alignment/aliasing issues, while staying decoupled from the AudioReactive usermod.
Applied to files:
wled00/FX.cpp
🧬 Code Graph Analysis (1)
wled00/FX.cpp (2)
usermods/mpu6050_imu/mpu6050_imu.cpp (1)
um_data(124-124)wled00/fcn_declare.h (3)
hw_random8(558-558)hw_random8(559-559)hw_random8(560-560)
🔇 Additional comments (3)
platformio.ini (1)
18-19: LGTM: extra_configs hook is correctly wiredIncluding platformio_override.ini via extra_configs aligns with the guidance above and keeps local overrides out of VCS. No issues.
wled00/FX.cpp (2)
10893-10894: LGTM: Music Strobe correctly registered among 1D audio effects.Registration and metadata look consistent with other audio-reactive modes.
10959-10986: Confirm FX IDs and MODE_COUNT update.Please ensure FX_MODE_MUSIC_STROBE and FX_MODE_FLAME_JET are defined in FX.h and MODE_COUNT is updated to highest ID + 1 (per project rules).
If not already done in this PR: add both constants and bump MODE_COUNT accordingly to avoid selection/index errors.
| @@ -10,7 +10,7 @@ | |||
| # ------------------------------------------------------------------------------ | |||
|
|
|||
| # CI/release binaries | |||
| default_envs = nodemcuv2, esp8266_2m, esp01_1m_full, nodemcuv2_160, esp8266_2m_160, esp01_1m_full_160, nodemcuv2_compat, esp8266_2m_compat, esp01_1m_full_compat, esp32dev, esp32_eth, lolin_s2_mini, esp32c3dev, esp32s3dev_16MB_opi, esp32s3dev_8MB_opi, esp32s3_4M_qspi, esp32_wrover, usermods | |||
| default_envs = nodemcuv2, esp8266_2m, esp01_1m_full, nodemcuv2_160, esp8266_2m_160, esp01_1m_full_160, nodemcuv2_compat, esp8266_2m_compat, esp01_1m_full_compat, esp32dev, esp32dev_V4, esp32_eth, lolin_s2_mini, esp32c3dev, esp32s3dev_16MB_opi, esp32s3dev_8MB_opi, esp32s3_4M_qspi, esp32_wrover, usermods | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Undefined environment in default_envs: esp32dev_V4
"default_envs" includes "esp32dev_V4", but there is no corresponding [env:esp32dev_V4] defined in this file. This will cause PlatformIO to fail when building CI/release binaries. The existing [env:esp32dev] already targets the V4 toolchain (esp32_idf_V4), so "esp32dev_V4" looks like a mistaken/duplicate entry.
Apply this diff to fix:
-default_envs = nodemcuv2, esp8266_2m, esp01_1m_full, nodemcuv2_160, esp8266_2m_160, esp01_1m_full_160, nodemcuv2_compat, esp8266_2m_compat, esp01_1m_full_compat, esp32dev, esp32dev_V4, esp32_eth, lolin_s2_mini, esp32c3dev, esp32s3dev_16MB_opi, esp32s3dev_8MB_opi, esp32s3_4M_qspi, esp32_wrover, usermods
+default_envs = nodemcuv2, esp8266_2m, esp01_1m_full, nodemcuv2_160, esp8266_2m_160, esp01_1m_full_160, nodemcuv2_compat, esp8266_2m_compat, esp01_1m_full_compat, esp32dev, esp32_eth, lolin_s2_mini, esp32c3dev, esp32s3dev_16MB_opi, esp32s3dev_8MB_opi, esp32s3_4M_qspi, esp32_wrover, usermodsRun to confirm there is no such env:
🏁 Script executed:
#!/bin/bash
rg -nP '^\s*\[env:esp32dev_V4\]' platformio.ini || echo "OK: env:esp32dev_V4 not defined (should not be in default_envs)"Length of output: 186
Remove undefined “esp32dev_V4” from default_envs
Confirmed there is no [env:esp32dev_V4] defined in platformio.ini, so including it in default_envs will break CI/builds.
Attention:
- platformio.ini (line 13):
default_envslist
Apply this diff:
-default_envs = nodemcuv2, esp8266_2m, esp01_1m_full, nodemcuv2_160, esp8266_2m_160, esp01_1m_full_160, nodemcuv2_compat, esp8266_2m_compat, esp01_1m_full_compat, esp32dev, esp32dev_V4, esp32_eth, lolin_s2_mini, esp32c3dev, esp32s3dev_16MB_opi, esp32s3dev_8MB_opi, esp32s3_4M_qspi, esp32_wrover, usermods
+default_envs = nodemcuv2, esp8266_2m, esp01_1m_full, nodemcuv2_160, esp8266_2m_160, esp01_1m_full_160, nodemcuv2_compat, esp8266_2m_compat, esp01_1m_full_compat, esp32dev, esp32_eth, lolin_s2_mini, esp32c3dev, esp32s3dev_16MB_opi, esp32s3dev_8MB_opi, esp32s3_4M_qspi, esp32_wrover, usermods🤖 Prompt for AI Agents
In platformio.ini around line 13, the default_envs list includes a non-existent
env `esp32dev_V4` which will break CI/builds; remove `esp32dev_V4` from the
default_envs entry (or alternatively add a matching `[env:esp32dev_V4]` section
if that env is required), ensure commas and spacing remain consistent after
removal, and save the file so CI uses only defined environments.
| for (uint16_t i_offset = 0; i_offset < SEGENV.aux0; i_offset++) { | ||
| uint16_t current_index = start_index + i_offset; | ||
| if (current_index >= SEGLEN) continue; // Boundary check | ||
|
|
||
| uint32_t base_color = SEGCOLOR(0); | ||
| CRGB fastled_color = CRGB(base_color); | ||
|
|
||
| uint8_t heat_reduction = map(i_offset, 0, SEGLEN, 0, 180); | ||
| fastled_color.g = qsub8(fastled_color.g, heat_reduction); | ||
| fastled_color.b = qsub8(fastled_color.b, heat_reduction * 2); | ||
|
|
||
| if (hw_random8() < 50) { | ||
| fastled_color.nscale8(220); | ||
| } | ||
| SEGMENT.setPixelColor(current_index, fastled_color); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Break early when 1D index passes SEGLEN; honor segment direction.
- The loop continues even after
current_indexexceedsSEGLEN, performing useless iterations. Break early to save cycles. - Consider drawing from the configured segment “end” when reversed. Many effects respect
SEGMENT.reverse.
- const uint16_t max_height = SEGLEN;
- uint16_t start_index = 0; // Or SEGLEN - 1 if you want it from the other end
+ const uint16_t max_height = SEGLEN;
+ const bool rev = SEGMENT.reverse;
+ uint16_t start_index = rev ? (SEGLEN - 1) : 0;
- for (uint16_t i_offset = 0; i_offset < SEGENV.aux0; i_offset++) {
- uint16_t current_index = start_index + i_offset;
- if (current_index >= SEGLEN) continue; // Boundary check
+ for (uint16_t i_offset = 0; i_offset < SEGENV.aux0; i_offset++) {
+ uint16_t current_index = rev ? (start_index - i_offset) : (start_index + i_offset);
+ if (current_index >= SEGLEN) break; // Boundary check & early exit📝 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.
| for (uint16_t i_offset = 0; i_offset < SEGENV.aux0; i_offset++) { | |
| uint16_t current_index = start_index + i_offset; | |
| if (current_index >= SEGLEN) continue; // Boundary check | |
| uint32_t base_color = SEGCOLOR(0); | |
| CRGB fastled_color = CRGB(base_color); | |
| uint8_t heat_reduction = map(i_offset, 0, SEGLEN, 0, 180); | |
| fastled_color.g = qsub8(fastled_color.g, heat_reduction); | |
| fastled_color.b = qsub8(fastled_color.b, heat_reduction * 2); | |
| if (hw_random8() < 50) { | |
| fastled_color.nscale8(220); | |
| } | |
| SEGMENT.setPixelColor(current_index, fastled_color); | |
| } | |
| const uint16_t max_height = SEGLEN; | |
| const bool rev = SEGMENT.reverse; | |
| uint16_t start_index = rev ? (SEGLEN - 1) : 0; | |
| for (uint16_t i_offset = 0; i_offset < SEGENV.aux0; i_offset++) { | |
| uint16_t current_index = rev | |
| ? (start_index - i_offset) | |
| : (start_index + i_offset); | |
| if (current_index >= SEGLEN) break; // Boundary check & early exit | |
| uint32_t base_color = SEGCOLOR(0); | |
| CRGB fastled_color = CRGB(base_color); | |
| uint8_t heat_reduction = map(i_offset, 0, SEGLEN, 0, 180); | |
| fastled_color.g = qsub8(fastled_color.g, heat_reduction); | |
| fastled_color.b = qsub8(fastled_color.b, heat_reduction * 2); | |
| if (hw_random8() < 50) { | |
| fastled_color.nscale8(220); | |
| } | |
| SEGMENT.setPixelColor(current_index, fastled_color); | |
| } |
🤖 Prompt for AI Agents
In wled00/FX.cpp around lines 10731 to 10746, the loop currently continues past
the segment end and ignores segment direction; change the logic to break the
loop when the computed 1D index would be out of bounds instead of continuing to
waste cycles, and compute the index using the segment direction (respect
SEGMENT.reverse) so pixels are drawn from the configured segment end when
reversed (e.g., derive current_index from start_index +/- i_offset based on
SEGMENT.reverse, check bounds once and break if out of range, then proceed with
color/heat logic).
Added audio-reactive strobe and flame jet effects.
The strobe effect adjusts the trigger threshold, the fade time, and the palette.
The flame jet effect adjusts the palette, trigger threshold, fade time, and flame height. The effect looks good on cylindrical matrices.
Summary by CodeRabbit
New Features
Improvements