Multifix #4568
Multifix #4568
Conversation
- new function fadeToSecondaryBy()
- also wait for strip before updating segment
- respect paletteBlend - bugfix in ColorFromPaletteWLED()
WalkthroughThis pull request primarily standardizes color handling across various modules by replacing the legacy Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
wled00/playlist.cpp (5)
13-15: Consider encapsulating global states into a dedicated structure or class.
While making themstaticlimits their visibility to this translation unit, relying on multiple global variables for managing playlist states can make the code less modular and harder to maintain.
17-20: Use RAII or container-based approaches for dynamic arrays.
Replacing raw pointers with a container likestd::vector(or a smart pointer) can improve safety and clarity by handling memory management automatically.
23-25: Group parent playlist fields into a single struct.
MergingparentPlaylistIndex,parentPlaylistRepeat, andparentPlaylistPresetIdinto a dedicated struct could improve readability and maintainability.
57-61: Provide user feedback for blocked nested playlists.
The current check correctly aborts if already within a nested playlist, but consider logging or notifying the user when this occurs to aid debugging.
159-162: Asynchronous preset application may cause brief visual artifacts.
When callingapplyPresetFromPlaylist()after unloading, be aware of potential flicker or a short transition if user interactions occur at the same time.wled00/FX_fcn.cpp (2)
1162-1181: Consider partial clearing
The newclear()method resets and restores geometry states before filling with black. For certain transitions, partial resets or preserving effect data might be more desirable for a smoother user experience.
1554-1584: Consider refactoring blending style logic
This switch block onblendingStylehas numerous cases and nested transitions. Moving these into helper functions or a more structured table-driven approach could improve maintainability.wled00/json.cpp (1)
397-428: LGTM! Enhanced thread safety and performance.The changes improve thread safety by ensuring segment modifications don't occur during effect execution. The batch deletion optimization for segments is a good performance improvement.
Consider adding a timeout to the wait loop to prevent potential deadlocks in case the strip servicing takes too long.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
usermods/usermod_rotary_brightness_color/usermod_rotary_brightness_color.h(4 hunks)usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h(3 hunks)wled00/FX.h(1 hunks)wled00/FX_2Dfcn.cpp(1 hunks)wled00/FX_fcn.cpp(13 hunks)wled00/button.cpp(2 hunks)wled00/colors.cpp(5 hunks)wled00/data/index.js(13 hunks)wled00/data/settings_leds.htm(9 hunks)wled00/fcn_declare.h(1 hunks)wled00/json.cpp(3 hunks)wled00/led.cpp(7 hunks)wled00/mqtt.cpp(2 hunks)wled00/playlist.cpp(7 hunks)wled00/presets.cpp(2 hunks)wled00/set.cpp(5 hunks)wled00/wled.cpp(3 hunks)wled00/wled.h(1 hunks)wled00/xml.cpp(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- wled00/wled.h
🧰 Additional context used
🪛 Biome (1.9.4)
wled00/data/index.js
[error] 412-412: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 413-413: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 1913-1913: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (48)
wled00/set.cpp (3)
147-147: Great improvement in readability!Using character literals ('0' and 'A') instead of magic numbers (48 and 55) makes the code more maintainable and self-documenting.
260-260: Consistent improvement in readability!The same improvement of using character literals is consistently applied here, maintaining a uniform coding style.
1194-1194: Documentation kept in sync with code changes!The comment update correctly reflects the change from
col[]tocolPri[], maintaining accurate documentation.wled00/playlist.cpp (4)
88-88: Confirm the max value fordur.
constrain(dur, 0, 65530)might be intentional to prevent overflow, but double-check whether 65530 is the correct upper bound or if 65535 should be used.
126-137: Double-check nested playlist transition logic.
This approach to resetting or preserving parent playlist data looks good. However, confirm that an endless parent playlist scenario doesn’t prevent reloading or cleanup.
149-149: Good safeguard against overflow.
UsingplaylistEntryDur < UINT16_MAXin the timing check helps prevent overflow in themillis() - presetCycledTime > 100 * playlistEntryDurexpression.
172-172: Correct use of a sentinel for entry duration.
SettingplaylistEntryDurtoUINT16_MAXfor zero durations avoids undefined handling in the subsequent time checks.wled00/FX_fcn.cpp (4)
273-291: Refine transitional data checks
The new transitional data logic looks good, but consider adding explicit checks for_tto avoid potential null pointer references in edge cases.
1209-1229: Potential rounding issues during fade
Using(256-rate) >> 1alongside integer math may introduce rounding artifacts at lower fade rates. If precise or smoother transitions are needed, consider alternative calculations or floating-point logic.
1231-1241: Fade to secondary color
This newfadeToSecondaryBy()method effectively transitions pixels towardcolors[1]. The overall approach seems solid.
1322-1336: Use of palette blending logic
Leveragingstrip.paletteBlendto select the blending type is appropriate. IfpaletteBlendcan go out of anticipated bounds, consider adding boundary checks for robustness.usermods/usermod_rotary_brightness_color/usermod_rotary_brightness_color.h (4)
98-100: Consistency of color references
Replacingcol[...]withcolPri[...]aligns with the broader move to thecolPriarray, ensuring code consistency.
109-111: Direct color assignment
Pulling FastLED components back intocolPriensures that changes persist and update the main color data correctly.
123-125: Mirroring color read
Re-acquiringcolPrivalues here guarantees that the new hue adjustments are consistently applied to all color references.
134-136: Color variable rename
UsingcolPrirather than the more genericcolclarifies that these fields represent the primary color array for the system.wled00/mqtt.cpp (2)
106-106: UtilizingcolPrifor MQTT color updates
Switching tocolPrialigns MQTT color commands with the primary color array used throughout the rest of the code.
172-172: MQTT color formatting
Publishing the color withcolPriensures consistency with the newly standardized primary color storage across modules.wled00/led.cpp (5)
12-15: LGTM! Consistent color array naming.The change from
coltocolPriis consistent with the PR objectives to standardize color handling.
42-42: LGTM! Consistent color array usage.The change from
coltocolPrimaintains consistency in color handling.
115-119: LGTM! Improved time handling.Using a local variable
nowfor current time ensures consistent timing calculations within the function.
154-154: LGTM! Improved code clarity.Using
CALL_MODE_INITconstant instead of a magic number improves code readability.
215-215: LGTM! Consistent color array usage in nightlight handling.The change from
coltocolPrimaintains consistency in color handling for nightlight functionality.wled00/presets.cpp (2)
25-27: LGTM! Good addition of a utility function.The new
presetNeedsSavingfunction follows single responsibility principle and improves code modularity.
276-276: LGTM! Improved documentation.The updated comment provides clearer information about playlist behavior, specifically about repeat counter increment and randomization.
wled00/button.cpp (2)
43-43: LGTM! Consistent color array usage in button handling.The change from
coltocolPrimaintains consistency in color handling for button actions.
233-233: LGTM! Consistent color array usage in analog handling.The change from
coltocolPrimaintains consistency in color handling for analog input processing.wled00/colors.cpp (3)
13-20: LGTM! Improved code clarity in color blending.The introduction of the
maskconstant and its consistent usage improves code readability and maintainability.
32-34: LGTM! Improved code clarity in color addition.The introduction of the
maskconstant and its consistent usage improves code readability and maintainability.
82-85: LGTM! Improved code clarity in color fading.The introduction of the
maskconstant and its consistent usage improves code readability and maintainability.wled00/fcn_declare.h (1)
328-328: LGTM! Well-placed function declaration.The new function
presetNeedsSaving()is appropriately placed in the presets.cpp section and has a clear, descriptive name that indicates its purpose.wled00/FX_2Dfcn.cpp (1)
687-687: Improved color blending for character rendering.The change from
ColorFromPaletteWLEDtoColorFromPalettewithLINEARBLEND_NOWRAPprovides smoother color transitions when rendering characters.wled00/xml.cpp (1)
14-14: Consistent use of primary color array.The changes from
coltocolPrimaintain consistency with the new color handling approach across the codebase.Also applies to: 23-23
wled00/wled.cpp (3)
119-122: Improved state management for playlists.Checking
presetNeedsSaving()before handling playlists ensures that presets are saved before playlist changes, preventing potential state conflicts.
470-470: Better maintainability with constant usage.Using
DEFAULT_MDNS_NAMEinstead of a hardcoded string improves code maintainability and self-documentation.
553-553: Enhanced color initialization.The changes improve color state initialization by:
- Using
colPriconsistently with the new color handling approach- Ensuring proper color state initialization with
colorUpdated(CALL_MODE_INIT)Also applies to: 559-559
wled00/data/settings_leds.htm (1)
27-27: LGTM! Good refactoring.The new
chrID(x)function centralizes the character conversion logic that was previously duplicated across multiple functions. This improves code maintainability and reduces the risk of inconsistencies.usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h (1)
521-521: LGTM! Consistent color handling.The changes from
coltocolPrialign with the project's transition in color handling, ensuring consistency across the codebase.Also applies to: 923-924, 953-954, 958-959, 962-963
wled00/json.cpp (2)
94-107: LGTM! Improved variable declarations.The new boolean variables improve code readability and maintainability by clearly separating segment properties.
209-219: LGTM! Improved property assignments.The property assignments are now more organized and consistent, with proper constraints being handled in the appropriate functions.
wled00/FX.h (2)
679-679: LGTM! Clear and concise method name.The
clear()method follows good naming conventions and has a clear purpose of resetting the segment's state.
682-682: LGTM! Well-named method with clear functionality.The
fadeToSecondaryBy()method follows good naming conventions and has a clear purpose of fading to the secondary color by a specified amount.wled00/data/index.js (7)
412-414: LGTM! Improved playlist type checkingThe function now properly handles both numeric preset IDs and preset objects, making it more robust.
🧰 Tools
🪛 Biome (1.9.4)
[error] 412-412: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 413-413: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
1910-1920: LGTM! Enhanced playlist selection UIThe changes improve the user experience by:
- Visually indicating playlists with a play icon (▶)
- Preventing infinite recursion by skipping endless playlists and self-references
🧰 Tools
🪛 Biome (1.9.4)
[error] 1913-1913: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
1947-1950: LGTM! Improved playlist entry handlingThe changes ensure that duration and transition arrays stay synchronized when adding new playlist entries.
1957-1960: LGTM! Improved playlist entry deletionThe changes ensure that duration and transition arrays stay synchronized when removing playlist entries, while preventing deletion of the last entry.
1977-1983: LGTM! Improved playlist transition validationThe changes ensure that transition durations cannot exceed playlist durations, preventing invalid states.
2003-2013: LGTM! Added manual playlist advancementThe new function properly handles manual playlist advancement by:
- Setting duration to 0 for manual mode and 10s for automatic mode
- Making duration inputs read-only in manual mode
2693-2698: LGTM! Improved color slot handlingThe changes ensure that color slot data and UI are properly updated when RGB values change.
| if (stop && (spc > 0 || m12 != map1D2D)) clear(); | ||
| /* | ||
| if (boundsUnchanged | ||
| && (!grp || (grouping == grp && spacing == spc)) | ||
| && (ofs == UINT16_MAX || ofs == offset) | ||
| && (m12 == map1D2D) | ||
| ) return; | ||
|
|
||
| */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid clearing segment data unexpectedly
Calling clear() on geometry changes can be surprising for some use cases. Ensure there's no scenario where an unrelated parameter change inadvertently discards segment content.
Plethora of fixes and tunings.
fade_out()color_blend(),color_add()andcolor_fade()color_from_palette()(WARNING may require some effects to change how they handlewrapparameter of the function)col[]Some of the commits may be backported/cherry-picked to
0_15_xbranch.Summary by CodeRabbit
New Features
Improvements