Update crude parallel I2S detection #4713
Conversation
My ESP32s are now reporting a maximum of 32 buses.
WalkthroughThe conditional logic in the UI() function within the settings_leds.htm file was updated. The check for identifying ESP32 boards was changed from comparing Changes
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/settings_leds.htm(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (e 8000 sp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Test cdata.js
| @@ -353,7 +353,7 @@ | |||
| }); | |||
| const S2 = (oMaxB == 14) && (maxV == 4); | |||
| const S3 = (oMaxB == 14) && (maxV == 6); | |||
| if (oMaxB == 19 || S2 || S3) { // TODO: crude ESP32 & S2/S3 detection | |||
| if (oMaxB == 32 || S2 || S3) { // TODO: crude ESP32 & S2/S3 detection | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM! Change correctly updates ESP32 detection for parallel I2S.
This change appropriately addresses the issue where ESP32 devices reporting 32 buses were missing the parallel I2S checkbox. The S2/S3 detection logic remains unchanged.
However, please verify backward compatibility and update the related comment inconsistency:
-
Backward compatibility concern: If older ESP32 firmware/hardware still reports
oMaxB == 19, those devices would lose the parallel I2S option. Please confirm that all target ESP32 devices now report 32 buses. -
Comment inconsistency: The comment in the
bLimitsfunction (line 46) states "20 - ESP32" for maxB, but the actual detection values have been 19 (old) and now 32 (new). Please update this comment to reflect the correct value.
🏁 Script executed:
#!/bin/bash
# Description: Check for other references to ESP32 bus counts to ensure consistency
# Expected: Find any hardcoded values or comments that might need updating
# Search for potential ESP32 bus count references
rg -i "esp32.*bus" -A 2 -B 2
rg "\b(19|20|32)\b.*esp32" -i -A 2 -B 2
rg "maxB.*esp32" -i -A 2 -B 2Length of output: 6635
Ensure ESP32 detection comment is updated and handle legacy bus counts if needed
Please address the following:
-
Update the comment in wled00/data/settings_leds.htm to reflect the new “32” bus count for ESP32:
- oMaxB = maxB = b; // maxB - max buses (can be changed if using ESP32 parallel I2S): 20 - ESP32, 14 - S3/S2, 6 - C3, 4 - 8266 + oMaxB = maxB = b; // maxB - max buses (can be changed if using ESP32 parallel I2S): 32 - ESP32, 14 - S3/S2, 6 - C3, 4 - 8266
-
If you need to support any older ESP32 variants still reporting 19 buses, consider restoring a fallback in the UI logic. For example:
if (oMaxB == 32 || oMaxB == 19 || S2 || S3) { … }
-
Confirm that all current ESP32 targets now report 32 buses and that no other hardcoded “19” or “20” bus‐count references remain.
🤖 Prompt for AI Agents
In wled00/data/settings_leds.htm at line 356, update the comment related to
ESP32 bus count to reflect the new value of 32 instead of 19 or 20.
Additionally, modify the condition to include a fallback for older ESP32 devices
that report 19 buses by changing the if statement to check for oMaxB == 32 or
oMaxB == 19 or S2 or S3. Finally, verify that no other parts of the codebase
contain hardcoded values of 19 or 20 for ESP32 bus counts and update or remove
them as necessary to ensure consistency and backward compatibility.
My ESP32s are reporting a maximum of 32 buses, so the parallel I2s tickbox was missing from the LED settings UI.
Summary by CodeRabbit