Bus level setPixelColor and getPixelColor optimizations #269
Conversation
…anager.cpp colorBalanceFromKelvin() is only called from inside bus_manager.cpp, so we can help the compiler optimize by making it a local (static) fuction
* bug: logf(temp-10) result becomes NaN when kelvin < 1200 * bug (RISC-V only): parameter of Bus::setCCT must be signed, to avoid undefined behaviour * minor optimization by replacing constrain() with min(max())
… hot path in sPC and gPC * optimize loops that scan through all busses * small speedups for Bus::autoWhiteCalc() * small speedups for ColorOrderMap::getPixelColorOrder() thanks to github Copilot for giving me the right ideas for this optimization
WalkthroughMoved Kelvin color-balance into BusManager, removed the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant BusManager
participant Bus
participant ColorBalance
rect rgb(240,255,240)
Caller->>BusManager: setPixelColor(pix, color)
note right of BusManager: new signature (no cct)\nuses cached lastBus/lastlen fast-path
end
alt cached fast-path (lastBus valid)
BusManager->>Bus: writePixelFast(pix, color)
Bus-->>BusManager: ack
else slow-path or cache miss
BusManager->>ColorBalance: colorBalanceFromKelvin(kelvin, color)
ColorBalance-->>BusManager: balancedColor
BusManager->>Bus: writePixel(pix, balancedColor)
Bus-->>BusManager: ack
end
BusManager-->>Caller: return
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Areas to review closely:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)wled00/**/*.{cpp,h}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-10-22T22:24:46.163ZApplied to files:
📚 Learning: 2025-10-22T21:50:25.839ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
🔇 Additional comments (5)
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 review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wled00/colors.cpp (2)
173-175: Verify performance claim for min/max over constrain().The comment states
min(max())is faster thanconstrain(), but in most implementations,constrain()is a macro that expands to similar logic. This may be a micro-optimization with negligible benefit unless proven otherwise on this specific platform.Consider benchmarking to confirm the performance improvement, or revert to
constrain()for better readability if the difference is negligible.
325-341: Consider removing the disabled code block.The
colorBalanceFromKelvinfunction is wrapped in#if 0since it has been moved tobus_manager.cpp. Consider removing this dead code entirely rather than leaving it disabled, as it may cause confusion for future maintainers.If there's a reason to preserve this for reference or potential rollback, add a comment explaining why it's being kept.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wled00/bus_manager.cpp(6 hunks)wled00/bus_manager.h(3 hunks)wled00/colors.cpp(3 hunks)wled00/fcn_declare.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/fcn_declare.hwled00/bus_manager.hwled00/bus_manager.cppwled00/colors.cpp
🧬 Code graph analysis (2)
wled00/bus_manager.h (3)
wled00/FX_fcn.cpp (9)
setCCT(566-576)setCCT(566-566)setCCT(2170-2176)setCCT(2170-2170)setPixelColor(941-941)setPixelColor(1183-1213)setPixelColor(1183-1183)setPixelColor(1972-1977)setPixelColor(1972-1972)wled00/bus_manager.cpp (10)
setPixelColor(216-233)setPixelColor(216-216)setPixelColor(316-367)setPixelColor(316-316)setPixelColor(450-459)setPixelColor(450-450)setPixelColor(521-555)setPixelColor(521-521)setPixelColor(1087-1087)setPixelColor(1305-1305)wled00/bus_wrapper.h (1)
setPixelColor(719-838)
wled00/bus_manager.cpp (3)
wled00/colors.cpp (2)
colorKtoRGB(143-177)colorKtoRGB(143-143)wled00/FX_fcn.cpp (9)
i(702-712)i(702-702)i(713-719)i(713-713)setPixelColor(941-941)setPixelColor(1183-1213)setPixelColor(1183-1183)setPixelColor(1972-1977)setPixelColor(1972-1972)wled00/bus_wrapper.h (1)
setPixelColor(719-838)
🔇 Additional comments (11)
wled00/colors.cpp (1)
145-153: Verify white channel value in fallback.The safety guard correctly prevents
logfdomain errors, butrgb[3] = 0sets the white channel to zero. For a pure white fallback (RGB 255,255,255), shouldrgb[3]also be 255, or is it intentionally left at 0 because this represents RGB-only white?Consider clarifying the intent with a comment, or verify that downstream consumers expect
rgb[3]=0for this fallback case.wled00/bus_manager.h (3)
178-180: Good fix for signed/unsigned conversion issue.Changing the parameter type to
int16_tcorrectly handles the sentinel value-1(used to indicate "no CCT"). Withuint16_t,-1would wrap to65535, causing undefined behavior.
466-466: Improved naming: lastend → lastlen.Renaming
lastendtolastlenimproves clarity. The new name clearly indicates it stores the length of the cached bus segment, avoiding ambiguity about whether it represents an exclusive or inclusive end index.Also applies to: 508-508
472-473: API change verified – all call sites correctly updated.The
setPixelColorsignature change has been successfully implemented. The old signature(uint16_t pix, uint32_t c, int16_t cct)has been removed and replaced with(uint16_t pix, uint32_t c). Verification confirms:
- No remaining calls use the removed three-parameter signature
- CCT handling has been properly migrated to separate
setCCT()methods (Segment, WS2812FX, and Bus classes)- All existing call sites use valid overloaded signatures: 2-arg (color), 4-arg (RGB), 5-arg (RGBA), or with anti-aliasing flag
- References to CCT handling now flow through the dedicated
setCCT()API rather than inline parameter passingwled00/fcn_declare.h (1)
69-69: LGTM: Public declaration correctly removed.The public declaration of
colorBalanceFromKelvinhas been appropriately removed since the function is nowstatic inlineinbus_manager.cpp. The comment clearly documents the relocation for better optimization.wled00/bus_manager.cpp (6)
82-98: Good relocation for optimization, verify default correctionRGB initialization.Moving
colorBalanceFromKelvintobus_manager.cppasstatic inlineallows the compiler to inline it at hot-path call sites, improving performance. The use ofuint_fast16_tfor calculations is also appropriate.Note: The default
correctionRGBis initialized to{255,255,255,0}(neutral white balance) instead of{0,0,0,0}. This is a better default, but verify this change is intentional and won't cause issues ifcolorKtoRGBis never called.
117-129: Excellent hot-path optimizations in getPixelColorOrder.The optimizations are well-thought-out:
__attribute__((hot))guides compiler optimization- Pre-computing
swapWavoids repeated bit shiftsuint_fast8_tleverages native fast typesconst auto &eliminates unnecessary copies- Unsigned arithmetic range check
(uint16_t)(pix - m.start) < m.lenis elegant and safe (underflow creates large value that fails the check)
132-153: Cleaner implementation with max/min functions.The refactored
autoWhiteCalcimproves readability by usingmax()andmin()instead of nested conditionals. The use ofuint_fast8_tfor channel values is appropriate for hot-path code.Note: Modern compilers typically optimize nested conditionals and
max()/min()calls to similar machine code, so the primary benefit here is improved readability and maintainability.
1305-1332: Well-implemented hot-path optimization with proper safeguards.The fast-path caching in
setPixelColoris a significant performance optimization for sequential pixel access patterns. The implementation correctly:
- Checks
lastBusfor nullptr before dereferencing- Uses short-circuit evaluation to safely skip the fast path if cache is invalid
- Falls back to full bus search when needed
- Uses unsigned arithmetic for efficient range checking
The
constpointer hints anduint_fast8_tloop variables should help compiler optimization.
1349-1374: Consistent optimization pattern applied to getPixelColor.The same efficient caching and fast-path logic from
setPixelColoris correctly applied here. The implementation maintains consistency across the API and provides the same performance benefits for read operations.
1376-1401: LGTM: Restored variant maintains optimization consistency.The
getPixelColorRestoredimplementation follows the same optimized pattern, correctly callingb->getPixelColorRestored()instead ofb->getPixelColor(). This ensures consistent performance across all pixel access methods.
previously gPC cached busses were disabled in slowMode (one pixel mapped to several outputs). However this should not be needed, because the first copy found should be like all other.
|
first test on HUB75 shows no regressions, and 2-4% speedup depending on effect. Things that should be faster now
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/bus_manager.cpp (1)
1349-1374: Optimizations look good, but note the slowMode handling difference.The fast-path optimization is well-implemented. However, lines 1364-1369 show commented-out
slowModechecks, meaning this function always caches the last bus, unlikesetPixelColorwhich respectsslowMode. This asymmetry might be intentional (reads may benefit more from caching), but consider whether both functions should handleslowModeconsistently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/bus_manager.cpp(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/bus_manager.cpp
🧬 Code graph analysis (1)
wled00/bus_manager.cpp (4)
wled00/colors.cpp (9)
hot(11-43)hot(83-111)hot(327-340)hot(437-443)hot(445-449)hot(469-472)hot(475-487)colorKtoRGB(143-177)colorKtoRGB(143-143)wled00/FX_fcn.cpp (18)
hot(941-1180)hot(1215-1325)hot(1406-1429)hot(1466-1505)hot(1508-1529)hot(1534-1571)hot(1616-1633)i(702-712)i(702-702)i(713-719)i(713-713)setPixelColor(941-941)setPixelColor(1183-1213)setPixelColor(1183-1183)setPixelColor(1972-1977)setPixelColor(1972-1972)getPixelColorRestored(1986-1991)getPixelColorRestored(1986-1986)wled00/FX_2Dfcn.cpp (2)
hot(198-204)hot(221-230)wled00/bus_manager.h (1)
getPixelColorRestored(318-318)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3_tinyUF2)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (esp8266_4MB_S)
- GitHub Check: Builds (seeed_esp32c3_4MB_S)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (esp32S3_8MB_M)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
🔇 Additional comments (6)
wled00/bus_manager.cpp (6)
82-98: LGTM with a note on the initialization improvement.The move to static inline and the change of
correctionRGBinitialization from{0,0,0,0}to{255,255,255,0}is correct. The new default represents neutral correction (no color change), whereas the old default would have incorrectly produced black output on the first call whenkelvinis 0.
117-129: Well-optimized hot path implementation.The unsigned arithmetic trick at line 124 efficiently checks whether
pixis in the range[m.start, m.start + m.len)with a single comparison. Extracting the W swap information once before the loop and usingconst auto&for the mapping reference are good micro-optimizations.
132-153: LGTM! Cleaner and more efficient.The simplified logic using
max()andmin()functions is more readable than nested conditionals while maintaining correctness. The use ofuint_fast8_tis appropriate for these byte-sized color values.
1305-1332: Excellent hot-path optimizations with proper safety checks.The fast-path cache with explicit null check on
lastBus(line 1308) prevents potential null-pointer dereferences. The unsigned arithmetic trick at lines 1308 and 1321 efficiently performs range checks. The rename fromlastendtolastlenbetter reflects what's being cached.Note: The removal of the
cctparameter is handled by moving CCT color correction into the individual bus implementations (e.g., lines 218, 320, 524).
1376-1401: Optimizations are correct and consistent with getPixelColor.The implementation follows the same pattern as
getPixelColorwith proper fast-path caching and unsigned arithmetic range checks. The same slowMode handling consideration fromgetPixelColorapplies here (lines 1391-1396 have commented-out checks).
1243-1243: LGTM - consistent with the cache variable rename.The initialization of
lastlento 0 is correct and consistent with the rename fromlastendtolastlen. This properly invalidates the cache when buses are added or removed.Also applies to: 1284-1284
Bus level setPixelColor and getPixelColor optimizations
Summary by CodeRabbit
Bug Fixes
Breaking Changes
Performance