Conversation
due to Arduino 3.0.1, a few updated libraries are required: * Tasmota Platform - official platfomio lacks arduino support for C6 * FastLED (latest + C6 build patches) * AsyncTCP (latest + C6 build patches) * AsyncWebServer (latest + C6 build patches) * NeoPixelBus (lastest)
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds ESP32-C5 and ESP32-C6 build environments and PlatformIO config; expands many CONFIG_IDF_TARGET_* guards across sources so C5/C6 (and P4) are treated like C3 for I2S/touch/PSRAM/DMX/pin-safety and other platform-specific logic. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
netmindz
commented
Nov 8, 2025
platformio.ini
Outdated
Comment on lines
14
to
28
| ;nodemcuv2 | ||
| ;esp8266_2m | ||
| ;esp01_1m_full | ||
| ;nodemcuv2_160 | ||
| ;esp8266_2m_160 | ||
| ;esp01_1m_full_160 | ||
| ;nodemcuv2_compat | ||
| ;esp8266_2m_compat | ||
| ;esp01_1m_full_compat | ||
| ;esp32dev | ||
| ;esp32dev_debug | ||
| ;esp32_eth | ||
| ;esp32_wrover | ||
| ; lolin_s2_mini ;; TODO: disabled NeoEsp32RmtMethodIsr | ||
| esp32c3dev | ||
| ;esp32c3dev |
Member
Author
There was a problem hiding this comment.
TODO: revert before merge, here to save CI build time
Member
Author
|
@softhack007 I've started trying to cherry-pick your changes over, quite a few don't apply due to the changes with MM, but I'll try and resolve the merge conflicts for those that are simple |
* more debug output * added my own fork of FastLED ( looks like more bugs to solve ....)
softhack007
reviewed
Nov 8, 2025
Member
@netmindz thanks, I'll try to help once I'm finished with some open ends in WLED-MM 😁 december |
Member
Author
|
Possibly also worth looking at #4626 to see if there are any changes there that are helpful |
1 task
Draft
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
also disabled some core debug flags
this was a temporary hack needed one year ago; FastLED should be compatible with -C6 now.
trying to reduce compile errors in bus_wrapper.h ... plus some preparation for P4 support
similar to C3
This comment was marked as resolved.
This comment was marked as resolved.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
usermods/audioreactive/audio_source.h (1)
25-33:⚠️ Potential issue | 🟡 MinorUpdate warning message to reflect all guarded targets, not just C2/C3.
ESP32-C61 correctly belongs in this guard (single-core, no hardware FPU). However, the warning text still only mentions C2 and C3, while the guard now includes C5, C6, C61, H2, and ESP8266/ESP8265. Either update the message to list all affected targets or refactor to use capability-based checks (e.g.,
SOC_CPU_HAS_FPU/SOC_CPU_CORES_NUM) for clarity.wled00/FX_fcn.cpp (1)
1194-1211:⚠️ Potential issue | 🟠 MajorFix single‑I2S memory accounting for ESP32/S2.
The
usesI2Scondition assumes single‑I2S uses the first digital bus (digitalCount == 1) and disables it entirely on S2. Current bus allocation puts single‑I2S on the last digital bus (ESP32 bus 8, S2 bus 4), so this underestimatesmaxI2Sand can let allocations exceedMAX_LED_MEMORY.Consider tracking the total digital bus count before the loop and using that to detect the last bus when
useParallelI2S == false.🔧 Suggested fix
- digitalCount = 0; + unsigned totalDigitalBusses = digitalCount; + digitalCount = 0; ... - const bool usesI2S = ((useParallelI2S && digitalCount <= 8) || (!useParallelI2S && digitalCount == 1)); + const bool usesI2S = (useParallelI2S && digitalCount <= 8) || + (!useParallelI2S && digitalCount == totalDigitalBusses); ... - const bool usesI2S = (useParallelI2S && digitalCount <= 8); + const bool usesI2S = (useParallelI2S && digitalCount <= 8) || + (!useParallelI2S && digitalCount == totalDigitalBusses);Based on learnings: “In WLED PR
#5267, the bus allocation strategy for single I2S mode was changed: ESP32 now uses single I2S on bus 8 (the last bus, digitalCount=9) instead of bus 0; ESP32-S2 uses single I2S on bus 4 (digitalCount=5). Memory calculation logic must account for which specific bus uses I2S in single mode, not just check digitalCount <= 8.”wled00/wled.h (1)
334-344:⚠️ Potential issue | 🟠 MajorRX/TX macro selection will fail builds for non-classic ESP32 variants without RX/TX definitions.
The condition incorrectly assumes RX/TX macros are available for all non-classic ESP32 targets (C3/C5/C6/etc.), even though Arduino-ESP32 doesn't guarantee these specific macro names across all board variants. Gate strictly on
defined(RX) && defined(TX)to prevent referencing undefined macros:🔧 Suggested fix
-#if defined(ARDUINO_ARCH_ESP32) && (!defined(CONFIG_IDF_TARGET_ESP32) || (defined(RX) && defined(TX))) +#if defined(ARDUINO_ARCH_ESP32) && defined(RX) && defined(TX) constexpr uint8_t hardwareRX = RX; constexpr uint8_t hardwareTX = TX; `#else` constexpr uint8_t hardwareRX = 3; constexpr uint8_t hardwareTX = 1; `#endif`
temporary workaround, to prevent that more than the possible 2 digital outputs are configured by users
Member
|
ToDO: rework chip identification code in settings_leds.htm WLED/wled00/data/settings_leds.htm Lines 66 to 71 in cffc57e ==> Instead of guessing the chip type from some parameters, we should simply add this info - possibly as a short string, similar to appendGPIOinfo Line 95 in cffc57e |
Member
|
@coderabbitai a user has reported problems with two ws2812b led strings on two pins
I want to know if this is a problem in the patched NeoPixelBus driver, or a problem in our handling of RMT channels in bus_manager.cpp and bus_wrapper.h. Please review both the WLED code for -C5 that's related to ws2812b led outputs, and the patched NeoPixelBus driver for -C5, and try to find the root cause of the user's problem. |
This comment was marked as duplicate.
This comment was marked as duplicate.
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9495 🏁 Script executed: Length of output: 154 🏁 Script executed: Length of output: 5125 🏁 Script executed: Length of output: 135 🏁 Script executed: Length of output: 4504 🏁 Script executed: Length of output: 6314
🔍 AnalysisESP32-C5 Hardware LimitationFrom ESP-IDF 5.5.2 `#define` SOC_RMT_TX_CANDIDATES_PER_GROUP 2 /*!< Only 2 channels can transmit */
`#define` SOC_RMT_RX_CANDIDATES_PER_GROUP 2 /*!< Only 2 channels can receive */
`#define` SOC_RMT_CHANNELS_PER_GROUP 4 /*!< 4 total channels */Critical: ESP32-C5 has 4 RMT channels, but only channels 0 and 1 can transmit. Channels 2-3 are RX-only. WLED Code Status ✅Your WLED code is correct:
The Problem
|
Member
@coderabbitai the neopixelbus C5 fork can be reached with this commit: https://github.com/jonny190/NeoPixelBus/tree/103cfb585ef8efcdd6645ebb9938bf7012d093fd Please complete your analysis with an inspection of the source code in this branch. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 285 🏁 Script executed: Length of output: 251 🏁 Script executed: Length of output: 254 🏁 Script executed: Length of output: 3643 🏁 Script executed: Length of output: 3110 🏁 Script executed: Length of output: 589 🏁 Script executed: Length of output: 3330 🏁 Script executed: Length of output: 1789 🏁 Script executed: Length of output: 2252
🐛 Critical Issues Found in NeoPixelBus C5 Fork1️⃣ Constructor Ignores Channel ParameterLocation: Line 70-75 in NeoEsp32RmtMethodBase(uint8_t pin, uint16_t pixelCount, size_t elementSize, size_t settingsSize,
NeoBusChannel channel = NeoBusChannel_0) :
_sizeData(pixelCount* elementSize + settingsSize),
_pin(pin),
_channel(NULL) // ❌ BUG: 'channel' parameter is NEVER used!
{
construct();
}WLED passes 2️⃣ Dynamic Channel Allocation Without RestrictionsLocation: Lines 104-116 in rmt_tx_channel_config_t config = {};
config.clk_src = RMT_CLK_SRC_DEFAULT;
config.gpio_num = static_cast<gpio_num_t>(_pin); // ✅ GPIO set
// ❌ NO channel specification!
config.mem_block_symbols = 192;
config.resolution_hz = T_SPEED::RmtTicksPerSecond;
config.trans_queue_depth = 4;
config.flags.invert_out = T_INVERTED::Inverted;
config.flags.with_dma = false;
ret += rmt_new_tx_channel(&config, &_channel); // ❌ Allocates ANY available channelThe problem: 3️⃣ Error Handling DisabledLocation: Lines 122-124 // if (ret) {
// AddLog(2,"RMT: initialized with error code: %u on pin: %u",ret, _pin);
// }Initialization failures are silently ignored, so users never see that the second bus failed. 4️⃣ ESP32-C5 Not in Supported Targets ListLocation: Line 362 `#if` defined(NEOPIXEL_ESP32_RMT_DEFAULT) || defined(CONFIG_IDF_TARGET_ESP32S2) || \
defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32C6) || \
defined(CONFIG_IDF_TARGET_ESP32S3)ESP32-C5 is missing! This may cause it to fall back to a different code path. 📋 Why Second Output Fails
✅ Required Fixes for NeoPixelBus ForkFix 1: Use Channel Parameter (Quick Fix)// In constructor - store the channel number:
_channelNum(static_cast<uint8_t>(channel)) // Add this member
// In Initialize() - force specific channel allocation:
`#if` defined(CONFIG_IDF_TARGET_ESP32C5)
// C5 only supports TX on channels 0-1
if (_channelNum > 1) {
ESP_LOGE(TAG, "ESP32-C5 only supports RMT TX on channels 0-1!");
return;
}
`#endif`Fix 2: Add ESP32-C5 to Target List`#if` defined(NEOPIXEL_ESP32_RMT_DEFAULT) || defined(CONFIG_IDF_TARGET_ESP32S2) || \
defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32C5) || \ // ADD THIS
defined(CONFIG_IDF_TARGET_ESP32C6) || defined(CONFIG_IDF_TARGET_ESP32S3)Fix 3: Enable Error Loggingret += rmt_new_tx_channel(&config, &_channel);
if (ret != ESP_OK) {
ESP_LOGE(TAG, "RMT channel init failed: 0x%x on pin %u", ret, _pin);
}Fix 4: Validate TX Capability`#if` defined(SOC_RMT_TX_CANDIDATES_PER_GROUP)
// Ensure we're using a TX-capable channel on C5/C6
if (_channelNum >= SOC_RMT_TX_CANDIDATES_PER_GROUP) {
ESP_LOGE(TAG, "Channel %u cannot transmit on this chip", _channelNum);
return;
}
`#endif`🔧 Temporary WLED WorkaroundUntil the NeoPixelBus fork is fixed, limit ESP32-C5 to 1 WS2812B output: In `#if` defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32C6)
`#define` WLED_MAX_DIGITAL_CHANNELS 2
`#elif` defined(CONFIG_IDF_TARGET_ESP32C5)
`#define` WLED_MAX_DIGITAL_CHANNELS 1 // ⚠️ Driver bug: second channel failsAdd a warning comment in the UI or documentation that C5 currently supports only 1 digital output until the driver is fixed. 🎯 Action Items
The good news: WLED code is correct — this is purely a NeoPixelBus driver issue that can be fixed with the above changes! 🚀 🧠 Learnings used |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 221 🏁 Script executed: Length of output: 11426 🏁 Script executed: Length of output: 2389
🐛 Critical Bug in NeoEsp32RmtXMethod.hThe Problem:Line 70-73 (Constructor): NeoEsp32RmtMethodBase(..., NeoBusChannel channel = NeoBusChannel_0) :
_sizeData(...),
_pin(pin),
_channel(NULL) // ❌ BUG: Ignores the 'channel' parameter!Line 112 (Initialize): ret += rmt_new_tx_channel(&config, &_channel); // ❌ Auto-allocates without constraintsLines 120-122 (Error handling): // if (ret) {
// AddLog(2,"RMT: initialized with error code: %u on pin: %u",ret, _pin);
// }
// ❌ BUG: Error logging is commented out!🔍 What's Happening:
✅ The Fix:Option 1: Use the channel parameter (cleanest)// In constructor (line 70-73):
NeoEsp32RmtMethodBase(..., NeoBusChannel channel = NeoBusChannel_0) :
_sizeData(...),
_pin(pin),
_channel(NULL),
_channelNum(static_cast<uint8_t>(channel)) // Store channel number
{ ... }
// In Initialize() (line 105-112):
void Initialize()
{
esp_err_t ret = ESP_OK;
rmt_tx_channel_config_t config = {};
config.clk_src = RMT_CLK_SRC_DEFAULT;
config.gpio_num = static_cast<gpio_num_t>(_pin);
`#ifdef` CONFIG_IDF_TARGET_ESP32C5
// C5: Force TX-capable channels (0-1 only)
if (_channelNum > 1) {
ESP_LOGE(TAG, "ESP32-C5 only supports RMT TX channels 0-1");
return;
}
`#endif`
config.mem_block_symbols = 192;
config.resolution_hz = T_SPEED::RmtTicksPerSecond;
config.trans_queue_depth = 4;
config.flags.invert_out = T_INVERTED::Inverted;
config.flags.with_dma = false;
ret = rmt_new_tx_channel(&config, &_channel);
// CRITICAL: Enable error logging!
if (ret != ESP_OK) {
ESP_LOGE(TAG, "RMT channel %d init failed on pin %u: error %d",
_channelNum, _pin, ret);
return;
}
// ... rest of initialization
}Option 2: Add C5-specific validation (minimal change)// In Initialize(), after rmt_new_tx_channel:
ret = rmt_new_tx_channel(&config, &_channel);
`#ifdef` CONFIG_IDF_TARGET_ESP32C5
// Verify we got a TX-capable channel
// (ESP-IDF internal channel ID is opaque, so we check initialization success)
if (ret != ESP_OK) {
ESP_LOGE(TAG, "RMT TX channel allocation failed on pin %u", _pin);
return;
}
`#endif`
// Enable error checking everywhere:
if (ret != ESP_OK) {
ESP_LOGE(TAG, "RMT initialized with error code: %d on pin: %u", ret, _pin);
}📋 Immediate Actions:
Temporary workaround for users: Only use 1 WS2812B output until the driver is fixed. Would you like me to prepare a complete patch for the driver? 🧠 Learnings used |
Member
|
@jonny190 there seems to be a bug in your NPB fork, please check #5048 (comment). |
it compiles, but not yet usable !!!
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
usermods/audioreactive/audio_source.h (1)
25-33:⚠️ Potential issue | 🟡 MinorStale
#error/#warningmessage text mentions only C2 and C3.Adding
CONFIG_IDF_TARGET_ESP32C61to the guard is semantically correct (RISC-V, single-core, no hardware FPU like its C6 sibling), but the error and warning messages on lines 30 and 32 still name only ESP32-C2 or ESP32-C3 while the guard now covers seven variants (C2, C3, C5, C6, C61, H2, ESP8266/8265). Any developer who hits either diagnostic on a C5, C6, or C61 board will be misled. This was already a pre-existing issue for C5/C6/H2 before this PR; adding C61 makes it one step worse.✏️ Suggested message update
`#if` !defined(SOC_I2S_NUM) || (SOC_I2S_NUM < 1) - `#error` This audio reactive usermod does not support ESP32-C2 or ESP32-C3. + `#error` This audio reactive usermod is not supported on this MCU (no I2S peripheral available). `#else` - `#warning` This audio reactive usermod does not support ESP32-C2 and ESP32-C3. + `#warning` This audio reactive usermod has limited support on this MCU (no hardware FPU, single core – expect degraded performance). `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/audioreactive/audio_source.h` around lines 25 - 33, The preprocessor diagnostics in the audio_source.h block are stale: the `#error` and `#warning` messages still say "ESP32-C2 or ESP32-C3" even though the guard now covers CONFIG_IDF_TARGET_ESP32C2, CONFIG_IDF_TARGET_ESP32C3, CONFIG_IDF_TARGET_ESP32C5, CONFIG_IDF_TARGET_ESP32C6, CONFIG_IDF_TARGET_ESP32C61, CONFIG_IDF_TARGET_ESP32H2, ESP8266 and ESP8265; update the text in the `#error` and `#warning` lines to either list all affected targets (C2, C3, C5, C6, C61, H2, ESP8266/ESP8265) or use a general phrase like "these single-core/RISC-V/no-FPU targets" so the diagnostic accurately reflects the condition checked by the surrounding `#if` that also references SOC_I2S_NUM.wled00/pin_manager.cpp (1)
249-271:⚠️ Potential issue | 🔴 CriticalAdd
CONFIG_IDF_TARGET_ESP32C61block toisPinOk— but use correct pin assignments for C61, not C6 pins.C61 is currently missing from the
#elifchain and would fall through to the classic ESP32 handler when built. However, the proposed C61 block is incorrect: C61 does not use GPIO 24–30 for SPI flash. According to Espressif's official documentation, C61 has GPIO 0–29 only and uses GPIO 14–17 and GPIO 19–21 as the primary SPI0/1 pins (typically for flash/PSRAM), making it more similar to classic ESP32 than to C6.Add:
Proposed C61 block (with correct pin assignments)
+#elif defined(CONFIG_IDF_TARGET_ESP32C61) + // ESP32-C61: SPI0/1 pins are GPIO 14-17, 19-21 (used for flash/PSRAM) + if ((gpio > 13 && gpio < 18) || (gpio > 18 && gpio < 22)) return false; // GPIO 14-17, 19-21 for SPI + `#if` ARDUINO_USB_CDC_ON_BOOT == 1 || ARDUINO_USB_DFU_ON_BOOT == 1 + if (gpio == 12 || gpio == 13) return false; // GPIO 12-13 USB-JTAG + `#endif` `#else`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/pin_manager.cpp` around lines 249 - 271, The isPinOk function lacks a CONFIG_IDF_TARGET_ESP32C61 branch and currently falls through to the classic-ESP32 handler; add an `#elif` block for CONFIG_IDF_TARGET_ESP32C61 in the same region that marks the correct SPI/PSRAM pins as unavailable (treat GPIO 14–17 and 19–21 as reserved for SPI0/1/PSRAM as per Espressif docs), handle GPIO 16 and 17 with psramFound() semantics similar to existing checks (16 returns !psramFound(), 17 returns true for specific exceptions else !psramFound()), and ensure other pins fall back to the classic behavior so the new branch prevents incorrect marking of GPIO 24–30 as flash pins; reference isPinOk, gpio, psramFound(), and ESP.getChipModel() when adding the block.wled00/bus_manager.cpp (1)
1151-1171:⚠️ Potential issue | 🟡 MinorI2S DMA buffer estimate may miss single‑I2S bus placement.
The memory calculation still keys offdigitalCount <= 8/== 1, which doesn’t align with the single‑I2S bus now being placed at the end (e.g., ESP32 bus 8, S2 bus 4). Consider deriving the I2S‑using bus from the actual bus index or a PolyBus helper instead of the count heuristic.Based on learnings: In WLED, both parallel I2S and single I2S modes require additional DMA buffers (4× the pixel buffer size). The memory calculation must account for which specific buses use I2S hardware, not just check digitalCount <= 8, because single I2S mode places the I2S bus at the END (ESP32 bus 8 with digitalCount=9, S2 bus 4 with digitalCount=5).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/bus_manager.cpp` around lines 1151 - 1171, The DMA buffer size logic incorrectly uses a running digitalCount heuristic to decide which bus uses I2S (checks with PolyBus::isParallelI2S1Output() && digitalCount <= 8 or digitalCount == 1), which fails when the single‑I2S bus is placed at the end; instead determine the I2S‑using bus explicitly (by bus index or a PolyBus helper) and only apply the i2sCommonSize/maxI2S update for that specific bus. Replace the digitalCount-based decision inside the loop (symbols: digitalCount, PolyBus::isParallelI2S1Output(), bus->isDigital(), bus->is2Pin(), maxI2S, bus->getLength(), bus->getNumberOfChannels(), bus->is16bit()) with logic that either queries a PolyBus API like getI2SBusIndex()/isI2SOutput() or computes the I2S bus as the last digital non‑2pin when single‑I2S mode is active, then compute i2sCommonSize only for that identified bus and update maxI2S.wled00/FX_fcn.cpp (1)
1194-1211:⚠️ Potential issue | 🟠 MajorI2S memory estimation still assumes single‑I2S is bus
#1.Line 1196 uses
digitalCount == 1to detect single‑I2S, but single‑I2S is now placed on the last digital bus (ESP32 bus 8 / ESP32‑S2 bus 4). This will miss I2S buffers and under‑estimatemaxI2S, allowing oversized configs. Please alignusesI2Swith the actual single‑I2S bus index per target (or reuse the same mapping logic/constants used for bus allocation).Based on learnings: In WLED PR
#5267, single I2S moved to bus 8 on ESP32 (digitalCount=9) and bus 4 on ESP32‑S2 (digitalCount=5); memory estimation must reflect that.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX_fcn.cpp` around lines 1194 - 1211, The usesI2S detection wrongly treats single‑I2S as digitalCount==1; update the logic so single‑I2S is detected by the actual single‑I2S bus index (e.g. the last digital bus) instead of hardcoding 1: change the condition that computes usesI2S to check the bus index against the single‑I2S mapping (for example compare bus.index == digitalCount-1 or call the shared mapping/constant used for bus allocation), keeping the rest of the block (Bus::isDigital(bus.type), Bus::is2Pin(bus.type), the stepFactor and i2sCommonSize calculation) intact so maxI2S is correctly estimated (see symbols usesI2S, digitalCount, bus.index, Bus::isDigital, Bus::is2Pin, maxI2S).wled00/wled.h (1)
334-344:⚠️ Potential issue | 🟠 MajorSimplify RX/TX pin detection to prevent compilation failure on ESP32 variants without framework pin definitions.
The current condition short-circuits on non-classic ESP32 targets (C3/S3/C6/etc), entering the if-block without verifying that
RXandTXmacros are actually defined. This contradicts the code comment acknowledging that "some arduino variants don't define RX and TX" and will cause compilation failures on any non-classic board that omits these definitions.Replace with the simpler and safer condition:
`#if` defined(ARDUINO_ARCH_ESP32) && defined(RX) && defined(TX)This ensures
RX/TXmacros are always verified before use, while boards lacking these definitions safely fall back to the defaults (3, 1).Proposed fix
-#if defined(ARDUINO_ARCH_ESP32) && (!defined(CONFIG_IDF_TARGET_ESP32) || (defined(RX) && defined(TX))) +#if defined(ARDUINO_ARCH_ESP32) && defined(RX) && defined(TX)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/wled.h` around lines 334 - 344, The conditional that selects RX/TX pins can incorrectly assume RX/TX exist on non-classic ESP32 variants; update the preprocessor check so RX and TX are only used when actually defined by changing the if condition to require both RX and TX (i.e., use "defined(ARDUINO_ARCH_ESP32) && defined(RX) && defined(TX)"), ensuring the existing constexpr variables hardwareRX and hardwareTX fall back to 3 and 1 when RX/TX are not defined.
♻️ Duplicate comments (13)
platformio.ini (2)
539-539:⚠️ Potential issue | 🟡 Minor
-D WLED_DEBUGin theesp32c5devCI artifact — is this intentional?
[env:esp32c5dev]is indefault_envs(Line 31), so CI publishes a debug build as a downloadable firmware artifact. Debug builds are larger and emit verbose serial output. For an explicitly EXPERIMENTAL target, this may be a deliberate choice to aid hardware testers, but it should be documented with a comment and removed once the target stabilises.If this is temporary, add a comment and track removal
-D WLED_DISABLE_ESPNOW ;; not yet tested - -D WLED_DEBUG + -D WLED_DEBUG ;; TODO: remove once esp32c5 target is stable🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` at line 539, The esp32c5dev CI artifact currently defines the debug macro (-D WLED_DEBUG) under the [env:esp32c5dev] configuration which is included in default_envs, causing a debug firmware to be published; either remove the -D WLED_DEBUG flag from the esp32c5dev environment in platformio.ini to produce a release build, or if this is intentional while experimental, add an inline comment next to the -D WLED_DEBUG entry referencing why it’s present and a TODO with an issue/PR number to remove it once the target stabilizes; locate the flag under the [env:esp32c5dev] block and update accordingly, and ensure default_envs (the list that includes esp32c5dev) remains accurate.
420-420: Pin the C5 NeoPixelBus fork to a commit hash for reproducible builds; verify repository availability.The current reference
https://github.com/jonny190/NeoPixelBus.git#wled-c5-stableuses a branch name. Branches can be force-pushed or deleted, breaking future builds silently. The CI artifactfirmware-esp32c5dev(1.14 MB) does confirm the repository is currently reachable, but production-quality builds should pin to a specific commit hash.Additionally, per the PR discussion, the fork has a known driver bug (RMT constructor ignores the requested channel; allocates channels 2–3 which are RX-only on C5, causing the second WS2812B output to fail). The bug fix mentioned in the PR comments should be tracked for inclusion before this experimental target is promoted.
- https://github.com/jonny190/NeoPixelBus.git#wled-c5-stable ;; wled-c5-stable for C5 support: I2S exclusion, bit-bang CSR/GPIO, channel ctor overload + https://github.com/jonny190/NeoPixelBus.git#<commit-hash> ;; pin to specific commit for reproducibility; wled-c5-stable branchgithub jonny190/NeoPixelBus wled-c5-stable ESP32-C5 NeoPixelBus fork🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` at line 420, Replace the branch reference "https://github.com/jonny190/NeoPixelBus.git#wled-c5-stable" in platformio.ini with a pinned commit hash (e.g. "https://github.com/jonny190/NeoPixelBus.git#<commit-hash>") to ensure reproducible builds, verify the commit hash exists and the repository is reachable (the CI artifact "firmware-esp32c5dev" confirms current reachability), and add a short TODO/note near that dependency to track the NeoPixelBus C5 driver bug (RMT constructor channel allocation issue affecting the second WS2812B output) so we can update the pinned hash when the upstream bugfix is published.wled00/src/dependencies/dmx/SparkFunDMX.cpp (1)
20-20:⚠️ Potential issue | 🟡 MinorTwo items from the prior review remain unresolved on this line.
Both were previously flagged (and marked ✅ addressed) but are still present in the current code:
!defined(ESP8266)is redundant — the outer#if defined(ARDUINO_ARCH_ESP32)guard on Line 17 already excludes ESP8266; this term can never be true inside that block.
CONFIG_IDF_TARGET_ESP32P4is missing from the exclusion list — the[esp32p4]section inplatformio.inidisables bothWLED_ENABLE_DMX_INPUTandWLED_ENABLE_DMXviabuild_unflags, butSparkFunDMX.cppwill still compile for P4. Since P4 hasSOC_UART_NUM >= 3, the#errorsafety net on Line 46 won't catch it. More critically, theinitRead()path uses the legacytimerBegin/timerAttachInterrupt/timerAlarmWrite/timerAlarmEnableAPI (Lines 86–89), which may fail to compile under the IDF V5 pioarduino platform used for P4.Proposed fix
-#if !defined(ESP8266) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32C5) && !defined(CONFIG_IDF_TARGET_ESP32C6) && !defined(CONFIG_IDF_TARGET_ESP32C61) && !defined(CONFIG_IDF_TARGET_ESP32S2) +#if !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32C5) && !defined(CONFIG_IDF_TARGET_ESP32C6) && !defined(CONFIG_IDF_TARGET_ESP32C61) && !defined(CONFIG_IDF_TARGET_ESP32P4) && !defined(CONFIG_IDF_TARGET_ESP32S2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/src/dependencies/dmx/SparkFunDMX.cpp` at line 20, The preprocessor exclusion on the SparkFunDMX.cpp top block is incorrect: remove the redundant !defined(ESP8266) from the `#if` condition (it's already inside the ARDUINO_ARCH_ESP32 guard) and add CONFIG_IDF_TARGET_ESP32P4 to the list of excluded targets so the file won't compile for ESP32-P4; additionally ensure the legacy initRead() path (which uses timerBegin/timerAttachInterrupt/timerAlarmWrite/timerAlarmEnable) is not compiled for P4 by the updated `#if` so the IDF v5/incompatible timer APIs are avoided. Locate the conditional that begins with the current `#if` and update it to drop ESP8266 and include CONFIG_IDF_TARGET_ESP32P4, keeping the intent of excluding unsupported targets.wled00/data/index.js (1)
705-706:⚠️ Potential issue | 🟡 MinorAvoid “Ch undefined” when channel is absent.
If
i.wifi.bandexists buti.wifi.channelis missing, the UI renders “Ch undefined”. Add a band‑only fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/index.js` around lines 705 - 706, The UI shows "Ch undefined" when i.wifi.band exists but i.wifi.channel is missing; update the template line that calls inforow for WiFi band so it checks i.wifi.channel and renders either "WiFi band" with band + " (Ch <channel>)" when present, or just the band label when i.wifi.channel is falsy. Modify the second expression that uses inforow("WiFi band", i.wifi.band + " (Ch " + i.wifi.channel + ")") to branch on i.wifi.channel (while still verifying i.wifi.band) so inforow receives a band‑only fallback string when channel is undefined.usermods/audioreactive/audio_reactive.cpp (3)
2072-2074:⚠️ Potential issue | 🟠 Major
_analogmicdefinition guard still mismatches its declaration.The definition is guarded by
ARDUINO_ARCH_ESP32(with exclusions) while the declaration now usesCONFIG_IDF_TARGET_ESP32. On C5/C6 this can produce a definition without a declaration. Use the same guard on both.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/audioreactive/audio_reactive.cpp` around lines 2072 - 2074, The definition of AudioReactive::_analogmic is wrapped in a different preprocessor condition than its declaration; update the `#if/`#endif around the _analogmic definition to use the exact same guard used for the declaration (i.e., the CONFIG_IDF_TARGET_ESP32-based condition with the same exclusions) so the declaration and definition are compiled under the same target conditions and no orphan definition is produced for AudioReactive::_analogmic.
1163-1231:⚠️ Potential issue | 🟠 MajorESP32‑C6 is still missing from these exclusion guards.
These blocks exclude C3/C5 but not C6, so C6 can still hit the PERIPH_I2S0 reset / PDM / ADC paths that are unsupported. Please add
CONFIG_IDF_TARGET_ESP32C6(or switch to a positive classic‑ESP32 guard).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/audioreactive/audio_reactive.cpp` around lines 1163 - 1231, The build guards missing CONFIG_IDF_TARGET_ESP32C6 allow ESP32‑C6 to enter unsupported I2S/PDM/ADC paths; update the preprocessor conditions that currently check CONFIG_IDF_TARGET_ESP32S2, CONFIG_IDF_TARGET_ESP32C3, CONFIG_IDF_TARGET_ESP32C5 (the blocks around the periph_module_reset call, the dmType PDM detection if-statement, the switch(dmType) stub cases and the ADC case creating I2SAdcSource) to also exclude CONFIG_IDF_TARGET_ESP32C6 (or invert to a positive check for classic ESP32), so paths that call periph_module_reset, treat dmType==5 as PDM, or create I2SAdcSource are not compiled for C6.
1800-1953:⚠️ Potential issue | 🟠 MajorAnalog‑mic config/UI guards still omit ESP32‑C6.
The analog mic JSON/UI blocks should be classic‑ESP32 only; missing C6 keeps unsupported options enabled on C6. Use
CONFIG_IDF_TARGET_ESP32(or add C6 to the exclusions) for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/audioreactive/audio_reactive.cpp` around lines 1800 - 1953, The preprocessor guards around the analog-mic JSON/UI (the `#if` blocks that create amic via FPSTR(_analogmic) in addToConfig and the corresponding UI prints in appendConfigData) exclude ESP32S2/C3/C5/S3 but omit ESP32C6, leaving unsupported analog options enabled on C6; update those conditionals to either use CONFIG_IDF_TARGET_ESP32 (so only classic ESP32 includes analog mic code) or add CONFIG_IDF_TARGET_ESP32C6 to the exclusion list, and apply the same fix to the matching guards in readFromConfig, addToConfig (FPSTR(_analogmic) creation) and appendConfigData (uiScript prints related to analog mic and MCLK info) so all analog-mic handling is consistently disabled on unsupported MCUs.wled00/bus_manager.cpp (1)
1261-1264: Comment doesn’t list C61.✏️ Suggested fix
- // ESP32-C5/C6/P4 use shared RMT method - idle level inversion not supported + // ESP32-C5/C6/C61/P4 use shared RMT method - idle level inversion not supported🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/bus_manager.cpp` around lines 1261 - 1264, The comment above the preprocessor check is missing "C61" while the `#if` condition includes CONFIG_IDF_TARGET_ESP32C61; update the comment for the block containing the preprocessor line "#if defined(CONFIG_IDF_TARGET_ESP32C5) || defined(CONFIG_IDF_TARGET_ESP32C6) || defined(CONFIG_IDF_TARGET_ESP32C61) || defined(CONFIG_IDF_TARGET_ESP32P4)" in bus_manager.cpp so it reads something like "ESP32-C5/C6/C61/P4 use shared RMT method - idle level inversion not supported" to accurately reflect the condition in that block.wled00/data/settings_wifi.htm (1)
253-275:⚠️ Potential issue | 🟡 MinorRegenerate embedded UI headers after this change.
Please run the UI build step so embedded headers stay in sync with the updated page.As per coding guidelines: “wled00/data/**: When modifying web UI files, run
npm run buildto regenerate embedded headers before any firmware build.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/settings_wifi.htm` around lines 253 - 275, You updated the WiFi UI controls (elements with id="bm" and id="tx", selects name="BM" and "TX") in the embedded web page; run the UI build step (npm run build) to regenerate the embedded UI headers so the firmware binary includes the updated headers before the next firmware build.wled00/bus_wrapper.h (1)
1375-1377: RMT-channel comment still doesn’t match the expanded target list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/bus_wrapper.h` around lines 1375 - 1377, The inline comment above the condition that returns I_NONE for num > 1 is outdated relative to the expanded target list; update the comment that currently reads "On ESP32-C3/C5/C6 only the first 2 RMT channels are usable for transmitting" to accurately list all targets in the `#elif` (e.g., ESP32-C3/C5/C6/C61/P4) or rephrase to a generic statement ("On these targets only the first 2 RMT channels are usable for transmitting") so it matches the condition that checks defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32C5) || defined(CONFIG_IDF_TARGET_ESP32C6) || defined(CONFIG_IDF_TARGET_ESP32C61) || defined(CONFIG_IDF_TARGET_ESP32P4) and the num > 1 return I_NONE logic.wled00/wled_boards.h (2)
74-84:⚠️ Potential issue | 🟡 MinorComment on line 81 still references "esp32-S3" instead of "esp32-S2".
The
WLED_BOARDassignment on line 83 was corrected toNODE_TYPE_ID_ESP32S2, but the comment on line 81 still says "esp32-S3 supports I2S parallel leds driver." This should read "esp32-S2."✏️ Suggested fix
- `#define` WLED_HAS_PARALLEL_I2S 1 // esp32-S3 supports I2S parallel leds driver (NeoPixelBus) + `#define` WLED_HAS_PARALLEL_I2S 1 // esp32-S2 supports I2S parallel leds driver (NeoPixelBus),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/wled_boards.h` around lines 74 - 84, The comment for CONFIG_IDF_TARGET_ESP32S2 incorrectly mentions "esp32-S3"; update the inline comment that precedes the WLED_HAS_PARALLEL_I2S macro so it refers to "esp32-S2" (i.e., change the text near the WLED_HAS_PARALLEL_I2S definition within the CONFIG_IDF_TARGET_ESP32S2 block) to match the corrected WLED_BOARD = NODE_TYPE_ID_ESP32S2 and avoid confusing references to S3.
130-136:⚠️ Potential issue | 🟡 MinorESP32-P4: "TBC" comments should be resolved.
Lines 133 and 135 still have "TBC" (to be confirmed) markers. Per ESP-IDF documentation, ESP32-P4 does support RTC FAST memory and has fast integer division (dual-core RISC-V with hardware multiply/divide). These markers are stale and should be cleaned up.
✏️ Suggested fix
- // TBC: is bitshift with rounding is faster than integer division ? - `#define` WLED_HAVE_FAST_int_DIVIDE 1 // integer division is fast (no need to use bitshifts with rounding instead of integer division + `#define` WLED_HAVE_FAST_int_DIVIDE 1 // integer division is fast (no need to use bitshifts with rounding instead of integer division) - `#define` WLED_HAVE_RTC_MEMORY_HEAP 1 // TBC: does it have byte-accessible fast RTC memory that can be used as heap ? + `#define` WLED_HAVE_RTC_MEMORY_HEAP 1 // has byte-accessible fast RTC memory that can be used as heap,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/wled_boards.h` around lines 130 - 136, The comments marked "TBC" on the ESP32-P4 capability defines are stale — remove the "TBC" markers and make the defines explicit: keep `#define` WLED_HAVE_FAST_FLOAT 1, keep `#define` WLED_HAVE_FAST_int_DIVIDE 1 (hardware integer divide available), and keep `#define` WLED_HAVE_RTC_MEMORY_HEAP 1 (RTC fast memory supported); update the inline comments next to WLED_HAVE_FAST_int_DIVIDE and WLED_HAVE_RTC_MEMORY_HEAP to state the confirmed behavior (e.g., "hardware integer divide" and "RTC fast memory available") and delete the "TBC" text so the intent is clear.wled00/json.cpp (1)
705-709:deviceIdremains absent from JSON on ESP-IDF V5+ builds.The entire
#elseblock is commented out, meaning IDF V5 devices emit nodeviceIdfield. The UI code (index.js:3390) assignsinfoData.deviceIdwithout a fallback, which will passundefinedto the upgrade reporting endpoint. At minimum, emit an empty string or placeholder so the field is always present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/json.cpp` around lines 705 - 709, The `#else` block currently leaves out root["deviceId"] entirely on ESP-IDF V5+ builds; ensure the JSON always contains the deviceId key by adding a fallback assignment in the same spot (set root[F("deviceId")] to an empty string or a fixed placeholder when getBootloaderSHA256Hex() is not used), e.g. add a line that assigns an empty String to root["deviceId"] so code referencing infoData.deviceId in index.js always gets a defined value; look for the root[...] assignment and getBootloaderSHA256Hex() usage in json.cpp to place the fallback.
🧹 Nitpick comments (5)
wled00/ota_update.cpp (1)
439-445: C5 validation block follows the established pattern — looks correct.The code correctly mirrors the existing unsupported-target pattern (C3/S3/C6). One forward-looking concern: when bootloader OTA is eventually enabled for C5, the hardcoded
bootloaderOffset = 0x1000inBootloaderUpdateContext(line 556) will be incorrect — C5 usesBOOTLOADER_OFFSET = 0x2000(line 18). Consider using theBOOTLOADER_OFFSETconstant instead of the hardcoded value when that time comes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/ota_update.cpp` around lines 439 - 445, The C5 validation is fine but future-enabling OTA for ESP32-C5 requires using the BOOTLOADER_OFFSET constant rather than the hardcoded 0x1000; update the BootloaderUpdateContext (replace the hardcoded bootloaderOffset = 0x1000) to reference BOOTLOADER_OFFSET so the correct 0x2000 value applies for ESP32-C5 (and keeps behavior consistent across targets).wled00/bus_wrapper.h (1)
502-513: Resolve or track the TODO in the channel remap block.Line 509 adds a TODO for simplifying the guard—please either complete it in this PR or link it to an issue so it doesn’t get lost.
If you want, I can draft a consolidated guard using the capability macros.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/bus_wrapper.h` around lines 502 - 513, The TODO about simplifying the long ifdef guard in the channel remap block must be resolved: either replace the two separate guards with a single consolidated guard (e.g., define and use a capability macro that expresses "classic ESP32 I2S1 remap" and then use that macro to wrap both the parallel-I2S adjustment and the I2S1 subtraction) or record a tracked issue and reference it in the comment; update the block around `_useParallelI2S` and the `if (!_useParallelI2S && channel > 0) channel--;` remap so it uses the new capability macro (or an explicit issue/PR link) instead of the ad-hoc TODO, ensuring the unique symbols `_useParallelI2S` and `channel` are handled exactly as before.wled00/wled_boards.h (1)
18-25: TODO list is useful for tracking remaining capability flags.The planned macros (
WLED_HAVE_TOUCH,WLED_HAVE_I2S0_LEDS,WLED_HAVE_I2S1_LEDS, etc.) will further reduce long#ifdefchains elsewhere. Consider filing a tracking issue so these don't get lost.Would you like me to open an issue to track the remaining TODO items listed here?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/wled_boards.h` around lines 18 - 25, Add a tracked issue for the TODO list in wled_boards.h and reference it in the comment: create an issue that enumerates the planned macros (WLED_HAVE_TOUCH, WLED_HAVE_I2S0_LEDS, WLED_HAVE_I2S1_LEDS, the RMT-channel limit flag for ESP32-C3/C5/C6, SparkFunDMX support flag for ESP32/S3/P4, and WLED_ALLOW_LOLIN_WIFI_FIX) and then update the TODO block to include the issue number/URL so the items are not lost and future PRs can link to that tracking issue.wled00/wled.h (2)
399-404: Note:txPowertype mismatch with IDF V5 range.The TODO on line 403 correctly notes that
txPowershould be changed toint8_tfor IDF V5 (which supports values fromWIFI_POWER_21dBm = 84down toWIFI_POWER_MINUS_1dBm = -4). Currently it'suint8_t, which cannot represent negative values. This is tracked but worth keeping visible.Would you like me to open an issue to track the
txPowertype change and UI update needed for IDF V5?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/wled.h` around lines 399 - 404, The txPower global is declared as uint8_t which cannot represent IDF V5 negative power values; change the declaration of WLED_GLOBAL uint8_t txPower _INIT(...) to WLED_GLOBAL int8_t txPower _INIT(...) in both branches (the LOLIN_WIFI_FIX branch and the else branch) and then audit/adjust all code and UI paths that read or write txPower (serialization, UI widgets, and any arithmetic or comparisons) to handle signed int8_t values and the extended WIFI_POWER_21dBm/WIFI_POWER_MINUS_1dBm range.
158-165: Duplicate DMX target guards should be centralized.The same
#if defined(CONFIG_IDF_TARGET_ESP32C5) || ... C6 ... C61 ... P4guard pattern appears at both lines 159 and 481. If a new target is added or an existing one gains DMX support, both locations must be updated in sync. Consider defining a macro likeWLED_DMX_UNSUPPORTED_TARGETinwled_boards.hand referencing it in both places.♻️ Example approach in wled_boards.h
+ // Define in wled_boards.h, in each unsupported target block: + `#define` WLED_NO_SPARKFUN_DMX 1 // no SparkFunDMX support on this targetThen in wled.h:
-#if defined(CONFIG_IDF_TARGET_ESP32C5) || defined(CONFIG_IDF_TARGET_ESP32C6) || defined(CONFIG_IDF_TARGET_ESP32C61) || defined(CONFIG_IDF_TARGET_ESP32P4) +#if defined(WLED_NO_SPARKFUN_DMX) `#error` "DMX output is not supported on this target (esp_dmx library excluded)"Also applies to: 481-486
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/wled.h` around lines 158 - 165, The duplicated target guard for DMX (the long `#if` defined(CONFIG_IDF_TARGET_ESP32C5) || ... CONFIG_IDF_TARGET_ESP32P4 pattern used when WLED_ENABLE_DMX is enabled) should be centralized: add a single preprocessor macro WLED_DMX_UNSUPPORTED_TARGET in the common board header (e.g., declare it via the existing board/target macros) and replace the long guard in wled.h (both the include-selection block that checks WLED_ENABLE_DMX and the second occurrence at the later DMX-related block) with a single check using defined(WLED_DMX_UNSUPPORTED_TARGET); update both places to use that macro so new targets only need one change.
🤖 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 549-561: The duty update path still writes
LEDC.channel_group[gr].channel[ch].duty.duty and hpoint.hpoint directly for many
targets; wrap those direct register writes with the same ESP_IDF_VERSION check
used elsewhere (e.g. use ifdef/if ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5,0,0)
around the LEDC.channel_group[...] assignments and ledc_update_duty call) and
for newer ESP‑IDF versions call the HAL API (ledc_set_duty_and_update) like the
C5/C61/P4 branch does; update the comment/TODO to note dithering handling for P4
and ensure you reference LEDC.channel_group[gr].channel[ch].duty.duty,
LEDC.channel_group[gr].channel[ch].hpoint.hpoint, ledc_update_duty and
ledc_set_duty_and_update when making the change.
In `@wled00/json.cpp`:
- Around line 805-814: The current WiFi band detection calls WiFi.getBand() and
checks SOC_WIFI_SUPPORT_5G which aren’t available in the active core, causing
build failures; update the wifi band handling in the block that fills wifi_info
(referencing WiFi.channel(), wifi_info and the existing `#if`
defined(SOC_WIFI_SUPPORT_5G) branch) so it does not call WiFi.getBand() unless
the build actually provides that API (or a corresponding feature macro),
otherwise fall back to a safe default (e.g., always set wifi_info["band"] =
"2.4GHz" or omit band reporting); in short, remove or guard the WiFi.getBand()
usage behind a reliable feature/version check or replace it with a stable
fallback to ensure compilation in the current build configuration.
In `@wled00/ota_update.cpp`:
- Around line 18-19: Update the stale inline comment on the BOOTLOADER_OFFSET
declaration to list all targets present in the preprocessor condition
(CONFIG_IDF_TARGET_ESP32S3, CONFIG_IDF_TARGET_ESP32C3,
CONFIG_IDF_TARGET_ESP32C6, CONFIG_IDF_TARGET_ESP32C61) so the comment matches
the `#if` clause; also add a note in the comment referencing that
verifyBootloaderImage currently lacks a branch for ESP32C61 and will return
"Unknown ESP32 target - bootloader update not supported" until a validation
branch is added when C61 bootloader updates are enabled.
In `@wled00/wled_boards.h`:
- Around line 44-45: The comments for the WLED_HAVE_FAST_int_DIVIDE macro are
missing a closing parenthesis in several board blocks (the comment text ending
"...no need to use bitshifts with rounding instead of integer division"); update
each occurrence of the WLED_HAVE_FAST_int_DIVIDE comment (ESP32-S3 / ESP32-S2 /
ESP32-P4 blocks) to add the missing ")" so the parenthetical closes properly and
the comment reads correctly.
In `@wled00/wled.h`:
- Around line 396-398: The access to the global variable wifiBandMode in set.cpp
is not guarded by the SOC_WIFI_SUPPORT_5G macro, causing build errors when 5G
support is absent; locate the conditional block that checks/assigns wifiBandMode
(the lines containing "if (bm != wifiBandMode) {" and "wifiBandMode = bm;") and
wrap that entire block with `#ifdef` SOC_WIFI_SUPPORT_5G ... `#endif` (or skip the
assignment/compare when SOC_WIFI_SUPPORT_5G is not defined) so that all
reads/writes to wifiBandMode are compiled only when SOC_WIFI_SUPPORT_5G is
enabled.
In `@wled00/xml.cpp`:
- Around line 322-329: Replace the magic numbers "2, 6" used for C3 emulation
with named constants (e.g., WLED_EMULATED_C3_MAX_DIGITAL_CHANNELS and
WLED_EMULATED_C3_MAX_ANALOG_CHANNELS) and use those in the conditional branch
where isC3 emulation is applied (the block currently substituting for
WLED_MAX_DIGITAL_CHANNELS and WLED_MAX_ANALOG_CHANNELS); add those defines in an
appropriate header if they don't exist, then update any references to keep
intent clear. Also fix indentation so the line with WLED_MAX_BUTTONS uses
2-space indentation consistent with other .cpp files. Ensure symbol names match
project naming conventions and compile.
---
Outside diff comments:
In `@usermods/audioreactive/audio_source.h`:
- Around line 25-33: The preprocessor diagnostics in the audio_source.h block
are stale: the `#error` and `#warning` messages still say "ESP32-C2 or ESP32-C3"
even though the guard now covers CONFIG_IDF_TARGET_ESP32C2,
CONFIG_IDF_TARGET_ESP32C3, CONFIG_IDF_TARGET_ESP32C5, CONFIG_IDF_TARGET_ESP32C6,
CONFIG_IDF_TARGET_ESP32C61, CONFIG_IDF_TARGET_ESP32H2, ESP8266 and ESP8265;
update the text in the `#error` and `#warning` lines to either list all affected
targets (C2, C3, C5, C6, C61, H2, ESP8266/ESP8265) or use a general phrase like
"these single-core/RISC-V/no-FPU targets" so the diagnostic accurately reflects
the condition checked by the surrounding `#if` that also references SOC_I2S_NUM.
In `@wled00/bus_manager.cpp`:
- Around line 1151-1171: The DMA buffer size logic incorrectly uses a running
digitalCount heuristic to decide which bus uses I2S (checks with
PolyBus::isParallelI2S1Output() && digitalCount <= 8 or digitalCount == 1),
which fails when the single‑I2S bus is placed at the end; instead determine the
I2S‑using bus explicitly (by bus index or a PolyBus helper) and only apply the
i2sCommonSize/maxI2S update for that specific bus. Replace the
digitalCount-based decision inside the loop (symbols: digitalCount,
PolyBus::isParallelI2S1Output(), bus->isDigital(), bus->is2Pin(), maxI2S,
bus->getLength(), bus->getNumberOfChannels(), bus->is16bit()) with logic that
either queries a PolyBus API like getI2SBusIndex()/isI2SOutput() or computes the
I2S bus as the last digital non‑2pin when single‑I2S mode is active, then
compute i2sCommonSize only for that identified bus and update maxI2S.
In `@wled00/FX_fcn.cpp`:
- Around line 1194-1211: The usesI2S detection wrongly treats single‑I2S as
digitalCount==1; update the logic so single‑I2S is detected by the actual
single‑I2S bus index (e.g. the last digital bus) instead of hardcoding 1: change
the condition that computes usesI2S to check the bus index against the
single‑I2S mapping (for example compare bus.index == digitalCount-1 or call the
shared mapping/constant used for bus allocation), keeping the rest of the block
(Bus::isDigital(bus.type), Bus::is2Pin(bus.type), the stepFactor and
i2sCommonSize calculation) intact so maxI2S is correctly estimated (see symbols
usesI2S, digitalCount, bus.index, Bus::isDigital, Bus::is2Pin, maxI2S).
In `@wled00/pin_manager.cpp`:
- Around line 249-271: The isPinOk function lacks a CONFIG_IDF_TARGET_ESP32C61
branch and currently falls through to the classic-ESP32 handler; add an `#elif`
block for CONFIG_IDF_TARGET_ESP32C61 in the same region that marks the correct
SPI/PSRAM pins as unavailable (treat GPIO 14–17 and 19–21 as reserved for
SPI0/1/PSRAM as per Espressif docs), handle GPIO 16 and 17 with psramFound()
semantics similar to existing checks (16 returns !psramFound(), 17 returns true
for specific exceptions else !psramFound()), and ensure other pins fall back to
the classic behavior so the new branch prevents incorrect marking of GPIO 24–30
as flash pins; reference isPinOk, gpio, psramFound(), and ESP.getChipModel()
when adding the block.
In `@wled00/wled.h`:
- Around line 334-344: The conditional that selects RX/TX pins can incorrectly
assume RX/TX exist on non-classic ESP32 variants; update the preprocessor check
so RX and TX are only used when actually defined by changing the if condition to
require both RX and TX (i.e., use "defined(ARDUINO_ARCH_ESP32) && defined(RX) &&
defined(TX)"), ensuring the existing constexpr variables hardwareRX and
hardwareTX fall back to 3 and 1 when RX/TX are not defined.
---
Duplicate comments:
In `@platformio.ini`:
- Line 539: The esp32c5dev CI artifact currently defines the debug macro (-D
WLED_DEBUG) under the [env:esp32c5dev] configuration which is included in
default_envs, causing a debug firmware to be published; either remove the -D
WLED_DEBUG flag from the esp32c5dev environment in platformio.ini to produce a
release build, or if this is intentional while experimental, add an inline
comment next to the -D WLED_DEBUG entry referencing why it’s present and a TODO
with an issue/PR number to remove it once the target stabilizes; locate the flag
under the [env:esp32c5dev] block and update accordingly, and ensure default_envs
(the list that includes esp32c5dev) remains accurate.
- Line 420: Replace the branch reference
"https://github.com/jonny190/NeoPixelBus.git#wled-c5-stable" in platformio.ini
with a pinned commit hash (e.g.
"https://github.com/jonny190/NeoPixelBus.git#<commit-hash>") to ensure
reproducible builds, verify the commit hash exists and the repository is
reachable (the CI artifact "firmware-esp32c5dev" confirms current reachability),
and add a short TODO/note near that dependency to track the NeoPixelBus C5
driver bug (RMT constructor channel allocation issue affecting the second
WS2812B output) so we can update the pinned hash when the upstream bugfix is
published.
In `@usermods/audioreactive/audio_reactive.cpp`:
- Around line 2072-2074: The definition of AudioReactive::_analogmic is wrapped
in a different preprocessor condition than its declaration; update the
`#if/`#endif around the _analogmic definition to use the exact same guard used for
the declaration (i.e., the CONFIG_IDF_TARGET_ESP32-based condition with the same
exclusions) so the declaration and definition are compiled under the same target
conditions and no orphan definition is produced for AudioReactive::_analogmic.
- Around line 1163-1231: The build guards missing CONFIG_IDF_TARGET_ESP32C6
allow ESP32‑C6 to enter unsupported I2S/PDM/ADC paths; update the preprocessor
conditions that currently check CONFIG_IDF_TARGET_ESP32S2,
CONFIG_IDF_TARGET_ESP32C3, CONFIG_IDF_TARGET_ESP32C5 (the blocks around the
periph_module_reset call, the dmType PDM detection if-statement, the
switch(dmType) stub cases and the ADC case creating I2SAdcSource) to also
exclude CONFIG_IDF_TARGET_ESP32C6 (or invert to a positive check for classic
ESP32), so paths that call periph_module_reset, treat dmType==5 as PDM, or
create I2SAdcSource are not compiled for C6.
- Around line 1800-1953: The preprocessor guards around the analog-mic JSON/UI
(the `#if` blocks that create amic via FPSTR(_analogmic) in addToConfig and the
corresponding UI prints in appendConfigData) exclude ESP32S2/C3/C5/S3 but omit
ESP32C6, leaving unsupported analog options enabled on C6; update those
conditionals to either use CONFIG_IDF_TARGET_ESP32 (so only classic ESP32
includes analog mic code) or add CONFIG_IDF_TARGET_ESP32C6 to the exclusion
list, and apply the same fix to the matching guards in readFromConfig,
addToConfig (FPSTR(_analogmic) creation) and appendConfigData (uiScript prints
related to analog mic and MCLK info) so all analog-mic handling is consistently
disabled on unsupported MCUs.
In `@wled00/bus_manager.cpp`:
- Around line 1261-1264: The comment above the preprocessor check is missing
"C61" while the `#if` condition includes CONFIG_IDF_TARGET_ESP32C61; update the
comment for the block containing the preprocessor line "#if
defined(CONFIG_IDF_TARGET_ESP32C5) || defined(CONFIG_IDF_TARGET_ESP32C6) ||
defined(CONFIG_IDF_TARGET_ESP32C61) || defined(CONFIG_IDF_TARGET_ESP32P4)" in
bus_manager.cpp so it reads something like "ESP32-C5/C6/C61/P4 use shared RMT
method - idle level inversion not supported" to accurately reflect the condition
in that block.
In `@wled00/bus_wrapper.h`:
- Around line 1375-1377: The inline comment above the condition that returns
I_NONE for num > 1 is outdated relative to the expanded target list; update the
comment that currently reads "On ESP32-C3/C5/C6 only the first 2 RMT channels
are usable for transmitting" to accurately list all targets in the `#elif` (e.g.,
ESP32-C3/C5/C6/C61/P4) or rephrase to a generic statement ("On these targets
only the first 2 RMT channels are usable for transmitting") so it matches the
condition that checks defined(CONFIG_IDF_TARGET_ESP32C3) ||
defined(CONFIG_IDF_TARGET_ESP32C5) || defined(CONFIG_IDF_TARGET_ESP32C6) ||
defined(CONFIG_IDF_TARGET_ESP32C61) || defined(CONFIG_IDF_TARGET_ESP32P4) and
the num > 1 return I_NONE logic.
In `@wled00/data/index.js`:
- Around line 705-706: The UI shows "Ch undefined" when i.wifi.band exists but
i.wifi.channel is missing; update the template line that calls inforow for WiFi
band so it checks i.wifi.channel and renders either "WiFi band" with band + "
(Ch <channel>)" when present, or just the band label when i.wifi.channel is
falsy. Modify the second expression that uses inforow("WiFi band", i.wifi.band +
" (Ch " + i.wifi.channel + ")") to branch on i.wifi.channel (while still
verifying i.wifi.band) so inforow receives a band‑only fallback string when
channel is undefined.
In `@wled00/data/settings_wifi.htm`:
- Around line 253-275: You updated the WiFi UI controls (elements with id="bm"
and id="tx", selects name="BM" and "TX") in the embedded web page; run the UI
build step (npm run build) to regenerate the embedded UI headers so the firmware
binary includes the updated headers before the next firmware build.
In `@wled00/json.cpp`:
- Around line 705-709: The `#else` block currently leaves out root["deviceId"]
entirely on ESP-IDF V5+ builds; ensure the JSON always contains the deviceId key
by adding a fallback assignment in the same spot (set root[F("deviceId")] to an
empty string or a fixed placeholder when getBootloaderSHA256Hex() is not used),
e.g. add a line that assigns an empty String to root["deviceId"] so code
referencing infoData.deviceId in index.js always gets a defined value; look for
the root[...] assignment and getBootloaderSHA256Hex() usage in json.cpp to place
the fallback.
In `@wled00/src/dependencies/dmx/SparkFunDMX.cpp`:
- Line 20: The preprocessor exclusion on the SparkFunDMX.cpp top block is
incorrect: remove the redundant !defined(ESP8266) from the `#if` condition (it's
already inside the ARDUINO_ARCH_ESP32 guard) and add CONFIG_IDF_TARGET_ESP32P4
to the list of excluded targets so the file won't compile for ESP32-P4;
additionally ensure the legacy initRead() path (which uses
timerBegin/timerAttachInterrupt/timerAlarmWrite/timerAlarmEnable) is not
compiled for P4 by the updated `#if` so the IDF v5/incompatible timer APIs are
avoided. Locate the conditional that begins with the current `#if` and update it
to drop ESP8266 and include CONFIG_IDF_TARGET_ESP32P4, keeping the intent of
excluding unsupported targets.
In `@wled00/wled_boards.h`:
- Around line 74-84: The comment for CONFIG_IDF_TARGET_ESP32S2 incorrectly
mentions "esp32-S3"; update the inline comment that precedes the
WLED_HAS_PARALLEL_I2S macro so it refers to "esp32-S2" (i.e., change the text
near the WLED_HAS_PARALLEL_I2S definition within the CONFIG_IDF_TARGET_ESP32S2
block) to match the corrected WLED_BOARD = NODE_TYPE_ID_ESP32S2 and avoid
confusing references to S3.
- Around line 130-136: The comments marked "TBC" on the ESP32-P4 capability
defines are stale — remove the "TBC" markers and make the defines explicit: keep
`#define` WLED_HAVE_FAST_FLOAT 1, keep `#define` WLED_HAVE_FAST_int_DIVIDE 1
(hardware integer divide available), and keep `#define` WLED_HAVE_RTC_MEMORY_HEAP
1 (RTC fast memory supported); update the inline comments next to
WLED_HAVE_FAST_int_DIVIDE and WLED_HAVE_RTC_MEMORY_HEAP to state the confirmed
behavior (e.g., "hardware integer divide" and "RTC fast memory available") and
delete the "TBC" text so the intent is clear.
---
Nitpick comments:
In `@wled00/bus_wrapper.h`:
- Around line 502-513: The TODO about simplifying the long ifdef guard in the
channel remap block must be resolved: either replace the two separate guards
with a single consolidated guard (e.g., define and use a capability macro that
expresses "classic ESP32 I2S1 remap" and then use that macro to wrap both the
parallel-I2S adjustment and the I2S1 subtraction) or record a tracked issue and
reference it in the comment; update the block around `_useParallelI2S` and the
`if (!_useParallelI2S && channel > 0) channel--;` remap so it uses the new
capability macro (or an explicit issue/PR link) instead of the ad-hoc TODO,
ensuring the unique symbols `_useParallelI2S` and `channel` are handled exactly
as before.
In `@wled00/ota_update.cpp`:
- Around line 439-445: The C5 validation is fine but future-enabling OTA for
ESP32-C5 requires using the BOOTLOADER_OFFSET constant rather than the hardcoded
0x1000; update the BootloaderUpdateContext (replace the hardcoded
bootloaderOffset = 0x1000) to reference BOOTLOADER_OFFSET so the correct 0x2000
value applies for ESP32-C5 (and keeps behavior consistent across targets).
In `@wled00/wled_boards.h`:
- Around line 18-25: Add a tracked issue for the TODO list in wled_boards.h and
reference it in the comment: create an issue that enumerates the planned macros
(WLED_HAVE_TOUCH, WLED_HAVE_I2S0_LEDS, WLED_HAVE_I2S1_LEDS, the RMT-channel
limit flag for ESP32-C3/C5/C6, SparkFunDMX support flag for ESP32/S3/P4, and
WLED_ALLOW_LOLIN_WIFI_FIX) and then update the TODO block to include the issue
number/URL so the items are not lost and future PRs can link to that tracking
issue.
In `@wled00/wled.h`:
- Around line 399-404: The txPower global is declared as uint8_t which cannot
represent IDF V5 negative power values; change the declaration of WLED_GLOBAL
uint8_t txPower _INIT(...) to WLED_GLOBAL int8_t txPower _INIT(...) in both
branches (the LOLIN_WIFI_FIX branch and the else branch) and then audit/adjust
all code and UI paths that read or write txPower (serialization, UI widgets, and
any arithmetic or comparisons) to handle signed int8_t values and the extended
WIFI_POWER_21dBm/WIFI_POWER_MINUS_1dBm range.
- Around line 158-165: The duplicated target guard for DMX (the long `#if`
defined(CONFIG_IDF_TARGET_ESP32C5) || ... CONFIG_IDF_TARGET_ESP32P4 pattern used
when WLED_ENABLE_DMX is enabled) should be centralized: add a single
preprocessor macro WLED_DMX_UNSUPPORTED_TARGET in the common board header (e.g.,
declare it via the existing board/target macros) and replace the long guard in
wled.h (both the include-selection block that checks WLED_ENABLE_DMX and the
second occurrence at the later DMX-related block) with a single check using
defined(WLED_DMX_UNSUPPORTED_TARGET); update both places to use that macro so
new targets only need one change.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (29)
package.jsonplatformio.iniusermods/audioreactive/audio_reactive.cppusermods/audioreactive/audio_source.hwled00/FX.hwled00/FX_fcn.cppwled00/FXparticleSystem.cppwled00/bus_manager.cppwled00/bus_wrapper.hwled00/button.cppwled00/cfg.cppwled00/const.hwled00/data/index.jswled00/data/settings_wifi.htmwled00/dmx_output.cppwled00/improv.cppwled00/json.cppwled00/ota_update.cppwled00/pin_manager.cppwled00/set.cppwled00/src/dependencies/dmx/ESPDMX.cppwled00/src/dependencies/dmx/SparkFunDMX.cppwled00/udp.cppwled00/util.cppwled00/wled.cppwled00/wled.hwled00/wled_boards.hwled00/ws.cppwled00/xml.cpp
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- wled00/set.cpp
- wled00/button.cpp
- wled00/improv.cpp
fixes compiler warning related to ignored attributes
wled00/button.cpp:399:31: warning: ignoring attribute 'section (".iram1.1")' because it conflicts with previous 'section (".iram1.0")' [-Wattributes]
399 | void IRAM_ATTR touchButtonISR()
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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 (1)
1160-1164:⚠️ Potential issue | 🟠 MajorSingle-I2S DMA accounting still assumes the first digital bus.
The non-parallel branch still checks
digitalCount == 1, which can miss the actual single-I2S bus placement and underestimatemaxI2S.🔧 Suggested fix
- if ((PolyBus::isParallelI2S1Output() && digitalCount <= 8) || (!PolyBus::isParallelI2S1Output() && digitalCount == 1)) { + bool usesI2SCommonBuffer = PolyBus::isParallelI2S1Output() && (digitalCount <= 8); + if (!PolyBus::isParallelI2S1Output()) { +#if defined(CONFIG_IDF_TARGET_ESP32) + usesI2SCommonBuffer = (digitalCount == 9); // single I2S on logical bus 8 +#elif defined(CONFIG_IDF_TARGET_ESP32S2) + usesI2SCommonBuffer = (digitalCount == 5); // single I2S on logical bus 4 +#endif + } + if (usesI2SCommonBuffer) {Based on learnings: “In WLED PR
#5267, single I2S moved to the end bus (ESP32 bus 8 / S2 bus 4), so memory logic must account for specific bus placement instead of first-bus assumptions.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/bus_manager.cpp` around lines 1160 - 1164, The code currently treats the single-I2S DMA case as "first digital bus" by checking digitalCount == 1; change the non-parallel branch to detect whether the current bus is the actual single-I2S DMA output instead of relying on digitalCount. Replace the (!PolyBus::isParallelI2S1Output() && digitalCount == 1) check with a call that identifies the bus as the single-I2S output (for example use bus->isI2S1Output() or an equivalent method/property that indicates this bus hosts the single I2S DMA), so the condition becomes ((PolyBus::isParallelI2S1Output() && digitalCount <= 8) || (!PolyBus::isParallelI2S1Output() && bus->isI2S1Output())) and adjust any maxI2S accounting accordingly.wled00/FX_fcn.cpp (1)
1195-1200:⚠️ Potential issue | 🟠 Major
usesI2Sstill assumes single-I2S is the first digital bus.On Line 1196,
(!useParallelI2S && digitalCount == 1)can under-estimatemaxI2Sfor layouts where single-I2S is assigned to a later bus. Please derive this from the same bus-placement logic used by allocation, not fixeddigitalCountthresholds.Based on learnings, single I2S placement is target-specific (e.g., tail bus on ESP32/S2), and memory calculation should follow that mapping rather than
digitalCountheuristics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX_fcn.cpp` around lines 1195 - 1200, The current usesI2S calculation assumes single-I2S is bus 0 (using (!useParallelI2S && digitalCount == 1)), which undercounts maxI2S for layouts where single-I2S is placed on a later bus; instead derive usesI2S from the same bus-placement mapping used by allocation: replicate or call the bus-placement logic to compute the index of the single-I2S bus for the target when useParallelI2S is false (target-specific placement), then set usesI2S = (useParallelI2S && digitalCount <= 8) || (!useParallelI2S && i2sBusIndex < digitalCount), and ensure any maxI2S/memory calculations use that derived i2sBusIndex rather than digitalCount == 1; update references to usesI2S, useParallelI2S, digitalCount and maxI2S accordingly.
♻️ Duplicate comments (8)
wled00/ota_update.cpp (1)
18-19:⚠️ Potential issue | 🟡 MinorUpdate the inline target comment to match the preprocessor condition.
Line 19 is stale: it omits
ESP32-C61and still labels C6 as future support while C6 is already included at Line 18.Suggested comment fix
-constexpr size_t BOOTLOADER_OFFSET = 0x0000; // esp32-S3, esp32-C3 and (future support) esp32-c6 +constexpr size_t BOOTLOADER_OFFSET = 0x0000; // esp32-S3, esp32-C3, esp32-C6, and (future support) esp32-C61🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/ota_update.cpp` around lines 18 - 19, The inline comment next to the BOOTLOADER_OFFSET definition is stale; update the comment to reflect the preprocessor condition (`#if` defined(CONFIG_IDF_TARGET_ESP32S3) || defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32C6) || defined(CONFIG_IDF_TARGET_ESP32C61)) by explicitly listing esp32-S3, esp32-C3, esp32-C6 and esp32-C61 and removing the "future support" wording for C6 so the comment accurately matches the condition for BOOTLOADER_OFFSET.usermods/audioreactive/audio_reactive.cpp (3)
1163-1166:⚠️ Potential issue | 🔴 Critical
periph_module_reset(PERIPH_I2S0_MODULE)will be called on ESP32-C6.The guard now correctly skips C3 and C5 but omits C6, which is the same RISC-V family and lacks
PERIPH_I2S0_MODULE. Extend the guard:🔧 Proposed fix
-#if !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32C5) +#if !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32C5) && !defined(CONFIG_IDF_TARGET_ESP32C6) delay(100); periph_module_reset(PERIPH_I2S0_MODULE); `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/audioreactive/audio_reactive.cpp` around lines 1163 - 1166, The conditional that skips calling periph_module_reset(PERIPH_I2S0_MODULE) currently excludes CONFIG_IDF_TARGET_ESP32C3 and CONFIG_IDF_TARGET_ESP32C5 but misses CONFIG_IDF_TARGET_ESP32C6, so periph_module_reset will be invoked on ESP32-C6 where PERIPH_I2S0_MODULE is unavailable; update the preprocessor guard around the delay(100) / periph_module_reset block to also check for CONFIG_IDF_TARGET_ESP32C6 (i.e., add && !defined(CONFIG_IDF_TARGET_ESP32C6) to the `#if`) so the reset call is skipped on C6 as well.
1906-1915:⚠️ Potential issue | 🟠 MajorC6 missing from
appendConfigDataUI option guards — invalid mic types shown on C6.C6 users would see "Generic Analog" and "Generic I2S PDM" options which are unsupported on C6. Add
!defined(CONFIG_IDF_TARGET_ESP32C6)to the three guards inappendConfigData:🔧 Proposed fix
- `#if` !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32C5) && !defined(CONFIG_IDF_TARGET_ESP32S3) + `#if` !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32C5) && !defined(CONFIG_IDF_TARGET_ESP32C6) && !defined(CONFIG_IDF_TARGET_ESP32S3) uiScript.print(F("addOption(dd,'Generic Analog',0);")); `#endif` - `#if` !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32C5) + `#if` !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32C5) && !defined(CONFIG_IDF_TARGET_ESP32C6) uiScript.print(F("addOption(dd,'Generic I2S PDM',5);")); `#endif` - `#if` !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32C5) && !defined(CONFIG_IDF_TARGET_ESP32S3) + `#if` !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32C5) && !defined(CONFIG_IDF_TARGET_ESP32C6) && !defined(CONFIG_IDF_TARGET_ESP32S3) uiScript.print(F("addInfo(uxp,3,'<i>only use -1, 0, 1 or 3</i>','I2S MCLK');")); `#else` uiScript.print(F("addInfo(uxp,3,'<i>master clock</i>','I2S MCLK');")); `#endif`Also applies to: 1949-1953
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/audioreactive/audio_reactive.cpp` around lines 1906 - 1915, appendConfigData currently conditionally prints mic type options but omits CONFIG_IDF_TARGET_ESP32C6, causing unsupported "Generic Analog" and "Generic I2S PDM" options to appear on C6 devices; update the three preprocessor guards around uiScript.print calls that output "addOption(dd,'Generic Analog',0);" and "addOption(dd,'Generic I2S PDM',5);" (appearing near lines where appendConfigData emits those addOption entries and again around the second block at ~1949-1953) to also check !defined(CONFIG_IDF_TARGET_ESP32C6) so the guards become e.g. `#if` !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32C5) && !defined(CONFIG_IDF_TARGET_ESP32C6).
1171-1182:⚠️ Potential issue | 🟠 MajorC6 missing from PDM auto-detect and stub case guards.
C6 lacks PDM and ADC-over-I2S support (same as C3/C5). Three guards here need C6 added:
- Line 1171 (PDM auto-detect exclusion): add
!defined(CONFIG_IDF_TARGET_ESP32C6)- Line 1176 (outer stub cases): add
|| defined(CONFIG_IDF_TARGET_ESP32C6)- Line 1179 (inner PDM stub): add
|| defined(CONFIG_IDF_TARGET_ESP32C6)🔧 Proposed fix
-#if !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32C5) +#if !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32C5) && !defined(CONFIG_IDF_TARGET_ESP32C6) if ((i2sckPin == I2S_PIN_NO_CHANGE) && ...) dmType = 5; `#endif` -#if defined(CONFIG_IDF_TARGET_ESP32S2) || defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32C5) || defined(CONFIG_IDF_TARGET_ESP32S3) +#if defined(CONFIG_IDF_TARGET_ESP32S2) || defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32C5) || defined(CONFIG_IDF_TARGET_ESP32C6) || defined(CONFIG_IDF_TARGET_ESP32S3) case 0: - `#if` defined(CONFIG_IDF_TARGET_ESP32S2) || defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32C5) + `#if` defined(CONFIG_IDF_TARGET_ESP32S2) || defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32C5) || defined(CONFIG_IDF_TARGET_ESP32C6) case 5: `#endif` `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/audioreactive/audio_reactive.cpp` around lines 1171 - 1182, The preprocessor guards omit ESP32C6 causing PDM/ADC-over-I2S to be incorrectly enabled on C6; update the three conditionals to include CONFIG_IDF_TARGET_ESP32C6: add !defined(CONFIG_IDF_TARGET_ESP32C6) to the PDM auto-detect exclusion that checks i2sckPin/i2ssdPin/i2swsPin/dmType, add || defined(CONFIG_IDF_TARGET_ESP32C6) to the outer stub guard that wraps the switch cases for dmType, and add || defined(CONFIG_IDF_TARGET_ESP32C6) to the inner PDM stub guard so case 5 (PDM Microphone) is correctly excluded on C6.wled00/src/dependencies/dmx/SparkFunDMX.cpp (1)
20-20:⚠️ Potential issue | 🟡 MinorRedundant
!defined(ESP8266)and missingCONFIG_IDF_TARGET_ESP32P4exclusion still present.These were flagged in a prior review and marked as addressed, but the current code still has both issues:
!defined(ESP8266)is dead weight —ESP8266is never defined whenARDUINO_ARCH_ESP32is active (line 17).CONFIG_IDF_TARGET_ESP32P4is absent —wled.hroutes P4 to the ESPDMX path, so SparkFunDMX compiles for P4 unnecessarily, producing dead code.🔧 Proposed fix
-#if !defined(ESP8266) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32C5) && !defined(CONFIG_IDF_TARGET_ESP32C6) && !defined(CONFIG_IDF_TARGET_ESP32C61) && !defined(CONFIG_IDF_TARGET_ESP32S2) +#if !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32C5) && !defined(CONFIG_IDF_TARGET_ESP32C6) && !defined(CONFIG_IDF_TARGET_ESP32C61) && !defined(CONFIG_IDF_TARGET_ESP32P4) && !defined(CONFIG_IDF_TARGET_ESP32S2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/src/dependencies/dmx/SparkFunDMX.cpp` at line 20, The preprocessor condition in SparkFunDMX.cpp still redundantly tests !defined(ESP8266) and omits CONFIG_IDF_TARGET_ESP32P4; update the guard that starts with `#if` !defined(ESP8266) && !defined(CONFIG_IDF_TARGET_ESP32C3)... to remove the !defined(ESP8266) term and add && !defined(CONFIG_IDF_TARGET_ESP32P4) so SparkFunDMX (SparkFunDMX.cpp) is excluded for the ESP32P4 target and avoids compiling unnecessary dead code.wled00/xml.cpp (1)
322-329: Replace temporary2, 6literals with named constants (and fix comment typo).This is still readable enough for now, but named constants make the emulation intent clearer and safer to maintain.
♻️ Proposed cleanup
`#if` defined(CONFIG_IDF_TARGET_ESP32C5) || defined(CONFIG_IDF_TARGET_ESP32C6) || defined(CONFIG_IDF_TARGET_ESP32C61) || defined(CONFIG_IDF_TARGET_ESP32P4) - // pretend to be a C3 - temporary hack to keep seetings_leds working -> isC3() => maxD == 2 && maxA == 6 - 2, 6, + constexpr uint8_t kUiMaxDigitalChannels = 2; + constexpr uint8_t kUiMaxAnalogChannels = 6; + // pretend to be a C3 - temporary hack to keep settings_leds working -> isC3() => maxD == 2 && maxA == 6 + kUiMaxDigitalChannels, kUiMaxAnalogChannels, `#else` WLED_MAX_DIGITAL_CHANNELS, WLED_MAX_ANALOG_CHANNELS, `#endif` - WLED_MAX_BUTTONS + WLED_MAX_BUTTONSBased on learnings: “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.”
As per coding guidelines: “wled00/**/*.cpp: Use 2-space indentation for C++ source files (.cpp)”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/xml.cpp` around lines 322 - 329, Replace the magic literals "2, 6" with named constants to clarify the C3 emulation intent and fix the comment typo; instead of hardcoded values use the existing symbols (e.g., WLED_MAX_DIGITAL_CHANNELS and WLED_MAX_ANALOG_CHANNELS or define WLED_EMULATED_C3_MAX_DIGITAL / WLED_EMULATED_C3_MAX_ANALOG if clearer) in the conditional block that pretends to be a C3 (the comment referencing isC3()); also correct the comment text from "seetings_leds" to "settings_leds" so the purpose is clear and maintainable while keeping WLED_MAX_BUTTONS unchanged.wled00/bus_manager.cpp (1)
1261-1263:⚠️ Potential issue | 🟡 MinorKeep the idle-inversion comment aligned with its guard.
The guard includes
CONFIG_IDF_TARGET_ESP32C61, but the comment text omits C61.✏️ Suggested fix
- // ESP32-C5/C6/P4 use shared RMT method - idle level inversion not supported + // ESP32-C5/C6/C61/P4 use shared RMT method - idle level inversion not supported🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/bus_manager.cpp` around lines 1261 - 1263, The preprocessor guard that checks CONFIG_IDF_TARGET_ESP32C61 is missing from the inline comment—update the comment above the return in bus_manager.cpp (the line with `#if` defined(...CONFIG_IDF_TARGET_ESP32C61...)) so it lists ESP32-C61 alongside ESP32-C5/C6/P4 (e.g., "ESP32-C5/C6/C61/P4 use shared RMT method - idle level inversion not supported") to keep the comment aligned with the guard.wled00/bus_wrapper.h (1)
1375-1377:⚠️ Potential issue | 🟡 MinorUpdate the RMT-channel comment to match the actual guard.
The condition includes
CONFIG_IDF_TARGET_ESP32C61andCONFIG_IDF_TARGET_ESP32P4, but the comment only mentions C3/C5/C6.✏️ Suggested fix
- // On ESP32-C3/C5/C6 only the first 2 RMT channels are usable for transmitting + // On ESP32-C3/C5/C6/C61/P4 only the first 2 RMT channels are usable for transmitting🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/bus_wrapper.h` around lines 1375 - 1377, The inline comment above the RMT-channel guard is outdated: the preprocessor condition in the if-statement (the guard that checks CONFIG_IDF_TARGET_ESP32C3, CONFIG_IDF_TARGET_ESP32C5, CONFIG_IDF_TARGET_ESP32C6, CONFIG_IDF_TARGET_ESP32C61, and CONFIG_IDF_TARGET_ESP32P4) mentions only C3/C5/C6; update the comment near that check (the block containing the if (num > 1) return I_NONE;) to accurately reflect all targets in the condition (e.g., include C61 and P4 or generalize to “ESP32-C3/C5/C6/C61/P4”) so the comment matches the guard.
🧹 Nitpick comments (3)
wled00/ota_update.cpp (1)
413-413: Replace raw chip-ID literals with named constants.Line 413 introduces another hardcoded chip ID; using a named constant would make updates safer and reduce duplication with Line 440.
Refactor sketch
+constexpr uint16_t CHIP_ID_ESP32_C5 = 0x0017; ... - // 0x0017 = ESP32-C5 + // CHIP_ID_ESP32_C5 = ESP32-C5 ... - if (chipId != 0x0017) { - *bootloaderErrorMsg = "Chip ID mismatch - expected ESP32-C5 (0x0017), got 0x" + String(chipId, HEX); + if (chipId != CHIP_ID_ESP32_C5) { + *bootloaderErrorMsg = "Chip ID mismatch - expected ESP32-C5 (0x0017), got 0x" + String(chipId, HEX); return false; }Based on learnings: 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.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/ota_update.cpp` at line 413, Replace the hardcoded chip-ID literal 0x0017 with a named constant (e.g., CHIP_ID_ESP32_C5) and use that constant wherever the literal appears (the occurrences around the current code in ota_update.cpp, including the spot at line 413 and the duplicate at line 440). Add the constant as a constexpr (uint16_t or appropriate type) alongside the other chip ID constants in this file or header so it can be reused and avoid duplication, then update any switch/case or comparison logic (functions handling chip IDs) to reference CHIP_ID_ESP32_C5 instead of the raw 0x0017 literal.wled00/improv.cpp (1)
13-16: Unresolved TODO: verify WiFi scan compatibility for C6, C61, and P4.The in-line
// ToDO check if C6, C61, P4 support thiscomment has not been acted on. If these targets share the same WiFi driver constraints as C5/C3/S2 (which all lack properWiFi.scanNetworks()support at this IDF level), they should be added to the#ifguard to avoid a silent no-op or crash whenstartImprovWifiScan()is called on those targets.Do you want me to open a new issue to track the C6/C61/P4 WiFi scan verification, or generate a proposed fix assuming those targets should also disable scanning?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/improv.cpp` around lines 13 - 16, The preprocessor TODO indicates C6, C61, and P4 targets may also lack WiFi.scanNetworks() support; update the guard in improv.cpp to include those target macros (e.g., add CONFIG_IDF_TARGET_ESP32C6 and CONFIG_IDF_TARGET_ESP32P4 / any vendor-provided macro names that correspond to C6, C61, P4) so WLED_DISABLE_IMPROV_WIFISCAN is defined for them too, and ensure startImprovWifiScan() will be disabled for those builds; if you cannot confirm the exact macro names, open an issue to track verification of C6/C61/P4 WiFi scan compatibility and reference improv.cpp and the WLED_DISABLE_IMPROV_WIFISCAN symbol.wled00/const.h (1)
70-71: Consider consolidating repeated target-condition groups.The same C3/C5/C6/C61/P4 condition is repeated in multiple places, which increases future drift risk.
♻️ Proposed cleanup
+// Shared target bucket for reduced-channel ESP32 variants +#if defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32C5) || defined(CONFIG_IDF_TARGET_ESP32C6) || defined(CONFIG_IDF_TARGET_ESP32C61) || defined(CONFIG_IDF_TARGET_ESP32P4) + `#define` WLED_TARGET_CLASS_LOW_CHANNEL 1 +#endif ... -#if defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32C5) || defined(CONFIG_IDF_TARGET_ESP32C6) || defined(CONFIG_IDF_TARGET_ESP32C61) || defined(CONFIG_IDF_TARGET_ESP32P4) +#if defined(WLED_TARGET_CLASS_LOW_CHANNEL)Also applies to: 516-517, 529-530, 645-646
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/const.h` around lines 70 - 71, Create a named preprocessor macro to consolidate the repeated target-condition group (e.g., define IS_ESP32_C3_C5_C6_C61_P4 that expands to the existing disjunction of CONFIG_IDF_TARGET_ESP32C3, CONFIG_IDF_TARGET_ESP32C5, CONFIG_IDF_TARGET_ESP32C6, CONFIG_IDF_TARGET_ESP32C61, CONFIG_IDF_TARGET_ESP32P4) and then replace every repeated condition occurrence (the ifdefs around the RMT/LEDC/I2S comments and the other repeated checks at the locations flagged) with a single check using that macro (e.g., `#if` defined(IS_ESP32_C3_C5_C6_C61_P4)); update any associated comments to reference the new macro name for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/audioreactive/audio_reactive.cpp`:
- Around line 1800-1803: The conditional that wraps use of FPSTR(_analogmic) in
addToConfig (and equivalently in readFromConfig) omits CONFIG_IDF_TARGET_ESP32C6
so the code referencing the static _analogmic compiles on C6 where it isn't
declared; update the preprocessor guard to match the declaration's positive
guard by including CONFIG_IDF_TARGET_ESP32C6 in the excluded-target list (i.e.,
add !defined(CONFIG_IDF_TARGET_ESP32C6) to the `#if` check that surrounds the
amic/FPSTR(_analogmic) blocks in addToConfig and readFromConfig) so the
FPSTR(_analogmic) usage is compiled only when the static exists.
- Around line 2072-2074: The out-of-line definition for
AudioReactive::_analogmic uses a different preprocessor guard than its
declaration, causing a build error on ESP32-C6; update the definition's
conditional to exactly match the declaration's guard (the one using
CONFIG_IDF_TARGET_ESP32) so that AudioReactive::_analogmic is only defined when
it was declared (i.e., use the same `#if` defined(CONFIG_IDF_TARGET_ESP32)
condition as the declaration).
In `@wled00/fcn_declare.h`:
- Line 32: Fix the typo in the comment on the SOC_TOUCH_VERSION_2 preprocessor
line: change "fucntion" to "function" in the comment text associated with the
`#ifdef` SOC_TOUCH_VERSION_2 in fcn_declare.h so the comment reads "...have a
function to check touch state...".
---
Outside diff comments:
In `@wled00/bus_manager.cpp`:
- Around line 1160-1164: The code currently treats the single-I2S DMA case as
"first digital bus" by checking digitalCount == 1; change the non-parallel
branch to detect whether the current bus is the actual single-I2S DMA output
instead of relying on digitalCount. Replace the
(!PolyBus::isParallelI2S1Output() && digitalCount == 1) check with a call that
identifies the bus as the single-I2S output (for example use bus->isI2S1Output()
or an equivalent method/property that indicates this bus hosts the single I2S
DMA), so the condition becomes ((PolyBus::isParallelI2S1Output() && digitalCount
<= 8) || (!PolyBus::isParallelI2S1Output() && bus->isI2S1Output())) and adjust
any maxI2S accounting accordingly.
In `@wled00/FX_fcn.cpp`:
- Around line 1195-1200: The current usesI2S calculation assumes single-I2S is
bus 0 (using (!useParallelI2S && digitalCount == 1)), which undercounts maxI2S
for layouts where single-I2S is placed on a later bus; instead derive usesI2S
from the same bus-placement mapping used by allocation: replicate or call the
bus-placement logic to compute the index of the single-I2S bus for the target
when useParallelI2S is false (target-specific placement), then set usesI2S =
(useParallelI2S && digitalCount <= 8) || (!useParallelI2S && i2sBusIndex <
digitalCount), and ensure any maxI2S/memory calculations use that derived
i2sBusIndex rather than digitalCount == 1; update references to usesI2S,
useParallelI2S, digitalCount and maxI2S accordingly.
---
Duplicate comments:
In `@usermods/audioreactive/audio_reactive.cpp`:
- Around line 1163-1166: The conditional that skips calling
periph_module_reset(PERIPH_I2S0_MODULE) currently excludes
CONFIG_IDF_TARGET_ESP32C3 and CONFIG_IDF_TARGET_ESP32C5 but misses
CONFIG_IDF_TARGET_ESP32C6, so periph_module_reset will be invoked on ESP32-C6
where PERIPH_I2S0_MODULE is unavailable; update the preprocessor guard around
the delay(100) / periph_module_reset block to also check for
CONFIG_IDF_TARGET_ESP32C6 (i.e., add && !defined(CONFIG_IDF_TARGET_ESP32C6) to
the `#if`) so the reset call is skipped on C6 as well.
- Around line 1906-1915: appendConfigData currently conditionally prints mic
type options but omits CONFIG_IDF_TARGET_ESP32C6, causing unsupported "Generic
Analog" and "Generic I2S PDM" options to appear on C6 devices; update the three
preprocessor guards around uiScript.print calls that output
"addOption(dd,'Generic Analog',0);" and "addOption(dd,'Generic I2S PDM',5);"
(appearing near lines where appendConfigData emits those addOption entries and
again around the second block at ~1949-1953) to also check
!defined(CONFIG_IDF_TARGET_ESP32C6) so the guards become e.g. `#if`
!defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) &&
!defined(CONFIG_IDF_TARGET_ESP32C5) && !defined(CONFIG_IDF_TARGET_ESP32C6).
- Around line 1171-1182: The preprocessor guards omit ESP32C6 causing
PDM/ADC-over-I2S to be incorrectly enabled on C6; update the three conditionals
to include CONFIG_IDF_TARGET_ESP32C6: add !defined(CONFIG_IDF_TARGET_ESP32C6) to
the PDM auto-detect exclusion that checks i2sckPin/i2ssdPin/i2swsPin/dmType, add
|| defined(CONFIG_IDF_TARGET_ESP32C6) to the outer stub guard that wraps the
switch cases for dmType, and add || defined(CONFIG_IDF_TARGET_ESP32C6) to the
inner PDM stub guard so case 5 (PDM Microphone) is correctly excluded on C6.
In `@wled00/bus_manager.cpp`:
- Around line 1261-1263: The preprocessor guard that checks
CONFIG_IDF_TARGET_ESP32C61 is missing from the inline comment—update the comment
above the return in bus_manager.cpp (the line with `#if`
defined(...CONFIG_IDF_TARGET_ESP32C61...)) so it lists ESP32-C61 alongside
ESP32-C5/C6/P4 (e.g., "ESP32-C5/C6/C61/P4 use shared RMT method - idle level
inversion not supported") to keep the comment aligned with the guard.
In `@wled00/bus_wrapper.h`:
- Around line 1375-1377: The inline comment above the RMT-channel guard is
outdated: the preprocessor condition in the if-statement (the guard that checks
CONFIG_IDF_TARGET_ESP32C3, CONFIG_IDF_TARGET_ESP32C5, CONFIG_IDF_TARGET_ESP32C6,
CONFIG_IDF_TARGET_ESP32C61, and CONFIG_IDF_TARGET_ESP32P4) mentions only
C3/C5/C6; update the comment near that check (the block containing the if (num >
1) return I_NONE;) to accurately reflect all targets in the condition (e.g.,
include C61 and P4 or generalize to “ESP32-C3/C5/C6/C61/P4”) so the comment
matches the guard.
In `@wled00/ota_update.cpp`:
- Around line 18-19: The inline comment next to the BOOTLOADER_OFFSET definition
is stale; update the comment to reflect the preprocessor condition (`#if`
defined(CONFIG_IDF_TARGET_ESP32S3) || defined(CONFIG_IDF_TARGET_ESP32C3) ||
defined(CONFIG_IDF_TARGET_ESP32C6) || defined(CONFIG_IDF_TARGET_ESP32C61)) by
explicitly listing esp32-S3, esp32-C3, esp32-C6 and esp32-C61 and removing the
"future support" wording for C6 so the comment accurately matches the condition
for BOOTLOADER_OFFSET.
In `@wled00/src/dependencies/dmx/SparkFunDMX.cpp`:
- Line 20: The preprocessor condition in SparkFunDMX.cpp still redundantly tests
!defined(ESP8266) and omits CONFIG_IDF_TARGET_ESP32P4; update the guard that
starts with `#if` !defined(ESP8266) && !defined(CONFIG_IDF_TARGET_ESP32C3)... to
remove the !defined(ESP8266) term and add && !defined(CONFIG_IDF_TARGET_ESP32P4)
so SparkFunDMX (SparkFunDMX.cpp) is excluded for the ESP32P4 target and avoids
compiling unnecessary dead code.
In `@wled00/xml.cpp`:
- Around line 322-329: Replace the magic literals "2, 6" with named constants to
clarify the C3 emulation intent and fix the comment typo; instead of hardcoded
values use the existing symbols (e.g., WLED_MAX_DIGITAL_CHANNELS and
WLED_MAX_ANALOG_CHANNELS or define WLED_EMULATED_C3_MAX_DIGITAL /
WLED_EMULATED_C3_MAX_ANALOG if clearer) in the conditional block that pretends
to be a C3 (the comment referencing isC3()); also correct the comment text from
"seetings_leds" to "settings_leds" so the purpose is clear and maintainable
while keeping WLED_MAX_BUTTONS unchanged.
---
Nitpick comments:
In `@wled00/const.h`:
- Around line 70-71: Create a named preprocessor macro to consolidate the
repeated target-condition group (e.g., define IS_ESP32_C3_C5_C6_C61_P4 that
expands to the existing disjunction of CONFIG_IDF_TARGET_ESP32C3,
CONFIG_IDF_TARGET_ESP32C5, CONFIG_IDF_TARGET_ESP32C6,
CONFIG_IDF_TARGET_ESP32C61, CONFIG_IDF_TARGET_ESP32P4) and then replace every
repeated condition occurrence (the ifdefs around the RMT/LEDC/I2S comments and
the other repeated checks at the locations flagged) with a single check using
that macro (e.g., `#if` defined(IS_ESP32_C3_C5_C6_C61_P4)); update any associated
comments to reference the new macro name for clarity.
In `@wled00/improv.cpp`:
- Around line 13-16: The preprocessor TODO indicates C6, C61, and P4 targets may
also lack WiFi.scanNetworks() support; update the guard in improv.cpp to include
those target macros (e.g., add CONFIG_IDF_TARGET_ESP32C6 and
CONFIG_IDF_TARGET_ESP32P4 / any vendor-provided macro names that correspond to
C6, C61, P4) so WLED_DISABLE_IMPROV_WIFISCAN is defined for them too, and ensure
startImprovWifiScan() will be disabled for those builds; if you cannot confirm
the exact macro names, open an issue to track verification of C6/C61/P4 WiFi
scan compatibility and reference improv.cpp and the WLED_DISABLE_IMPROV_WIFISCAN
symbol.
In `@wled00/ota_update.cpp`:
- Line 413: Replace the hardcoded chip-ID literal 0x0017 with a named constant
(e.g., CHIP_ID_ESP32_C5) and use that constant wherever the literal appears (the
occurrences around the current code in ota_update.cpp, including the spot at
line 413 and the duplicate at line 440). Add the constant as a constexpr
(uint16_t or appropriate type) alongside the other chip ID constants in this
file or header so it can be reused and avoid duplication, then update any
switch/case or comparison logic (functions handling chip IDs) to reference
CHIP_ID_ESP32_C5 instead of the raw 0x0017 literal.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (30)
package.jsonplatformio.iniusermods/audioreactive/audio_reactive.cppusermods/audioreactive/audio_source.hwled00/FX.hwled00/FX_fcn.cppwled00/FXparticleSystem.cppwled00/bus_manager.cppwled00/bus_wrapper.hwled00/button.cppwled00/cfg.cppwled00/const.hwled00/data/index.jswled00/data/settings_wifi.htmwled00/dmx_output.cppwled00/fcn_declare.hwled00/improv.cppwled00/json.cppwled00/ota_update.cppwled00/pin_manager.cppwled00/set.cppwled00/src/dependencies/dmx/ESPDMX.cppwled00/src/dependencies/dmx/SparkFunDMX.cppwled00/udp.cppwled00/util.cppwled00/wled.cppwled00/wled.hwled00/wled_boards.hwled00/ws.cppwled00/xml.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- wled00/data/index.js
- wled00/FXparticleSystem.cpp
- wled00/cfg.cpp
- wled00/json.cpp
Member
|
ToDo for P4:
--> possible solution #include "esp_wifi_remote_api.h"
esp_err_t esp_wifi_remote_get_mac(wifi_interface_t ifx, uint8_t mac[6]); |
${common.build_flags} was missing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
C6 needs the newer ESP-IDF, so have one branch for those changes and keep this one just for the C6 specific things
Building on top of V5
Inspired by C6 Experiments
Help Us with Testing
If you own a C6 or C5 board, we like to hear from your experiences! Basic functions are working on our dev boards, but any problem you find will help us stabilize the C5/C6 versions faster. If you find an error, please create an issue ticket (bug report).
Important: Our V5 Branch is Experimental (early stage of development)
You can find a summary of currently open ends here (open ends for C5, C6), here (modernization opportunities) and here (generic problems with V5).
Using Development Tools (VSCode) - recommended
You can build your own C5 or C6 firmware using our standard development environment (VSCode+platformio) on your own computer. Use Platformio "upload and monitor" to write firmware to your board.
Experimental Binaries
In case you are not feeling comfortable using development tools, you can find some firmware binaries that are automatically created by GitHub after each commit.
WLED CI / wled_build / Build Environments (esp32c6dev_4MB) (push)Help Us with Coding
Helpful information and migration guidance
coding guides
We should make sure that the "V5" branch will still compile for "V4" as a backup solution.
When adding code that only works in the new framework, it should be conditionally compiled, like in the examples below
related
Summary by CodeRabbit
New Features
Bug Fixes / Compatibility
Chores