Prevent parallel I2S use if different LED types are used. #4763
Conversation
blazoncek
commented
Jul 9, 2025
blazoncek
commented
- helps with Mixed bus timings in parallel I2S cause incorrect output #4315
- add-on to bus_wrapper: Use parallel I2S first when enabled #4762
- increases parallel I2S LED limit to 600 per bus
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 (
|
|
Large led count may need thorough testing with several usermods enabled. |
|
@coderabbitai please review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll examine the PR changes to provide a comprehensive review. 🏁 Script executed: Length of output: 8934 @blazoncek Here's my review of PR #4763: SummaryThis PR implements two main changes:
Technical Analysis✅ Good ChangesMixed bus type detection (FX_fcn.cpp): if (busType == 0) busType = bus.type; // remember first bus type
if (busType != bus.type) {
DEBUG_PRINTF_P(PSTR("Mixed digital bus types detected! Forcing single I2S output.\n"));
useParallelI2S = false; // mixed bus types, no parallel I2S
}This is a solid safety measure that prevents potential issues with mixed LED types. Scope fix: Moving JavaScript refactoring: The new helper functions (
|
|
would we also want to merge this into main @blazoncek ? |
|
|
@netmindz yes |
|
I have now ran a local test with WLED
Test Results:
I also had a second look at the LED Settings HTML changes of this PR. I see now that it receives the limit from an API call and then looks at the Max Analog, Max Digital and Max Virtual bus counts to identify the chip type. That solution works, but if those limits ever change, there's a risk of subtle bugs in the future where someone may forget to also change the chip identification in I think that part of the code should be changed to add a backend API change which returns the chip as a string, such as And then make the backend settings API return that string in the "get current settings" response. And then change function is8266() { return maxA == 5 && maxD == 3; } // NOTE: see const.h
function is32() { return maxA == 16 && maxD == 16; } // NOTE: see const.h
function isC3() { return maxA == 6 && maxD == 2; } // NOTE: see const.h
function isS2() { return maxA == 8 && maxD == 12 && maxV == 4; } // NOTE: see const.h
function isS3() { return maxA == 8 && maxD == 12 && maxV == 6; } // NOTE: see const.hTo this: function is8266() { return chipType === "ESP8266"; }
function is32() { return chipType === "ESP32"; }
function isC3() { return chipType === "ESP32-C3"; }
function isS2() { return chipType === "ESP32-S2"; }
function isS3() { return chipType === "ESP32-S3"; }Other than that part, I think everything here looks very good. Thank you Blaz for being such an amazing dude and amazing developer. ❤️ However, I am not able to test the memory limits to check for overflows with huge amounts of LEDs and many usermods. Is that only likely to be a problem on ESP32-S2 with many usermods enabled at the same time, though? |
Prevent parallel I2S use if different LED types are used.
Prevent parallel I2S use if different LED types are used.
