Added support for LSC Smart connect led strips - #5177
Conversation
Support is added for LSC Smart Connect led strips which are sold at ACTION stores around Europe. These strips are special because they use dual SM16703 chips, the first is used for controlling a RGB LED, the second is used for WW and CW. For controlling the strip this gives 6 channels in a row: G,R,B,WW,CW,N/C which means the 3th channel of the second SM16703 chip is not used. Firmware was built using these files and tested on Wemo D1 mini.
In this commit,support is added for LSC Smart Connect led strips which are sold at ACTION stores around Europe. These strips are special because they use dual SM16703 chips, the first is used for controlling a RGB LED, the second is used for WW and CW. For controlling the strip this gives 6 channels in a row: G,R,B,WW,CW,N/C which means the 3th channel of the second SM16703 chip is not used. Recommitted because CodeRabbit found that the skipping LED function calculation was not yet up to date with the usage of 2 IC`s - Firmware was built using these files and tested on Wemo D1 mini. - In the UI the strip is called “SM16703 RGB+CCT (2x)” - Product: https://www.action.com/nl-nl/p/3218153/lsc-smart-connect-ledstrip/
📝 WalkthroughWalkthroughAdds a new digital LED type Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as Controller
participant BusDigital as BusDigital
participant PolyBus as PolyBus
participant HW as LED_Hardware
Controller->>BusDigital: setPixelColor(logicalIndex, RGB, CCT)
BusDigital->>BusDigital: map logicalIndex -> firstIdx, secondIdx (dual)
BusDigital->>BusDigital: compute RGB for first chip
BusDigital->>BusDigital: compute WW/CW via CCT (+swap)
BusDigital->>PolyBus: write firstIdx (RGB)
PolyBus->>HW: emit RGB to physical LED (first chip)
BusDigital->>PolyBus: write secondIdx (WW/CW)
PolyBus->>HW: emit WW/CW to physical LED (second chip)
BusDigital->>BusDigital: update ABL sums for both chips
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
wled00/bus_manager.cpp (2)
375-396: MissingTYPE_SM16703_DUALhandling ingetPixelColor().While
setPixelColor()has special dual-chip handling,getPixelColor()doesn't account for the 2x hardware pixel mapping. This will cause incorrect color reads forTYPE_SM16703_DUAL:// Current code reads from hardware index `pix`, but should read from `pix * 2` uint32_t c = restoreColorLossy(PolyBus::getPixelColor(_busPtr, _iType, ... pix ...), _NPBbri);Consider adding dual-chip handling similar to
setPixelColor():if (_type == TYPE_SM16703_DUAL) { if (_reversed) pix = _len - pix - 1; pix += _skip; unsigned firstIdx = pix * 2; // Read RGB from first chip, ignore second chip's WW/CW const uint8_t co = _colorOrderMap.getPixelColorOrder(pix + _start, _colorOrder); return restoreColorLossy(PolyBus::getPixelColor(_busPtr, _iType, firstIdx, co), _NPBbri); }This affects features like reading back pixel state for effects or transitions.
261-270: MissingTYPE_SM16703_DUALhandling inapplyBriLimit().The hardware length calculation on line 263 doesn't account for dual-chip mode, causing brightness limiting to only process half the physical pixels:
unsigned hwLen = _len; if (_type == TYPE_WS2812_1CH_X3) hwLen = NUM_ICS_WS2812_1CH_3X(_len); // Missing: if (_type == TYPE_SM16703_DUAL) hwLen = (_len + _skip) * 2;This will result in incomplete brightness correction for dual-chip strips. Add the dual-chip case:
unsigned hwLen = _len; if (_type == TYPE_WS2812_1CH_X3) hwLen = NUM_ICS_WS2812_1CH_3X(_len); + if (_type == TYPE_SM16703_DUAL) hwLen = (_len + _skip) * 2;Additionally, the loop logic would need adjustment since it currently calls
getPixelColor/setPixelColoron PolyBus directly with a single index, but dual-chip needs paired indices and separate RGB/WW-CW handling.
🧹 Nitpick comments (1)
wled00/bus_manager.cpp (1)
308-316: Consider including WW/CW in the wacky ABL model fallback.The wacky WS2815 power model (when
_milliAmpsPerLed == 255) ignores the WW/CW channels and only uses max RGB. SinceTYPE_SM16703_DUALhas two separate chips, you might want to include WW/CW contribution in the power estimation even for the wacky model:} else { // wacky model not expected; fall back to max RGB uint8_t maxRgb = (r > g) ? ((r > b) ? r : b) : ((g > b) ? g : b); - _colorSum += maxRgb; + _colorSum += maxRgb + ww + cw; // include WW/CW from second chip }This is a minor enhancement since the wacky model is WS2815-specific and unlikely to be used with SM16703 strips.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wled00/bus_manager.cpp(3 hunks)wled00/bus_manager.h(1 hunks)wled00/bus_wrapper.h(2 hunks)wled00/const.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/**/!(html_*)*.h
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for non-generated C++ header files (.h)
Files:
wled00/bus_manager.hwled00/bus_wrapper.hwled00/const.h
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/bus_manager.cpp
🧠 Learnings (25)
📓 Common learnings
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
📚 Learning: 2025-09-18T03:17:30.107Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.107Z
Learning: In WLED BusManager (wled00/FX_fcn.cpp), direct access to BusManager::busses is part of the intended public API design, not an encapsulation violation. The wrapper accessor methods are considered unnecessary obfuscation that should be removed in future refactoring.
Applied to files:
wled00/bus_manager.hwled00/bus_wrapper.hwled00/bus_manager.cpp
📚 Learning: 2025-09-18T03:17:30.107Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.107Z
Learning: In WLED bus management code, dead code branches in error handling (like unreachable else breaks) are intentionally preserved to accommodate potential future API changes, even when currently unreachable.
Applied to files:
wled00/bus_manager.hwled00/bus_wrapper.hwled00/bus_manager.cpp
📚 Learning: 2025-09-16T18:08:42.848Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Applied to files:
wled00/bus_manager.hwled00/bus_wrapper.hwled00/bus_manager.cpp
📚 Learning: 2025-09-21T20:44:09.502Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4928
File: wled00/bus_wrapper.h:1248-1275
Timestamp: 2025-09-21T20:44:09.502Z
Learning: In WLED bus_wrapper.h, the _useParallelI2S flag is used for selecting the correct function pointer (between parallel I2S and non-parallel I2S bus types) rather than for changing memory calculations. Both parallel and non-parallel I2S configurations have the same memory requirements, so memUsage() correctly doesn't differentiate based on this flag.
Applied to files:
wled00/bus_wrapper.hwled00/bus_manager.cpp
📚 Learning: 2025-09-18T03:17:30.107Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.107Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), digitalCount is intentionally incremented even for placeholder buses to maintain consistent resource mapping across all bus configurations.
Applied to files:
wled00/bus_wrapper.hwled00/bus_manager.cpp
📚 Learning: 2025-09-02T01:45:58.047Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:31-36
Timestamp: 2025-09-02T01:45:58.047Z
Learning: For the NeoEsp32RmtHI driver, RISC-V ESP32-C3 support is currently disabled via bus_wrapper.h rather than compile-time guards, as the maintainer willmmiles is working on resolving underlying nested interrupt issues and prefers to centralize the workaround in one location.
Applied to files:
wled00/bus_wrapper.h
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
wled00/const.hwled00/bus_manager.cpp
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Applied to files:
wled00/const.hwled00/bus_manager.cpp
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated embedded web header files (wled00/html_*.h)
Applied to files:
wled00/const.h
📚 Learning: 2025-04-26T19:19:07.600Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
Applied to files:
wled00/const.h
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/const.h
📚 Learning: 2025-09-01T10:26:17.959Z
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/wled_eeprom.cpp:0-0
Timestamp: 2025-09-01T10:26:17.959Z
Learning: In WLED PR #4876, the DMXStartLED EEPROM backward compatibility issue was partially addressed by keeping it at address 2550 and reading it as a 16-bit value, with DMXChannelsValue array moved to addresses 2552-2566. This maintains compatibility with pre-0.11 EEPROM layouts for DMXStartLED, though legacy "Set to 255" (code 6) configurations may still need migration logic.
Applied to files:
wled00/const.hwled00/bus_manager.cpp
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/const.h
📚 Learning: 2025-11-16T19:40:46.260Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
Applied to files:
wled00/const.h
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/const.hwled00/bus_manager.cpp
📚 Learning: 2025-12-01T07:01:16.949Z
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Applied to files:
wled00/const.h
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/const.h
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-09-28T09:53:42.670Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/data/index.js:2406-2413
Timestamp: 2025-09-28T09:53:42.670Z
Learning: In WLED bus management, bus objects can exist with valid configuration data even when isOK() returns false. The getPins() method can be safely called on buses that are not OK, as the bus object and its configuration exist independently of the isOK() status, which specifically indicates whether the NeoPixel bus was successfully created.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-09-02T01:56:43.841Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:173-180
Timestamp: 2025-09-02T01:56:43.841Z
Learning: willmmiles prefers to maintain consistency with upstream NeoPixelBus patterns (like unchecked malloc in construct() methods) rather than diverging until improvements are made upstream first, to minimize maintenance burden and keep the codebase aligned.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-09-13T13:13:36.092Z
Learnt from: netmindz
Repo: wled/WLED PR: 3777
File: wled00/bus_manager.cpp:982-989
Timestamp: 2025-09-13T13:13:36.092Z
Learning: In the WLED HUB75 implementation, when HUB75 matrix initialization fails after pin allocation, pins are intentionally not deallocated because they're physically connected to the HUB75 panel and cannot be reused for other purposes. This design choice prioritizes simplicity over strict software resource management consistency.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-11-22T12:12:53.486Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5105
File: wled00/FX.h:968-968
Timestamp: 2025-11-22T12:12:53.486Z
Learning: In WLED's WS2812FX class (wled00/FX.h), the _pixels array stores LED colors indexed by unmapped (logical) LED positions, not physical positions. The getPixelColor(n) function intentionally checks if the logical LED at index n maps to a valid physical position via getMappedPixelIndex(n), then accesses _pixels[n] with the unmapped index. If the mapping is invalid (e.g., -1 for gaps in LED maps, or out of bounds), it returns 0 (black). This allows peek preview to correctly show gaps in LED mappings as black pixels. The mapping from logical to physical positions is only applied when writing to the hardware bus, not when reading from the pixel buffer.
<!-- [add_learning]
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-11-14T05:48:44.673Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5040
File: wled00/image_loader.cpp:84-96
Timestamp: 2025-11-14T05:48:44.673Z
Learning: In WLED (wled00/FX_2Dfcn.cpp), the Segment::setPixelColorXY() function performs internal bounds checking against vWidth() and vHeight(), returning early if coordinates are out of bounds. No additional guards are needed when calling this function, even in upscaling loops where coordinates might exceed segment dimensions.
Applied to files:
wled00/bus_manager.cpp
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
wled00/bus_manager.cpp
🔇 Additional comments (9)
wled00/const.h (1)
308-308: LGTM!The new constant
TYPE_SM16703_DUALis correctly defined with value 35, fitting within the digital type range (16-39). The descriptive comment accurately documents the dual-chip configuration (RGB chip + WW/CW chip with unused B channel).wled00/bus_manager.h (2)
177-178: LGTM!
TYPE_SM16703_DUALis correctly added tohasWhite()with a clear comment. Since the second SM16703 chip controls WW/CW channels, this type rightfully has white capability.
186-186: LGTM!
TYPE_SM16703_DUALis correctly included inhasCCT()alongside other CCT-capable types likeTYPE_WS2805andTYPE_SM16825.wled00/bus_wrapper.h (2)
1318-1319: LGTM!
TYPE_SM16703_DUALcorrectly maps to the standard NeoPixel 3-channel bus type on ESP8266. This is appropriate since SM16703 uses the same protocol as WS2812, and the dual-chip pixel handling is managed at a higher level inBusDigital::setPixelColor().
1382-1383: LGTM!ESP32 path mirrors the ESP8266 mapping, correctly routing
TYPE_SM16703_DUALtoI_32_RN_NEO_3 + offset.wled00/bus_manager.cpp (4)
189-195: LGTM on initialization logic.The length calculation
(bc.count + _skip) * 2correctly allocates two hardware pixels per logical pixel (including skip pixels). The conditionalPolyBus::create()call appropriately handles the already-computed length for dual-chip mode versus adding skip separately for other types.
198-201: LGTM!Sacrificial pixel handling correctly doubles the skip count for the dual-chip configuration, ensuring both physical chips per logical skip pixel are initialized to black.
324-326: LGTM!Bounds checking correctly validates against the hardware buffer length and uses
secondIdx(the larger index of the two chips) to ensure both writes are safe.
418-418: LGTM!The new LED type is properly exposed in the public API with a clear, descriptive name "SM16703 RGB+CCT (2x)" that communicates the dual-chip configuration.
| // Second chip: R=WW, G=CW by default; respect WW/CW swap flag (upper nibble of color order) | ||
| const uint8_t coSecond = _colorOrderMap.getPixelColorOrder(logicalPix + _start, _colorOrder); | ||
| bool swapWhites = true; // hardware wiring has WW/CW reversed relative to CCT | ||
| if ((coSecond >> 4) & 0x01) swapWhites = !swapWhites; // UI swap toggles | ||
| if (swapWhites) { uint8_t tmp = ww; ww = cw; cw = tmp; } | ||
| uint32_t wwCwColor = RGBW32(ww, cw, 0, 0); | ||
| PolyBus::setPixelColor(_busPtr, _iType, secondIdx, wwCwColor, COL_ORDER_GRB, 0); | ||
| return; |
There was a problem hiding this comment.
Potential issue with WW/CW swap detection.
The swap logic on line 334 checks (coSecond >> 4) & 0x01, but the WW/CW swap flag in the color order upper nibble is value 4 (as seen in bus_wrapper.h line 809: case 4: std::swap(cctWW, cctCW);). Checking bit 0 won't detect value 4:
| Upper nibble | Binary | & 0x01 result |
|---|---|---|
| 0 (no swap) | 0000 | false |
| 1 (W&B) | 0001 | true |
| 4 (WW&CW) | 0100 | false |
If the intent is to respect the WW/CW swap setting from the UI, the condition should check for value 4 specifically:
- bool swapWhites = true; // hardware wiring has WW/CW reversed relative to CCT
- if ((coSecond >> 4) & 0x01) swapWhites = !swapWhites; // UI swap toggles
+ bool swapWhites = true; // hardware wiring has WW/CW reversed relative to CCT
+ if ((coSecond >> 4) == 4) swapWhites = !swapWhites; // UI WW/CW swap togglesPlease verify this matches the intended behavior for the LSC Smart Connect strips.
🤖 Prompt for AI Agents
In wled00/bus_manager.cpp around lines 331 to 338, the WW/CW swap check shifts
the upper nibble then tests bit 0 which misses the UI swap value 4; change the
condition to extract the upper nibble (coSecond >> 4) and compare it to 4 (or
test ( (coSecond >> 4) & 0x07 ) == 4 if you want to be explicit about possible
values) so the WW/CW swap is honored; keep the rest of the swap logic and pixel
write unchanged.
|
Hey! This pull request has been open for quite some time without any new comments now. It will be closed automatically in a week if no further activity occurs. |
github-actions
Bot
added
the
stale
|
@Yannicflight thanks for taking a stab at it. |
|
@coderabbitai please re-review, using the new coding guideline in the |
🧩 Analysis chainI'll gather the coding guidelines from the main branch and the PR diff for a thorough re-review. 🏁 Script executed: Length of output: 8064 Good, got the diff. Now let me read the coding guidelines and more context from the existing code. 🏁 Script executed: Length of output: 13295 Now let me look at more of bus_manager.cpp for the full context, especially 🏁 Script executed: Length of output: 7631 🏁 Script executed: Length of output: 4005
Coding Guidelines Summary (from
|
| # | Area | Severity | Issue |
|---|---|---|---|
| 1 | getPixelColor() |
🔴 Bug | Wrong hardware index — returns garbage color |
| 2 | applyBriLimit() |
🔴 Bug | Second chip (WW/CW) never brightness-corrected |
| 3 | estimateCurrent() |
🔴 Bug | clrUnitsPerChannel = 4×255 but 5 channels summed |
| 4 | setPixelColor() |
🔴 Bug | coSecond color order discarded; second chip always COL_ORDER_GRB |
| 5 | setPixelColor() |
🔴 Logic | swapWhites = true inverts UI swap semantics |
| 6 | setPixelColor() |
🟡 Quality | ABL _colorSum block duplicated |
| 7 | setPixelColor() |
🟡 Quality | hwLen recomputed per pixel on hot path |
| 8 | setPixelColor() |
🟡 Quality | logicalPix duplicates standard index normalization |
| 9 | const.h |
🟢 Style | Missing space after // in comment |
| 10 | setPixelColor() |
🟢 Style | Misleading "wacky model" comment |
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:794-830
Timestamp: 2026-03-31T13:42:00.444Z
Learning: In WLED PR `#5456` (Matter over WiFi usermod, CMakeLists.txt + platformio.ini): The GCC 14 chip::to_underlying compatibility issue with CHIP SDK's TypeTraits.h is fixed by building in gnu++20 mode (not gnu++2b/gnu++23). CMakeLists.txt uses `idf_build_replace_option_from_property` to swap `-std=gnu++2b` for `-std=gnu++20` when the matter usermod is present. The `matter_gcc14_compat.h` shim file (which pre-defines `chip::to_underlying` and sets `CHIP_TO_UNDERLYING_DEFINED`) is dead code under this configuration — it is never included anywhere. TypeTraits.h's broken C++23 `using std::to_underlying` alias path is only taken in gnu++23 mode; in gnu++20 mode CHIP defines its own `chip::to_underlying` function template normally, so no shim is needed. Additionally, upstream connectedhomeip TypeTraits.h already has the fix built-in. ESP-IDF v5.5 uses GCC 14.2.0. Do NOT flag the missing `-include` for `matter_gcc14_compat.h` as a build issue.
Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:794-830
Timestamp: 2026-03-31T13:42:00.444Z
Learning: In WLED PR `#5456` (Matter over WiFi usermod, CMakeLists.txt + platformio.ini): The GCC 14 chip::to_underlying compatibility issue with CHIP SDK's TypeTraits.h is fixed by building in gnu++20 mode (not gnu++2b/gnu++23). CMakeLists.txt uses `idf_build_replace_option_from_property` to swap `-std=gnu++2b` for `-std=gnu++20` when the matter usermod is present. The `matter_gcc14_compat.h` shim file (which pre-defines `chip::to_underlying` and sets `CHIP_TO_UNDERLYING_DEFINED`) is dead code under this configuration — it is never included anywhere. TypeTraits.h's broken C++23 `using std::to_underlying` alias path is only taken in gnu++23 mode; in gnu++20 mode CHIP defines its own `chip::to_underlying` function template normally, so no shim is needed. Do NOT flag the missing `-include` for `matter_gcc14_compat.h` as a build issue.
✅ Actions performed
Full review triggered.
softhack007
added
enhancement
waiting for feedback
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/bus_manager.cpp`:
- Around line 189-199: Bus memory accounting underestimates RAM/DMA usage for
TYPE_SM16703_DUAL because constructor creates (count + _skip) * 2 hardware
pixels; update the memory calculations in BusConfig::memUsage() and
BusManager::memUsage() to mirror the allocation logic used for PolyBus::create
(i.e., compute lenToCreate the same way: if type == TYPE_SM16703_DUAL then use
(count + _skip) * 2, if type == TYPE_WS2812_1CH_X3 use
NUM_ICS_WS2812_1CH_3X(count), otherwise use count + _skip) so reported memUsage
reflects the doubled hardware length (and continue to handle the skip/_skip
multiplier consistently).
- Around line 308-315: estimateCurrent() currently normalizes ABL white-capable
buses using 4 * 255 but your _colorSum for TYPE_SM16703_DUAL (updated in
BusManager::_useABL branch where _colorSum += r + g + b + ww + cw and
_milliAmpsPerLed < 255) uses five channels; change the normalization/divisor
logic in estimateCurrent() to use 5 * 255 for this dual white-capable bus (or
detect when _useABL and both ww/cw are included) so current is calculated
against 5 channels instead of 4, referencing BusManager::_useABL, _colorSum,
_milliAmpsPerLed, estimateCurrent(), and TYPE_SM16703_DUAL to locate the
relevant code paths.
- Around line 302-338: The new SM16703_DUAL path maps one logical pixel to two
hardware indices (firstIdx/secondIdx using logicalPix = (_reversed ? _len - pix
- 1 : pix) + _skip and hw index = logicalPix*2/ +1), but getPixelColor() and
applyBriLimit()/ABL repaint still assume 1:1 mapping; extract that mapping into
a shared helper (e.g., MapLogicalToHwIndices or BusManager::mapLogicalPixel)
that takes pix and returns {logicalPix, firstIdx, secondIdx, hwLen} (and
consults _reversed, _skip, _start, _len, and _colorOrderMap/_colorOrder as
needed), then update getPixelColor(), applyBriLimit(), and any ABL repaint code
to call that helper and operate on both hardware indices (and both chips'
channels / _colorSum adjustments) instead of using pix directly so readback and
ABL touch the correct chips.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11993b7b-c2e9-4f4b-936b-097da861048c
📒 Files selected for processing (4)
wled00/bus_manager.cppwled00/bus_manager.hwled00/bus_wrapper.hwled00/const.h
| if (bc.type == TYPE_SM16703_DUAL) lenToCreate = (bc.count + _skip) * 2; // two SM16703 chips per logical pixel (including skip) | ||
| if (bc.type == TYPE_WS2812_1CH_X3) lenToCreate = NUM_ICS_WS2812_1CH_3X(bc.count); // only needs a third of "RGB" LEDs for NeoPixelBus | ||
| _busPtr = PolyBus::create(_iType, _pins, lenToCreate + _skip, nr); | ||
| if (bc.type == TYPE_SM16703_DUAL) { | ||
| _busPtr = PolyBus::create(_iType, _pins, lenToCreate, nr); // lenToCreate already includes skip * 2 | ||
| } else { | ||
| _busPtr = PolyBus::create(_iType, _pins, lenToCreate + _skip, nr); | ||
| } | ||
| _valid = (_busPtr != nullptr) && bc.count > 0; | ||
| // fix for wled#4759 | ||
| if (_valid) for (unsigned i = 0; i < _skip; i++) { | ||
| unsigned skipHW = (bc.type == TYPE_SM16703_DUAL) ? _skip * 2 : _skip; | ||
| if (_valid) for (unsigned i = 0; i < skipHW; i++) { |
There was a problem hiding this comment.
Update memory accounting for the doubled hardware length.
This constructor now allocates (count + skip) * 2 pixels for TYPE_SM16703_DUAL, but the existing BusConfig::memUsage() / BusManager::memUsage() paths still size the bus from logical pixels. That underestimates RAM/DMA usage for this type and can make initialization heuristics overly optimistic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/bus_manager.cpp` around lines 189 - 199, Bus memory accounting
underestimates RAM/DMA usage for TYPE_SM16703_DUAL because constructor creates
(count + _skip) * 2 hardware pixels; update the memory calculations in
BusConfig::memUsage() and BusManager::memUsage() to mirror the allocation logic
used for PolyBus::create (i.e., compute lenToCreate the same way: if type ==
TYPE_SM16703_DUAL then use (count + _skip) * 2, if type == TYPE_WS2812_1CH_X3
use NUM_ICS_WS2812_1CH_3X(count), otherwise use count + _skip) so reported
memUsage reflects the doubled hardware length (and continue to handle the
skip/_skip multiplier consistently).
| if (_type == TYPE_SM16703_DUAL) { | ||
| // logical pixel uses two SM16703 chips: first RGB, second WW/CW (B unused) | ||
| uint8_t r = R(c), g = G(c), b = B(c); | ||
| uint8_t ww = 0, cw = 0; | ||
| if (hasCCT()) Bus::calculateCCT(c, ww, cw); | ||
|
|
||
| if (BusManager::_useABL) { | ||
| if (_milliAmpsPerLed < 255) { | ||
| _colorSum += r + g + b + ww + cw; // include both chips' channels for ABL | ||
| } else { | ||
| // wacky model not expected; fall back to max RGB | ||
| uint8_t maxRgb = (r > g) ? ((r > b) ? r : b) : ((g > b) ? g : b); | ||
| _colorSum += maxRgb; | ||
| } | ||
| } | ||
|
|
||
| unsigned logicalPix = pix; | ||
| if (_reversed) logicalPix = _len - pix - 1; | ||
| logicalPix += _skip; | ||
| unsigned firstIdx = logicalPix * 2; | ||
| unsigned secondIdx = firstIdx + 1; | ||
|
|
||
| // bounds safeguard (should not trigger if lenToCreate set correctly) | ||
| unsigned hwLen = (_len + _skip) * 2; | ||
| if (secondIdx >= hwLen) return; | ||
|
|
||
| const uint8_t coFirst = _colorOrderMap.getPixelColorOrder(logicalPix + _start, _colorOrder); | ||
| PolyBus::setPixelColor(_busPtr, _iType, firstIdx, c, coFirst, 0); | ||
|
|
||
| // Second chip: R=WW, G=CW by default; respect WW/CW swap flag (upper nibble of color order) | ||
| const uint8_t coSecond = _colorOrderMap.getPixelColorOrder(logicalPix + _start, _colorOrder); | ||
| bool swapWhites = true; // hardware wiring has WW/CW reversed relative to CCT | ||
| if ((coSecond >> 4) & 0x01) swapWhites = !swapWhites; // UI swap toggles | ||
| if (swapWhites) { uint8_t tmp = ww; ww = cw; cw = tmp; } | ||
| uint32_t wwCwColor = RGBW32(ww, cw, 0, 0); | ||
| PolyBus::setPixelColor(_busPtr, _iType, secondIdx, wwCwColor, COL_ORDER_GRB, 0); | ||
| return; |
There was a problem hiding this comment.
Mirror this 2:1 mapping in readback and ABL repaint paths.
This new path writes one logical pixel to two hardware indices, but getPixelColor() below still reads pix directly and applyBriLimit() still uses the generic 1:1 repaint logic. After pixel 0, readback resolves to the wrong chip, and ABL dimming will only touch part of the strip / wrong channels when it kicks in. Please extract the logical→hardware mapping into a shared helper and reuse it from set/get/applyBriLimit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/bus_manager.cpp` around lines 302 - 338, The new SM16703_DUAL path
maps one logical pixel to two hardware indices (firstIdx/secondIdx using
logicalPix = (_reversed ? _len - pix - 1 : pix) + _skip and hw index =
logicalPix*2/ +1), but getPixelColor() and applyBriLimit()/ABL repaint still
assume 1:1 mapping; extract that mapping into a shared helper (e.g.,
MapLogicalToHwIndices or BusManager::mapLogicalPixel) that takes pix and returns
{logicalPix, firstIdx, secondIdx, hwLen} (and consults _reversed, _skip, _start,
_len, and _colorOrderMap/_colorOrder as needed), then update getPixelColor(),
applyBriLimit(), and any ABL repaint code to call that helper and operate on
both hardware indices (and both chips' channels / _colorSum adjustments) instead
of using pix directly so readback and ABL touch the correct chips.
| if (BusManager::_useABL) { | ||
| if (_milliAmpsPerLed < 255) { | ||
| _colorSum += r + g + b + ww + cw; // include both chips' channels for ABL | ||
| } else { | ||
| // wacky model not expected; fall back to max RGB | ||
| uint8_t maxRgb = (r > g) ? ((r > b) ? r : b) : ((g > b) ? g : b); | ||
| _colorSum += maxRgb; | ||
| } |
There was a problem hiding this comment.
Use a 5-channel ABL divisor for this type.
_colorSum now includes r + g + b + ww + cw, but estimateCurrent() still normalizes white-capable buses with 4 * 255. For TYPE_SM16703_DUAL that overestimates current by 25% and dims earlier than necessary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/bus_manager.cpp` around lines 308 - 315, estimateCurrent() currently
normalizes ABL white-capable buses using 4 * 255 but your _colorSum for
TYPE_SM16703_DUAL (updated in BusManager::_useABL branch where _colorSum += r +
g + b + ww + cw and _milliAmpsPerLed < 255) uses five channels; change the
normalization/divisor logic in estimateCurrent() to use 5 * 255 for this dual
white-capable bus (or detect when _useABL and both ww/cw are included) so
current is calculated against 5 channels instead of 4, referencing
BusManager::_useABL, _colorSum, _milliAmpsPerLed, estimateCurrent(), and
TYPE_SM16703_DUAL to locate the relevant code paths.
|
closing since this appears abandoned, also this will be possible with no additional changes once #5704 is merged. |
3 participants
Support is added for LSC Smart Connect led strips which are sold at ACTION stores around Europe. These strips are special because they use dual SM16703 chips, the first is used for controlling a RGB LED, the second is used for WW and CW. For controlling the strip this gives 6 channels in a row: G,R,B,WW,CW,N/C which means the 3th channel of the second SM16703 chip is not used.
Recommitted because CodeRabbit found that the skipping LED function calculation was not yet up to date with the usage of 2 IC`s
Summary by CodeRabbit
New Features
Bug Fixes
Behavior