Skip to content

Conversation

@DedeHai
Copy link
Collaborator

@DedeHai DedeHai commented Nov 13, 2025

backport of #5014

allows any bus to be changed again, not only the last.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 13, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@DedeHai DedeHai changed the title Dynamic LED type selection, backport Dynamic LED type selection, backport to 0.15 Nov 13, 2025
@DedeHai
Copy link
Collaborator Author

DedeHai commented Nov 13, 2025

@coderabbitai please review and check for issues compared against #5014

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 13, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch details of PR #5084 (current PR)
echo "=== PR #5084 Details ==="
gh pr view 5084 --repo wled/WLED --json title,body,files,additions,deletions

echo -e "\n=== PR #5084 Diff ==="
gh pr diff 5084 --repo wled/WLED

echo -e "\n=== PR #5014 Details ==="
gh pr view 5014 --repo wled/WLED --json title,body,files,additions,deletions

echo -e "\n=== PR #5014 Diff ==="
gh pr diff 5014 --repo wled/WLED

Length of output: 11166


@DedeHai I've completed the comparison between PR #5084 (this backport to 0.15) and PR #5014 (original implementation). Here's my analysis:

✅ Core Functionality: Identical

The main updateTypeDropdowns() function that implements the dynamic LED type selection is completely identical in both PRs. This is the critical part, and the backport has preserved it perfectly.

📊 Differences (Expected & Appropriate)

The differences I found are branch-specific adaptations for 0.15, which appear correct:

1. Virtual Bus Limits

2. Bus Number Extraction (Line ~257/270)

🎯 Conclusion

The backport looks solid. The core logic that enables changing any bus (not just the last one) has been correctly ported. The differences are appropriate branch-specific adaptations. The implementation:

  • ✅ Removes the restriction on changing non-last buses
  • ✅ Adds dynamic dropdown updates on change
  • ✅ Implements identical bus type counting and limiting logic
  • ✅ Handles the temporary virtual type assignment during dropdown updates
  • ✅ Calls updateTypeDropdowns() at appropriate times (initialization and on change)

No issues detected - the backport faithfully implements the same functionality with appropriate adjustments for the 0.15 branch.


🧠 Learnings used
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: 4982
File: wled00/data/matrixtool/omggif.js:32-35
Timestamp: 2025-10-05T15:41:15.810Z
Learning: The file `wled00/data/matrixtool/omggif.js` is an external third-party library from https://github.com/deanm/omggif by Dean McNamee and should not be reviewed for code issues as it's maintained externally.

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.

Learnt from: willmmiles
Repo: wled/WLED PR: 4859
File: usermods/udp_name_sync/udp_name_sync.cpp:62-70
Timestamp: 2025-08-21T00:00:17.196Z
Learning: In WLED codebase, UDP packet processing code must never rely on null termination being present on the wire and must always validate packet size first. The codebase already has some vulnerabilities where malformed data can crash the software, so avoiding introduction of new ones is a priority.

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.

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.

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.

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.

@DedeHai DedeHai added this to the 0.15.2 milestone Nov 15, 2025
@DedeHai
Copy link
Collaborator Author

DedeHai commented Nov 15, 2025

tested on ESP8266 and ESP32 C3 and works as intended.
What I did see once is that a bus got dropped when switching back-and-forth between this PR and before this PR but I was not able to reproduce it. From my side, this has green light.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant