Conversation
- copied relevant functions - optimized some of them for ESP32
- new rainbow function is faster and more accurate than original fastled function
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR removes the FastLED dependency and introduces a lightweight FastLED-slim implementation, refactors color and palette APIs to CRGB/CRGBW/CHSV types and spectrum-aware conversions, adds a PRNG and beat helpers, and updates FX, particle, IR, utils, and several usermods to use the new color/prng infrastructure. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
wled00/FX.cpp (2)
72-73: Global PRNG usage.
Declaring PRNG as a global instance is consistent with the new random approach. However, confirm whether concurrency or reproducibility concerns require a more localized scope or synchronization.
4923-4923: Commented-out call tosetPixelColorXY.
If this line is no longer needed, consider removing it to avoid confusion.wled00/colors.h (3)
96-147: Consider initializing the default CHSV constructor.
CHSVwarns the default constructor leaves fields uninitialized, which can produce undefined hue/sat/value if not set. If unintentional, consider a safe default.- inline CHSV() __attribute__((always_inline)) = default; + inline CHSV() __attribute__((always_inline)) : h(0), s(0), v(0) { }
218-314: Extensive operator coverage.
Multiple arithmetic operators onCRGBsignificantly improve usability. However, they can be numerous for embedded environments. Ensure all these operators are covered by unit tests to prevent regressions.Would you like help drafting unit tests for these operators?
811-899: Inconsistent conversion methods (rainbow vs. spectrum).
CRGBW(CHSV32)andCRGBW(CHSV)usehsv2rgb_rainbow, whereasadjust_hue()callshsv2rgb_spectrum. Verify this is intended, as "rainbow" and "spectrum" produce visually different hue mappings.Consider unifying or clarifying these behaviors to avoid unexpected color shifts.
wled00/fcn_declare.h (3)
373-374: Legacy alias check.
#define inoise8 perlin8and#define inoise16 perlin16help maintain compatibility with old FastLED function calls. Confirm these aliases are still needed long-term.
411-419: Perlin noise API.
The new perlin noise functions (perlin1D_raw,perlin2D_raw,perlin3D_raw,perlin16,perlin8) significantly expand effect possibilities. They look correct at a glance; consider thorough performance testing if used intensively in tight loops.
522-544: Pseudo-Random Number Generator class.
PRNGhelps replicate old FastLED behavior. It is not thread-safe, but typically that’s acceptable in single-thread microcontroller contexts. If concurrency is planned, consider synchronization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wled00/FX.cpp(22 hunks)wled00/colors.h(1 hunks)wled00/fcn_declare.h(6 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
wled00/colors.h (1)
wled00/colors.cpp (18)
color_blend(20-30)color_blend(20-20)fill_solid_RGB(732-736)fill_solid_RGB(732-732)fill_gradient_RGB(739-769)fill_gradient_RGB(739-739)fill_gradient_RGB(771-774)fill_gradient_RGB(771-771)fill_gradient_RGB(776-781)fill_gradient_RGB(776-776)fill_gradient_RGB(783-790)fill_gradient_RGB(783-783)hsv2rgb_spectrum(254-254)hsv2rgb_spectrum(303-308)hsv2rgb_spectrum(303-303)rgb2hsv(418-418)rgb2hsv(440-444)rgb2hsv(440-440)
wled00/FX.cpp (3)
wled00/colors.h (8)
color(55-55)color(872-872)color(872-872)r(405-409)r(421-423)r(896-898)color_fade(66-66)color_blend(63-63)wled00/FX.h (40)
i(613-613)i(613-613)i(661-661)i(666-666)i(667-667)i(667-667)i(668-668)i(668-668)i(673-673)i(674-674)i(935-935)r(679-679)b(627-627)c(678-678)c(745-745)c(745-745)c(882-882)c(882-882)col(769-769)col(769-769)t(879-879)t(879-879)t(885-885)t(885-885)n(588-588)n(588-588)n(623-623)n(662-662)n(662-662)n(663-663)n(663-663)n(664-664)n(664-664)n(682-682)n(682-682)n(683-683)n(683-683)n(684-684)n(684-684)n(685-685)wled00/wled_math.cpp (4)
sin8_t(82-86)sin8_t(82-82)cos8_t(88-90)cos8_t(88-88)
🔇 Additional comments (28)
wled00/FX.cpp (18)
16-16: Includecolors.his aligned with the new color management.
No immediate issues with this import, and it supports the PR objective of replacing FastLED.
1760-1783: Re-seeding PRNG for effect consistency.
Storing the old seed inprevSeedand restoring it after rendering ensures predictable random output for this effect. Verify thatunsignedhas sufficient bits for the seed (consideruint32_tinstead) ifprng.getSeed()returns 32 bits.
2546-2546: Newtwinklefox_one_twinklefunction.
The function name and parameters are clear for a special twinkle effect. No immediate concerns.
2581-2581: Setting up local CRGBW.
Straightforward variable declaration; no issues noticed with memory or usage.
2596-2596: Assigning black color.
Settingc = 0is a valid approach to clear color. Looks fine.
2619-2626: Dynamically fading background color.
Logic for scaling based onbglightis consistent. No apparent issues withcolor_fadeusage.
4147-4157: Seed temporarily set to 535.
This ensures repeatable randomness inmode_twinkleup. The finalprng.setSeed(prevSeed)properly restores the original seed. No immediate concerns.
4179-4179: Mix of hardware and pseudo-random.
Usinghw_random8(64)differs from the new PRNG approach. If reproducibility is needed, consider the pinned PRNG. Otherwise, this is fine for true randomness.
5698-5698: Perlin calculation.
Shifting the noise value by 2 to produce a ~[-32, 32] range is valid. No issues found.
5825-5827: Target coordinates useprng.random8(0, w)andprng.random8(0, h).
Ensurewandhstay within 255 if the distribution must span the entire range. Otherwise, re-check logic for larger dimensions.
5840-5843: Seeding PRNG withstrip.now.
Using the current time as a seed repeats the approach seen elsewhere, ensuring variation. No immediate issues.
6014-6014: Incrementing color by 4.
Steady color shifting is an effective approach for subtle hue changes. No concerns.
7510-7510: Blending withnscale8andease8InOutCubic.
This is standard functionality to produce smooth transitions. Looks correct.
7819-7819: Hybrid approach withhw_random16(SEGMENT.custom1 >> 3).
Allows a random range for vertical velocity. The usage is consistent with the new 2D particle code.
7863-7863: Random explosion shape.
Leveraginghw_random() & 1is a neat trick for a 50% chance of a circular explosion. No issues spotted.
7889-7889: [Duplicate of previous comment regarding clipped saturation]
If the intention is a random saturation up to 150,min((uint16_t)150, hw_random16())nearly always becomes 150 becausehw_random16()can be up to 65535. Use a narrower range to avoid clipping.
9724-9724: Gravity offset arithmetic.
(int16_t)cos8_t(...) - 128properly centers gravity around 0. No immediate issues.
10051-10051: Calculating mid-frequency loudness.
Summation followed bysqrt32_bwis straightforward. Implementation looks fine.wled00/colors.h (5)
6-11: Confirm correct macro usage for 32-bit color extraction.
These macros for packing and unpacking 32-bit color components appear logically sound. The casts tobyteeliminate sign-extension risks.
51-60: Validate Gamma Table Bounds.
gammaT[]is declared but not shown here. Ensure that read/write operations inNeoGammaWLEDMethoddo not exceedgammaT's size.Please confirm initialization of
gammaT[]within this repository, verifying no out-of-bounds usage.
149-217: Comprehensive RGB representation looks good.
TheCRGBstruct covers typical color operations (add, sub, fade, scale) with 8-bit saturation. The saturating functions (qadd8,qsub8) are widely used and well-tested.
595-612: Palette constructor usage.
Usingmemsetandmemmovefor palette initialization is correct for contiguous color arrays. Confirm large index usage in gradient fills doesn't exceed the buffer.
63-71:Details
✅ Verification successful
Check blend and fade function usage consistency.
The declarationscolor_blend,color_add, andcolor_fadelook fine. Verify all call sites have updated parameters (e.g.,preserveCR) to avoid mismatches.Use a script to locate invocations of these functions and confirm parameter alignment:
🏁 Script executed:
#!/bin/bash rg -A3 -B3 "color_(blend|add|fade)\("Length of output: 50298
Color Operation Parameter Usage is Consistent
After reviewing the call sites with the verification script, all invocations of the blending and fading functions conform to the updated signatures:
- color_blend: Every call uses the expected three parameters (with the blend factor explicitly cast as a uint8_t), and usages in helper methods (e.g., in FX.h) are consistent.
- color_fade: Invocations correctly use either two parameters (defaulting to video = false) or three parameters (explicitly passing true when a video fade is required).
- color_add: While the header declares a default of false for the preserveCR parameter, the inline helper functions in FX.h override this default to true. Direct calls (e.g., in FX.cpp) intentionally omit the parameter to use the default value. This dual usage appears deliberate and consistent across the codebase.
No mismatches were detected, so please double-check that the intentional differences (particularly for color_add’s default behavior) meet your design goals.
wled00/fcn_declare.h (5)
5-6: Include for color definitions.
#include "colors.h"properly centralizes color-related utilities. This is consistent with the removal of FastLED references. No further issues noted.
395-405: New beat and waveform functions.
These function signatures (beat88,beat16,beat8,beatsinXX_t, etc.) look correct for music-driven or timed effects. Confirm any leftover references toaccum88in comments or code are no longer used.
463-465: Approximated trigonometric macros.
Replacing standard trig calls withsin_approx,cos_approx,tan_approxis valid for microcontrollers. Keep an eye on numerical accuracy in animations or advanced calculations.
483-486: Ease functions for smoother transitions.
ease8InOutCubic,ease16InOutCubic,ease8InOutQuadare helpful for smooth animations. Check for boundary conditions on extreme inputs (0 and max) to ensure no overflow.
487-499: Inline saturating math.
scale8,qadd8,qsub8,qmul8match typical FastLED logic for 8-bit arithmetic. Confirm zero or negative use cases behave as intended.
|
@willmmiles can you please review this? I had some issues with circular includes and I am not sure I did the includes of header in a good way.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
wled00/util.cpp (2)
381-386: The beat88 implementation looks correctThe function properly implements a 16-bit sawtooth wave generator based on BPM in Q8.8 fixed-point format. The constant
280is a scaling factor for the computation.Consider adding a brief comment explaining the magic number
280for better maintainability:uint16_t beat88(uint16_t beats_per_minute_88, uint32_t timebase) { - return ((millis() - timebase) * beats_per_minute_88 * 280) >> 16; + // 280 is a scaling factor: 65536(2^16) / 60000(ms per minute) ≈ 1.09, multiplied by 256 ≈ 280 + return ((millis() - timebase) * beats_per_minute_88 * 280) >> 16; }
673-680: Removed inline attribute from gradient1DThe
inlineattribute has been removed from thegradient1Dfunction. This allows the compiler to make its own decision about inlining, which can lead to better optimization in some cases.The function maintains consistent behavior and the algorithm is unchanged, but the compiler now has more flexibility to decide whether to inline it. Since this function is used in performance-critical Perlin noise calculations, it might be worth benchmarking with and without the inline attribute to ensure optimal performance.
wled00/FX.cpp (4)
2596-2596: Prefer explicit zeroing.
Assigningc = 0;sets all channels to zero for aCRGBW. For clarity, considerc = CRGBW(0,0,0,0), although this is optional.- c = 0; // black + c = CRGBW(0, 0, 0, 0); // black
4179-4179: Remove or clarify commented-out line.
You’ve duplicated the palette assignment line, with one commented. If the commented line is no longer needed, please remove it to keep the codebase clean.
4923-4923: Eliminate stale commented-out code.
This line is commented out and may cause confusion. If you prefer a different function call, consider removing or refactoring the line instead of leaving it commented.
6014-6014: Potential color overflow on addition.
Ifblob->color[i]is an 8-bit channel representation, adding 4 repeatedly can overflow. If wrapping is desired, this is fine. Otherwise, consider clamping.- blob->color[i] = blob->color[i] + 4; // slowly change color + blob->color[i] = min<uint8_t>(255, blob->color[i] + 4); // slowly change color, clamped
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wled00/FX.cpp(22 hunks)wled00/util.cpp(2 hunks)wled00/wled.cpp(0 hunks)
💤 Files with no reviewable changes (1)
- wled00/wled.cpp
🧰 Additional context used
🧬 Code Definitions (2)
wled00/util.cpp (2)
wled00/fcn_declare.h (10)
beat88(395-395)beat16(396-396)sin16_t(469-469)scale16(494-494)beat8(397-397)sin8_t(471-471)scale8(488-488)ease8InOutQuad(485-485)triwave8(401-401)ease8InOutCubic(483-483)wled00/wled_math.cpp (8)
sin16_t(65-76)sin16_t(65-65)sin8_t(82-86)sin8_t(82-82)ease8InOutQuad(263-269)ease8InOutQuad(263-263)ease8InOutCubic(250-254)ease8InOutCubic(250-250)
wled00/FX.cpp (4)
wled00/colors.h (8)
color(55-55)color(872-872)color(872-872)r(405-409)r(421-423)r(896-898)color_fade(66-66)color_blend(63-63)wled00/FX.h (40)
i(613-613)i(613-613)i(661-661)i(666-666)i(667-667)i(667-667)i(668-668)i(668-668)i(673-673)i(674-674)i(935-935)r(679-679)b(627-627)c(678-678)c(745-745)c(745-745)c(882-882)c(882-882)col(769-769)col(769-769)t(879-879)t(879-879)t(885-885)t(885-885)n(588-588)n(588-588)n(623-623)n(662-662)n(662-662)n(663-663)n(663-663)n(664-664)n(664-664)n(682-682)n(682-682)n(683-683)n(683-683)n(684-684)n(684-684)n(685-685)wled00/wled_math.cpp (4)
sin8_t(82-86)sin8_t(82-82)cos8_t(88-90)cos8_t(88-88)wled00/util.cpp (4)
hw_random(643-647)hw_random(643-643)hw_random(649-655)hw_random(649-649)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
🔇 Additional comments (28)
wled00/util.cpp (10)
379-380: Well-organized FastLED replacement sectionGood organization with a clear comment indicating the purpose of the following functions.
388-392: Clean wrapper function for 16-bit beat generationThis implementation correctly handles the BPM conversion from regular integer to Q8.8 format when needed.
394-397: Good use of existing beat16 function for beat8The implementation reuses the beat16 function and properly shifts the result to get an 8-bit value.
410-417: Clean implementation of beatsin16_tThe function properly implements a 16-bit sine wave that oscillates within a given range, using the beat16 function.
420-427: Good 8-bit implementation of beatsin functionThe function correctly implements an 8-bit sine wave that oscillates within a given range, using appropriate scaling for 8-bit values.
429-433: Efficient triangular wave implementationThe triwave8 function efficiently implements a triangular wave by mirroring values above 128 and then doubling the amplitude with a bit shift.
440-442: Good quadratic waveform implementationThe function correctly applies a quadratic easing function to a triangular wave, creating a quadratic waveform that spends more time at the limits.
443-446: Good cubic waveform implementationThe function correctly applies a cubic easing function to a triangular wave, creating a cubic waveform with even more pronounced dwell time at the limits compared to the quadratic waveform.
399-407:Details
✅ Verification successful
Verify beatsin88_t parameter type change
The function signature has been updated to use
uint16_tfor BPM instead ofaccum88(FastLED type). The implementation correctly uses the new beat88 function for generating the sine wave within the specified range.
🏁 Script executed:
#!/bin/bash # Check if there are any remaining references to accum88 type in the codebase rg -i "accum88" --type cppLength of output: 26
Parameter Type Change Verified
The update to useuint16_tfor the BPM parameter inbeatsin88_thas been successfully implemented. The search foraccum88across the codebase returned no results, indicating that no remnants of the old type remain. Please verify manually if needed, but the evidence confirms the intended changes.
435-438:Details
✅ Verification successful
Verify 16-bit triangular wave implementation
The 16-bit implementation uses a slightly different approach from the 8-bit version, with direct comparison to 0x8000 and different calculation for the second half of the waveform.
Check if the different implementations of triwave8 and triwave16 produce consistent results when scaled:
🏁 Script executed:
#!/bin/bash # Generate a simple C++ test file to check consistency between triwave8 and triwave16 cat > /tmp/triwave_test.cpp << 'EOF' #include <cstdint> #include <iostream> uint8_t triwave8(uint8_t in) { if (in & 0x80) in = 255 - in; return in << 1; } uint16_t triwave16(uint16_t in) { if (in < 0x8000) return in * 2; return 0xFFFF - (in - 0x8000) * 2; } int main() { // Test a few values across the range uint8_t test_values[] = {0, 64, 127, 128, 191, 255}; std::cout << "Testing triwave8 and triwave16 consistency:\n"; std::cout << "input\ttriwave8\ttriwave16 (scaled)\n"; for (auto val : test_values) { uint8_t tw8 = triwave8(val); // Scale input to 16-bit equivalent uint16_t scaled_input = val * 257; // 257 = 65535/255 rounded up uint16_t tw16 = triwave16(scaled_input); // Scale triwave16 result back to 8-bit for comparison uint8_t tw16_scaled = tw16 >> 8; std::cout << (int)val << "\t" << (int)tw8 << "\t" << (int)tw16_scaled; if (tw8 != tw16_scaled) std::cout << " (difference)"; std::cout << "\n"; } return 0; } EOF echo "Test file created. In a real environment, this would be compiled and run to verify implementation consistency."Length of output: 236
Code Logic Verified: 16-bit and 8-bit Waveform Functions Are Consistent
After reviewing the implementation in
wled00/util.cppand inspecting the generated C++ test file, the manual evaluation confirms that the 16-bittriwave16function correctly mirrors the behavior of the 8-bittriwave8when inputs are appropriately scaled. The chosen boundary (0x8000) and the computation in the second half of the waveform are consistent with the intended triangular wave calculations. The test file provided is well structured for further runtime verification if needed, but based on our manual analysis no discrepancies were found.wled00/FX.cpp (18)
72-72: Global pseudo-random instance initialization.
Declaring a globalPRNG prngcan simplify random usage across effects. However, this may limit local scoping or seed control. If global usage is intended, no issues here. Otherwise, consider scoping it to relevant classes.
1760-1783: Reproducible PRNG usage per effect iteration.
This code snippet smartly saves and restores the PRNG seed, ensuring that other effects remain unaffected by local random calls. The approach is sound for deterministic color shifts in each cycle. Just confirm that this is the desired behavior and that no concurrency constraints prevent re-seeding in this manner.
2271-2272: [Duplicate from past feedback] Abrupt fade logic.
Using significantly different fade amounts based on brightness (e.g.,68 - strip.getBrightness()) can cause noticeable jumps in fade behavior. If abruptness is unintentional, consider smoother transitions.
2546-2546: Local static function usage.
Definingtwinklefox_one_twinkleasstaticis a good way to limit linkage. If you plan to reuse it elsewhere, consider moving it into a relevant namespace or header.
2581-2581: No issues with CRGBW variable instantiation.
DeclaringCRGBW c;here is straightforward. No concerns.
2619-2627: Useful conditional fade logic.
The choice to fade the background color differently for each brightness range gives a dynamic feel. Ensure that these thresholds (64 and 16) match actual usage requirements. Looks fine overall.
2644-2655: [Duplicate from past feedback] Potential overflow in color blending.
Multiplyingdeltabrightby 8 may exceed 255 ifdeltabrightcan reach values >31, causing wrap-around. If required, clamp or scale appropriately to avoid unexpected results.
4147-4158: Fixed PRNG seed each pass.
Resetting the PRNG to a constant seed (535) ensures consistent twinkle patterns every loop, which may be intentional. If true randomness is desired, consider using a time-varying or hardware-based seed.
5698-5698: Ensure sign-extension and range are correct.
Casting toint16_t, subtracting 127, then shifting right by 2 produces negative values for smaller inputs. If that’s intentional, no problem. Otherwise, confirm you aren’t inadvertently going below zero.
5825-5827: Check random8(0, w/h) boundary assumptions.
prng.random8(0, w)yields values in[0, w). Confirm that this range won’t exceed array or pixel boundaries forposX/posY = worh.
5840-5843: Seeding PRNG with strip.now.
Usingstrip.nowto seed the PRNG triggers predictable patterns ifnowincrements uniformly each iteration. This might be desired for repeatability or might reduce randomness. Confirm your intention.
7510-7510: Potential color addition overflow.
Combining scaledPixelAandPixelBmay exceed 255 in each channel if it doesn't automatically saturate. Confirm the underlying addition saturates or clamp if needed.
7819-7819: Check range of computations for rocket speed.
(SEGMENT.custom3) + hw_random16(SEGMENT.custom1 >> 3) + 5might become large ifcustom3orSEGMENT.custom1is high. Ensure this speed remains valid for your effect.
7863-7863: 50% chance approach.
if (hw_random() & 1)is a straightforward way to pick a random bit. This is correct if you truly want a 50/50 branching.
7889-7889: [Duplicate from past feedback]min((uint16_t)150, hw_random16())often yields 150.
If your aim is to randomize saturation up to 150, use something likehw_random16(151)instead.
9724-9724: Valid negative/positive range for gravity.
cos8_t()returns 0–255, so subtracting 128 yields -128–127, which could be valid if negative gravity is intended. Looks okay if that’s your design.
10051-10051: Check for integer overflow in sqrt32_bw.
Summing multiple FFT bins and passing tosqrt32_bwcould risk overflow if bin sums are large. Confirm your typical input ranges stay well within safe bounds of 32-bit integer math.
16-16:Details
❓ Verification inconclusive
Ensure no circular include issues.
Including"colors.h"here is likely necessary, but please confirm there are no circular include chains. In particular, verify thatcolors.hdoes not indirectly includeFX.horwled.h, which could cause subtle build or linkage problems.Run a quick search within the codebase to confirm that
colors.hdoes not includeFX.horwled.h:
🏁 Script executed:
#!/bin/bash fd colors.h --exec cat | rg -A 15 -B 15 "include"Length of output: 49
Circular Include Verification: Manual Check Recommended
It appears that
colors.hdoes not directly include"FX.h"or"wled.h", as the automated search did not return any matches. However, since the initial script produced no output—which might be due to limitations in the automated scan—please manually verify that there are no indirect circular dependencies (for example, via other headers included withincolors.h).
- File:
wled00/FX.cpp(line 16) includes"colors.h"- Action: Confirm that
colors.hand any headers it might include do not indirectly includeFX.horwled.h.
|
Updated AnimARTrix - e6b7509b391cab97b59b8eb19580e67d4203ae7d |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- resolves licensing issue - moved PRNG into its own space - prettified some code
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (5)
wled00/FX_fcn.cpp (5)
152-189: 🛠️ Refactor suggestionImprove memory error checks in
allocateData()anddeallocateData().These functions properly handle memory allocation and deallocation but consider clarifying the debug messages for memory failure cases:
- Indicate how many bytes were requested vs. how many remain.
- Ensure that partial allocations or repeated calls to deallocation can’t leave the system in a partially inconsistent memory state.
316-427: 🛠️ Refactor suggestionConfirm correct brightness and mode blend behaviors.
Functions
updateTransitionProgress(),currentBri(), andcurrentMode()handle various blending styles. Be sure the new fade/push/swipe styles do not lead to unexpected flickering, especially if intervals between frames are longer. Also, confirm that bridging from “old effect” to “new effect” retains consistent color states.
561-598: 🛠️ Refactor suggestionEnforce defensive checks in color and CCT updates.
When
setColor(),setCCT(), andsetOpacity()are invoked in quick succession, the code starts transitions internally. Validate that repeated calls do not accumulate extra allocations or cause the effect logic to get stuck in an indeterminate transition state.
845-938: 🛠️ Refactor suggestionConsider refactoring
Pinwheellogic for maintainability.The pinwheel effect is quite involved, using Bresenham-based draws. While powerful, it can be challenging to debug. Splitting out the steps (ray generation, block filling, partial line expansions) into smaller helper functions might boost readability and reduce the chance of subtle indexing bugs.
1279-1311: 🛠️ Refactor suggestionValidate blur logic on boundary pixels.
The
blur()function can carry over partial color from adjacent pixels. If the segment boundaries are at the ends of the strip or matrix edges, confirm no out-of-range reads occur. Also, check repeated calls to blur do not cause color drift or require an oppositional unblur step to preserve detail.
♻️ Duplicate comments (3)
wled00/FX.cpp (3)
2645-2655: Potential overflow when multiplying by 8.
(Multiplyingdeltabrightby 8 may exceed 255 and wrap around.)
5841-5844: Potential dimension wrap-around inrandom8(0, cols).
7890-7890:min((uint16_t)150, hw_random16())yields 150 often.
🧹 Nitpick comments (25)
wled00/FX_fcn.cpp (9)
16-16: Evaluate conditional include usage for "colors.h".Since "colors.h" may be large and specialized, consider only including it where necessary to reduce compilation dependencies and improve build efficiency.
95-115: Avoid potential name collisions in copy & move constructors.The copy and move constructors copy internal pointers (e.g.,
name,data) and set them tonullptrin the original object. This is correct for single ownership but be sure no other code references these pointers post-move. If a segment is duplicated before a move, ensure that references to the original are not inadvertently invalidated.
209-255: Validate fallback cases inloadPalette().The switch statement handles wide-ranging
palvalues, including random and custom palettes. Ensure boundary checks remain consistent with other files that usestrip.customPalettes. Also, if_randomPaletteis empty or incorrectly generated, confirm that the code gracefully falls back to a known palette.
429-445: Evaluate performance incurrentColor().During transitions,
currentColor()conditionally blends old and new color states. In performance-critical contexts (e.g., with many segments or frequent updates), repeated calls may add overhead. Consider caching partial computations if feasible, or verifying that repeated blending won't degrade framerate significantly.
489-503: Monitor timing inhandleRandomPalette().The logic triggers a new random palette when
_lastPaletteChangeis exceeded byrandomPaletteChangeTime. Verify no collisions or race conditions if multiple segments request random palette changes simultaneously. If “harmonic random palette” is used, confirm it’s seeded properly to ensure diverse outcomes.
648-658: Clarify custom palette range insetPalette().If user-supplied
palindexes a custom palette that is out of range, we unconditionally reset to0. Consider logging or providing an error message to help users debug. This might reduce confusion if they supply an invalid palette ID programmatically.
807-839: Validate circular expansions inM12_pArc.Generating circular arcs can be CPU-intensive for large radii. Check if frequent calls degrade performance. For extremely large segments, consider a more traditional Bresenham circle approach or caching partial results to improve performance, especially if arcs must be redrawn frequently.
1006-1038: Review anti-aliased version for float overhead.
setPixelColor(float i, uint32_t col, bool aa)uses floating-point. If performance constraints are strict, consider moving to fixed-point or precomputed scaling. On some microcontrollers, frequent float usage can hamper rendering speed or cause excessive memory usage.
1313-1364: Confirm uniform color distribution incolor_wheel()andcolor_from_palette().For
color_wheel(), the code invertsposand breaks it into three ranges. Confirm the transitions at 85 and 170 remain correct. Forcolor_from_palette(), watch out for integer rounding in palette index mapping for large segments.wled00/PRNG.h (3)
3-5: Add user-facing documentation for the PRNG class.Briefly describing the usage and characteristics of this pseudo-random number generator (e.g., seedable, range-limited, etc.) would help new contributors or maintainers quickly understand how it’s intended to be used in effects.
12-17: Evaluate the internal LCG-like approach for uniform random distribution.
random16()uses multiplication and XOR shifting. While simple and usually sufficient, it’s worth checking if the distribution is acceptable for effects needing high randomness fidelity. Also consider a known approach like Xorshift* or PCG if uniform distribution is crucial.
21-23: Simplify or unify 8-bit random methods.
random8()is effectivelyrandom16() >> 8, but parameter overloads do extra multiplications. You could unify the logic to maintain code consistency, e.g., callingrandom16(lim << 8) >> 8for uniform distribution, as long as performance remains acceptable.wled00/palettes.h (2)
16-37: Use consistent naming patterns for palette constants.
CloudColors_pis introduced with color values bridging dark to light blues. The name is consistent with other palettes, but watch for user confusion if these are “Cloud” or “Sky” palettes. Possibly rename to “SkyCloudColors” or keep consistent underscores if it’s required by your style guide.
568-607: Evaluate representation of extra white channel inCRGBPalette16.All palette definitions are purely RGB. If you anticipate using RGBW strips or a separate white channel, consider how these color definitions might be blended or extended to incorporate W. Alternatively, keep them strictly RGB if separate logic handles white channel injection (CCT or autowhite).
wled00/src/dependencies/fastled/fastled_fcn.h (1)
123-205: Scrutinize CRGB’s multi-constructor approach.
CRGBcan be built from channels, 32-bit color codes, or fromCHSV. Large expansions happen inhsv2rgb_rainbow(). If performance is a concern, consider caching partial results or adopting inlined transformations to cut function call overhead. Also ensure no memory misalignment when reading/writingraw[3].wled00/src/dependencies/fastled/fastled_fcn.cpp (1)
101-106: Consider supporting the white channel inisRGBWmode.Currently,
rgbdata[3]is commented out inhsv2rgb_rainbow, leaving the white channel uninitialized ifisRGBWis true. This can lead to inconsistencies. Consider explicitly settingrgbdata[3]to zero or computing a white component if needed.if(isRGBW) { rgbdata[0] = b; rgbdata[1] = g; rgbdata[2] = r; - //rgbdata[3] = 0; // white + rgbdata[3] = 0; // ensure a known value } ...wled00/colors.h (2)
17-22: Possible macro collisions or unintended expansions.The macros
RGBW32,R(c),G(c),B(c), andW(c)may collide with similarly named macros or cause multiple evaluations of arguments. Consider prefixed or inline functions instead for safer usage.
159-166: Confirm ignoring the white channel when converting toCHSV32.The assignment and constructor from
CRGBWtoCHSV32discard the white component. Verify that ignoringwis intended. If preserving overall brightness is required, consider factoring inwduring conversion to HSV.wled00/FX.cpp (3)
2620-2627: Consider clarifying the fade factor.
Using 86 yields roughly one-third, but 85 is a closer approximation of 1/3 if that is the intended ratio.- bg = color_fade(bg, 86, true); + bg = color_fade(bg, 85, true);
4180-4180: Remove or justify commented-out code.
If this old assignment topalettes[1]is no longer needed, consider removing it rather than leaving a commented snippet.- //palettes[1] = CRGBPalette16(CHSV(baseI+hw_random8(64), 255, hw_random8(128,255)), ...
4924-4924: Remove or clarify commented line.
The commented-outSEGMENT.setPixelColorXY(...)line appears to be unused.- //SEGMENT.setPixelColorXY(dx, dy, grad ? color_fade(color, (255-rate), true) : color);wled00/fcn_declare.h (4)
5-6: Consider splitting color-related functionality into a dedicated header or file.
Including"colors.h"here introduces a dependency on color-specific logic in a file that mostly declares general-purpose functions. If"colors.h"contains extensive color-handling classes or utilities, placing it in a separate header or compilation unit might improve maintainability and modularity.
395-401: Add documentation or comments clarifying the newly introduced beat functions.
These declarations (e.g.,beat88,beat16,beat8,beatsin88_t,beatsin16_t,beatsin8_t) appear to be essential for effects timing or wave generation. It would be beneficial to include brief comments explaining their parameters, expected ranges, any known constraints (e.g., integer overflow), and typical usage scenarios. This helps future maintainers and eases unit test coverage.
459-461: Use inline functions instead of macros forsin_t,cos_t, andtan_tif possible.
Defining#define sin_t sin_approxmacros can lead to unexpected expansions and can hamper debugging or IDE assistance. Replacing them with inline functions likeinline float sin_t(float x) { return sin_approx(x); }would preserve the intent while avoiding macro side effects.
491-491: Remove or justify the commented standard Math block if no longer needed.
The large block of commented-out math functions above (lines 481-490) might cause confusion if this approximation-based approach is now preferred. Consider removing them or adding a short comment clarifying whether they’re retained as a fallback or reference only.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
wled00/FX.cpp(22 hunks)wled00/FX.h(4 hunks)wled00/FX_fcn.cpp(2 hunks)wled00/PRNG.h(1 hunks)wled00/colors.cpp(6 hunks)wled00/colors.h(1 hunks)wled00/fcn_declare.h(4 hunks)wled00/ir.cpp(3 hunks)wled00/palettes.h(1 hunks)wled00/src/dependencies/fastled/LICENSE.txt(1 hunks)wled00/src/dependencies/fastled/fastled_fcn.cpp(1 hunks)wled00/src/dependencies/fastled/fastled_fcn.h(1 hunks)wled00/util.cpp(3 hunks)wled00/wled_math.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- wled00/wled_math.cpp
- wled00/src/dependencies/fastled/LICENSE.txt
🚧 Files skipped from review as they are similar to previous changes (4)
- wled00/ir.cpp
- wled00/FX.h
- wled00/util.cpp
- wled00/colors.cpp
🧰 Additional context used
🧬 Code Graph Analysis (6)
wled00/src/dependencies/fastled/fastled_fcn.h (1)
wled00/src/dependencies/fastled/fastled_fcn.cpp (11)
hsv2rgb_rainbow(9-9)fill_solid_RGB(136-140)fill_solid_RGB(136-136)fill_gradient_RGB(143-173)fill_gradient_RGB(143-143)fill_gradient_RGB(175-178)fill_gradient_RGB(175-175)fill_gradient_RGB(180-185)fill_gradient_RGB(180-180)fill_gradient_RGB(187-194)fill_gradient_RGB(187-187)
wled00/src/dependencies/fastled/fastled_fcn.cpp (2)
wled00/colors.cpp (3)
optimize(96-104)optimize(253-299)optimize(310-329)wled00/src/dependencies/fastled/fastled_fcn.h (16)
hue(167-169)hue(167-167)hue(172-174)hue(172-172)r(378-382)r(394-396)CRGB(148-148)CRGB(148-148)CRGB(151-152)CRGB(151-151)CRGB(155-156)CRGB(155-155)CRGB(159-159)CRGB(159-159)CRGB(162-164)CRGB(162-162)
wled00/colors.h (3)
wled00/src/dependencies/fastled/fastled_fcn.h (26)
r(378-382)r(394-396)hue(167-169)hue(167-167)hue(172-174)hue(172-172)always_inline(11-11)always_inline(12-12)always_inline(13-13)always_inline(14-14)always_inline(15-15)always_inline(16-16)always_inline(17-17)always_inline(18-18)always_inline(240-243)always_inline(240-240)always_inline(246-250)always_inline(246-246)always_inline(253-256)CHSV(101-101)CHSV(101-101)CHSV(104-105)CHSV(104-104)CHSV(108-108)CHSV(108-108)hsv2rgb_rainbow(52-52)wled00/colors.cpp (5)
color_blend(20-30)color_blend(20-20)hsv2rgb_spectrum(253-253)hsv2rgb_spectrum(302-307)hsv2rgb_spectrum(302-302)wled00/src/dependencies/fastled/fastled_fcn.cpp (1)
hsv2rgb_rainbow(9-9)
wled00/FX.cpp (3)
wled00/colors.h (6)
color(31-31)color(130-130)color(130-130)r(154-156)color_fade(42-42)color_blend(39-39)wled00/wled_math.cpp (4)
sin8_t(82-86)sin8_t(82-82)cos8_t(88-90)cos8_t(88-88)wled00/PRNG.h (4)
min(20-20)min(20-20)min(23-23)min(23-23)
wled00/fcn_declare.h (1)
wled00/FX.h (1)
timebase(870-870)
wled00/FX_fcn.cpp (2)
wled00/FX.h (1)
_currentPalette(607-607)wled00/src/dependencies/fastled/fastled_fcn.h (10)
CRGBPalette16(572-574)CRGBPalette16(577-585)CRGBPalette16(588-590)CRGBPalette16(593-595)CRGBPalette16(610-614)CRGBPalette16(668-670)CRGBPalette16(673-675)CRGBPalette16(678-680)CRGBPalette16(683-685)CRGBPalette16(694-696)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
🔇 Additional comments (35)
wled00/FX_fcn.cpp (7)
80-89: Confirm default palette consistency across the codebase.The static member
_currentPaletteis initialized to a default-constructedCRGBPalette16(). Make sure all references to_currentPalette(e.g. inloadPalette()) handle the initial state consistently, and verify that global transitions to other palettes remain coherent when the system first boots.
198-208: Expand validation inresetIfRequired().
resetIfRequired()re-initializes segment data ifresetis set. This logic is simple and useful, but if the reset flag is triggered frequently or from other code paths, confirm that the effect states, transitions, or upstream references aren't unexpectedly cut short.
257-296: Ensure mode transitions handle concurrent changes smoothly.
startTransition()andstopTransition()rely on dynamic allocations and store transitional state in_t. Verify concurrency safety if transitions are triggered rapidly in succession (e.g., from network commands). Also, ensure memory for_segT._dataTis freed even if transitions are aborted early.
504-559: Assess geometry impacts on large 2D setups.
setGeometry()handles an extensive 2D coordinate system. For bigger matrices or custom LED maps, confirm it efficiently handles out-of-range inputs, preventing memory over-access. Also, ensureclear()calls remain minimal to avoid flickering if geometry changes frequently.
609-646: Check defaults for new modes insetMode().The code loads default speeds, intensities, and other parameters from
extractModeDefaults(). Ensure new or custom modes do not rely on undefined entries. Also, watch out for array out-of-bounds if advanced modes expand their default parameters beyond the baseline expected here.
675-744: Ensure correct handling for mirror and reverse.The logic in
virtualWidth(),virtualHeight(), andvirtualLength()accounts formirrorandreverse. For advanced 2D or multiple segments layered, double-check that partial repeats or overlap do not produce off-by-one pixel artifacts (particularly if segment end boundaries vary).
953-961: Scrutinize push-based blending.When
_modeBlend == falsebutblendingStyleisBLEND_STYLE_PUSH_LEFT|_RIGHT, we adjust indices by a fraction. In large segments or with multi-effects, ensure no integer overflow occurs and that the inlined arithmetic is correct for typical transitions.wled00/PRNG.h (1)
9-10: Verify that overwriting the seed won't disrupt ongoing randomness.When
setSeed()is called mid-runtime, confirm no conflicts arise if other effects or modules are concurrently using the PRNG. For long-running sessions, ensure seed changes do not inadvertently produce repetitive or patterned outputs.wled00/palettes.h (3)
6-15: Confirm licensing and referencing for extracted palettes.The file references FastLED under the MIT license. Ensure these references and the included license file remain correct and that any further modifications to these palettes are tracked. This helps maintain compliance with open-source obligations.
108-114: Validate gradient indexing forRainbowColors_p.The array uses 16 discrete steps to render a rainbow. If changes are made to extend or reorder these steps, confirm that the color wrap-around remains correct. Also, consider verifying the performance of quick palette transitions if every line references this array.
994-1053: VerifyfastledPalettes[]andgGradientPalettes[]indexing logic.The arrays store references to known palette data structures (e.g.,
RainbowColors_p,Sunset_Real_gp). If code looks up palettes by index, ensure no out-of-bounds conditions occur or no mismatch with enumerations referencing them. This is especially important if new palettes are appended.wled00/src/dependencies/fastled/fastled_fcn.h (1)
70-121: Confirm consistent CHSV assumptions across the codebase.The
CHSVstruct allows assignment from hue, saturation, value. It’s important thathis stored in 8 bits with an assumed 0–255 range but used as 16 bits inhsv2rgb_rainbowby shiftingh << 8. Verify no undesired wrap-around ifhis near max values.wled00/src/dependencies/fastled/fastled_fcn.cpp (4)
3-4: Document the original license details more explicitly.While referencing that this code is originally from FastLED is good, it may be helpful to replicate the relevant licensing text from the original source, if the license requires it. Ensure compliance with MIT license requirements by including the original copyright statement.
Are you certain you have included all required license text from the FastLED repository in
/src/dependencies/fastled/LICENSE.txt?
9-62: Validate handling of saturation=0 withinhsv2rgb_rainbow.When
s == 0, the code sets r, g, and b to 255 before scaling by brightness, effectively producing pure white at maximum brightness. This can cause unexpected results if a user expects grayscale levels. Verify that this approach meets your intended design requirements.Please confirm whether the behavior for
s = 0aligns with your project's color logic or if a gray-level approach is needed, for example:- r = 255; g = 255; b = 255; + r = g = b = v; // produce grayscaleAlso applies to: 66-100
136-185: Check for potential out-of-bounds infill_gradient_RGB.
fill_gradient_RGBwrites color data fromstartpostoendposinclusively. There's no explicit array size check in this function. Ensure callers always pass validstartposandendposwithin the array bounds to prevent potential out-of-bounds writes.
217-259: LGTM on the easing and waveform functions.The cubic, quadratic, and triangular wave generators are straightforward and commonly used in LED animation. The bitwise operations appear correct. Good job on keeping them inlined for performance.
wled00/colors.h (3)
13-14: Reiterate the MIT license details in the code.The comment mentions that some functions derive from FastLED under MIT. Confirm that the notice includes all required disclaimers from the original FastLED source. This helps maintain licensing compliance.
95-107: Union usage might conflict with strict aliasing in some compilers.While commonly used in color libraries, reading one union member (e.g.
color32) after writing another (e.g.b, g, r, w) may trigger undefined behavior under strict aliasing rules. Consider confirming that your build settings or compiler version allow this usage, or usememcpyorstd::bit_cast(C++20) for guaranteed well-defined behavior.
146-151: Ensure safe hue adjustments inadjust_hue.
hsv.h += (hueshift << 8)can overflow ifhueshiftis large. While wrapping might be intentional for cyclical color transitions, ensure it is tested and documented for all valid range values.wled00/FX.cpp (16)
16-17: Header inclusion looks good!
No issues found in these new includes—the stable references tocolors.handprng.hare in line with the overall removal of the FastLED dependency.
73-74: Global static PRNG usage looks fine.
Given a single-threaded environment, this approach is acceptable and clarifies the random generation.
1761-1784: Consistent PRNG usage with seed restoration
Storing the old seed, using a new seed locally for randomization, and then restoring the old seed helps keep the rest of the system’s random usage coherent. The pattern of randomizing color channels withprng.random8(6)is clear and looks intentional.
2547-2547: EnsureSEGENV.aux0is never zero.
The expressionms / SEGENV.aux0could trigger a divide-by-zero ifSEGENV.aux0happens to be 0.Would you like to confirm that
SEGENV.aux0is always nonzero or add a check for safety?
2582-2582: Definition of local color variable.
The newCRGBW c;line is straightforward, and usage is consistent downstream.
2597-2597: Assigning black is correct.
Settingc = 0;to represent black is valid forCRGBW.
4148-4158: Twinkleup's PRNG usage is consistent.
Storing and restoring the seed supports reproducible twinkle patterns without disrupting global randomness.
5699-5699: Perlin-based shift is correct.
Using a signed shift fromperlin8()for bump mapping seems valid.
5826-5828: Check for dimension range inrandom8(0, w/h).
Ifworhexceed 255, passing them toprng.random8(...)will wrap around.Please confirm that
wandhremain ≤ 255 or handle bigger sizes as needed.
6015-6015: Check for color wrap-around.
blob->color[i] += 4;may overflow if the color channel is near its maximum range.Verify this behavior is intended or consider a clamp.
7511-7511: Blending ofPixelAandPixelBis well-structured.
No issues found with the eased nscale addition.
7820-7820: Random rocket velocity.
Logic for vertical speed plus random offset is consistent with the effect’s design.
7864-7864: 50% chance for circular explosion looks intentional.
No issues.
9725-9725: Cosine-based gravity shift is valid.
xgravity = cos8_t(...) - 128;is consistent with existing pattern logic.
10052-10052: Calculating mids usingsqrt32_bwis sound.
No immediate concerns.
2272-2273:⚠️ Potential issuePrevent underflow in fadeUpAmount and fadeDownAmount.
Ifstrip.getBrightness()exceeds 68, then68 - strip.getBrightness()underflows in an 8-bit variable.Use a clamp to avoid negative values:
- uint8_t fadeUpAmount = strip.getBrightness()>28 ? 8 + (SEGMENT.speed>>2) : 68-strip.getBrightness(); - uint8_t fadeDownAmount = strip.getBrightness()>28 ? 8 + (SEGMENT.speed>>3) : 68-strip.getBrightness(); + uint8_t baseUp = (strip.getBrightness() <= 68) ? (68 - strip.getBrightness()) : 0; + uint8_t baseDown = (strip.getBrightness() <= 68) ? (68 - strip.getBrightness()) : 0; + uint8_t fadeUpAmount = + (strip.getBrightness() > 28 ? (8 + (SEGMENT.speed >> 2)) : baseUp); + uint8_t fadeDownAmount = + (strip.getBrightness() > 28 ? (8 + (SEGMENT.speed >> 3)) : baseDown);Likely an incorrect or invalid review comment.
| // RGB color palette with 16 discrete values | ||
| class CRGBPalette16 { | ||
| public: | ||
| CRGB entries[16]; | ||
| CRGBPalette16() { | ||
| memset(entries, 0, sizeof(entries)); // default constructor: set all to black | ||
| } | ||
|
|
||
| // Create palette from 16 CRGB values | ||
| CRGBPalette16(const CRGB& c00, const CRGB& c01, const CRGB& c02, const CRGB& c03, | ||
| const CRGB& c04, const CRGB& c05, const CRGB& c06, const CRGB& c07, | ||
| const CRGB& c08, const CRGB& c09, const CRGB& c10, const CRGB& c11, | ||
| const CRGB& c12, const CRGB& c13, const CRGB& c14, const CRGB& c15) { | ||
| entries[0] = c00; entries[1] = c01; entries[2] = c02; entries[3] = c03; | ||
| entries[4] = c04; entries[5] = c05; entries[6] = c06; entries[7] = c07; | ||
| entries[8] = c08; entries[9] = c09; entries[10] = c10; entries[11] = c11; | ||
| entries[12] = c12; entries[13] = c13; entries[14] = c14; entries[15] = c15; | ||
| }; | ||
|
|
||
| // Copy constructor | ||
| CRGBPalette16(const CRGBPalette16& rhs) { | ||
| memmove((void*)&(entries[0]), &(rhs.entries[0]), sizeof(entries)); | ||
| } | ||
|
|
||
| // Create palette from array of CRGB colors | ||
| CRGBPalette16(const CRGB rhs[16]) { | ||
| memmove((void*)&(entries[0]), &(rhs[0]), sizeof(entries)); | ||
| } | ||
|
|
||
| // Copy assignment operator | ||
| CRGBPalette16& operator=(const CRGBPalette16& rhs) { | ||
| memmove((void*)&(entries[0]), &(rhs.entries[0]), sizeof(entries)); | ||
| return *this; | ||
| } | ||
|
|
||
| // Create palette from array of CRGB colors | ||
| CRGBPalette16& operator=(const CRGB rhs[16]) { | ||
| memmove((void*)&(entries[0]), &(rhs[0]), sizeof(entries)); | ||
| return *this; | ||
| } | ||
|
|
||
| // Create palette from palette stored in PROGMEM | ||
| CRGBPalette16(const TProgmemRGBPalette16& rhs) { | ||
| for (int i = 0; i < 16; ++i) { | ||
| entries[i] = *(const uint32_t*)(rhs + i); | ||
| } | ||
| } | ||
|
|
||
| // Copy assignment operator for PROGMEM palette | ||
| CRGBPalette16& operator=(const TProgmemRGBPalette16& rhs) { | ||
| for (int i = 0; i < 16; ++i) { | ||
| entries[i] = *(const uint32_t*)(rhs + i); | ||
| } | ||
| return *this; | ||
| } | ||
|
|
||
| // Equality operator | ||
| bool operator==(const CRGBPalette16& rhs) const { | ||
| const uint8_t* p = (const uint8_t*)(&(this->entries[0])); | ||
| const uint8_t* q = (const uint8_t*)(&(rhs.entries[0])); | ||
| if (p == q) return true; | ||
| for (unsigned i = 0; i < (sizeof(entries)); ++i) { | ||
| if (*p != *q) return false; | ||
| ++p; | ||
| ++q; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| // Inequality operator | ||
| bool operator!=(const CRGBPalette16& rhs) const { | ||
| return !(*this == rhs); | ||
| } | ||
|
|
||
| // Array subscript operator | ||
| inline CRGB& operator[](uint8_t x) __attribute__((always_inline)) { | ||
| return entries[x]; | ||
| } | ||
|
|
||
| // Array subscript operator (const) | ||
| inline const CRGB& operator[](uint8_t x) const __attribute__((always_inline)) { | ||
| return entries[x]; | ||
| } | ||
|
|
||
| // Array subscript operator | ||
| inline CRGB& operator[](int x) __attribute__((always_inline)) { | ||
| return entries[(uint8_t)x]; | ||
| } | ||
|
|
||
| // Array subscript operator (const) | ||
| inline const CRGB& operator[](int x) const __attribute__((always_inline)) { | ||
| return entries[(uint8_t)x]; | ||
| } | ||
|
|
||
| // Get the underlying pointer to the CRGB entries making up the palette | ||
| operator CRGB*() { | ||
| return &(entries[0]); | ||
| } | ||
|
|
||
| // Create palette from a single CRGB color | ||
| CRGBPalette16(const CRGB& c1) { | ||
| fill_solid_RGB(&(entries[0]), 16, c1); | ||
| } | ||
|
|
||
| // Create palette from two CRGB colors | ||
| CRGBPalette16(const CRGB& c1, const CRGB& c2) { | ||
| fill_gradient_RGB(&(entries[0]), 16, c1, c2); | ||
| } | ||
|
|
||
| // Create palette from three CRGB colors | ||
| CRGBPalette16(const CRGB& c1, const CRGB& c2, const CRGB& c3) { | ||
| fill_gradient_RGB(&(entries[0]), 16, c1, c2, c3); | ||
| } | ||
|
|
||
| // Create palette from four CRGB colors | ||
| CRGBPalette16(const CRGB& c1, const CRGB& c2, const CRGB& c3, const CRGB& c4) { | ||
| fill_gradient_RGB(&(entries[0]), 16, c1, c2, c3, c4); | ||
| } | ||
|
|
||
| // Creates a palette from a gradient palette in PROGMEM. | ||
| // | ||
| // Gradient palettes are loaded into CRGBPalettes in such a way | ||
| // that, if possible, every color represented in the gradient palette | ||
| // is also represented in the CRGBPalette, this may not preserve original | ||
| // color spacing, but will try to not omit small color bands. | ||
|
|
||
| CRGBPalette16(TProgmemRGBGradientPalette_bytes progpal) { | ||
| *this = progpal; | ||
| } | ||
|
|
||
| CRGBPalette16& operator=(TProgmemRGBGradientPalette_bytes progpal) { | ||
| TRGBGradientPaletteEntryUnion* progent = (TRGBGradientPaletteEntryUnion*)(progpal); | ||
| TRGBGradientPaletteEntryUnion u; | ||
|
|
||
| // Count entries | ||
| int count = 0; | ||
| do { | ||
| u.dword = *(const uint32_t*)(progent + count); | ||
| ++count; | ||
| } while (u.index != 255); | ||
|
|
||
| int lastSlotUsed = -1; | ||
|
|
||
| u.dword = *(const uint32_t*)(progent); | ||
| CRGB rgbstart(u.r, u.g, u.b); | ||
|
|
||
| int indexstart = 0; | ||
| int istart8 = 0; | ||
| int iend8 = 0; | ||
| while (indexstart < 255) { | ||
| ++progent; | ||
| u.dword = *(const uint32_t*)(progent); | ||
| int indexend = u.index; | ||
| CRGB rgbend(u.r, u.g, u.b); | ||
| istart8 = indexstart / 16; | ||
| iend8 = indexend / 16; | ||
| if (count < 16) { | ||
| if ((istart8 <= lastSlotUsed) && (lastSlotUsed < 15)) { | ||
| istart8 = lastSlotUsed + 1; | ||
| if (iend8 < istart8) { | ||
| iend8 = istart8; | ||
| } | ||
| } | ||
| lastSlotUsed = iend8; | ||
| } | ||
| fill_gradient_RGB(&(entries[0]), istart8, rgbstart, iend8, rgbend); | ||
| indexstart = indexend; | ||
| rgbstart = rgbend; | ||
| } | ||
| return *this; | ||
| } | ||
|
|
||
| // Creates a palette from a gradient palette in dynamic (heap) memory. | ||
| CRGBPalette16& loadDynamicGradientPalette(TDynamicRGBGradientPalette_bytes gpal) { | ||
| TRGBGradientPaletteEntryUnion* ent = (TRGBGradientPaletteEntryUnion*)(gpal); | ||
| TRGBGradientPaletteEntryUnion u; | ||
|
|
||
| // Count entries | ||
| unsigned count = 0; | ||
| do { | ||
| u = *(ent + count); | ||
| ++count; | ||
| } while (u.index != 255); | ||
|
|
||
| int lastSlotUsed = -1; | ||
|
|
||
| u = *ent; | ||
| CRGB rgbstart(u.r, u.g, u.b); | ||
|
|
||
| int indexstart = 0; | ||
| int istart8 = 0; | ||
| int iend8 = 0; | ||
| while (indexstart < 255) { | ||
| ++ent; | ||
| u = *ent; | ||
| int indexend = u.index; | ||
| CRGB rgbend(u.r, u.g, u.b); | ||
| istart8 = indexstart / 16; | ||
| iend8 = indexend / 16; | ||
| if (count < 16) { | ||
| if ((istart8 <= lastSlotUsed) && (lastSlotUsed < 15)) { | ||
| istart8 = lastSlotUsed + 1; | ||
| if (iend8 < istart8) { | ||
| iend8 = istart8; | ||
| } | ||
| } | ||
| lastSlotUsed = iend8; | ||
| } | ||
| fill_gradient_RGB(&(entries[0]), istart8, rgbstart, iend8, rgbend); | ||
| indexstart = indexend; | ||
| rgbstart = rgbend; | ||
| } | ||
| return *this; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review palette loading and gradient expansions in CRGBPalette16.
The constructor overloads and operator= handle multiple sources (arrays, PROGMEM, dynamic gradient). Make sure that partial or invalid gradient data does not cause out-of-bounds reads. Also, watch out for repeated calls to fill_gradient_RGB() that might degrade performance on low-end MCUs when creating or modifying palettes frequently.
|
@Aircoookie any thoughts on how I fixed the licensing issue?
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
from the library. edit: |
git hash updated to the same version used in WLED-MM - does not include <FastLED.h> any more
I've just updated the git hash to the same version that's used in WLED-MM. Compiles locally, fingers crossed for the CI build |
|
@DedeHai how about renaming the folder |
good idea, so it is unmistakenly labeled as "not full fastled". |
"fastled-slim" sounds good :-) |
@DedeHai I think you did even more than what was needed, by creating a separate component that stays on MIT terms. So all good 🎉 FastLED uses MIT license, so importing their code into our EUPL project is always possible, because our license conditions (attribution, contribution) are stronger than MIT - EUPL satisfies the MIT needs easily, while an MIT project cannot re-use EUPL source code without explicit agreement from the EUPL project. Basically, MIT is the "grab and run" license - anybody can re-use their source code (even under closed source) if originator and limited liability notices are preserved. |
the "WLED" variant was never part of our "official" API.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/FX.cpp (1)
6134-6153:⚠️ Potential issue | 🟠 MajorGive Crazy Bees its own PRNG state.
Line 6149 reseeds the shared global
prng, and Lines 6134-6136 consume from that same generator insidebee_t::aimed(). Because later re-aims keep advancing the shared instance, this effect’s paths depend on whatever other segments/effects usedprngbefore it, and it perturbs them in return. That undermines the repeatable/sync behavior this PR is introducing. Please keep a per-segment seed/PRNG for Crazy Bees, or save and restore the global seed around every call toaimed().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.cpp` around lines 6134 - 6153, The Crazy Bees effect currently reseeds and consumes the shared global prng (see prng.setSeed(strip.now), bee_t::aimed(), and later re-aims), which breaks determinism; fix by giving Crazy Bees its own PRNG instance/state: introduce a local PRNG member or local variable used only by this segment (seeded from strip.now and/or a segment-specific value) and use that PRNG for all calls to bee_t::aimed(), both during initialization and on subsequent re-aims, or alternatively save the global prng state before each aimed() call and restore it afterward; update references to prng in the bee_t::aimed implementation and in the loop that initializes bee[i].aimed(cols, rows) to use the new per-segment PRNG/state (or the save/restore calls) so other effects and SEGENV.call behavior remain unaffected.
♻️ Duplicate comments (3)
wled00/FX.h (1)
755-756:⚠️ Potential issue | 🟠 MajorKeep the
WLED_DISABLE_2Dstubs aligned with the 2D API.The
#elsebranch still exposesblurRow/blurCol, drops severalconstqualifiers, and changesaddPixelColorXY(..., bool)frompreserveCR = truetofalse. That makes 1D-only builds diverge in both compile surface and blending behavior.🛠️ Proposed fix
- inline bool isPixelXYClipped(int x, int y) { return isPixelClipped(x); } - inline uint32_t getPixelColorXY(int x, int y) { return getPixelColor(x); } - inline void blendPixelColorXY(uint16_t x, uint16_t y, uint32_t c, uint8_t blend) { blendPixelColor(x, c, blend); } - inline void blendPixelColorXY(uint16_t x, uint16_t y, CRGB c, uint8_t blend) { blendPixelColor(x, RGBW32(c.r,c.g,c.b,0), blend); } - inline void addPixelColorXY(int x, int y, uint32_t color, bool saturate = false) { addPixelColor(x, color, saturate); } - inline void addPixelColorXY(int x, int y, byte r, byte g, byte b, byte w = 0, bool saturate = false) { addPixelColor(x, RGBW32(r,g,b,w), saturate); } - inline void addPixelColorXY(int x, int y, CRGB c, bool saturate = false) { addPixelColor(x, RGBW32(c.r,c.g,c.b,0), saturate); } - inline void fadePixelColorXY(uint16_t x, uint16_t y, uint8_t fade) { fadePixelColor(x, fade); } - //inline void box_blur(unsigned i, bool vertical, uint8_t blur_amount) {} - inline void blur2D(uint8_t blur_x, uint8_t blur_y, bool smear = false) {} - inline void blurRow(int row, uint8_t blur_amount, bool smear = false) {} - inline void blurCol(int col, uint8_t blur_amount, bool smear = false) {} + inline bool isPixelXYClipped(int x, int y) const { return isPixelClipped(x); } + inline uint32_t getPixelColorXY(int x, int y) const { return getPixelColor(x); } + inline void blendPixelColorXY(uint16_t x, uint16_t y, uint32_t c, uint8_t blend) const { blendPixelColor(x, c, blend); } + inline void blendPixelColorXY(uint16_t x, uint16_t y, CRGB c, uint8_t blend) const { blendPixelColor(x, RGBW32(c.r,c.g,c.b,0), blend); } + inline void addPixelColorXY(int x, int y, uint32_t color, bool preserveCR = true) const { addPixelColor(x, color, preserveCR); } + inline void addPixelColorXY(int x, int y, byte r, byte g, byte b, byte w = 0, bool preserveCR = true) { addPixelColorXY(x, y, RGBW32(r,g,b,w), preserveCR); } + inline void addPixelColorXY(int x, int y, CRGB c, bool preserveCR = true) const { addPixelColor(x, RGBW32(c.r,c.g,c.b,0), preserveCR); } + inline void fadePixelColorXY(uint16_t x, uint16_t y, uint8_t fade) const { fadePixelColor(x, fade); } + //inline void box_blur(unsigned i, bool vertical, uint8_t blur_amount) {} + inline void blur2D(uint8_t blur_x, uint8_t blur_y, bool smear = false) const {} + inline void blurCols(uint8_t blur_amount, bool smear = false) const {} + inline void blurRows(uint8_t blur_amount, bool smear = false) const {}Also applies to: 784-795
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.h` around lines 755 - 756, The WLED_DISABLE_2D `#else` stubs must match the 2D API exactly: make the stubbed methods blurCols and blurRows call blur2D with the same signatures and const qualifiers (i.e., inline void blurCols(uint8_t, bool smear=false) const { blur2D(0, blur_amount, smear); } and inline void blurRows(...) const { blur2D(...); }), restore any removed const qualifiers, and ensure addPixelColorXY(...) in the 1D-only branch preserves the original default preserveCR = true; update the stub declarations for the functions referenced (blurCols, blurRows, blur2D, addPixelColorXY) so the compile surface and blending behavior remain identical under WLED_DISABLE_2D.wled00/colors.cpp (2)
376-380:⚠️ Potential issue | 🔴 CriticalRestore the gray fast path before the hue division.
Any non-black gray reaches this block with
delta == 0, so Lines 378-380 divide by zero.🛠️ Proposed fix
delta = maxval - minval; - hsv.s = delta == maxval ? 255 : (255 * delta) / maxval; // faster on fully saturated colors, slightly slower otherwise - // note: early return if s==0 is omitted here to increase speed as gray values are rarely used + if (delta == 0) { + hsv.h = 0; + hsv.s = 0; + return; + } + hsv.s = delta == maxval ? 255 : (255 * delta) / maxval; // faster on fully saturated colors, slightly slower otherwise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/colors.cpp` around lines 376 - 380, The hue calculation divides by delta without checking for delta==0, causing divide-by-zero for non-black grays; restore the gray fast path by checking if delta==0 (or s==0) immediately after computing hsv.s and before the hue assignment, and set hsv.h to 0 (or an appropriate default) and return/skip hue math; update the block around hsv.s, hsv.h, delta, maxval, r, g, b to perform this early exit so Lines computing hsv.h (the 10923/21845/43690 multipliers) never execute when delta==0.
52-53:⚠️ Potential issue | 🔴 CriticalInclude
win the preserve-ratio max calculation.If only the white channel overflows,
maxvalstays0here and the next line divides by zero.color_add(RGBW32(0,0,0,255), RGBW32(0,0,0,255), true)is enough to trigger it.🛠️ Proposed fix
- uint32_t maxval = (r > g) ? ((r > b) ? r : b) : ((g > b) ? g : b); // find max value. note: faster than using max() function or on par - const uint32_t scale = (uint32_t(255)<<8) / maxval; // division of two 8bit (shifted) values does not work -> use bit shifts and multiplaction instead + uint32_t maxval = r; + if (g > maxval) maxval = g; + if (b > maxval) maxval = b; + if (w > maxval) maxval = w; + const uint32_t scale = (uint32_t(255) << 8) / maxval; // division of two 8bit (shifted) values does not work -> use bit shifts and multiplication instead🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/colors.cpp` around lines 52 - 53, The max calculation for scaling ignores the white channel and can leave maxval zero when only w is nonzero (e.g., color_add / RGBW32 with w=255), causing a divide-by-zero in the computation of scale; update the max determination (the expression that computes maxval) to include w alongside r, g, b so maxval = max(r,g,b,w), and ensure the same symbols (maxval, r, g, b, w, scale, color_add, RGBW32) are used so the division uses a nonzero denominator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/colors.h`:
- Around line 178-183: The left-shift of a potentially negative signed parameter
causes undefined behavior; in adjust_hue (colors.h) and adjust_color
(colors.cpp) replace the "hueshift << 8" style shift with a safe multiplication
by 256, ensuring the multiplication is done in a signed type before adding to
the HSV hue field (e.g., cast hueshift to int32_t and multiply by 256, then
add/assign to hsv.h) so negative values behave correctly and no left-shift on a
negative signed integer occurs.
In `@wled00/FX.cpp`:
- Around line 1814-1817: The PRNG state is being advanced by SEGENV.step =
RGBW32(prng.random8(), prng.random8(), prng.random8(), 0) and SEGENV.aux0 =
prng.random16() before prevSeed is captured, so the first render still perturbs
the shared prng; move the snapshot so prevSeed = prng.getSeed() occurs before
any prng.random* calls (or capture seed into a variable first and use that to
seed a local PRNG), then leave the existing restore logic intact at the end of
mode_random_chase()/the surrounding function to restore the global prng state.
---
Outside diff comments:
In `@wled00/FX.cpp`:
- Around line 6134-6153: The Crazy Bees effect currently reseeds and consumes
the shared global prng (see prng.setSeed(strip.now), bee_t::aimed(), and later
re-aims), which breaks determinism; fix by giving Crazy Bees its own PRNG
instance/state: introduce a local PRNG member or local variable used only by
this segment (seeded from strip.now and/or a segment-specific value) and use
that PRNG for all calls to bee_t::aimed(), both during initialization and on
subsequent re-aims, or alternatively save the global prng state before each
aimed() call and restore it afterward; update references to prng in the
bee_t::aimed implementation and in the loop that initializes bee[i].aimed(cols,
rows) to use the new per-segment PRNG/state (or the save/restore calls) so other
effects and SEGENV.call behavior remain unaffected.
---
Duplicate comments:
In `@wled00/colors.cpp`:
- Around line 376-380: The hue calculation divides by delta without checking for
delta==0, causing divide-by-zero for non-black grays; restore the gray fast path
by checking if delta==0 (or s==0) immediately after computing hsv.s and before
the hue assignment, and set hsv.h to 0 (or an appropriate default) and
return/skip hue math; update the block around hsv.s, hsv.h, delta, maxval, r, g,
b to perform this early exit so Lines computing hsv.h (the 10923/21845/43690
multipliers) never execute when delta==0.
- Around line 52-53: The max calculation for scaling ignores the white channel
and can leave maxval zero when only w is nonzero (e.g., color_add / RGBW32 with
w=255), causing a divide-by-zero in the computation of scale; update the max
determination (the expression that computes maxval) to include w alongside r, g,
b so maxval = max(r,g,b,w), and ensure the same symbols (maxval, r, g, b, w,
scale, color_add, RGBW32) are used so the division uses a nonzero denominator.
In `@wled00/FX.h`:
- Around line 755-756: The WLED_DISABLE_2D `#else` stubs must match the 2D API
exactly: make the stubbed methods blurCols and blurRows call blur2D with the
same signatures and const qualifiers (i.e., inline void blurCols(uint8_t, bool
smear=false) const { blur2D(0, blur_amount, smear); } and inline void
blurRows(...) const { blur2D(...); }), restore any removed const qualifiers, and
ensure addPixelColorXY(...) in the 1D-only branch preserves the original default
preserveCR = true; update the stub declarations for the functions referenced
(blurCols, blurRows, blur2D, addPixelColorXY) so the compile surface and
blending behavior remain identical under WLED_DISABLE_2D.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 494688e4-15b9-47f4-a72c-f10624132da0
📒 Files selected for processing (11)
usermods/user_fx/user_fx.cppusermods/usermod_v2_animartrix/library.jsonwled00/FX.cppwled00/FX.hwled00/FX_fcn.cppwled00/colors.cppwled00/colors.hwled00/const.hwled00/json.cppwled00/palettes.cppwled00/util.cpp
| // adjust hue: input range is 256 for full color cycle, input can be negative | ||
| inline void adjust_hue(int hueshift) __attribute__((always_inline)) { | ||
| CHSV32 hsv = *this; | ||
| hsv.h += hueshift << 8; | ||
| hsv2rgb_spectrum(hsv, *this); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find where adjust_hue is used and with what arguments
rg "adjust_hue" --type cpp --type h -B 2 -A 2Repository: wled/WLED
Length of output: 1688
🏁 Script executed:
# Check CHSV32 definition and related code around colors.h
grep -n "class CHSV32\|struct CHSV32" wled00/colors.h -A 20Repository: wled/WLED
Length of output: 2423
🏁 Script executed:
# Look for any explicit handling of negative shifts or hue adjustments in the codebase
rg "hueshift|hue.*shift|shift.*hue" --type cpp --type h -iRepository: wled/WLED
Length of output: 1816
🏁 Script executed:
# Find the adjust_hue call in ir.cpp and get context around it
grep -n "adjust_hue(amount)" wled00/ir.cpp -B 10 -A 5Repository: wled/WLED
Length of output: 781
🏁 Script executed:
# Check what adjust_color does with hueshift to see if it handles negative differently
grep -n "adjust_color\|hueShift" wled00/colors.cpp -B 2 -A 5 | head -50Repository: wled/WLED
Length of output: 773
🏁 Script executed:
# Verify C++ standard behavior - check if any comments or documentation about this exist
rg "undefined|UB|shift.*negative|negative.*shift" --type cpp --type h wled00/ | grep -i "hue\|shift"Repository: wled/WLED
Length of output: 2345
🏁 Script executed:
# Get more context on where 'amount' comes from in ir.cpp
grep -n "amount\s*=" wled00/ir.cpp | head -20Repository: wled/WLED
Length of output: 35
🏁 Script executed:
# Search for the function containing the adjust_hue call to see what amount is
grep -n "adjustHue\|void.*hue" wled00/ir.cpp -B 5 -A 15Repository: wled/WLED
Length of output: 35
🏁 Script executed:
# Get broader context around the adjust_hue call in ir.cpp (lines 130-140)
sed -n '100,160p' wled00/ir.cppRepository: wled/WLED
Length of output: 2126
🏁 Script executed:
# Search for where amount variable is defined before the adjust_hue call
sed -n '1,135p' wled00/ir.cpp | tail -50Repository: wled/WLED
Length of output: 1635
🏁 Script executed:
# Let's check if there are actual negative values passed or if amount is always positive
rg "amount\s*=|int\s+amount" wled00/ir.cpp -B 5 -A 2Repository: wled/WLED
Length of output: 35
🏁 Script executed:
# Get the full function signature and context
sed -n '110,165p' wled00/ir.cppRepository: wled/WLED
Length of output: 1977
🏁 Script executed:
# Check if there are any other places where left-shift is used with signed values and how they handle it
rg "<<\s*8|<<\s*16" wled00/colors.cpp wled00/colors.h -B 1 -A 1 | head -40Repository: wled/WLED
Length of output: 2570
🏁 Script executed:
# Check the adjust_hue function definition and signature one more time
sed -n '175,190p' wled00/colors.hRepository: wled/WLED
Length of output: 446
🏁 Script executed:
# Let's verify the C++ standard on left-shift of signed integers
# Check if there are any compiler flags or pragmas that might affect this behavior
grep -r "pragma\|attribute" wled00/colors.h | grep -i "shift\|ub\|undefined"Repository: wled/WLED
Length of output: 124
🏁 Script executed:
# Check if similar shift operations on signed values appear elsewhere with explicit handling
rg "<<.*8|<<.*16" wled00/ --type cpp -B 2 -A 2 | grep -E "int8_t|int16_t|int32_t" -B 3 -A 1 | head -40Repository: wled/WLED
Length of output: 2234
🏁 Script executed:
# Check the C++ standard used by WLED
find . -name "CMakeLists.txt" -o -name "*.pio" -o -name "platformio.ini" -o -name ".clang-tidy" -o -name ".clang-format" | head -5Repository: wled/WLED
Length of output: 83
🏁 Script executed:
# Check for CMakeLists.txt and what C++ version is specified
grep -r "std=c++\|CXX_STANDARD\|cxx_std" . --include="CMakeLists.txt" --include="platformio.ini" 2>/dev/null | head -10Repository: wled/WLED
Length of output: 35
🏁 Script executed:
# Check if there's any documentation about this in the code or comments
rg "undefined.*behavior|left.shift.*negative|negative.*shift" --type cpp --type h wled00/colors.h wled00/colors.cppRepository: wled/WLED
Length of output: 35
🏁 Script executed:
# Check platformio.ini for C++ standard settings
grep -i "std\|cpp\|cxx" ./platformio.ini | head -20Repository: wled/WLED
Length of output: 35
🏁 Script executed:
# Look at the actual contents of platformio.ini
head -100 ./platformio.iniRepository: wled/WLED
Length of output: 4654
🏁 Script executed:
# Check compiler flags in platformio.ini more carefully
grep -i "build_flags\|cxxflags\|cflags" platformio.ini | head -30Repository: wled/WLED
Length of output: 2253
🏁 Script executed:
# Let's also check if there are environment-specific settings
grep -A 30 "\[env:" platformio.ini | grep -E "build_flags|cxxflags|std=" | head -20Repository: wled/WLED
Length of output: 2264
🏁 Script executed:
# Check what the actual behavior is - let's look for Arduino/esp8266 compiler documentation or any related notes in the code
rg "c\+\+11|c\+\+14|c\+\+17|std=" . --type cpp --type h --type ini | head -10Repository: wled/WLED
Length of output: 80
🏁 Script executed:
# Let's check the actual C++ standard compliance by looking for any compiler options related to undefined behavior or sanitizers
grep -r "fsanitize\|Wshift\|pedantic" platformio.ini ./.clang-tidy 2>/dev/nullRepository: wled/WLED
Length of output: 35
🏁 Script executed:
# Look at the .clang-tidy file
cat ./.clang-tidy 2>/dev/null || echo "File not found or empty"Repository: wled/WLED
Length of output: 829
🏁 Script executed:
# Now let's write a test to understand the actual behavior
python3 << 'EOF'
# According to C++ standard, left-shifting a negative signed integer is undefined behavior.
# Let's verify the semantics of the suggested fix.
# Original: hsv.h += hueshift << 8;
# where hsv.h is uint16_t and hueshift is int
# Example with hueshift = -5 (int8_t or int)
hueshift = -5
# What the original code tries to do: multiply by 256
# Left-shift by 8 is equivalent to multiply by 256 for positive values
# For negative: -5 << 8 is undefined behavior
# Suggested fix: int32_t(hueshift) * 256
result = int(hueshift) * 256
print(f"hueshift = {hueshift}")
print(f"hueshift * 256 = {result}")
print(f"Result as uint16_t (wraparound): {result & 0xFFFF}")
# The fix converts to signed 32-bit and multiplies, avoiding the shift on negative
# Then the result gets added to hsv.h (uint16_t) which triggers unsigned arithmetic
print("\nThe semantics appear equivalent for small negative values.")
EOFRepository: wled/WLED
Length of output: 188
🏁 Script executed:
# Search for any documented standards compliance or known issues related to this
rg "shift.*negative|negative.*shift|undefined.*behavior" --type cpp --type h wled00/ -iRepository: wled/WLED
Length of output: 2220
Fix left-shift of potentially negative hueshift value.
Line 181 left-shifts hueshift even though the function accepts negative inputs (as confirmed by the comment and actual usage in ir.cpp::changeEffectSpeed(), which calls adjust_hue(amount) with signed amount that can be negative). Left-shifting a negative signed integer is undefined behavior in C++. The same issue exists in colors.cpp::adjust_color() at line 103.
Use multiplication instead to avoid undefined behavior:
Safer arithmetic
- hsv.h += hueshift << 8;
+ hsv.h = uint16_t(int32_t(hsv.h) + int32_t(hueshift) * 256);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/colors.h` around lines 178 - 183, The left-shift of a potentially
negative signed parameter causes undefined behavior; in adjust_hue (colors.h)
and adjust_color (colors.cpp) replace the "hueshift << 8" style shift with a
safe multiplication by 256, ensuring the multiplication is done in a signed type
before adding to the HSV hue field (e.g., cast hueshift to int32_t and multiply
by 256, then add/assign to hsv.h) so negative values behave correctly and no
left-shift on a negative signed integer occurs.
| SEGENV.step = RGBW32(prng.random8(), prng.random8(), prng.random8(), 0); | ||
| SEGENV.aux0 = prng.random16(); | ||
| } | ||
| unsigned prevSeed = random16_get_seed(); // save seed so we can restore it at the end of the function | ||
| unsigned prevSeed = prng.getSeed(); // save seed so we can restore it at the end of the function |
There was a problem hiding this comment.
Snapshot the PRNG state before the init draws.
Lines 1814-1815 consume from the shared prng before Line 1817 captures prevSeed, so the first render of mode_random_chase() still perturbs the global sequence even though the steady-state loop restores it.
🛠️ Minimal fix
- if (SEGENV.call == 0) {
- SEGENV.step = RGBW32(prng.random8(), prng.random8(), prng.random8(), 0);
- SEGENV.aux0 = prng.random16();
- }
- unsigned prevSeed = prng.getSeed(); // save seed so we can restore it at the end of the function
+ unsigned prevSeed = prng.getSeed(); // save seed so we can restore it at the end of the function
+ if (SEGENV.call == 0) {
+ SEGENV.step = RGBW32(prng.random8(), prng.random8(), prng.random8(), 0);
+ SEGENV.aux0 = prng.random16();
+ }📝 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.
| SEGENV.step = RGBW32(prng.random8(), prng.random8(), prng.random8(), 0); | |
| SEGENV.aux0 = prng.random16(); | |
| } | |
| unsigned prevSeed = random16_get_seed(); // save seed so we can restore it at the end of the function | |
| unsigned prevSeed = prng.getSeed(); // save seed so we can restore it at the end of the function | |
| unsigned prevSeed = prng.getSeed(); // save seed so we can restore it at the end of the function | |
| if (SEGENV.call == 0) { | |
| SEGENV.step = RGBW32(prng.random8(), prng.random8(), prng.random8(), 0); | |
| SEGENV.aux0 = prng.random16(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/FX.cpp` around lines 1814 - 1817, The PRNG state is being advanced by
SEGENV.step = RGBW32(prng.random8(), prng.random8(), prng.random8(), 0) and
SEGENV.aux0 = prng.random16() before prevSeed is captured, so the first render
still perturbs the shared prng; move the snapshot so prevSeed = prng.getSeed()
occurs before any prng.random* calls (or capture seed into a variable first and
use that to seed a local PRNG), then leave the existing restore logic intact at
the end of mode_random_chase()/the surrounding function to restore the global
prng state.
|
@DedeHai two newer effects still have small compatibility problems: Compiling .pio\build\usermods\libda9\user_fx\user_fx.cpp.o
usermods/user_fx/user_fx.cpp: In function 'void mode_spinning_wheel()':
usermods/user_fx/user_fx.cpp:151:5: error: 'random16_set_seed' was not declared in this scope
random16_set_seed(hw_random16());
^~~~~~~~~~~~~~~~~
usermods/user_fx/user_fx.cpp:159:5: error: 'random16_add_entropy' was not declared in this scope
random16_add_entropy(hw_random16());
^~~~~~~~~~~~~~~~~~~~
usermods/user_fx/user_fx.cpp: In static member function 'static void mode_spinning_wheel()::virtualStrip::runStrip(uint16_t, uint32_t*, bool, bool, unsigned int)':
usermods/user_fx/user_fx.cpp:218:33: error: 'random16' was not declared in this scope
state[VELOCITY_IDX] = random16(200, 900) * 655; // fixed-point velocity scaling (approx. 65536/100)
^~~~~~~~
usermods/user_fx/user_fx.cpp:226:44: error: 'random16' was not declared in this scope
state[SLOWDOWN_TIME_IDX] = now + random16(2000, 6000);
^~~~~~~~==> should we add some pseudo-random stuff to the lib, or is hw_random() equally fast? Compiling .pio\build\usermods\src\FX.cpp.o
wled00/FX.cpp: In function 'void mode_ColorClouds()':
wled00/FX.cpp:5010:12: error: 'hsv2rgb' was not declared in this scope
else { hsv2rgb(CHSV32(hue, 255, vol), pixel); }
^~~~~~~==> what was the CHSV32 equivalent for |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
usermods/user_fx/user_fx.cpp (1)
149-159:⚠️ Potential issue | 🔴 CriticalReplace the removed FastLED RNG calls before this lands.
The palette-call swap on Line 317 is fine, but
mode_spinning_wheel()still referencesrandom16_set_seed,random16_add_entropy, andrandom16, and CI is already failing on those symbols. This file will not build after the FastLED removal until those calls are migrated to thePRNGclass instance (matching the pattern inwled00/FX.cpp:86) or tohw_random*functions if determinism is not required.Also applies to: 218-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/user_fx/user_fx.cpp` around lines 149 - 159, mode_spinning_wheel and its surrounding init currently call FastLED RNG symbols (random16_set_seed, random16_add_entropy, random16) which were removed; replace those calls with the project's PRNG instance methods (use the same PRNG usage pattern as in FX.cpp where PRNG.seed()/PRNG.rand16()/PRNG.addEntropy() are used) or call hw_random16()/hw_random* directly if you don't need deterministic PRNG state; update references to random16_set_seed -> PRNG.seed(hw_random16() or value), random16_add_entropy -> PRNG.addEntropy(hw_random16()), and random16 -> PRNG.rand16() (or hw_random16()) and ensure SEGENV/aux1 handling and any cached scaling logic remains unchanged in mode_spinning_wheel and the other affected block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@usermods/user_fx/user_fx.cpp`:
- Around line 149-159: mode_spinning_wheel and its surrounding init currently
call FastLED RNG symbols (random16_set_seed, random16_add_entropy, random16)
which were removed; replace those calls with the project's PRNG instance methods
(use the same PRNG usage pattern as in FX.cpp where
PRNG.seed()/PRNG.rand16()/PRNG.addEntropy() are used) or call
hw_random16()/hw_random* directly if you don't need deterministic PRNG state;
update references to random16_set_seed -> PRNG.seed(hw_random16() or value),
random16_add_entropy -> PRNG.addEntropy(hw_random16()), and random16 ->
PRNG.rand16() (or hw_random16()) and ensure SEGENV/aux1 handling and any cached
scaling logic remains unchanged in mode_spinning_wheel and the other affected
block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6c14093d-dc6d-458a-a157-e9e08ff990b4
📒 Files selected for processing (1)
usermods/user_fx/user_fx.cpp
I only pulled in main, I did not test it yet. The main issue is: I did so many optimizations strewn accross several PRs that all were hanging in there for way too long, now I do not remember which version was the most optimized... so I took a best guess when merging Main back in. It may have broken some things. I still need to test low-brightness colors again in case I optimized and now de-optimized with this PR.
hw_random is "instant" it just reads a hardware register and truncates, so should always be fastest. PRNG is only to be used if predictable/repeatable randomness is required.
same. its an overloaded function (or it used to be or will be, whichever is the most "future" PR) :) |
I did it on purpose, I do not want to claim ownership of those functions, if Fastled wants the optimized versions back (after they clean up after their AI's ;) ) they can use my versions. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/FX.cpp (1)
6133-6153:⚠️ Potential issue | 🔴 CriticalFix coordinate and error field overflow in Crazy Bees effect.
The
Beestructure usesuint8_tfor positions whileaimed()receivesuint16_tmatrix dimensions and callsrandom8(uint8_t min, uint8_t lim), causing implicit truncation. On matrices > 256 pixels wide or tall, bounds are truncated (e.g., width 512 becomes 0), preventing bees from reaching far edges. Additionally,deltaX,deltaY, anderrorfields areint8_tbut can hold values exceeding ±127 during Bresenham calculations, causing integer overflow. Widen the position/error fields touint16_t/int16_t, passrandom16()calls with a per-segment PRNG instance seeded fromSEGENVrather than the shared globalprng.Suggested fix
typedef struct Bee { - uint8_t posX, posY, aimX, aimY, hue; - int8_t deltaX, deltaY, signX, signY, error; - void aimed(uint16_t w, uint16_t h) { - aimX = prng.random8(0, w); - aimY = prng.random8(0, h); - hue = prng.random8(); + uint16_t posX, posY, aimX, aimY; + uint8_t hue; + int16_t deltaX, deltaY, error; + int8_t signX, signY; + void aimed(uint16_t w, uint16_t h, PRNG& rng) { + aimX = rng.random16(w); + aimY = rng.random16(h); + hue = rng.random8(); deltaX = abs(aimX - posX); deltaY = abs(aimY - posY); signX = posX < aimX ? 1 : -1; signY = posY < aimY ? 1 : -1; error = deltaX - deltaY; }; } bee_t;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.cpp` around lines 6133 - 6153, The bee_t structure and its aimed() initializer must be updated to avoid truncation and overflow: change posX/posY to uint16_t and deltaX/deltaY/error to int16_t, update the aimed(uint16_t w, uint16_t h) signature to use those types, and replace prng.random8(...) calls with prng.random16(...) (or random16(min, lim)) so matrix dimensions >256 are handled correctly; also stop using the shared global prng for per-segment initialization—instantiate or derive a local PRNG seeded from SEGENV (e.g., seed = SEGENV.seed or strip.now stored per-segment) and use that when seeding and generating random positions/aims in the SEGENV.call == 0 initialization block.
♻️ Duplicate comments (2)
wled00/colors.h (1)
178-183:⚠️ Potential issue | 🟠 MajorAvoid undefined behavior in
adjust_hue().The comment says
hueshiftcan be negative, but Line 181 doeshueshift << 8. In C++, left-shifting a negative signed value is undefined behavior, so this helper can produce compiler-dependent hue changes.🛠️ Proposed fix
inline void adjust_hue(int hueshift) __attribute__((always_inline)) { CHSV32 hsv = *this; - hsv.h += hueshift << 8; + hsv.h = uint16_t(int32_t(hsv.h) + int32_t(hueshift) * 256); hsv2rgb_spectrum(hsv, *this); }#!/bin/bash # Confirm the signed left shift remains in hue-adjust paths. sed -n '178,183p' wled00/colors.h rg -nP 'hueshift\s*<<\s*8|hueShift\s*<<\s*8' wled00/colors.h -C2 fd 'colors\.cpp$' wled00 -x rg -nP 'hueshift\s*<<\s*8|hueShift\s*<<\s*8' {} -C2Expected: no signed hue shifts remain; use signed multiplication before casting back.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/colors.h` around lines 178 - 183, The adjust_hue helper uses a left shift on a potentially negative signed value (hueshift << 8) which is undefined; change adjust_hue in colors.h so you compute the 8-bit-scaled offset using a defined signed operation (e.g. multiply hueshift by 256 into a signed integer) and then add that to CHSV32::h before calling hsv2rgb_spectrum; update the line modifying hsv.h (the current "hsv.h += hueshift << 8") to use a signed multiplication/cast to avoid shifting a negative value and ensure correct wrapping behavior.wled00/FX.cpp (1)
1813-1817:⚠️ Potential issue | 🟡 MinorCapture the shared PRNG state before the first-call draws.
Line 1817 runs after Line 1814 and Line 1815 already consumed from
prng, so the first render still perturbs the shared sequence even though you restore it at the end.🛠️ Minimal fix
- if (SEGENV.call == 0) { - SEGENV.step = RGBW32(prng.random8(), prng.random8(), prng.random8(), 0); - SEGENV.aux0 = prng.random16(); - } - unsigned prevSeed = prng.getSeed(); // save seed so we can restore it at the end of the function + unsigned prevSeed = prng.getSeed(); // save seed so we can restore it at the end of the function + if (SEGENV.call == 0) { + SEGENV.step = RGBW32(prng.random8(), prng.random8(), prng.random8(), 0); + SEGENV.aux0 = prng.random16(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.cpp` around lines 1813 - 1817, Currently the code saves the PRNG state with prng.getSeed() after calling prng.random8()/random16(), which still mutates the shared sequence; move the call to prng.getSeed() so it runs before any calls to prng.random8() or prng.random16() (i.e., capture prevSeed at the top of the branch that checks SEGENV.call == 0) so you can restore the exact original state at function end; update references around SEGENV.call, SEGENV.step, SEGENV.aux0 and the subsequent prng.restore/seed-restore logic to use that saved prevSeed.
🧹 Nitpick comments (5)
wled00/colors.h (1)
4-4: Makecolors.hself-contained forstd::vector.
colors.hdeclaresstd::vector<CRGBPalette16>on line 64 but never includes<vector>.fastled_slim.hdoes not provide this header transitively, so the code relies on include order from consuming files. This breaks the header's self-containment.♻️ Suggested change
+#include <vector> `#include` "src/dependencies/fastled_slim/fastled_slim.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/colors.h` at line 4, The header colors.h is not self-contained: it uses std::vector<CRGBPalette16> (and possibly other std types) but does not include <vector>; add the missing standard header(s) to colors.h (e.g., `#include` <vector>) so that CRGBPalette16 and std::vector are usable without relying on transitive includes from fastled_slim.h; update colors.h to directly include the required <vector> header (and any other needed <utility>/<cstddef> if used) so the declarations compile when colors.h is included standalone.wled00/FX.cpp (1)
10461-10486: Reuse the precomputedmidsvalue.Line 10485 shadows Line 10461, so the FFT reduction and
sqrt32_bw()still run once per particle in this hot path. Compute it once, ideally only whenSEGMENT.check1is enabled.♻️ Small cleanup
- int mids = sqrt32_bw((int)fftResult[5] + (int)fftResult[6] + (int)fftResult[7] + (int)fftResult[8] + (int)fftResult[9] + (int)fftResult[10]); // average the mids, bin 5 is ~500Hz, bin 10 is ~2kHz (see audio_reactive.h) + const int mids = SEGMENT.check1 + ? sqrt32_bw((int)fftResult[5] + (int)fftResult[6] + (int)fftResult[7] + (int)fftResult[8] + (int)fftResult[9] + (int)fftResult[10]) + : 0; @@ - int mids = sqrt32_bw((int)fftResult[5] + (int)fftResult[6] + (int)fftResult[7] + (int)fftResult[8] + (int)fftResult[9] + (int)fftResult[10]); // average the mids, bin 5 is ~500Hz, bin 10 is ~2kHz (see audio_reactive.h) PartSys->particles[i].hue += (mids * perlin8(PartSys->particles[i].x << 2, SEGMENT.step << 2)) >> 9; // color by perlin noise from mid frequencies🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.cpp` around lines 10461 - 10486, The variable mids is recomputed inside the particle loop (using sqrt32_bw on fftResult bins) causing expensive repeated work; compute mids once outside the for-loop only when SEGMENT.check1 is true (or if you need it earlier, compute it once when SEGMENT.check1 is enabled) and then reuse that precomputed mids inside the loop where PartSys->particles[i].hue is modified; update references to the inner declaration so they use the outer mids and remove the duplicated sqrt32_bw call to avoid per-particle FFT reduction.wled00/src/dependencies/fastled_slim/fastled_slim.h (1)
99-100: Nitpick: Typo "UNITIALIZED" should be "UNINITIALIZED".- // `@warning` default values are UNITIALIZED! + // `@warning` Default values are UNINITIALIZED!🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/src/dependencies/fastled_slim/fastled_slim.h` around lines 99 - 100, Fix the typo in the comment above the CHSV default constructor: change "UNITIALIZED" to "UNINITIALIZED" in the comment that precedes the inline CHSV() __attribute__((always_inline)) = default; declaration so the warning reads "// `@warning` default values are UNINITIALIZED!".wled00/src/dependencies/fastled_slim/fastled_slim.cpp (1)
231-231: Nitpick: Typo "quadradic" should be "quadratic".-// quadradic ease function (S-curve: x^2) +// quadratic ease function (S-curve: x^2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/src/dependencies/fastled_slim/fastled_slim.cpp` at line 231, Fix the spelling typo in the comment describing the ease function: change "quadradic" to "quadratic" in the comment that currently reads "quadradic ease function (S-curve: x^2)" so the comment correctly reads "quadratic ease function (S-curve: x^2)"; locate this comment near the ease function implementation in fastled_slim.cpp to update the text.wled00/util.cpp (1)
1002-1010: Optional:gradient1Dis no longer force-inlined unlikegradient2D/gradient3D.The comment on line 1002 states "forcing inline produces smaller code and makes it 3x faster," but
gradient1Dwas changed fromstatic inline __attribute__((always_inline))to juststatic, whilegradient2Dandgradient3Dretain the force-inline attribute. This inconsistency may be intentional, but if the 3× speedup applies to 1D as well, consider restoring the inline attribute for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/util.cpp` around lines 1002 - 1010, The comment notes that forcing inline yields much smaller/faster code but gradient1D was changed to plain static while gradient2D/gradient3D still use force-inlining; restore the original inline attribute on gradient1D (e.g., add back static inline __attribute__((always_inline))) so its definition matches gradient2D/gradient3D and benefits from the same optimization; locate the function named gradient1D and update its declaration to match the inlining used for gradient2D/gradient3D (keeping existing logic that uses hashToGradient and PERLIN_SHIFT).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/colors.h`:
- Line 4: The license attribution path in colors.h is inconsistent with the
renamed dependency folder; update the license reference lines that currently
point to /src/dependencies/fastled/LICENSE.txt to the new path under
src/dependencies/fastled_slim/LICENSE.txt so the in-file attribution matches the
`#include` "src/dependencies/fastled_slim/fastled_slim.h" (locate the include and
the license lines in colors.h and change the path text to
fastled_slim/LICENSE.txt).
In `@wled00/src/dependencies/fastled_slim/fastled_slim.cpp`:
- Line 13: The comment for the bitmask on variable `offset` is wrong: `offset =
h & 0x1FFF` yields a 13-bit range (0..8191), not 0..31; update the inline
comment next to `offset` in `fastled_slim.cpp` to indicate the correct range
(e.g., `// 0..8191 (13 bits)`) and optionally note that `third16 = offset *
21846` / `third = third16 >> 21` expect that 13-bit range so the arithmetic
remains correct.
In `@wled00/src/dependencies/fastled_slim/fastled_slim.h`:
- Around line 609-621: Replace unsafe direct dereferences of PROGMEM in the
CRGBPalette16(const TProgmemRGBPalette16& rhs) constructor and the
CRGBPalette16::operator=(const TProgmemRGBPalette16& rhs) with proper PROGMEM
reads; include <pgmspace.h> and use pgm_read_dword(rhs + i) when loading each
32-bit entry from TProgmemRGBPalette16 in both CRGBPalette16::CRGBPalette16 and
CRGBPalette16::operator= to ensure ESP8266 compatibility and avoid flash access
crashes.
In `@wled00/util.cpp`:
- Around line 400-422: beatsin88_t currently treats its beats_per_minute_88 as
already Q8.8 and thus slows timings when callers pass plain integer BPMs; fix by
encoding plain BPM inputs to Q8.8 inside beatsin88_t (use the same heuristic as
beat16: if beats_per_minute_88 < 256 then left-shift by 8) before calling
beat88; update beatsin88_t (and any internal uses of the beats_per_minute_88
local) to perform this conversion so FX.cpp callers can keep passing plain BPMs,
and reference beat88 and beat16 while making the change.
---
Outside diff comments:
In `@wled00/FX.cpp`:
- Around line 6133-6153: The bee_t structure and its aimed() initializer must be
updated to avoid truncation and overflow: change posX/posY to uint16_t and
deltaX/deltaY/error to int16_t, update the aimed(uint16_t w, uint16_t h)
signature to use those types, and replace prng.random8(...) calls with
prng.random16(...) (or random16(min, lim)) so matrix dimensions >256 are handled
correctly; also stop using the shared global prng for per-segment
initialization—instantiate or derive a local PRNG seeded from SEGENV (e.g., seed
= SEGENV.seed or strip.now stored per-segment) and use that when seeding and
generating random positions/aims in the SEGENV.call == 0 initialization block.
---
Duplicate comments:
In `@wled00/colors.h`:
- Around line 178-183: The adjust_hue helper uses a left shift on a potentially
negative signed value (hueshift << 8) which is undefined; change adjust_hue in
colors.h so you compute the 8-bit-scaled offset using a defined signed operation
(e.g. multiply hueshift by 256 into a signed integer) and then add that to
CHSV32::h before calling hsv2rgb_spectrum; update the line modifying hsv.h (the
current "hsv.h += hueshift << 8") to use a signed multiplication/cast to avoid
shifting a negative value and ensure correct wrapping behavior.
In `@wled00/FX.cpp`:
- Around line 1813-1817: Currently the code saves the PRNG state with
prng.getSeed() after calling prng.random8()/random16(), which still mutates the
shared sequence; move the call to prng.getSeed() so it runs before any calls to
prng.random8() or prng.random16() (i.e., capture prevSeed at the top of the
branch that checks SEGENV.call == 0) so you can restore the exact original state
at function end; update references around SEGENV.call, SEGENV.step, SEGENV.aux0
and the subsequent prng.restore/seed-restore logic to use that saved prevSeed.
---
Nitpick comments:
In `@wled00/colors.h`:
- Line 4: The header colors.h is not self-contained: it uses
std::vector<CRGBPalette16> (and possibly other std types) but does not include
<vector>; add the missing standard header(s) to colors.h (e.g., `#include`
<vector>) so that CRGBPalette16 and std::vector are usable without relying on
transitive includes from fastled_slim.h; update colors.h to directly include the
required <vector> header (and any other needed <utility>/<cstddef> if used) so
the declarations compile when colors.h is included standalone.
In `@wled00/FX.cpp`:
- Around line 10461-10486: The variable mids is recomputed inside the particle
loop (using sqrt32_bw on fftResult bins) causing expensive repeated work;
compute mids once outside the for-loop only when SEGMENT.check1 is true (or if
you need it earlier, compute it once when SEGMENT.check1 is enabled) and then
reuse that precomputed mids inside the loop where PartSys->particles[i].hue is
modified; update references to the inner declaration so they use the outer mids
and remove the duplicated sqrt32_bw call to avoid per-particle FFT reduction.
In `@wled00/src/dependencies/fastled_slim/fastled_slim.cpp`:
- Line 231: Fix the spelling typo in the comment describing the ease function:
change "quadradic" to "quadratic" in the comment that currently reads "quadradic
ease function (S-curve: x^2)" so the comment correctly reads "quadratic ease
function (S-curve: x^2)"; locate this comment near the ease function
implementation in fastled_slim.cpp to update the text.
In `@wled00/src/dependencies/fastled_slim/fastled_slim.h`:
- Around line 99-100: Fix the typo in the comment above the CHSV default
constructor: change "UNITIALIZED" to "UNINITIALIZED" in the comment that
precedes the inline CHSV() __attribute__((always_inline)) = default; declaration
so the warning reads "// `@warning` default values are UNINITIALIZED!".
In `@wled00/util.cpp`:
- Around line 1002-1010: The comment notes that forcing inline yields much
smaller/faster code but gradient1D was changed to plain static while
gradient2D/gradient3D still use force-inlining; restore the original inline
attribute on gradient1D (e.g., add back static inline
__attribute__((always_inline))) so its definition matches gradient2D/gradient3D
and benefits from the same optimization; locate the function named gradient1D
and update its declaration to match the inlining used for gradient2D/gradient3D
(keeping existing logic that uses hashToGradient and PERLIN_SHIFT).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6e61501d-7f75-4e68-98ea-9f00326fcba7
📒 Files selected for processing (8)
usermods/user_fx/user_fx.cppwled00/FX.cppwled00/colors.hwled00/palettes.cppwled00/src/dependencies/fastled_slim/LICENSE.txtwled00/src/dependencies/fastled_slim/fastled_slim.cppwled00/src/dependencies/fastled_slim/fastled_slim.hwled00/util.cpp
✅ Files skipped from review due to trivial changes (2)
- wled00/src/dependencies/fastled_slim/LICENSE.txt
- usermods/user_fx/user_fx.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/palettes.cpp
I checked, there was none, I added it now for compatibility. the "new way" is to use implicit cast: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
wled00/colors.h (2)
58-58:⚠️ Potential issue | 🔴 CriticalParameter order mismatch between declaration and definition.
The declaration here has
(int32_t hueShift, int32_t valueChange, int32_t satChange), but the implementation incolors.cpp:99uses(int32_t hueShift, int32_t satChange, int32_t valueChange). This will cause callers to silently swap saturation and value changes.Proposed fix
-void adjust_color(CRGBW& rgb, int32_t hueShift, int32_t valueChange, int32_t satChange); +void adjust_color(CRGBW& rgb, int32_t hueShift, int32_t satChange, int32_t valueChange);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/colors.h` at line 58, The declaration of adjust_color in colors.h lists parameters as (int32_t hueShift, int32_t valueChange, int32_t satChange) but the implementation adjust_color in colors.cpp uses (int32_t hueShift, int32_t satChange, int32_t valueChange); make the two signatures identical by reordering the parameters to the intended order (preferably (hueShift, satChange, valueChange) or vice versa) and update any callers to match the chosen order so saturation and value changes aren't swapped at runtime.
179-183:⚠️ Potential issue | 🟠 MajorLeft-shift of potentially negative
hueshiftis undefined behavior.Line 181 performs
hueshift << 8wherehueshiftis a signedint. Left-shifting a negative signed integer is undefined behavior in C++. Use multiplication instead.Proposed fix
inline void adjust_hue(int hueshift) __attribute__((always_inline)) { CHSV32 hsv = *this; - hsv.h += hueshift << 8; + hsv.h = uint16_t(int32_t(hsv.h) + int32_t(hueshift) * 256); hsv2rgb_spectrum(hsv, *this); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/colors.h` around lines 179 - 183, In adjust_hue, avoid undefined behavior from left-shifting a potentially negative signed int by replacing the shift expression `hueshift << 8` with a multiplication (e.g., `hueshift * 256`) or an explicit signed-to-wide-type multiply (e.g., `static_cast<int32_t>(hueshift) * 256`) so the math is well-defined before adding to hsv.h; update the expression in the adjust_hue method accordingly.wled00/colors.cpp (3)
375-381:⚠️ Potential issue | 🔴 CriticalDivision by zero for gray colors (when delta == 0).
Lines 378-380 divide by
delta, but for non-black gray colors (wherer == g == b != 0),deltawill be 0. The comment on line 377 states the early return is omitted for speed, but this causes undefined behavior.Proposed fix
hsv.v = maxval; delta = maxval - minval; + if (delta == 0) { + hsv.h = 0; + hsv.s = 0; + return; + } hsv.s = delta == maxval ? 255 : (255 * delta) / maxval; // faster on fully saturated colors, slightly slower otherwise - // note: early return if s==0 is omitted here to increase speed as gray values are rarely used if (maxval == r) hsv.h = (10923 * (g - b)) / delta; else if (maxval == g) hsv.h = 21845 + (10923 * (b - r)) / delta; else hsv.h = 43690 + (10923 * (r - g)) / delta;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/colors.cpp` around lines 375 - 381, The code computes delta = maxval - minval then unconditionally divides by delta when computing hsv.h, which causes divide-by-zero for gray colors (r==g==b). Fix by handling the delta==0 case before any division: when delta == 0 set hsv.s = 0 and set hsv.h to a safe value (e.g., 0 or leave unchanged) and skip the hue-division branches; otherwise compute hsv.s and run the existing hue formulas that divide by delta. Update the block that computes hsv.s and hsv.h (references: delta, maxval, minval, hsv.s, hsv.h, and r/g/b) so divisions only occur when delta > 0.
52-53:⚠️ Potential issue | 🔴 CriticalWhite channel missing from max calculation can cause division by zero.
The
maxvalcalculation ignores the white channelw. If only the white channel overflows (e.g.,color_add(RGBW32(0,0,0,255), RGBW32(0,0,0,255), true)),maxvalwill be 0 causing division by zero on line 53.Proposed fix
- uint32_t maxval = (r > g) ? ((r > b) ? r : b) : ((g > b) ? g : b); // find max value. note: faster than using max() function or on par + uint32_t maxval = r; + if (g > maxval) maxval = g; + if (b > maxval) maxval = b; + if (w > maxval) maxval = w; const uint32_t scale = (uint32_t(255)<<8) / maxval; // division of two 8bit (shifted) values does not work -> use bit shifts and multiplaction instead🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/colors.cpp` around lines 52 - 53, The max value calculation in colors.cpp uses only r, g, b (variable maxval) which ignores the white channel w and can lead to division-by-zero in the subsequent scale calculation; update the max computation used by color_add/RGBW32 logic to include w (i.e., compute maxval as the maximum of r, g, b, and w) and additionally guard the division by ensuring maxval is non-zero (e.g., set to 1 if zero) before computing scale so the scale computation cannot divide by zero.
99-107:⚠️ Potential issue | 🟠 MajorLeft-shift of potentially negative
hueShiftis undefined behavior.Line 103 performs
hueShift << 8wherehueShiftisint32_t. Left-shifting a negative signed integer is undefined behavior. The same issue exists incolors.h:181.Proposed fix
- hsv.h += (hueShift << 8); // shift hue (hue is 16 bits) + hsv.h = uint16_t(int32_t(hsv.h) + hueShift * 256); // shift hue (hue is 16 bits)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/colors.cpp` around lines 99 - 107, The code left-shifts a signed int (hueShift << 8) which is undefined for negative values; change the shift to a safe arithmetic expression (e.g. compute an int32_t delta = hueShift * 256 or cast to unsigned before shifting: (int32_t)((uint32_t)hueShift << 8)) and use that delta when updating hsv.h in adjust_color (and apply the same fix to the other occurrence referenced in colors.h:181, replacing the unsafe left-shift with the safe multiplication/cast approach).
🧹 Nitpick comments (1)
wled00/src/dependencies/fastled_slim/fastled_slim.h (1)
4-4: Consider using<string.h>instead of<cstring>for embedded compatibility.Static analysis reports
'cstring' file not found. While<cstring>is standard C++, some embedded toolchains prefer the C-style<string.h>header. Since onlymemsetandmemmoveare used, switching to<string.h>would improve portability.Proposed fix
-#include <cstring> // for mem operations +#include <string.h> // for mem operations🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/src/dependencies/fastled_slim/fastled_slim.h` at line 4, Replace the C++ header with the C header for better embedded-toolchain compatibility: in fastled_slim.h remove the `#include` <cstring> and use `#include` <string.h> instead, since only C-style functions (memset, memmove) are used; update any comments if needed to reflect the change and ensure references like memset/memmove in this file continue to compile with the C header.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/src/dependencies/fastled_slim/fastled_slim.h`:
- Around line 698-738: The operator= implementation for CRGBPalette16 reads
gradient entries via direct dereferences of progmem pointers (uses expressions
like u.dword = *(const uint32_t*)(progent) and *(const uint32_t*)(progent +
count)), which breaks on platforms where palettes live in PROGMEM (ESP8266);
replace those direct loads with safe progmem-aware reads (e.g., use
pgm_read_dword_near or memcpy_P into a local uint32_t before assigning to
u.dword) wherever you see assignments to u.dword from progent (the initial count
loop, the initial u.dword read that seeds rgbstart, and the subsequent loop read
that sets indexend), ensuring you only copy into local variables then use u to
construct CRGB and continue the existing logic in operator=.
---
Duplicate comments:
In `@wled00/colors.cpp`:
- Around line 375-381: The code computes delta = maxval - minval then
unconditionally divides by delta when computing hsv.h, which causes
divide-by-zero for gray colors (r==g==b). Fix by handling the delta==0 case
before any division: when delta == 0 set hsv.s = 0 and set hsv.h to a safe value
(e.g., 0 or leave unchanged) and skip the hue-division branches; otherwise
compute hsv.s and run the existing hue formulas that divide by delta. Update the
block that computes hsv.s and hsv.h (references: delta, maxval, minval, hsv.s,
hsv.h, and r/g/b) so divisions only occur when delta > 0.
- Around line 52-53: The max value calculation in colors.cpp uses only r, g, b
(variable maxval) which ignores the white channel w and can lead to
division-by-zero in the subsequent scale calculation; update the max computation
used by color_add/RGBW32 logic to include w (i.e., compute maxval as the maximum
of r, g, b, and w) and additionally guard the division by ensuring maxval is
non-zero (e.g., set to 1 if zero) before computing scale so the scale
computation cannot divide by zero.
- Around line 99-107: The code left-shifts a signed int (hueShift << 8) which is
undefined for negative values; change the shift to a safe arithmetic expression
(e.g. compute an int32_t delta = hueShift * 256 or cast to unsigned before
shifting: (int32_t)((uint32_t)hueShift << 8)) and use that delta when updating
hsv.h in adjust_color (and apply the same fix to the other occurrence referenced
in colors.h:181, replacing the unsafe left-shift with the safe
multiplication/cast approach).
In `@wled00/colors.h`:
- Line 58: The declaration of adjust_color in colors.h lists parameters as
(int32_t hueShift, int32_t valueChange, int32_t satChange) but the
implementation adjust_color in colors.cpp uses (int32_t hueShift, int32_t
satChange, int32_t valueChange); make the two signatures identical by reordering
the parameters to the intended order (preferably (hueShift, satChange,
valueChange) or vice versa) and update any callers to match the chosen order so
saturation and value changes aren't swapped at runtime.
- Around line 179-183: In adjust_hue, avoid undefined behavior from
left-shifting a potentially negative signed int by replacing the shift
expression `hueshift << 8` with a multiplication (e.g., `hueshift * 256`) or an
explicit signed-to-wide-type multiply (e.g., `static_cast<int32_t>(hueshift) *
256`) so the math is well-defined before adding to hsv.h; update the expression
in the adjust_hue method accordingly.
---
Nitpick comments:
In `@wled00/src/dependencies/fastled_slim/fastled_slim.h`:
- Line 4: Replace the C++ header with the C header for better embedded-toolchain
compatibility: in fastled_slim.h remove the `#include` <cstring> and use `#include`
<string.h> instead, since only C-style functions (memset, memmove) are used;
update any comments if needed to reflect the change and ensure references like
memset/memmove in this file continue to compile with the C header.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1e7964d3-94b3-4657-b3c6-651dca5638ae
📒 Files selected for processing (6)
wled00/colors.cppwled00/colors.hwled00/palettes.cppwled00/src/dependencies/fastled_slim/fastled_slim.cppwled00/src/dependencies/fastled_slim/fastled_slim.hwled00/util.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- wled00/src/dependencies/fastled_slim/fastled_slim.cpp
- wled00/util.cpp
|
@coderabbitai please make suggestions on a practical testing procedure to check if all color conversions and FX still work the same. i.e. 1D/2D setup, RGB/RGBW LEDs and which effects to test. |
|
Tip For best results, initiate chat on the files or code changes.
WLED FastLED Replacement — Practical Testing Guide1. Hardware Configurations to Cover
2. Core Color Conversion Checks (HSV ↔ RGB)These are the highest-risk changes. Focus on visual correctness of hue, saturation, and brightness. Procedure:
Effects to run: Solid, Color Clouds, Pride 2015, Colorwaves 3. Palette RenderingThe default palette was changed from Procedure:
4. Effects That Were Directly ModifiedRun each of these and compare visually against a reference build if possible, or look for obvious anomalies (stuck pixels, wrong colors, flickering).
5. IR Remote / Color Adjustment (RGBW-specific)The Procedure (requires IR remote):
6. Blur Functions (2D Matrix)
Procedure:
7. Beat / Timing Functions
Procedure:
8. PRNG Repeatability Check
Procedure:
9. RGBW White Channel PreservationProcedure:
10. Regression Checklist Summary
🧠 Learnings used |
|
I tested the rabbits suggestions and made some improvements to twinkle fox (background was mostly faded to black due to gamma correction, fixed by applying inverse gamma: there would be other ways to fix it but it would change legacy behaviour even more). I am sure there is bugs left but in general the FX look good, colors are correct, palettes work. Ready to merge from my side. |
6 participants
Completely removed dependency on fastled library for better color control and conversions. This also prevents accidentally pulling in fastled functions that were previously replaced (random() for example).
Summary by CodeRabbit
New Features
Improvements
Refactor