Add new effect for a single EQ bar with peak marker and decay #4760
Conversation
WalkthroughA new audio-reactive LED effect, "Single EQ Bar," was introduced. This effect visualizes a selected FFT frequency bin as a vertical or horizontal bar with a decaying peak indicator, configurable via segment parameters. The implementation includes a new function, effect mode ID, and corresponding metadata. Changes
✨ 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: 5
🧹 Nitpick comments (2)
wled00/FX.cpp (2)
7265-7265: Fix typo in comment.There's a typo in the comment: "varible" should be "variable".
- // User controls. Speed is referenced directly instead of creating a new varible. + // User controls. Speed is referenced directly instead of creating a new variable.
7268-7271: Simplify peak decay calculation.The peak decay mapping logic is overly complex and could be simplified for better readability.
if (peakDecay > 0){ - // Grab the intensity value for peak decay speed, bound it between 1 and 32, invert for ease of use. - peakDecay = map(255 - SEGMENT.intensity, 1, 255, 1, 32); + // Map intensity to peak decay speed (higher intensity = faster decay) + peakDecay = map(SEGMENT.intensity, 1, 255, 32, 1); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/FX.cpp(2 hunks)wled00/FX.h(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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: 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.
wled00/FX.h (2)
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.
wled00/FX.cpp (3)
undefined
<retrieved_learning>
Learnt from: netmindz
PR: #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.
</retrieved_learning>
<retrieved_learning>
Learnt from: netmindz
PR: #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.
</retrieved_learning>
<retrieved_learning>
Learnt from: DedeHai
PR: #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.
</retrieved_learning>
🧬 Code Graph Analysis (1)
wled00/FX.cpp (1)
wled00/FX.h (16)
i(540-540)i(540-540)i(541-541)i(541-541)i(655-655)i(655-655)i(693-693)i(693-693)i(695-695)i(696-696)i(696-696)i(697-697)i(697-697)i(699-699)i(700-700)i(892-892)
🔇 Additional comments (2)
wled00/FX.cpp (2)
7335-7335: Verify effect metadata string format.The metadata string should be validated to ensure it follows WLED's expected format for effect parameters and UI rendering.
Please verify that the metadata string format matches WLED's conventions and that all parameters are correctly defined.
7906-7906: Confirm effect registration placement.The effect registration is correctly placed in the initialization sequence.
| #define MODE_COUNT 218 | ||
| #define FX_MODE_SINGLE_EQBAR 219 | ||
|
|
There was a problem hiding this comment.
MODE_COUNT is now off-by-one – update to reflect the new effect ID
MODE_COUNT must always be highest FX_MODE_ value + 1 (see long-term learning).
With FX_MODE_SINGLE_EQBAR added as 219, MODE_COUNT should be 220; leaving it at 218 will:
- truncate the
_mode/_modeDatavectors (they’re pre-reserved with_modeCount = MODE_COUNT) - make array-index checks wrong and silently skip registering the last two modes
- open up latent out-of-bounds writes if code assumes
id < MODE_COUNT.
Suggested fix:
-#define MODE_COUNT 218
+#define MODE_COUNT 220 // 0 … 219Also confirm that any build-time assertions or size-dependent allocations relying on MODE_COUNT are re-compiled.
🤖 Prompt for AI Agents
In wled00/FX.h around lines 376 to 378, the MODE_COUNT macro is incorrectly set
to 218 while the highest FX_MODE_ value is now 219 with FX_MODE_SINGLE_EQBAR.
Update MODE_COUNT to 220 to be one greater than the highest FX_MODE_ value. This
ensures correct sizing of _mode and _modeData vectors and prevents out-of-bounds
errors. Also verify that any build-time assertions or allocations depending on
MODE_COUNT are recompiled to reflect this change.
| if (*prevBarHeight < SEGLEN) { | ||
|
|
||
| if (is2d) { | ||
| for (int j = 0; j < SEG_W; j++) { | ||
| SEGMENT.setPixelColorXY(j, *prevBarHeight + 1, BLACK); // clear next pixel immediately. | ||
| } | ||
| } else { | ||
| SEGMENT.setPixelColor(*prevBarHeight + 1, BLACK); // clear next pixel immediately. | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix inconsistent bounds checking for 2D vs 1D modes.
The bounds check uses SEGLEN for both 1D and 2D modes, but 2D mode should use SEG_H for vertical bars.
- if (*prevBarHeight < SEGLEN) {
+ if (*prevBarHeight < (is2d ? SEG_H - 1 : SEGLEN - 1)) {
if (is2d) {
for (int j = 0; j < SEG_W; j++) {
SEGMENT.setPixelColorXY(j, *prevBarHeight + 1, BLACK); // clear next pixel immediately.
}
} else {
SEGMENT.setPixelColor(*prevBarHeight + 1, BLACK); // clear next pixel immediately.
}
}📝 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.
| if (*prevBarHeight < SEGLEN) { | |
| if (is2d) { | |
| for (int j = 0; j < SEG_W; j++) { | |
| SEGMENT.setPixelColorXY(j, *prevBarHeight + 1, BLACK); // clear next pixel immediately. | |
| } | |
| } else { | |
| SEGMENT.setPixelColor(*prevBarHeight + 1, BLACK); // clear next pixel immediately. | |
| } | |
| } | |
| if (*prevBarHeight < (is2d ? SEG_H - 1 : SEGLEN - 1)) { | |
| if (is2d) { | |
| for (int j = 0; j < SEG_W; j++) { | |
| SEGMENT.setPixelColorXY(j, *prevBarHeight + 1, BLACK); // clear next pixel immediately. | |
| } | |
| } else { | |
| SEGMENT.setPixelColor(*prevBarHeight + 1, BLACK); // clear next pixel immediately. | |
| } | |
| } |
🤖 Prompt for AI Agents
In wled00/FX.cpp around lines 7321 to 7330, the bounds check incorrectly uses
SEGLEN for both 1D and 2D modes. Update the condition to check against SEG_H
when in 2D mode to correctly limit vertical bar height, while keeping SEGLEN for
1D mode. This ensures proper bounds checking depending on the mode.
| if (*prevBarHeight > 0) { | ||
|
|
||
| if (is2d) { | ||
| for (int j = 0; j < SEG_W; j++) { | ||
| SEGMENT.setPixelColorXY(j, *prevBarHeight, SEGCOLOR(2)); | ||
| } | ||
| } else { | ||
| SEGMENT.setPixelColor(*prevBarHeight, SEGCOLOR(2)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add bounds checking for peak pixel drawing.
The peak pixel is drawn at *prevBarHeight without bounds checking, which could cause out-of-bounds access.
// Set peak pixel and clear pixels over peak (otherwise they would fadewith value from speed slider)
- if (*prevBarHeight > 0) {
+ if (*prevBarHeight > 0 && *prevBarHeight < (is2d ? SEG_H : SEGLEN)) {
if (is2d) {
for (int j = 0; j < SEG_W; j++) {
SEGMENT.setPixelColorXY(j, *prevBarHeight, SEGCOLOR(2));
}
} else {
SEGMENT.setPixelColor(*prevBarHeight, SEGCOLOR(2));
}
}📝 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.
| if (*prevBarHeight > 0) { | |
| if (is2d) { | |
| for (int j = 0; j < SEG_W; j++) { | |
| SEGMENT.setPixelColorXY(j, *prevBarHeight, SEGCOLOR(2)); | |
| } | |
| } else { | |
| SEGMENT.setPixelColor(*prevBarHeight, SEGCOLOR(2)); | |
| } | |
| } | |
| // Set peak pixel and clear pixels over peak (otherwise they would fadewith value from speed slider) | |
| if (*prevBarHeight > 0 && *prevBarHeight < (is2d ? SEG_H : SEGLEN)) { | |
| if (is2d) { | |
| for (int j = 0; j < SEG_W; j++) { | |
| SEGMENT.setPixelColorXY(j, *prevBarHeight, SEGCOLOR(2)); | |
| } | |
| } else { | |
| SEGMENT.setPixelColor(*prevBarHeight, SEGCOLOR(2)); | |
| } | |
| } |
🤖 Prompt for AI Agents
In wled00/FX.cpp around lines 7311 to 7320, the code draws a peak pixel at
*prevBarHeight without verifying if this value is within valid bounds. Add
bounds checking to ensure *prevBarHeight is within the allowed pixel range
before calling setPixelColorXY or setPixelColor to prevent out-of-bounds access
errors.
| bool is2d = (strip.isMatrix || SEGMENT.is2D()); | ||
|
|
||
| // User controls. Speed is referenced directly instead of creating a new varible. | ||
| uint8_t freqBin = map(SEGMENT.custom1, 0, 255, 0, 15); // 0-15 (or up to available bins) |
There was a problem hiding this comment.
Add bounds checking for frequency bin access.
The frequency bin mapping doesn't ensure the selected bin is within the available FFT data range, which could lead to out-of-bounds access.
- uint8_t freqBin = map(SEGMENT.custom1, 0, 255, 0, 15); // 0-15 (or up to available bins)
+ uint8_t freqBin = map(SEGMENT.custom1, 0, 255, 0, min(15, um_data->u_size[2] - 1)); // 0-15 (or up to available bins)🤖 Prompt for AI Agents
In wled00/FX.cpp at line 7266, the frequency bin calculated by mapping
SEGMENT.custom1 to a range 0-15 may exceed the actual available FFT bins,
risking out-of-bounds access. Add bounds checking after the map call to clamp
freqBin within the valid range of available FFT bins before using it.
| if (barHeight > 0) { | ||
| if (is2d) { | ||
| for (int j = 0; j < SEG_W; j++) { | ||
| SEGMENT.setPixelColorXY(j, barHeight, SEGMENT.color_from_palette(barHeight, true, PALETTE_SOLID_WRAP, 0)); | ||
| } | ||
| } else { | ||
| SEGMENT.setPixelColor(barHeight, SEGMENT.color_from_palette(barHeight, true, PALETTE_SOLID_WRAP, 0)); | ||
| } |
There was a problem hiding this comment.
Fix potential out-of-bounds access for peak pixel.
When drawing the peak pixel at barHeight, there's no bounds checking to ensure it's within the segment dimensions.
if (peakDecay == 0){
// No peak pixel if decay is set to zero, just draw the peak pixel of the bar.
- if (barHeight > 0) {
+ if (barHeight > 0 && barHeight < (is2d ? SEG_H : SEGLEN)) {
if (is2d) {
for (int j = 0; j < SEG_W; j++) {
SEGMENT.setPixelColorXY(j, barHeight, SEGMENT.color_from_palette(barHeight, true, PALETTE_SOLID_WRAP, 0));
}
} else {
SEGMENT.setPixelColor(barHeight, SEGMENT.color_from_palette(barHeight, true, PALETTE_SOLID_WRAP, 0));
}
}
}📝 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.
| if (barHeight > 0) { | |
| if (is2d) { | |
| for (int j = 0; j < SEG_W; j++) { | |
| SEGMENT.setPixelColorXY(j, barHeight, SEGMENT.color_from_palette(barHeight, true, PALETTE_SOLID_WRAP, 0)); | |
| } | |
| } else { | |
| SEGMENT.setPixelColor(barHeight, SEGMENT.color_from_palette(barHeight, true, PALETTE_SOLID_WRAP, 0)); | |
| } | |
| if (peakDecay == 0){ | |
| // No peak pixel if decay is set to zero, just draw the peak pixel of the bar. | |
| if (barHeight > 0 && barHeight < (is2d ? SEG_H : SEGLEN)) { | |
| if (is2d) { | |
| for (int j = 0; j < SEG_W; j++) { | |
| SEGMENT.setPixelColorXY(j, barHeight, | |
| SEGMENT.color_from_palette(barHeight, true, PALETTE_SOLID_WRAP, 0)); | |
| } | |
| } else { | |
| SEGMENT.setPixelColor(barHeight, | |
| SEGMENT.color_from_palette(barHeight, true, PALETTE_SOLID_WRAP, 0)); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In wled00/FX.cpp around lines 7297 to 7304, the code sets the pixel color at
barHeight without verifying if barHeight is within valid segment bounds. Add
bounds checking to ensure barHeight is less than SEG_W for 2D mode and less than
the segment length for 1D mode before setting the pixel color to prevent
out-of-bounds access.
|
While I agree with the general sentiment that we don't want a new effect that is just a variant of an existing one, I'm unclear @blazoncek as to how we could add this new behaviour to the existing effect. Could you please elaborate on how the user would achieve this effect using only parameters |
|
|
So a slider that maps custom3 to which FFT to use, only if custom1 is sat to 1 otherwise has no affect? |
|
Yes, that's what was on my mind. |
|
If you save a preset, then a WLED upgrade adds an extra param to get effect, what value is used for that param when the preset loads @blazoncek ? 0, Max or the default value set for that effect? If would be nicer if the new slider selected the first (or maybe last) FFT value to use, rather than only working if the bar count is 1 However I'm keen not to break anyone's existing presets |
|
Depends on the "default" values specified in effect metadata. If there are none, value is not modified. It may break only rare case that user only has width/# of bands == 1 and custom3 from previous preset is used (selecting random bin, no default value). |
|
U assume by single segment you mean 1D, it works fine on a single 2D segment. I can check if I can come up with something. |
|
Please continue any further discussion on #4764 |
Currently there is not anything that can duplicate the geq-style functionality for a selected frequency bin on a single segment - so I wrote one.
Summary by CodeRabbit