Fix bootloops, remove stray flag#5749
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughESP32 build environments remove stray flags and add target-specific USB-mode defines. Device fingerprint generation preserves ADC-derived entropy across ESP-IDF versions, while audioreactive adds ESP-IDF 5.x 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
@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
|
after a lot of trial and error I found a solution to preserve the device ID's on all platforms. It is quite an elaborate solution to dance around the different APIs but it works, I got the same ID on all devices I tested (about 8 of all varieties ESP32, S2, S3, C3). I do not have an S2 with the "version 1" ADC calibration so could not test that but I think it is implemented correctly, worst case we get a few S2 that change IDs. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wled00/util.cpp (1)
1455-1455: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake
get_calib_entropystatic and rename to camelCase.The sibling
getLegacyAdcCoeffs()helpers all have internal linkage (static), butget_calib_entropy()is a non-staticfree function, giving it external linkage and risking name collisions across translation units. The name also uses snake_case.♻️ Suggested change
-uint32_t get_calib_entropy(void) { +static uint32_t getCalibEntropy() {Update the call site at Line 1518 accordingly.
As per coding guidelines: "Use camelCase for C++ functions and variables" and "use static for methods not accessing instance state".
🤖 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 `@wled00/util.cpp` at line 1455, Update get_calib_entropy to a static camelCase helper named getCalibEntropy, giving it internal linkage and matching the surrounding getLegacyAdcCoeffs() helpers. Update its call site near the indicated location to use the renamed function.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@wled00/util.cpp`:
- Line 1455: Update get_calib_entropy to a static camelCase helper named
getCalibEntropy, giving it internal linkage and matching the surrounding
getLegacyAdcCoeffs() helpers. Update its call site near the indicated location
to use the renamed function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a15e8f0-0a00-445c-b553-c950107517d9
📒 Files selected for processing (2)
usermods/audioreactive/audio_reactive.cppwled00/util.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- usermods/audioreactive/audio_reactive.cpp
|
@willmmiles after a long debug session I found the new compiler version is quite a bit different: I had flickering issues on BB output and it turned out that the IRAM function was inlined and put into flash. using |
Yes and no. In an "is the design future proof" sense, then yes: doing our own low-level efuse reads and making whatever hash suits us best from it is guaranteed to be stable. In an "is the code future proof" sense, I don't think ESP-IDF guarantees stability of the macro APIs for doing low-level access, so there'll always be the possibility that the next IDF version means we have to track down the new macro names. From a code wrangling point of view, I'd say it's OK for now -- later we'll pull it in to the HAL.
Oh, that'd do it! Yes, IRAM and inlining have a bit of a history -- if I recall correctly, NPB has some comments in places about various less-than-ideal workarounds to get the right code in the right places. That said, outside of our custom hardware drivers, we're usiing IRAM mostly as a performance tuning tool, and realistically we're going to end up re-doing a lot of the performance tuning for the new compiler anyways. In most places, I expect that if the compiler can inline functions, it'll often be faster even if that code doesn't end up in IRAM. The inlined code will be loaded from flash along with the rest of the surrounding function, so it'll "effectively" be IRAM but without the function call overhead .... right up to when the calling function gets to be so big it spans flash segments. So "often" is doing all the heavy lifting there -- we'll have to benchmark things. (This does point towards dropping the V4 envs sooner rather than later - trying to tune for both compiler versions might be tricky.) |
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