improved & refactored Android FX #4522
Conversation
- returns FRAMETIME -> no more flickering in transitions and overlay - no more double-painting of pixels
DedeHai
added
effect
optimization
|
I would like someone to confirm it still looks the same as before. |
|
Maybe worth mentioning testing required on discord if you haven't done already? |
|
I did test it, I even compared the variable contents in excel and it was identical, but I did only test up to 256 pixels. I am pretty sure it is identical but there may be edge cases... |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis change introduces multiple new LED effect modes to the WS2812FX class. The new methods implement a variety of visual patterns and animations, such as static patterns with alternating colors, plasma effects with rotation, simulated TV sunrise/sunset, multi-candle flickering, and several 2D visual effects including spaceships, crazy bees, ghost rider, and more. Each method is added as a public entity in the WS2812FX class within the FX.cpp file. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Application
participant WS2812FX
User->>Application: Select LED effect (e.g., mode_flow)
Application->>WS2812FX: Invoke effect method (mode_flow)
WS2812FX->>WS2812FX: Execute effect logic based on new mode
WS2812FX-->>Application: Return status/confirmation
Application-->>User: Update applied effect
✨ Finishing Touches
🪧 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: 1
🔭 Outside diff range comments (4)
wled00/FX.cpp (4)
1401-1408: Potential off-by-one error in Two Dots effect.The effect uses
SEGLEN-1for indexing but doesn't validate thatindexRandindexBare within bounds.Add bounds checking to prevent potential buffer overflows:
- SEGMENT.setPixelColor(indexR, color1); - SEGMENT.setPixelColor(indexB, color2); + if (indexR < SEGLEN) SEGMENT.setPixelColor(indexR, color1); + if (indexB < SEGLEN) SEGMENT.setPixelColor(indexB, color2);
2069-2154: Fire2012 effect needs optimization for memory usage.The effect allocates memory for each virtual strip which could be excessive for large matrices.
Consider:
- Using a single buffer and recomputing for each strip
- Adding bounds checking for array accesses
- Optimizing memory allocation by reusing buffers
- if (!SEGENV.allocateData(dataSize * strips)) return mode_static(); + if (!SEGENV.allocateData(dataSize)) return mode_static();
6333-6397: 2D Swirl effect needs better error handling.The effect doesn't properly handle allocation failures or invalid dimensions.
Add proper error handling:
+ if (cols < 2 || rows < 2) return mode_static(); + if (!SEGENV.allocateData(sizeof(float) * 3)) return mode_static();
7659-7678: Potential issue in effect addition logic.The
addEffect()function could allow duplicate effect IDs under certain conditions.Add additional checks:
uint8_t WS2812FX::addEffect(uint8_t id, mode_ptr mode_fn, const char *mode_name) { + if (id >= 255) return 255; // Invalid ID + if (mode_fn == nullptr || mode_name == nullptr) return 255; // Invalid parameters if (id == 255) { for (size_t i=1; i<_mode.size(); i++) if (_modeData[i] == _data_RESERVED) { id = i; break; } }
🧹 Nitpick comments (2)
wled00/FX.cpp (2)
3401-3495: Potential performance bottleneck in 2D Blobs effect.The effect performs many floating point calculations per frame which could be slow on some hardware.
Consider:
- Using fixed-point math instead of floating point
- Pre-calculating common values
- Reducing the number of trigonometric calculations
Example optimization:
// Pre-calculate sine/cosine values const float sinTable[256]; const float cosTable[256]; // Use lookup tables instead of direct calculations float sin_val = sinTable[angle & 0xFF];
7679-7897: Effect setup needs optimization for memory usage.The
setupEffectData()function allocates a lot of memory upfront which could be problematic on memory-constrained devices.Consider:
- Lazy loading of effects
- Dynamic allocation of effect data
- Grouping similar effects to share resources
Consider implementing a plugin system to allow:
- Dynamic loading/unloading of effects
- Better memory management
- Easier addition of new effects
| /* | ||
| * Android loading circle | ||
| * Android loading circle, refactored by @dedehai | ||
| */ | ||
| uint16_t mode_android(void) { | ||
|
|
||
| if (!SEGENV.allocateData(sizeof(uint32_t))) return mode_static(); | ||
| uint32_t* counter = reinterpret_cast<uint32_t*>(SEGENV.data); | ||
| unsigned size = SEGENV.aux1 >> 1; // upper 15 bit | ||
| unsigned shrinking = SEGENV.aux1 & 0x01; // lowest bit | ||
| if(strip.now >= SEGENV.step) { | ||
| SEGENV.step = strip.now + 3 + ((8 * (uint32_t)(255 - SEGMENT.speed)) / SEGLEN); | ||
| if (size > (SEGMENT.intensity * SEGLEN) / 255) | ||
| shrinking = 1; | ||
| else if (size < 2) | ||
| shrinking = 0; | ||
| if (!shrinking) { // growing | ||
| if ((*counter % 3) == 1) | ||
| SEGENV.aux0++; // advance start position | ||
| else | ||
| size++; | ||
| } else { // shrinking | ||
| SEGENV.aux0++; | ||
| if ((*counter % 3) != 1) | ||
| size--; | ||
| } | ||
| SEGENV.aux1 = size << 1 | shrinking; // save back | ||
| (*counter)++; | ||
| if (SEGENV.aux0 >= SEGLEN) SEGENV.aux0 = 0; | ||
| } | ||
| uint32_t start = SEGENV.aux0; | ||
| uint32_t end = (SEGENV.aux0 + size) % SEGLEN; | ||
| for (unsigned i = 0; i < SEGLEN; i++) { | ||
| SEGMENT.setPixelColor(i, SEGMENT.color_from_palette(i, true, PALETTE_SOLID_WRAP, 1)); | ||
| } | ||
|
|
||
| if (SEGENV.aux1 > (SEGMENT.intensity*SEGLEN)/255) | ||
| { | ||
| SEGENV.aux0 = 1; | ||
| } else | ||
| { | ||
| if (SEGENV.aux1 < 2) SEGENV.aux0 = 0; | ||
| } | ||
|
|
||
| unsigned a = SEGENV.step & 0xFFFFU; | ||
|
|
||
| if (SEGENV.aux0 == 0) | ||
| { | ||
| if (SEGENV.call %3 == 1) {a++;} | ||
| else {SEGENV.aux1++;} | ||
| } else | ||
| { | ||
| a++; | ||
| if (SEGENV.call %3 != 1) SEGENV.aux1--; | ||
| } | ||
|
|
||
| if (a >= SEGLEN) a = 0; | ||
|
|
||
| if (a + SEGENV.aux1 < SEGLEN) | ||
| { | ||
| for (unsigned i = a; i < a+SEGENV.aux1; i++) { | ||
| SEGMENT.setPixelColor(i, SEGCOLOR(0)); | ||
| } | ||
| } else | ||
| { | ||
| for (unsigned i = a; i < SEGLEN; i++) { | ||
| SEGMENT.setPixelColor(i, SEGCOLOR(0)); | ||
| } | ||
| for (unsigned i = 0; i < SEGENV.aux1 - (SEGLEN -a); i++) { | ||
| if ((start < end && i >= start && i < end) || (start >= end && (i >= start || i < end))) | ||
| SEGMENT.setPixelColor(i, SEGCOLOR(0)); | ||
| } | ||
| else | ||
| SEGMENT.setPixelColor(i, SEGMENT.color_from_palette(i, true, PALETTE_SOLID_WRAP, 1)); | ||
| } | ||
| SEGENV.step = a; | ||
|
|
||
| return 3 + ((8 * (uint32_t)(255 - SEGMENT.speed)) / SEGLEN); | ||
| return FRAMETIME; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improved Android loading circle effect with better memory management and cleaner code.
The refactored Android loading circle effect now:
- Uses proper memory allocation with
allocateData() - Stores state in a single 32-bit counter variable
- Improves readability by using clearer variable names and logic
- Fixes potential memory issues by properly handling allocation failures
Consider adding bounds checking for size to prevent potential buffer overflows:
- if (size > (SEGMENT.intensity * SEGLEN) / 255)
+ if (size > min((SEGMENT.intensity * SEGLEN) / 255, SEGLEN/2))
Summary by CodeRabbit