Full fastled replacement #4615
Full fastled replacement #4615
Conversation
- copied relevant functions - optimized some of them for ESP32
- new rainbow function is faster and more accurate than original fastled function
WalkthroughThis pull request refactors several code modules by removing dependencies on the FastLED library and replacing them with custom color management and noise generation implementations. The changes include updates to function signatures, conversion logic for color handling (e.g., shifting from CRGB to CRGBW), the addition of new effects and palettes, and the introduction of new mathematical waveform and Perlin noise functions across various files. Changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (16)
wled00/colors.cpp (4)
2-14: Improve documentation clarity.
The new explanatory comments about color conversions and data structures are very helpful. Consider adding a brief mention of typical ranges or usage examples for each color format (e.g., recommended range for CHSV32’s hue). This will further aid new contributors.
92-105: Leverage faster transformations if hue change is unnecessary.
The newadjust_color()function neatly handles RGB→HSV→RGB for color tweaks. For speed-critical paths or frequent calls, consider directly adjusting brightness or saturation if hue remains unchanged, bypassing the full transform.
107-136: Efficient brightness blend in ColorFromPalette().
This function multiplies palette entries by brightness, which may be invoked in tight loops. Looks correct. An alternative approach is to skip the calculation for brightness=255 or partial checks for 0 to optimize corner cases.
311-415: Optimize hsv2rgb_rainbow for typical CPU constraints.
This function does significant arithmetic for color transformations. If performance is critical on lower-end MCUs, consider further micro-optimizations or remove some partial additions if high accuracy is not mandatory.wled00/util.cpp (2)
379-380: Inform developers about the new waveform replacements.
A comment references “fastled beat per minute and beatsin replacements.” Consider adding a brief note about any potential differences or corner cases (e.g., rounding) compared to the original FastLED functions.
503-553: SimulateSound logic.
This addition provides a pseudo-audio feedback for testing. The random intervals inUMS_WeWillRockYouor other patterns are a great demonstration. For advanced testing, you might want to parametrize the “ms%2000 < 200” durations.wled00/fcn_declare.h (1)
395-405: Ensure graceful fallback for extreme BPM values.
The new beat functions are declared here. Consider clearly documenting acceptable BPM range or do a clamp in the function if unbounded user input is possible.wled00/FX.cpp (7)
72-72: Global PRNG usage.
Introducing a global instance of the pseudo-random number generator can be fine given WLED’s typical single-threaded flow. However, if multiple threads or asynchronous effects ever read or write this state, consider a more localized or thread-safe PRNG model.
2546-2549: Document function parameters.
twinklefox_one_twinkle(uint32_t ms, uint8_t salt, bool cat)lacks an explicit comment describing whatsaltandcatdo. Adding a brief explanation would improve maintainability and clarity.
2596-2596: Clarify assignment to black.
Assigningc = 0at line 2596 presumably relies on an overloaded operator to set the CRGBW to black. Consider using an explicit constructor call likec = CRGBW(0,0,0,0)for clarity.
4179-4179: Remove or justify commented code.
Line 4179 appears to be an obsolete assignment topalettes[1]. If it is no longer needed, consider removing it to keep the codebase clean.
4923-4923: Commented-out alternative approach.
At line 4923,setPixelColorXYis commented out in favor ofaddPixelColorXY. For clarity, remove or convert it into a developer note explaining the two approaches’ visual differences.
5825-5827: Check random bounds.
prng.random8(0, w)only covers 0..255. Ifwexceeds 255, the result might not reach beyond 255. Consider using a 16-bit range function or clampingwif large sizes aren’t supported.
5840-5843: Same random bounds concern.
Here as well,prng.random8(0, cols)andprng.random8(0, rows)might not fully cover larger arrays ifcolsorrowsexceed 255.wled00/colors.h (2)
6-7: Prefer inline functions to macros for type safety.The macros
RGBW32and friends can lead to unexpected results if the arguments have side effects (e.g. increment expressions). Substituting inline functions preserves type safety and helps with debugging.-#define RGBW32(r,g,b,w) (uint32_t((byte(w) << 24) | (byte(r) << 16) | (byte(g) << 8) | (byte(b)))) +inline uint32_t RGBW32(uint8_t r, uint8_t g, uint8_t b, uint8_t w) { + return uint32_t((uint32_t{w} << 24) | (uint32_t{r} << 16) + | (uint32_t{g} << 8) | uint32_t{b}); +}
125-128: Avoid uninitialized default constructor for CHSV.Declaring a default constructor without initializing member values may lead to unpredictable hue, saturation, and value. Consider default-initializing them (e.g., zero) to avoid potential runtime issues.
-inline CHSV() __attribute__((always_inline)) = default; +inline CHSV() __attribute__((always_inline)) + : h(0), s(0), v(0) { }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
platformio.ini(0 hunks)usermods/Analog_Clock/Analog_Clock.cpp(1 hunks)usermods/audioreactive/audio_reactive.cpp(2 hunks)usermods/usermod_rotary_brightness_color/usermod_rotary_brightness_color.cpp(2 hunks)wled00/FX.cpp(22 hunks)wled00/FX.h(5 hunks)wled00/FX_fcn.cpp(1 hunks)wled00/FXparticleSystem.cpp(4 hunks)wled00/bus_manager.cpp(1 hunks)wled00/colors.cpp(8 hunks)wled00/colors.h(1 hunks)wled00/fcn_declare.h(6 hunks)wled00/ir.cpp(3 hunks)wled00/palettes.h(1 hunks)wled00/util.cpp(2 hunks)wled00/wled.cpp(0 hunks)wled00/wled.h(1 hunks)wled00/wled_math.cpp(1 hunks)
💤 Files with no reviewable changes (2)
- wled00/wled.cpp
- platformio.ini
🧰 Additional context used
🧬 Code Definitions (8)
usermods/usermod_rotary_brightness_color/usermod_rotary_brightness_color.cpp (2)
wled00/colors.h (2)
rgb2hsv(75-75)rgb2hsv(76-76)wled00/colors.cpp (3)
rgb2hsv(418-418)rgb2hsv(440-444)rgb2hsv(440-440)
wled00/FX_fcn.cpp (2)
wled00/FX.h (1)
_currentPalette(614-614)wled00/colors.h (10)
CRGBPalette16(604-606)CRGBPalette16(609-617)CRGBPalette16(620-622)CRGBPalette16(625-627)CRGBPalette16(642-646)CRGBPalette16(700-702)CRGBPalette16(705-707)CRGBPalette16(710-712)CRGBPalette16(715-717)CRGBPalette16(726-728)
usermods/Analog_Clock/Analog_Clock.cpp (1)
usermods/MY9291/MY9291.cpp (1)
c(27-38)
wled00/ir.cpp (2)
wled00/colors.h (12)
CRGBW(856-856)CRGBW(856-856)CRGBW(859-859)CRGBW(859-859)CRGBW(862-862)CRGBW(862-862)CRGBW(865-865)CRGBW(865-865)CRGBW(868-868)CRGBW(868-868)CRGBW(871-871)CRGBW(871-871)wled00/FX.h (14)
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)seg(878-878)seg(886-886)seg(886-886)
wled00/colors.cpp (2)
wled00/colors.h (60)
r(410-414)r(426-428)r(901-903)rgb(837-837)rgb(886-886)rgb(886-886)rgb(909-912)rgb(909-909)hsv(880-880)hsv(880-880)hsv(883-883)hsv(883-883)rgb2hsv(75-75)rgb2hsv(76-76)hsv2rgb_spectrum(72-72)hsv2rgb_spectrum(73-73)CRGB(174-174)CRGB(174-174)CRGB(177-178)CRGB(177-177)CRGB(181-182)CRGB(181-181)CRGB(185-185)CRGB(185-185)CRGB(188-190)CRGB(188-188)hue(193-195)hue(193-193)hue(198-200)hue(198-198)CRGBW(856-856)CRGBW(856-856)CRGBW(859-859)CRGBW(859-859)CRGBW(862-862)CRGBW(862-862)CRGBW(865-865)CRGBW(865-865)CRGBW(868-868)CRGBW(868-868)CRGBW(871-871)CRGBW(871-871)CHSV(127-127)CHSV(127-127)CHSV(130-131)CHSV(130-130)CHSV(134-134)CHSV(134-134)CHSV(834-834)CHSV32(825-825)CHSV32(825-825)CHSV32(828-829)CHSV32(828-828)CHSV32(830-831)CHSV32(830-830)CHSV32(832-833)CHSV32(832-832)CHSV32(836-836)CHSV32(906-908)CHSV32(906-906)wled00/fcn_declare.h (9)
r(448-448)r(448-448)s(527-527)s(527-527)s(529-535)b(301-301)b(301-301)c(328-328)c(328-328)
wled00/colors.h (4)
wled00/FX.h (38)
r(679-679)b(627-627)c(678-678)c(745-745)c(745-745)c(882-882)c(882-882)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)blend(607-607)blend(607-607)index(929-932)index(929-929)x(518-518)x(711-711)x(712-712)x(712-712)x(713-713)x(713-713)x(714-714)x(714-714)x(715-715)x(715-715)x(717-717)x(718-718)x(718-718)x(719-719)x(719-719)x(721-721)wled00/FX.cpp (2)
w(5823-5833)w(5823-5823)wled00/colors.cpp (15)
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)wled00/util.cpp (3)
always_inline(664-670)always_inline(682-688)always_inline(690-697)
wled00/FX.cpp (3)
wled00/colors.h (8)
color(54-54)color(877-877)color(877-877)r(410-414)r(426-428)r(901-903)color_fade(65-65)color_blend(62-62)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)
wled00/FX.h (7)
timebase(877-877)millis(924-924)t(879-879)t(879-879)t(885-885)t(885-885)b(627-627)wled00/fcn_declare.h (11)
beat88(395-395)beat16(396-396)sin16_t(469-469)beat8(397-397)sin8_t(471-471)scale8(488-488)ease8InOutQuad(485-485)triwave8(401-401)ease8InOutCubic(483-483)b(301-301)b(301-301)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/colors.h (17)
scale8(19-19)x(117-119)x(117-117)x(121-123)x(121-121)x(168-168)x(168-168)x(171-171)x(171-171)x(675-677)x(675-675)x(680-682)x(680-680)x(685-687)x(685-685)x(690-692)x(690-690)
🔇 Additional comments (70)
wled00/wled_math.cpp (3)
248-254: Well-implemented cubic easing function for 8-bit valuesNice addition of a cubic ease-in-out function that smoothly transitions between 0 and 255. The implementation correctly uses fixed-point math with bit shifting for efficient calculation. This will be useful for creating smoother animations in effects.
256-260: Good implementation of 16-bit cubic easing functionThis 16-bit version of the cubic easing function follows the same algorithm as the 8-bit version, but properly adapts the bit shifting to work with 16-bit values. This allows for higher precision transitions when needed.
263-269: Clean implementation of quadratic easing functionThe quadratic easing function provides a simpler alternative to the cubic ease with a different curve profile. The mirroring logic for values > 127 ensures a smooth symmetric curve. This provides more options for effect designers to create different movement feels.
wled00/FX_fcn.cpp (1)
79-79: Simplified palette initializationReplaced
CRGBPalette16(CRGB::Black)with the default constructorCRGBPalette16(). Both approaches initialize the palette to all black, but using the default constructor is more direct and aligns with the FastLED replacement effort.From
colors.h, we can see that the default constructor already initializes all entries to black withmemset(entries, 0, sizeof(entries)), making the explicit black parameter redundant.usermods/Analog_Clock/Analog_Clock.cpp (1)
119-119: Type change from FastLED-specific to standard typeChanged parameter type from
fract8(FastLED-specific type) to standarduint8_t. This is part of the FastLED replacement effort and improves code portability and reduces external dependencies while maintaining the same functionality.usermods/usermod_rotary_brightness_color/usermod_rotary_brightness_color.cpp (4)
99-99: Replaced approximate HSV conversion with more accurate functionChanged from
rgb2hsv_approximate(fastled_col)torgb2hsv(fastled_col)to use the new implementation that provides more accurate color conversion. This should result in better color representation when using the rotary encoder.
106-107: Simplified HSV to RGB conversionRemoved the call to
hsv2rgb_rainbow(prim_hsv, fastled_col)and replaced with direct assignmentfastled_col = prim_hsv. This works because of the operator overloading in the new color handling implementation, making the code more concise.
124-124: Replaced approximate HSV conversion with more accurate functionChanged from
rgb2hsv_approximate(fastled_col)torgb2hsv(fastled_col)to use the new implementation that provides more accurate color conversion. This matches the similar change in the clockwise rotation handler.
131-132: Simplified HSV to RGB conversionRemoved the call to
hsv2rgb_rainbow(prim_hsv, fastled_col)and replaced with direct assignmentfastled_col = prim_hsv. This matches the similar change in the clockwise rotation handler and streamlines the code.wled00/bus_manager.cpp (1)
27-27: Include added for color handling functions.The added colors.h include replaces the previously defined color management macros and functions, centralizing color handling into a dedicated header file.
wled00/ir.cpp (3)
5-5: Include added for FastLED color replacement functions.This include provides access to the new custom color handling classes and functions that replace the FastLED dependency.
132-137: Improved hue adjustment with RGBW support.The code has been updated to use the new
CHSV32andCRGBWclasses instead of FastLED'sCHSVandCRGB. The changes properly maintain the white channel component during color transformations, which was likely lost in the previous implementation. The bit shifting operation (amount<<8) enables higher precision for hue adjustments.
174-181: Enhanced saturation adjustment with white channel preservation.Similar to the hue adjustment changes, this implementation now uses the custom
CHSV32class for better saturation control while properly preserving the white channel value from the original color. This prevents loss of white channel information during HSV color space conversions.usermods/audioreactive/audio_reactive.cpp (3)
872-873: Replacement of inoise8 with perlin8 aligns with FastLED removal.This change replaces the FastLED
inoise8function with the new customperlin8implementation for simulating analog microphone input when sound processing is disabled. This aligns well with the PR objective of eliminating FastLED dependencies.
1997-1998: HSV to RGB conversion updated for FastLED removal.The code now uses direct assignment of CHSV to the RGB value variable instead of going through the FastLED's
hsv2rgb_rainbowfunction. This suggests that the project now has its own HSV to RGB conversion implementation, likely through operator overloading or implicit conversion in the new color handling system.
2002-2003: HSV to RGB conversion updated for FastLED removal.Similar to the previous instance, the code now uses direct assignment of CHSV to the RGB value variable instead of relying on FastLED's conversion functions. This change is consistent with the PR objective of optimizing color conversions and removing FastLED dependencies.
wled00/wled.h (1)
187-189: Effective replacement of FastLED dependency with custom color handlingThese changes align perfectly with the PR objective to eliminate the FastLED dependency. The project now relies on custom implementations through the new
colors.hheader file, which will likely provide improved color control with native 32-bit color handling as mentioned in the PR description.This approach should lead to:
- More precise control over color manipulation functions
- Potential performance improvements through optimized implementations
- Removal of unnecessary dependencies
wled00/FXparticleSystem.cpp (9)
580-580: Changed color representation from CRGB to CRGBWThe variable
baseRGBhas been changed fromCRGBtoCRGBWtype, enabling support for the additional white channel as part of the FastLED replacement. This change aligns with the PR's objective of enhancing color control.
651-652: Updated color palette function callThe function call has been changed from
ColorFromPaletteWLEDtoColorFromPalette, consistent with the FastLED replacement strategy. The parameter structure remains the same, ensuring compatibility.
655-656: Updated color palette function callSimilar to the previous change, the function call has been updated from
ColorFromPaletteWLEDtoColorFromPaletteto use the new native implementation instead of FastLED.
657-660: Updated HSV color handlingThe HSV color handling has been updated to use the new
CHSV32type andhsv2rgb_spectrumfunction from the custom implementation, replacing FastLED's HSV conversion functionality.
662-662: Added explicit CRGB cast for backward compatibilityAn explicit cast to
CRGBhas been added when passingbaseRGBto therenderParticlefunction. This ensures compatibility with the function signature while allowing the variable to maintain its extended CRGBW functionality internally.
1533-1533: Changed color representation from CRGB to CRGBWSimilar to the change in the 2D particle system, the 1D particle system's
baseRGBvariable has been updated fromCRGBtoCRGBWtype to support the additional white channel.
1580-1581: Updated color palette function callThe function call has been changed from
ColorFromPaletteWLEDtoColorFromPalettein the 1D particle system, consistent with the FastLED replacement strategy.
1584-1587: Updated HSV color handlingThe HSV color handling has been updated to use the new
CHSV32type andhsv2rgb_spectrumfunction in the 1D particle system, similar to the changes in the 2D system.
1589-1589: Added explicit CRGB cast for backward compatibilityAn explicit cast to
CRGBhas been added when passingbaseRGBto therenderParticlefunction in the 1D particle system, ensuring compatibility with existing function signatures.wled00/palettes.h (9)
17-38: Great addition of CloudColors_p palette!This new palette follows the existing code structure and adds a cloudy color scheme with various blue and white tones. This implementation helps replace the FastLED dependency as outlined in the PR objectives.
40-61: Well-implemented LavaColors_p palette.The lava color palette is properly implemented with appropriate dark reds, maroons, and oranges that simulate lava colors. The palette follows the proper structure and helps with the FastLED replacement.
63-84: OceanColors_p palette looks great.This ocean-themed palette with various blues and greens is well-structured and follows the same pattern as other palettes in the codebase.
86-107: ForestColors_p is implemented correctly.The forest colors palette with appropriate greens is well-defined and provides a good range of nature-inspired tones.
109-114: Good implementation of RainbowColors_p.The rainbow palette is concisely defined with hexadecimal color values instead of CRGB constants, which is appropriate for this palette type.
116-117: Useful alias for RainbowStripesColors_p.Creating this alias helps maintain backward compatibility with code that might have used the alternative spelling.
119-124: RainbowStripeColors_p implemented properly.The striped rainbow palette alternates between colors and black, creating a nice visual effect consistent with the original FastLED implementation.
126-134: Well-documented PartyColors_p palette.The party colors palette is properly implemented with vibrant colors and includes a helpful comment explaining its intended use for lighting at clubs or parties.
974-982: Correctly added new palettes to fastledPalettes array.All the newly implemented palettes are properly added to the fastledPalettes array, making them accessible through the existing palette selection system.
wled00/FX.h (5)
23-23: Appropriate inclusion of colors.h.Replacing FastLED with a custom colors.h header is a key part of the FastLED removal strategy outlined in the PR objectives.
41-46: Fixed comment syntax for color mangling macros.Proper comment syntax makes the code more readable and maintainable.
511-511: Simplified CRGBPalette16 initialization.Using the default constructor
CRGBPalette16()instead of explicitly initializing withCRGB::Blacksuggests the custom implementation has been updated to initialize properly by default.
692-697: Type change from fract8 to uint8_t for blur functions.Changing the parameter type from FastLED-specific
fract8to standarduint8_taligns with the goal of removing FastLED dependencies. This type change preserves functionality while using standard C++ types.
766-766: Updated commented-out function signature.Ensuring consistent type usage even in commented code is good practice for future-proofing.
wled00/colors.cpp (6)
37-65: Validate potential overflow in color_add().
The code checks whether each color component might exceed 255 and scales accordingly ifpreserveCRis true. The implementation looks correct and well-optimized. No immediate issues found.
67-89: Confirm expected fade behavior.
The “video scaling” path ensures non-zero colors remain above zero, which is consistent with fade-to-black requirements. Please verify that upstream code indeed relies on these “video” semantics. Otherwise, some edges might unintentionally never fully fade out.
253-300: Ensure correctness of hsv2rgb_spectrum for boundary hues.
The switch-case logic for the six hue sectors is well laid out. However, boundary conditions (e.g., hue near 65535) can inadvertently roll over. Confirm that the final color does not result in an off-by-one.
303-309: Preserve color consistency between overloads.
You introduced a second overload ofhsv2rgb_spectrumtakingCHSVinstead ofCHSV32. Verify both produce closely matched results for standard hue/sat/val values.
417-444: Confirm ease-of-use for rgb2hsv.
Thergb2hsvfunction is more precise than the FastLED version, which is great. Just ensure that the higher accuracy doesn’t cause unexpected differences in effect outcomes that previously relied on approximations.
501-520: HeatColor correctness.
This new implementation matches a typical blackbody approximation. The stepwise logic for R/G/B increments is clear. Looks good to merge.wled00/util.cpp (3)
381-397: Check alignment of BPM ranges.
Your beat functions (beat88,beat16,beat8) rely on different scaling factors. Confirm the usage in calling code so that 8-bit or Q8.8 BPM inputs do not lead to confusion or an off-by-one in the wave period.
399-427: Validate default wave extents in beatsin functions.
The newly introducedbeatsinXX_tfunctions appear correct for generating sine waves within[lowest, highest]. Ensure that the default arguments (lowest=0, highest=65535or255) do not produce undesired amplitude in existing effects.
429-446: Confirm triwave and cubicwave usage.
The triwave and cubicwave functions are neat expansions for wave generation. Double-check they’re accepted in the existing effect engine without risk of confusion or overshadowing older wave calls.wled00/fcn_declare.h (4)
5-6: Header dependency on colors.h
Includingcolors.hearly ensures all color structs are available. This is a beneficial reordering given the broader refactor.
373-374: Legacy aliases for inoise8 and inoise16.
Replacing them with perlin-based implementations is consistent with the PR’s objective to remove FastLED dependencies. Confirm any code that specifically relied on FastLED’s noise signature remains unaffected.
411-419: Refined Perlin-based noise prototypes.
These function signatures match the definitions inutil.cppwell. Good alignment. No issues found.
521-542: Implemented PRNG class.
This class is a handy fallback for non-hardware-random usage, especially for 8-bit or 16-bit draws. It’s simpler than the original FastLED PRNG and should integrate smoothly.wled00/FX.cpp (14)
16-16: Include looks correct.
Including"colors.h"here appropriately integrates the new color management utilities, fully removing reliance on FastLED’s color headers.
1757-1786: Seed saving and restoring.
The logic for savingprevSeedat line 1763 and restoring it at line 1783 is a good practice, preventing interference with PRNG usage in other effects. This approach preserves deterministic randomness for this effect without affecting others.
2581-2581: Variable declaration is straightforward.
Declaring a localCRGBW c;at line 2581 is a standard step for color manipulation. No issues identified.
2619-2626: Scaled background color logic.
Usingcolor_fade(bg, X, true)to scale brightness based on average light is consistent with the new color utilities. The overall approach looks correct.
2644-2655: Check the(bg == 0)comparison.
At line 2648, comparingbg == 0relies on an operator overload for CRGBW. Make sure it correctly detects a “black” background. If not, an explicit check ofbg.getAverageLight()or similar might be more robust.
4147-4157: Consistent use of custom seed.
Line 4147-4157 properly sets a known seed for predictable twinkle behavior, then restores the previous seed. This technique is consistent with the approach inmode_random_chase.
5698-5698: Check signed arithmetic.
((int16_t)perlin8(i * someVal, j * someVal, t) - 0x7F) / 3can produce negative values. Verify this is expected. If partial negative results aren’t desired, clamp or handle them accordingly.
6014-6014: Color addition may wrap.
Incrementingblob->color[i]by 4 could overflow if the stored color is near its maximum. Confirm if such rollover is intended or if it should cap at 255.
7510-7510: Smooth color scaling.
The combinednscale8calls at line 7510 neatly blend transitions. No issues flagged.
7819-7819: Potential large rocket speed.
hw_random16(SEGMENT.custom1 >> 3)can yield a sizable range, which might produce unbounded vertical velocity. If large speeds are unwanted, add clamping logic.
7863-7863: No concerns for random bit usage.
Usinghw_random() & 1as a 50% chance check is straightforward and valid.
9724-9724: Cosine offset usage appears correct.
(int16_t)cos8_t(SEGENV.aux0) - 128yields a range of -128..127, suitable for gravity or drift. No major issues here.
10051-10051: Potential overflow in sums before sqrt.
sqrt32_bw(...)might overflow if the sum of these FFT bins exceeds 65535. If the bins can be large, consider using a larger intermediate type or clamping.
2271-2272:⚠️ Potential issuePotential negative result for fade amounts.
Ifstrip.getBrightness()exceeds 68, the expression(68 - brightness)becomes negative and underflows in auint8_t, creating unintended large 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 safeFadeUp = (strip.getBrightness() > 68) ? 0 : (68 - strip.getBrightness()); +uint8_t safeFadeDown = (strip.getBrightness() > 68) ? 0 : (68 - strip.getBrightness()); +uint8_t fadeUpAmount = (strip.getBrightness() > 28) ? (8 + (SEGMENT.speed >> 2)) : safeFadeUp; +uint8_t fadeDownAmount = (strip.getBrightness() > 28) ? (8 + (SEGMENT.speed >> 3)) : safeFadeDown;Likely an incorrect or invalid review comment.
wled00/colors.h (3)
4-4: Confirm applicable licensing when copying FastLED code.Since this file references code originally from FastLED, please ensure that you're in full compliance with FastLED’s license terms.
Could you verify the license compatibility and confirm that appropriate attributions or disclaimers are included where necessary?
604-606: Palette initialization is correct.Using
memsetto initialize the default constructor values to black (0) is straightforward and efficient. No concerns here.
893-899: Validate hue shift boundaries in adjust_hue.The
hueshiftparameter is added without clamping. If negative values or excessively large shifts are passed in, the hue could wrap in unexpected ways. Evaluate whether to clamp or allow wrap-around.Would you like to confirm the wrapping behavior with tests or clamp the hue shift within a 0-255 range?
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:✅ 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:✅ 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:✅ 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:❓ 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wled00/fcn_declare.h (2)
463-465: Macro usage for approximations is acceptable, but consider clarity.
Redefiningsin_t,cos_t, andtan_tvia macros is a quick way to switch to approximate versions; however, it can hide the “approx” detail from future maintainers. If clarity is a priority, rename them to something likesinApprox(), etc.-#define sin_t sin_approx -#define cos_t cos_approx -#define tan_t tan_approx +inline float sin_t(float x) { return sin_approx(x); } +inline float cos_t(float x) { return cos_approx(x); } +inline float tan_t(float x) { return tan_approx(x); }
510-531: Custom PRNG nicely complements hardware RNG.
The pseudo-random generator (PRNG) is useful for reproducible randomness in effects. Note that it lacks checks for invalid range inputs (e.g.,min > lim). If that scenario is possible, consider adding an assertion or clamp. Otherwise, this is a lightweight alternative tohw_random.uint16_t random16(uint16_t min, uint16_t lim) { + if (lim <= min) { + // handle error or clamp + return min; + } uint16_t delta = lim - min; return random16(delta) + min; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/fcn_declare.h(5 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
wled00/fcn_declare.h (5)
wled00/FX.h (16)
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)t(879-879)t(879-879)t(885-885)t(885-885)b(627-627)wled00/util.cpp (3)
always_inline(664-670)always_inline(682-688)always_inline(690-697)wled00/colors.h (13)
always_inline(267-270)always_inline(267-267)always_inline(273-277)always_inline(273-273)always_inline(280-283)always_inline(280-280)always_inline(286-290)always_inline(286-286)always_inline(400-402)always_inline(579-581)always_inline(583-585)always_inline(587-589)always_inline(591-593)wled00/bus_manager.h (5)
b(124-124)b(124-124)b(193-199)b(193-193)b(240-240)wled00/bus_wrapper.h (2)
b(337-337)b(337-337)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
🔇 Additional comments (5)
wled00/fcn_declare.h (5)
5-6: Include placement looks good.
The addition of#include "colors.h"is appropriate given the removal of color structures from this file. It cleanly centralizes color definitions, reducing duplication.
395-405: Consider unit testing boundary inputs for beat/wave functions.
The new beat and wave function declarations appear consistent with typical FastLED-style utilities. However, verify how they handle edge cases (e.g.,beats_per_minute = 0or extremely large values). Automated tests would ensure correctness for any out-of-range inputs.Please confirm whether the calling code validates input ranges or whether we need to add checks within these functions. If necessary, I can generate a test script or additional code to confirm safe behavior near boundary values.
483-486: Easing API addition is beneficial for creating smooth animations.
The new easing function declarations (ease8InOutCubic,ease16InOutCubic,ease8InOutQuad) align well with typical LED animations. No issues found here.
487-490: Scale functions mirror familiar FastLED semantics.
Thescale8variants correctly handle integer multiplication. Always confirm these are used in performance-critical paths and documented to avoid confusion about their integer-based saturations.
491-496: Fast saturating arithmetic looks correct.
These inline definitions (qadd8,qsub8,qmul8, etc.) are standard saturating operations from FastLED. They are typically used for color and brightness manipulations and appear correct.
| #ifndef WLED_COLORS_H | ||
| #define WLED_COLORS_H | ||
|
|
||
| // note: some functions/structs have been copied from fastled library, modified and optimized for WLED |
There was a problem hiding this comment.
Soooo....
Technically speaking, you cannot copy MIT licensed code without retaining the original copyright notice. But you also can't re-license WLED from MIT to EUPL without getting consent of every single author of the current code base (or removing their contributions), so apparently WLED doesn't care about the legality of licensing anyway.
There was a problem hiding this comment.
that is a valid point, could try to find the original author of the functions copied and ask.
as for the re-licensing: AFAIK all major contributors were contacted and asked for permission.
There was a problem hiding this comment.
licensing is all about technicality. And technically, you need permission from every single contributor whose contributions were not removed prior to the license change. And I know for a fact that you didn't ask everyone.
While I think it's unlikely that anything comes out of this, if someone wants to cause trouble because of this, you expose a lot of attack surface with this move
| #define WLED_COLORS_H | ||
|
|
||
| // note: some functions/structs have been copied from fastled library, modified and optimized for WLED | ||
|
|
There was a problem hiding this comment.
I would also like the version of fastled the code was taken from to be documented, so you can compare the code to upstream
There was a problem hiding this comment.
it is FastLED @ 3.6.0, could include that in the comment
| @@ -600,6 +506,30 @@ uint32_t sqrt32_bw(uint32_t x); | |||
| #define fmod_t fmodf | |||
| #define floor_t floorf | |||
| */ | |||
|
|
|||
| // PRNG for 16bit and 8bit random numbers used by some effects (fastled replacement) | |||
| class PRNG { | |||
There was a problem hiding this comment.
Is adding yet another class to this already overloaded header a good idea?
(Is adding a class to a file dedicated to function declarations a good idea? (Is adding implementations to a file dedicated to declarations a good idea?))
There was a problem hiding this comment.
I am open to suggestions. I did not want to alter the WLED file structure. What would be a better place to put this?
| @@ -68,17 +69,14 @@ | |||
| //#define MAX_FREQUENCY 5120 | |||
| //#define MAX_FREQ_LOG10 3.71f | |||
|
|
|||
| PRNG prng; // pseudo-random number generator class | |||
There was a problem hiding this comment.
| PRNG prng; // pseudo-random number generator class | |
| namespace { | |
| PRNG prng; // pseudo-random number generator instance | |
| } |
There was a problem hiding this comment.
you are probably right, I am not familiar with the use of namespaces
There was a problem hiding this comment.
this is an unnamed namespace, which achieves the same thing as static PRNG prng;: it makes this token visible only to the current translation unit (the cpp file), instead of globally. why unnamed namespace instead of static? It's the newer version of doing this. is it any better? I don't know.
| SEGENV.step = RGBW32(random8(), random8(), random8(), 0); | ||
| SEGENV.aux0 = random16(); | ||
| SEGENV.step = RGBW32(prng.random8(), prng.random8(), prng.random8(), 0); | ||
| SEGENV.aux0 = prng.random16(); |
There was a problem hiding this comment.
To me these 2 lines look like they don't need a pseudo random number and would be fine with real random numbers
There was a problem hiding this comment.
not the way the FX works, see seed retention below. the idea is to be able to have two segments in sync.
There was a problem hiding this comment.
yes, and that works with the prng usage blelow. but if we want to also synchronize the initial state, we either need to synchroinze step and aux anyway, or we need to make sure prng's seed is synchronized at this point in time, and I don't think either happens.
| int32_t r = (rgb>>16)&0xFF; | ||
| int32_t g = (rgb>>8)&0xFF; | ||
| int32_t b = rgb&0xFF; | ||
| // CHSV to CRGB, dumb conversion: slower so this should not be used in time critical code, use rainbow version instead |
There was a problem hiding this comment.
There are a lot of color space conversion functions now. from a quick glance, I can't tell what their differences are. I would love a short summary of which function to use when to be added to all conversion function comments, and, if necessary, a more detailed comparison comment block before the first one or at the top of the header. If a short summary is too long for all of them, at least point to the detailed comparison
| if (hsv.s == 0) return; // gray value | ||
| hsv.s = delta == maxval ? 255 : (255 * delta) / maxval; // faster on fully saturated colors, slightly slower otherwise | ||
| //hsv.s = (255 * delta) / maxval; | ||
| //if (hsv.s == 0) return; // gray value // assuming gray values are passed rarely, this can be omitted to increase speed |
| // it can be useful to avoid back and forth conversions between uint32_t and fastled CRGB | ||
| struct CRGBW { | ||
| union { | ||
| uint32_t color32; // Access as a 32-bit value (0xWWRRGGBB) |
There was a problem hiding this comment.
I believe this technically violates strict aliasing and is undefined behavior, but whatever ^^
There was a problem hiding this comment.
it is a bit problematic (ambiguous conversions), but it allows direct comparison, constructor etc. from 32bit colors and speed is key in colors as its done for every pixel. Is there a better way that ensures speed?
There was a problem hiding this comment.
c++20 has std::bit_cast, gcc has -fno-strict-aliasing (which may be applied selectively per compilation unit), and there is memcpy which may be optimized out. C would also allow this code without changes.
| prim_hsv.h = (byte)new_val; | ||
| hsv2rgb_rainbow(prim_hsv, fastled_col); | ||
| CHSV32 prim_hsv = CRGBW(sseg.colors[0]); | ||
| prim_hsv.h += (amount<<8); |
There was a problem hiding this comment.
Isn't that what adjust_hue is for?
There was a problem hiding this comment.
I think yes, I updated files out of order, this was updated before I added the adjust_hue. will check.
| @@ -513,10 +513,6 @@ void WLED::setup() | |||
| DEBUG_PRINTF_P(PSTR("heap %u\n"), ESP.getFreeHeap()); | |||
| #endif | |||
|
|
|||
| // Seed FastLED random functions with an esp random value, which already works properly at this point. | |||
| const uint32_t seed32 = hw_random(); | |||
| random16_set_seed((uint16_t)seed32); | |||
There was a problem hiding this comment.
I don't think you restore this behavior in another location, do you?
There was a problem hiding this comment.
no, I removed this under the impression that all FX do their own seed now, but that could be wrong. will check if this is still required.
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).
@netmindz Animartrix does not compile anymore since fastled.h is not available now. Can you please remove
#include <FastLED.h>from ANIMartRIX.h? 0.16 compiles just fine without that include as well. I did not check older versions though, maybe some other measures are appropriate, I let you be the judge of that :)Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor