Fix bootloops, remove stray flag, rescue DeviceID and audioreactive V5 builds#5749
Fix bootloops, remove stray flag, rescue DeviceID and audioreactive V5 builds#5749DedeHai wants to merge 6 commits into
Conversation
|
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: 4 (Complex) | ~40 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches⚔️ Resolve merge conflicts
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 |
|
@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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@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.) |
That's a very interesting one, well spotted 😃. I was always wondering what happens if the optimizer runs into "inline this function" together with "but put it into the IRAM section", Now we know - it seems that inlining "wins". The new compiler has LTO (link-time optimization) enabled by default, so i'd expect that a lot more functions will become candidates for inlining, because the link-time optimizer sees the whole program. |
|
Once things are running more smoothly we should definitely revisit optimisations. Regarding this PR: please test some of your devices to confirm ID at /json/info is the same in 16.0, I could only test a few. It should be merged asap. |
softhack007
changed the title
As you have tested and I trust your testing, confirmation by others is a nice to have not blocker to merge |
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