CCT fix #4744
CCT fix #4744
Conversation
|
""" WalkthroughThe changes introduce a per-pixel CCT (Correlated Color Temperature) buffer to the WS2812FX class. CCT values are now tracked and set for each pixel during output, replacing the previous segment-level approach. This involves memory management for the new buffer and updates to blending and output logic to ensure accurate per-pixel CCT handling. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. All modifications directly relate to implementing per-pixel CCT handling as required by the linked issue. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
wled00/FX_fcn.cpp (1)
1321-1321: Consider using a more descriptive variable name.The variable
cctcould be renamed tosegmentCCTorcurrentSegmentCCTfor better clarity.- uint8_t cct = topSegment.currentCCT(); + uint8_t segmentCCT = topSegment.currentCCT();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/FX.h(4 hunks)wled00/FX_fcn.cpp(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- 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)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Test cdata.js
🔇 Additional comments (6)
wled00/FX.h (2)
805-805: Comment update is accurate.The class size increase from 104 to 108 bytes correctly reflects the addition of the 4-byte pointer member on ESP32.
832-832: Proper memory management for the new CCT buffer pointer.The
_pixelCCTpointer is correctly initialized tonullptrin the constructor and properly freed in the destructor.Also applies to: 861-861
wled00/FX_fcn.cpp (4)
1394-1394: Correct implementation of per-pixel CCT assignment.The CCT values are properly assigned to the buffer with appropriate null checks, and the implementation correctly handles mirrored pixels in 2D segments.
Also applies to: 1406-1409, 1481-1481, 1486-1486
1619-1631: Well-optimized per-pixel CCT implementation.The code efficiently handles per-pixel CCT by:
- Only updating CCT when the value changes between pixels
- Properly saving and restoring the global CCT value
- Correctly handling the
cctFromRgbcase
1632-1633: Proper cleanup of CCT buffer.The buffer is correctly freed and the pointer is set to nullptr, preventing use-after-free issues.
1597-1633: Potential memory leak if show() is called recursively.If
show()can be called recursively (e.g., through the_callbackon line 1612), the CCT buffer would be allocated multiple times but only freed once, causing a memory leak. Consider adding a guard to prevent recursive allocation.#!/bin/bash # Check if show() can be called recursively through callbacks ast-grep --pattern 'void show() { $$$ }' # Search for potential recursive calls to show() in callback functions rg -A 5 "show_callback.*=.*show" rg -A 10 "_callback.*show\(\)"
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/FX_fcn.cpp (1)
1594-1600: Consider persistent CCT buffer allocation for better performance.Allocating and freeing the CCT buffer on every
show()call could impact performance, especially at high frame rates. Consider keeping the buffer allocated and only reallocating when the strip size changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX_fcn.cpp(6 hunks)
⏰ 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 (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
🔇 Additional comments (4)
wled00/FX_fcn.cpp (4)
1321-1321: Per-pixel CCT implementation looks correct.The CCT value is properly obtained from the segment and consistently applied to all pixels, including mirrored variants. The null checks for
_pixelCCTensure safe operation when CCT buffer allocation fails.Also applies to: 1394-1409, 1481-1487
1619-1631: Efficient per-pixel CCT application during output.The implementation efficiently tracks CCT changes between pixels and only calls
setSegmentCCTwhen necessary. The original CCT value is properly preserved and restored for ABL adjustments.
1632-1633: Proper cleanup of CCT buffer.Good practice to free the buffer and set the pointer to nullptr, preventing use-after-free issues.
1594-1596: Verify architectural change from segment-level to per-pixel CCT.The comment mentions that "WLED doesn't handle CCT on pixel level but on Segment level instead", but this implementation adds per-pixel CCT support. Please confirm this architectural change is intentional and aligns with the PR objectives for fixing CCT issues.
fixes #4734
Summary by CodeRabbit
New Features
Bug Fixes