Fix bootloops, remove stray flag#5749
Conversation
WalkthroughESP32 build environments remove stray directives and add target-specific USB-mode defines. Device-fingerprint ADC calibration is restricted to pre-5.0 ESP-IDF builds, while audioreactive adds ESP-IDF 5.x compatibility stubs for legacy ADC/I2S symbols. ChangesESP32 compatibility
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
softhack007
left a comment
softhack007
left a comment
There was a problem hiding this comment.
Looks good to me 👍
I was always wondering what -DCO is about, it was probably an ancient copy&paste from tasmota
|
@netmindz this changes the "device ID" for the stats, at least for C3 and S3 - just a heads-up |
| #endif | ||
|
|
||
| #if CONFIG_IDF_TARGET_ESP32 || CONFIG_IDF_TARGET_ESP32S2 || CONFIG_IDF_TARGET_ESP32C3 || CONFIG_IDF_TARGET_ESP32S3 | ||
| #if ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0) && (CONFIG_IDF_TARGET_ESP32 || CONFIG_IDF_TARGET_ESP32S2 || CONFIG_IDF_TARGET_ESP32C3 || CONFIG_IDF_TARGET_ESP32S3) |
There was a problem hiding this comment.
This will mean that every unique deviceId will change. Is this really our only option?
If so then we need to start rolling out a dual deviceid approach into current firmware so we can try and migrate
There was a problem hiding this comment.
I think we could go back to my initial approach of reading the values directly from the fuses - which was replaced with this as it is "dangerous low level stuff" and better to use a higher level API but since it is no longer available that would now be our only option.
There was a problem hiding this comment.
If so then we need to start rolling out a dual deviceid approach into current firmware
I think our DeviceIDs will definitely change with V5 - the old API is gone; it gave us some calibration data to salt the DeviceID with. We cannot even re-calculate the old IDs in V5 firmware.
There was a problem hiding this comment.
maybe I can salvage it, the ESP32 and the S2 still support the old API (at least in the V5 version we are using, AI said it got removed in 5.6 or something). For the C3 and S3 we may be able to replicate the value from the fuse value, need to check.
|
@DedeHai i've just fixed some merge conflicts with my latest platformio.ini changes, hope this doesn't screw up any uncommitted changes you may have. |
|
@softhack007 I have no uncommited changes yet. I am just running V5 on some of my test devices - C3 and S2 are completely screwed using RMT - not sure we are missing some parameters for NPB or if it just plain is not working. Symptoms: frames get pushed out while the last frame is still sending (FPS limiter disabled). When trying to save any LED settings, it hangs at |
@DedeHai I'd suggest fixing up pixelbus and merging it instead of debugging NPB. We can continue to iterate on it in main. |
Wow, that's heavy. It might be due to using the "core3" neopixelbus branch that uses the new RMT channelizer API 🤔 Lines 254 to 256 in f7cff3d Also the V4 builds now seem to use NPB "core3", I'll try to separate that out so V5 and V4 can have different NPB versions. In principle - if nothing in wled references the new RMT api - we could even continue to use the "normal" NPB version with the old RMT driver. Edit: or following the suggestion by @willmmiles, we wait a bit until WLEDpixelBus is ready for merging (must run on esp-idf V5 and V4 !), and then completely swap out NPB for all esp32 variants. |
exactly my thought, already updated the branch to main but oh so many compile errors ;) |
|
@softhack007 what is the reason we need everything to run on V4 still? |
Right now I still keep V4 as a "fallback" option, for back-to-back compare and for users that need features which are currently not working with V5 (analog mic, DMX serial, RMTHI flicker fixer, etc.). Its just a complication in platformio.ini, and some If it gets too complicated to handle, we can drop the V4 support. |
|
@DedeHai @netmindz as this is currently a show-stopper for S3 and C3 🤔 how about limiting the deviceID workaround to these two MCUs and then merge to main, and we'll try to rescue the old deviceID code in a separate PR? It means we will collect some duplicate/bad IDs in the usage database. I guess there will just be a few reports that could be removed later, by searching for date + version. Additionally, nightly builds are off, and I hope that only a few users will install from |
|
@softhack007 I have a suggested fix from claude but did not check if it is legit and actually working. |
|
And I have the new WLEDpixelbus running on an S2 - RMT, I2S and BitBang appear to be working but RMT is quite fragile without the high priority interrupt: using more than two RMT outputs gives me horrible flickering but at least it works. I2S seems glitch free and bit bang as well so good progress on that front |
|
@softhack007 I don't know why it worked before but now I can not get classic ESP32 to boot, seems to be the same legacy adc driver conflict. If I do not compile AR UM it runs. |
|
Well, I got AR to compile with V5 yesterday - all V5 builds before yesterday did not include AR. I need to cross-check. If driver/i2s.h is enough to create the legacy driver conflict, it means we need to exclude AR from esp32. I'll check during the day. In fact classic esp32 is the only boards where i2s supported adc-over-i2s, so it makes sense that classic esp32 now has the adc driver conflict. |
|
@softhack007 if you can also check if S3 works, I also got a bootloop with AR but not sure I was not on the new LED driver branch which may not be fully up to date with main |
|
regarding device ID: I have a fix for the C3, ESP32 and S2 still work correctly using the deprecated function (it does not pull in the legacy driver) BUT the legacy adc stuff is no longer available in newer IDFs so I will try if I can do it without that function. S3 I could not get to produce the same ID as in 16.0. @netmindz how many S3 are in the stats and how bad would it be if those changed ID? |
|
@DedeHai bootloop on classic esp32 confirmed - it happens when audioreactive is included Backtrace: 0x40082f81:0x3ffe3b30 0x4008d651:0x3ffe3b50 0x40090d89:0x3ffe3b70 0x4017a80f:0x3ffe3be0 0x4016e01e:0x3ffe3c00 0x40082501:0x3ffe3c30 0x400792bb:0x3ffe3c80 |<-CORRUPTED
ELF file SHA256: b149e4e6e
Rebooting...Stack Trace
|
S3 works for me, also with AR. But I'll still push a change that avoids including adc.h |
|
I hit also the boot loop. on C3 and classic ESP32, put the discovery (from claude) as a reference here: Confirming this fixes the C3 here, and flagging that the classic ESP32 has a second, independent path to the same Re: "While it works on ESP32 and S2" — the
ReproductionBoard: ESP32-D0WD-V3 rev v3.0, 4MB flash. Tree at 147 crash cycles in 12 s. Decoded: i.e. it aborts in a C constructor during ConfirmationRebuilding One reset, zero aborts. Notes
Hope it is useful, otherwise I can delete it. |
Thanks, this confirms our observation (The tech-babble from claude is not adding anything useful, unfortunately; esp-idf is written in plain C, there is no such thing as a "C constructor". The abort is simply from the esp-idf startup code that checks if a weak symbol is not NULL). It means that for classic esp32 only, audioreactive does not work any more in V5. It needs a full port to the new APIs. |
|
@softhack007 in general you can check the .map file: if it contains "adc_legacy" it wont boot and something pulled that in. |
@softhack007 eventually yes but as I noted above, I do have a "hack fix" but I have no hardware to test if the I2S mic still works. Since the other option is to disable AR completely, I will add it to this PR for you to test with actual audio hardware. i.e. this:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@usermods/audioreactive/audio_reactive.cpp`:
- Around line 8-49: Update the four legacy ADC shims in the ESP32/ESP-IDF 5
block to match the exact ESP-IDF prototypes, including using adc_unit_t and
adc_channel_t for adc_i2s_mode_init, and correcting the parameter types and
return signatures of adc_set_i2s_data_source and adc1_lock_release. Preserve the
existing stub behavior and extern "C" linkage while including the appropriate
ADC declarations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 22eb3a87-98bf-48c2-a980-23247ed26e1a
📒 Files selected for processing (3)
platformio.iniusermods/audioreactive/audio_reactive.cppwled00/util.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- platformio.ini
- wled00/util.cpp
| #endif | ||
|
|
||
| // AI: below section was generated by an AI | ||
| #if defined(CONFIG_IDF_TARGET_ESP32) && defined(ARDUINO_ARCH_ESP32) && (ESP_IDF_VERSION_MAJOR >= 5) | ||
| /* | ||
| * Bootloop workaround for classic ESP32 on esp-idf 5.x (WLED V5): | ||
| * AR still uses the legacy I2S driver (driver/i2s.h) which pulls in the legacy ADC driver. | ||
| * This causes a "abort" on boot as IDF V5 does not allow the legacy ADC driver to be used | ||
| * if any other function (i.e. Arduino's analogRead()) pulls in the new driver. | ||
| * | ||
| * The no-op stubs below satisfy exactly the symbols i2s_legacy.c.obj needs, so the real | ||
| * adc_i2s_deprecated.c.obj / adc_legacy.c.obj are never linked. This is safe because on IDF >= 5 | ||
| * the analog built-in-ADC mic class (I2SAdcSource) is compiled out anyway (see audio_source.h), | ||
| * so these paths are never executed; digital I2S/PDM mics don't use them. | ||
| * Delete this block once the usermod is ported to the new I2S driver (driver/i2s_std.h). | ||
| */ | ||
| #include <esp_err.h> | ||
|
|
||
| // Stub: would configure the (removed) I2S built-in ADC mode. Never called for digital mics. | ||
| extern "C" esp_err_t adc_i2s_mode_init(int adc_unit, int channel) { | ||
| (void)adc_unit; (void)channel; | ||
| return ESP_ERR_NOT_SUPPORTED; | ||
| } | ||
|
|
||
| // Stub: would select the ADC data source for I2S DMA. Never called for digital mics. | ||
| extern "C" esp_err_t adc_set_i2s_data_source(int adc_unit, int pattern) { | ||
| (void)adc_unit; (void)pattern; | ||
| return ESP_ERR_NOT_SUPPORTED; | ||
| } | ||
|
|
||
| // Stub: would acquire ADC1 for DMA (built-in ADC mode only). Report failure if ever called. | ||
| extern "C" esp_err_t adc1_dma_mode_acquire(void) { | ||
| return ESP_ERR_NOT_SUPPORTED; | ||
| } | ||
|
|
||
| // Stub: would release the legacy ADC1 lock. No lock is ever taken, so nothing to do. | ||
| extern "C" esp_err_t adc1_lock_release(int adc_unit) { | ||
| (void)adc_unit; | ||
| return ESP_OK; | ||
| } | ||
| #endif | ||
| // AI: end |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline usermods/audioreactive/audio_reactive.cpp --view expanded || true
echo
echo "== relevant references in repo =="
rg -n "adc_i2s_mode_init|adc_set_i2s_data_source|adc1_dma_mode_acquire|adc1_lock_release|adc_i2s_source_t" usermods/audioreactive docs -S || true
echo
echo "== local snippet around stubs =="
sed -n '1,70p' usermods/audioreactive/audio_reactive.cppRepository: wled/WLED
Length of output: 3928
🌐 Web query:
ESP-IDF adc_i2s_mode_init adc1_dma_mode_acquire adc1_lock_release adc_set_i2s_data_source signatures
💡 Result:
The requested functions are part of the deprecated legacy ADC driver in ESP-IDF [1]. Because these APIs are deprecated, they have been removed from modern versions of the framework in favor of the newer ADC Continuous Read (DMA) driver [2][1]. The function signatures for these legacy APIs are: 1. esp_err_t adc_i2s_mode_init(adc_unit_t adc_unit, adc_channel_t channel); This function was used to initialize I2S ADC mode [3][4]. 2. esp_err_t adc1_dma_mode_acquire(void); This function was used by I2S DMA to claim exclusive access to ADC1, preventing other tasks from using it [5][6]. 3. esp_err_t adc1_lock_release(void); This function was used to release the ADC1 lock, allowing other tasks to use the peripheral [6][7]. Note: A separate, non-deprecated function named adc_lock_release(adc_unit_t adc_unit) exists in more recent ESP-IDF versions as part of the hardware control support [8]. 4. esp_err_t adc_set_i2s_data_source(adc_i2s_source_t src); This function was used to set the source for I2S DMA data (e.g., from GPIO or ADC) [9][10]. If you are developing for modern ESP-IDF versions, you should migrate to the ADC Continuous Read driver using the headers found in esp_adc/adc_continuous.h [2][1].
Citations:
- 1: https://docs.espressif.com/projects/esp-idf/en/latest/esp32/migration-guides/release-5.x/5.0/peripherals.html?highlight=i2c
- 2: https://github.com/espressif/esp-idf/blob/v5.3.5/docs/en/api-reference/peripherals/adc_continuous.rst
- 3: https://docs.espressif.com/projects/esp-idf/en/v4.4.8/esp32/api-reference/peripherals/adc.html
- 4: https://github.com/espressif/esp-idf/blob/v4.3.7/components/driver/esp32/include/driver/adc.h
- 5: https://sourcevu.sysprogs.com/espressif/esp-idf/symbols/adc1_dma_mode_acquire
- 6: https://sourcevu.sysprogs.com/espressif/esp-idf/files/components/driver/deprecated/adc1_private.h
- 7: https://sourcevu.sysprogs.com/espressif/esp-idf/symbols/adc1_lock_release
- 8: https://github.com/espressif/esp-idf/blob/12f36a02/components/esp_hw_support/include/esp_private/adc_share_hw_ctrl.h
- 9: https://github.com/espressif/esp-idf/blob/a9d0f22193a/components/driver/include/driver/adc.h
- 10: https://docs.espressif.com/projects/esp-idf/en/v4.2.5/esp32/api-reference/peripherals/adc.html
Match the legacy ADC stub signatures exactly
adc_set_i2s_data_source and adc1_lock_release don’t match the ESP-IDF declarations, and adc_i2s_mode_init should use the real adc_unit_t / adc_channel_t types. These extern "C" shims should mirror the upstream prototypes exactly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@usermods/audioreactive/audio_reactive.cpp` around lines 8 - 49, Update the
four legacy ADC shims in the ESP32/ESP-IDF 5 block to match the exact ESP-IDF
prototypes, including using adc_unit_t and adc_channel_t for adc_i2s_mode_init,
and correcting the parameter types and return signatures of
adc_set_i2s_data_source and adc1_lock_release. Preserve the existing stub
behavior and extern "C" linkage while including the appropriate ADC
declarations.
Source: Path instructions
| #endif | ||
|
|
||
| // AI: below section was generated by an AI | ||
| #if defined(CONFIG_IDF_TARGET_ESP32) && defined(ARDUINO_ARCH_ESP32) && (ESP_IDF_VERSION_MAJOR >= 5) |
There was a problem hiding this comment.
can be shortened a bit, because IDF_TARGET_ESP32 implies ARDUINO_ARCH_ESP32
#if defined(CONFIG_IDF_TARGET_ESP32) && (ESP_IDF_VERSION_MAJOR > 4)
@DedeHai thanks, I'll check on my boards. I can do a quick test today, and tomorrow test with real I2S and PDM hardware. Edit: first test without mic: SUCCESS (esp32 boot complete) |
DedeHai
changed the title
5 participants
C3 and S3 would just bootloop. There are two issues:
edit: adding
-march=rv32imcis not needed, it was related to local PIO breaking when switching from V4 to V5 without deleting the .pio folder:-march=rv32imcforces the compiler to use software "atomic" -> no more "invalid instruction"I also removed the stray "-DCO" flags that have no meaning and are probably truncated
DCORE_DEBUG_LEVELflagsedit: also fixes bootloop on ESP32 when using AR
Summary by CodeRabbit
Summary by CodeRabbit