Validate gzipped firmware (on ESP8266)#5550
Open
willmmiles wants to merge 1 commit intowled:mainfrom
Open
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
WalkthroughThis PR adds optional gzipped-OTA support for ESP8266 by introducing the uzlib library dependency, a new JSON lock identifier, gzip detection via magic bytes, and decompression/validation logic that runs during upload. A tri-state result enum replaces the boolean return to handle cases where validation is deferred pending lock availability. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as OTA Handler<br/>(wled_server.cpp)
participant Flash as Flash/Memory
participant Decoder as gzip Decoder<br/>(unzipFlash)
participant Validator as OTA Validator<br/>(validateGzippedOTA)
Client->>Handler: POST /update (gzipped data)
Handler->>Flash: Write chunk to flash
Handler->>Decoder: Detect magic bytes & decompress
alt Gzip detected
Decoder->>Flash: Read compressed data
Decoder->>Flash: Decompress to temp buffer
Decoder->>Validator: Pass decompressed metadata
Validator->>Validator: validateOTA (check compatibility)
alt Validation passes
Validator->>Flash: Write final validated chunk
Validator-->>Handler: releaseCheckPassed = true
else Validation pending (JSON lock unavailable)
Handler-->>Client: TryAgain (defer response)
Client->>Handler: Retry request
else Validation fails
Handler-->>Client: Error response
end
else No gzip
Handler-->>Validator: Standard OTA flow
end
alt All chunks complete & validated
Handler-->>Client: Ready + reboot success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platformio.ini (1)
219-255:⚠️ Potential issue | 🟠 MajorPlease get maintainer sign-off on the shared ESP8266 dependency change.
This updates the global ESP8266 dependency set for all inheriting environments, so it should not merge without explicit maintainer/WLED org approval. As per coding guidelines, "Modifications to
platformio.inimust be explicitly approved by a maintainer or WLED organization member, as changes to the global build environment may break GitHub Actions builds".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` around lines 219 - 255, The change updates shared ESP8266/global build deps (see symbols lib_deps, platform_compat, platform_packages_compat, lib_deps_compat in the diff) which affects all inheriting environments; do not merge this change until you obtain explicit maintainer/WLED org approval—either add a maintainer review request on the PR and document the required approval in the PR description, or revert these global changes and move them to a single environment-specific section (or a draft branch) until sign-off is granted.
🤖 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/ota_update.cpp`:
- Around line 181-190: The code marks context->uploadComplete = true regardless
of whether the deferred final gzip chunk was actually written; change this so
uploadComplete is only set after confirming Update.hasError() is false and
Update.write(...) returned the full buffer size (use the same const auto& buf =
context->releaseMetadataBuffer and the Update.write(...) call), and do not set
uploadComplete if Update.hasError() is true or the write short-wrote — this
ensures endOTA() (or any call path that runs Update.end(true)) only runs when
the final chunk was successfully written.
- Around line 102-149: The gzip reader callback gzip_flash_read_cb currently
advances past the uploaded image because s_gzipFlashSrc has no upper bound;
update the data model (e.g., add a field like flashEnd or flashSize to
s_gzipFlashSrc and set it in unzipFlash when called) and modify
gzip_flash_read_cb to stop reading once flashOffset reaches that limit (return
-1 / EOF), only read the remaining bytes up to the limit (adjust buffer fill
size accordingly), and set uncomp_ctx.source/ source_limit from the actual
remaining bytes so uzlib cannot read past the OTA image region.
---
Outside diff comments:
In `@platformio.ini`:
- Around line 219-255: The change updates shared ESP8266/global build deps (see
symbols lib_deps, platform_compat, platform_packages_compat, lib_deps_compat in
the diff) which affects all inheriting environments; do not merge this change
until you obtain explicit maintainer/WLED org approval—either add a maintainer
review request on the PR and document the required approval in the PR
description, or revert these global changes and move them to a single
environment-specific section (or a draft branch) until sign-off is granted.
🪄 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: a6a3bfca-9732-4788-94da-849af494c644
📒 Files selected for processing (5)
platformio.iniwled00/const.hwled00/ota_update.cppwled00/ota_update.hwled00/wled_server.cpp
Comment on lines
+181
to
+190
| // Write the final block | ||
| if (!Update.hasError()) { | ||
| const auto& buf = context->releaseMetadataBuffer; | ||
| if (Update.write(const_cast<uint8_t*>(buf.data()), buf.size()) != buf.size()) { | ||
| DEBUG_PRINTF_P(PSTR("OTA write failed on final chunk: %s\n"), Update.UPDATE_ERROR()); | ||
| } | ||
| } | ||
|
|
||
| DEBUG_PRINTLN(F("OTA Update End")); // for symmetry with the non-gzip path | ||
| context->uploadComplete = true; // Final block was passed to Update.write() |
Contributor
There was a problem hiding this comment.
Only mark the gzip upload complete after the deferred last chunk is actually written.
uploadComplete is set to true unconditionally here. If Update.hasError() was already set, or Update.write() short-writes the deferred final block, endOTA() still runs Update.end(true) as though the full image reached flash. That defeats the safety check around withholding the last block.
Suggested fix
- if (!Update.hasError()) {
+ bool finalChunkWritten = false;
+ if (!Update.hasError()) {
const auto& buf = context->releaseMetadataBuffer;
- if (Update.write(const_cast<uint8_t*>(buf.data()), buf.size()) != buf.size()) {
+ finalChunkWritten = (Update.write(const_cast<uint8_t*>(buf.data()), buf.size()) == buf.size());
+ if (!finalChunkWritten) {
DEBUG_PRINTF_P(PSTR("OTA write failed on final chunk: %s\n"), Update.UPDATE_ERROR());
}
}
@@
- context->uploadComplete = true; // Final block was passed to Update.write()
+ context->uploadComplete = finalChunkWritten;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Write the final block | |
| if (!Update.hasError()) { | |
| const auto& buf = context->releaseMetadataBuffer; | |
| if (Update.write(const_cast<uint8_t*>(buf.data()), buf.size()) != buf.size()) { | |
| DEBUG_PRINTF_P(PSTR("OTA write failed on final chunk: %s\n"), Update.UPDATE_ERROR()); | |
| } | |
| } | |
| DEBUG_PRINTLN(F("OTA Update End")); // for symmetry with the non-gzip path | |
| context->uploadComplete = true; // Final block was passed to Update.write() | |
| // Write the final block | |
| bool finalChunkWritten = false; | |
| if (!Update.hasError()) { | |
| const auto& buf = context->releaseMetadataBuffer; | |
| finalChunkWritten = (Update.write(const_cast<uint8_t*>(buf.data()), buf.size()) == buf.size()); | |
| if (!finalChunkWritten) { | |
| DEBUG_PRINTF_P(PSTR("OTA write failed on final chunk: %s\n"), Update.UPDATE_ERROR()); | |
| } | |
| } | |
| DEBUG_PRINTLN(F("OTA Update End")); // for symmetry with the non-gzip path | |
| context->uploadComplete = finalChunkWritten; // Final block was passed to Update.write() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/ota_update.cpp` around lines 181 - 190, The code marks
context->uploadComplete = true regardless of whether the deferred final gzip
chunk was actually written; change this so uploadComplete is only set after
confirming Update.hasError() is false and Update.write(...) returned the full
buffer size (use the same const auto& buf = context->releaseMetadataBuffer and
the Update.write(...) call), and do not set uploadComplete if Update.hasError()
is true or the write short-wrote — this ensures endOTA() (or any call path that
runs Update.end(true)) only runs when the final chunk was successfully written.
Member
Author
There was a problem hiding this comment.
This is not a correct analysis. uploadComplete merely tracks that all blocks were processed -- its responsible for managing the case where the upload size is not known prior to the update process beginning. If it were meant to mean "uploadSucessful", it would have been named such. The Update object keeps track of its own error status internally. You may observe that the existing handleOTAData function also sets uploadComplete regardless of the return value of Upload.write() on the final block. If the update process discovers an error with the last block, it will be handled in the if (Update.hasError()) branch.
Contributor
There was a problem hiding this comment.
@willmmiles, you're absolutely right — I apologize for the incorrect analysis. uploadComplete signals to Update.end() that all expected bytes were fed to the updater (i.e., the upload stream was complete), not that every individual write succeeded. The Update object tracks its own error state internally, and Update.end(false) would incorrectly signal an incomplete upload to the updater library rather than a write error. The existing handleOTAData path confirms this identical pattern. My suggested fix was wrong and would have broken the semantics.
✏️ Learnings added
Learnt from: willmmiles
Repo: wled/WLED PR: 5550
File: wled00/ota_update.cpp:181-190
Timestamp: 2026-04-29T12:37:33.066Z
Learning: In WLED's OTA update implementation (wled00/ota_update.cpp), `UpdateContext::uploadComplete` signals to `Update.end()` that all expected bytes of the upload stream were delivered to the updater — it does NOT mean the update succeeded. The `Update` (ESP8266/ESP32 Updater) object tracks its own internal error state; errors from individual `Update.write()` calls are surfaced via `Update.hasError()` and the return value of `Update.end()`. Setting `uploadComplete = true` unconditionally after the final chunk (regardless of `Update.write()` return value) is intentional and correct, consistent with the non-gzip path in `handleOTAData`. Do not flag this as a bug or suggest gating `uploadComplete` on the write return value.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: willmmiles
Repo: wled/WLED PR: 4998
File: wled00/ota_update.cpp:120-124
Timestamp: 2025-10-11T17:56:24.923Z
Learning: In the WLED OTA update implementation (wled00/ota_update.cpp), the parameter name for bypassing OTA validation is "skipValidation", not "ignoreRelease". The intent evolved from the original spec in issue `#4929` to disable all validation checks, not just release name checks, which is why the broader parameter name "skipValidation" is used.
Learnt from: blazoncek
Repo: wled/WLED PR: 4700
File: wled00/wled_server.cpp:409-414
Timestamp: 2025-05-26T16:09:34.325Z
Learning: In wled00/wled_server.cpp, the OTA update access control logic intentionally allows OTA updates from different subnets when otaSameSubnet is true AND a PIN is set. This was a conscious design decision by blazoncek to provide flexibility for remote OTA updates with PIN protection, though they acknowledged it may not be the optimal security model.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled.cpp:1037-1051
Timestamp: 2026-03-29T16:14:17.321Z
Learning: In WLED PR `#5048` (wled00/wled.cpp, CONFIG_IDF_TARGET_ESP32P4 block): `ESP_HostedOTA.h` and `updateEspHostedSlave()` are NOT in the git repository. They are provided by the pioarduino ESP32-P4 platform package (platform-espressif32.zip, referenced in platformio.ini [esp32p4] section). The header and function are part of the ESP-Hosted Wi-Fi co-processor framework integration bundled within the pioarduino framework for ESP32-P4, not a separate Arduino library. Do NOT flag `#include "ESP_HostedOTA.h"` or calls to `updateEspHostedSlave()` as missing/undefined for ESP32-P4 builds.
Learnt from: softhack007
Repo: wled/WLED PR: 5482
File: wled00/wled.h:579-579
Timestamp: 2026-04-08T21:23:19.258Z
Learning: In WLED PR `#5482` (wled00/wled.h), the `denyWsecUpload` flag MUST default to `false` (not `true`). Rationale confirmed via FMEA by softhack007:
1. Circular-dependency risk: `denyWsecUpload` is stored inside `wsec.json`; if that file is corrupted/deleted, `_INIT(true)` would permanently block the upload-recovery path without serial access.
2. Upgrade safety: `_INIT(true)` would silently break wsec.json upload recovery on all existing installations after a firmware upgrade.
3. Recovery parity: uploading wsec.json does not expose previous secrets; it is a legitimate recovery mechanism (e.g., after a flash wipe).
The flag is valuable as an optional hardening opt-in for installations that do not need the upload-recovery path. The real defence against malformed-JSON bootloops should be input validation at load time, not upload blocking.
Learnt from: wled-install
Repo: wled/WLED PR: 5426
File: wled00/safe_boot_functions.cpp:97-98
Timestamp: 2026-03-15T12:16:44.322Z
Learning: In wled00/safe_boot_functions.cpp (WLED SafeBootloader integration), the return values of `esp_partition_erase_range()` and `esp_partition_write()` in `update_ota_crc()` and `update_spiffs_crc()` are intentionally not checked. If these operations fail and an incorrect or corrupted CRC ends up stored, the SafeBootloader detects the CRC mismatch on boot and automatically restores data from the other (backup) partition. This is a deliberate design decision relying on the SafeBootloader's dual-partition recovery mechanism, consistent with the same reasoning applied to unchecked `esp_partition_read()` in `calc_crc()`.
Learnt from: softhack007
Repo: wled/WLED PR: 5482
File: wled00/wled.h:579-579
Timestamp: 2026-04-08T21:25:44.777Z
Learning: In WLED PR `#5482` (wled00/wled.h), the `denyWsecUpload` flag is confirmed to default to `false`. The complementary defence against malformed wsec.json content (bootloop/brick risk from malformed JSON) is handled by PR `#5484`, which hardens JSON file reading/parsing at load time. Do not suggest moving `denyWsecUpload` default to `true` or replacing JSON validation with upload blocking — both mechanisms serve different purposes and should coexist.
Learnt from: softhack007
Repo: wled/WLED PR: 5435
File: wled00/file.cpp:42-55
Timestamp: 2026-03-21T22:55:37.522Z
Learning: In WLED (wled00/file.cpp), `closeFile()` intentionally clears `doCloseFile = false` *before* the `strip.suspend()` / `strip.waitForLEDs()` / `f.close()` / `strip.resume()` sequence. This is the correct design: it prevents a concurrent or re-entrant call (via `if (doCloseFile) closeFile()` at lines 287 and 353) from attempting a double-close of the shared static `f` during the wait window. The inverse risk — another task reopening `f` via `WLED_FS.open()` while the wait is in progress — is not realistic because: (1) on ESP8266 (single-core), `delay(1)` only yields to lwIP/TCP callbacks which never call `writeObjectToFile`/`readObjectFromFile`; (2) on standard ESP32 without shared RMT, the wait block is not compiled so `f.close()` is immediate; (3) in WLED's architecture, file operations are dispatched from the main loop task only. Do not recommend moving the `doCloseFile = false` assignment to after `f.close()`.
Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:794-830
Timestamp: 2026-03-31T13:42:00.444Z
Learning: In WLED PR `#5456` (Matter over WiFi usermod, CMakeLists.txt + platformio.ini): The GCC 14 chip::to_underlying compatibility issue with CHIP SDK's TypeTraits.h is fixed by building in gnu++20 mode (not gnu++2b/gnu++23). CMakeLists.txt uses `idf_build_replace_option_from_property` to swap `-std=gnu++2b` for `-std=gnu++20` when the matter usermod is present. The `matter_gcc14_compat.h` shim file (which pre-defines `chip::to_underlying` and sets `CHIP_TO_UNDERLYING_DEFINED`) is dead code under this configuration — it is never included anywhere. TypeTraits.h's broken C++23 `using std::to_underlying` alias path is only taken in gnu++23 mode; in gnu++20 mode CHIP defines its own `chip::to_underlying` function template normally, so no shim is needed. Additionally, upstream connectedhomeip TypeTraits.h already has the fix built-in. ESP-IDF v5.5 uses GCC 14.2.0. Do NOT flag the missing `-include` for `matter_gcc14_compat.h` as a build issue.
Learnt from: netmindz
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-04-26T10:35:51.266Z
Learning: In WLED release notes for 16.0.0, the OTA upgrade risk for ESP32 devices first flashed with v0.13.x or earlier (old bootloader) should be described as a potential risk with uncertain outcome, NOT as a definitive failure. Do not include specific flash recovery instructions (e.g. `esptool erase_flash`) in release notes — there are multiple recovery options and it is too detailed. The recommended messaging is: users who cannot easily re-flash via USB should exercise caution until the community better understands the risks.
Learnt from: netmindz
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-04-26T10:35:51.245Z
Learning: In WLED release notes for 16.0.0, the OTA upgrade risk for ESP32 devices first flashed with v0.13.x or earlier (old bootloader) should be described as a potential risk with uncertain outcome, NOT as a definitive failure. Do not include specific flash recovery instructions (e.g. `esptool erase_flash`) in release notes — there are multiple recovery options and it is too detailed. The recommended messaging is: users who cannot easily re-flash via USB should exercise caution until the community better understands the risks.
Learnt from: netmindz
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-04-26T10:07:33.454Z
Learning: In WLED, the OTA upgrade compatibility for classic ESP32 to 16.0.0 depends on which version was originally first-flashed onto the device, NOT the last installed version. WLED has been using the ESP32 IDF v4 bootloader since at least v0.14.x in its official binaries. Therefore: only devices whose first-ever flash was WLED v0.13.x or earlier carry the old IDF v3.3.x bootloader and require a full `esptool erase_flash` before installing 16.0.0. Any ESP32 device originally flashed with WLED v0.14.0 or later can OTA update directly to 16.0.0 without erasing. The `[esp32_idf_V4]` experimental environments in platformio.ini (which existed from v0.14.x) were separate self-compile-only targets and NOT used for official binary releases; the official `WLED_x.x.x_ESP32.bin` releases from 0.14.x onward already embedded the IDF v4 bootloader.
Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:794-830
Timestamp: 2026-03-31T13:42:00.444Z
Learning: In WLED PR `#5456` (Matter over WiFi usermod, CMakeLists.txt + platformio.ini): The GCC 14 chip::to_underlying compatibility issue with CHIP SDK's TypeTraits.h is fixed by building in gnu++20 mode (not gnu++2b/gnu++23). CMakeLists.txt uses `idf_build_replace_option_from_property` to swap `-std=gnu++2b` for `-std=gnu++20` when the matter usermod is present. The `matter_gcc14_compat.h` shim file (which pre-defines `chip::to_underlying` and sets `CHIP_TO_UNDERLYING_DEFINED`) is dead code under this configuration — it is never included anywhere. TypeTraits.h's broken C++23 `using std::to_underlying` alias path is only taken in gnu++23 mode; in gnu++20 mode CHIP defines its own `chip::to_underlying` function template normally, so no shim is needed. Do NOT flag the missing `-include` for `matter_gcc14_compat.h` as a build issue.
Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:173-180
Timestamp: 2025-09-02T01:56:43.841Z
Learning: willmmiles prefers to maintain consistency with upstream NeoPixelBus patterns (like unchecked malloc in construct() methods) rather than diverging until improvements are made upstream first, to minimize maintenance burden and keep the codebase aligned.
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Learnt from: softhack007
Repo: wled/WLED PR: 4893
File: wled00/set.cpp:95-95
Timestamp: 2026-03-14T20:56:46.543Z
Learning: Guideline: Ensure WiFi hostname is set after WiFi.mode() but before WiFi.begin() to avoid default esp-XXXXXX hostname being used in DHCP. This ordering only takes effect after the STA interface exists (so avoid placing hostname setting before initConnection). In WLED, place the hostname configuration inside initConnection() (after WiFi.disconnect(true) and before WiFi.begin()) rather than in earlier boot code like deserializeConfig(). This rule should be applied in code reviews for WLED’s network initialization paths in wled00/*.cpp, and note that on ESP8266 the ordering is less strict but still desirable for consistency.
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:45.980Z
Learning: When calling raw lwIP APIs (e.g., around `ntpUdp.begin()` or any `lwIP`/`tcpip`-layer call) in this codebase on ESP32 Arduino-ESP32 platforms where core locking/checking is enabled, wrap the lwIP call with `LOCK_TCPIP_CORE()` / `UNLOCK_TCPIP_CORE()` from `lwip/tcpip.h`. This prevents thread-safety/core-violation crashes without requiring `sdkconfig` changes.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:45.980Z
Learning: When using lwIP “raw” APIs in WLED on ESP32 (Arduino-ESP32 / IDF 5.5+), don’t rely on LOCK_TCPIP_CORE()/UNLOCK_TCPIP_CORE() unless CONFIG_LWIP_TCPIP_CORE_LOCKING=y is guaranteed. CONFIG_LWIP_CHECK_THREAD_SAFETY=y can trigger the assertion “Required to lock TCPIP core functionality!” when raw lwIP calls occur off the TCPIP thread. If the locking mode isn’t enabled (or can’t be changed via sdkconfig), schedule lwIP work (e.g., ntpUdp.begin() and similar raw lwIP calls) via tcpip_callback() so it runs on the TCPIP thread; this should work regardless of the locking-mode setting. Review any similar raw lwIP usage for correct thread/context handling.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled.cpp:698-700
Timestamp: 2026-03-28T01:37:15.541Z
Learning: In this WLED codebase, when using `DEBUG_PRINTLN(F("..."))`, an explicit trailing `\n` inside the `F("...")` string (e.g., `DEBUG_PRINTLN(F("Warning!\n"))`) may be intentional to create a blank line in debug output as a visual separator. During code review, do not automatically flag these as “double newline” or recommend removing the `\n`—first verify with the author/context that the extra newline is deliberate.
Learnt from: willmmiles
Repo: wled/WLED PR: 5462
File: wled00/json.cpp:1189-1198
Timestamp: 2026-03-30T15:32:02.808Z
Learning: When working with WLED strings stored in PROGMEM on ESP8266/ESP32, assume you can’t use `strchr_P` for character scanning. If you need to scan a PROGMEM string for a character (e.g., searching for '@' within effect data), review changes to ensure they perform an explicit RAM copy first (e.g., via `strncpy_P` into a buffer) and then scan in RAM. Avoid proposing incremental/streaming PROGMEM character-by-character scans without a RAM buffer on these targets, since it’s impractical with the available APIs.
Learnt from: DedeHai
Repo: wled/WLED PR: 5464
File: wled00/FX_fcn.cpp:1699-1701
Timestamp: 2026-04-09T07:26:09.816Z
Learning: In WLED’s WLED code, if a pixel/buffer allocation uses `BFRALLOC_NOBYTEACCESS` (and especially on classic ESP32 where byte-level access to IRAM-resident buffers is unsafe), avoid using byte-wise operations like `memset`/`memcpy` on that buffer. Specifically, do not combine `BFRALLOC_CLEAR | BFRALLOC_NOBYTEACCESS` and do not perform `memcpy`/`memset` over `Segment::pixels` (e.g., in `setGeometry()`/`finalizeInit()` or copy ctor/assignment). Instead, use element-wise 32-bit access (loop over `uint32_t*` and assign/copy per element) to ensure 32-bit access only.
Learnt from: DedeHai
Repo: wled/WLED PR: 5464
File: wled00/FX_fcn.cpp:1699-1701
Timestamp: 2026-04-09T09:14:29.218Z
Learning: In WLED, when reviewing ESP32 code that uses pixel buffers allocated with `BFRALLOC_NOBYTEACCESS` (e.g., `WS2812FX::_pixels` and `Segment::pixels`), treat `memset`/`memcpy` on those buffers as an intentionally low-risk, accepted pattern. These buffers are designed to be IRAM-safe for 32-bit-only access, and the Xtensa newlib assembly implementations perform 32-bit word stores for word-aligned, word-multiple-sized buffers (so the IRAM constraint is satisfied in practice). Do not flag `memset`/`memcpy` on `BFRALLOC_NOBYTEACCESS` pixel buffers as a critical bug; if any concern arises, downgrade severity to low and focus on potential future regressions rather than an immediate correctness violation.
Learnt from: softhack007
Repo: wled/WLED PR: 5508
File: wled00/FX.cpp:4137-4142
Timestamp: 2026-04-16T09:49:58.587Z
Learning: In WLED effect/overlay implementations (C++ under wled00), ensure effects operate through the SEGMENT.* read/write APIs for their own pixels/overlay state. Reserve global frame reads/writes like strip.getPixelColor* for copy_* style post-processing only. Do not manually composite/blend across lower layers inside the effect; segment-to-segment blending is handled later by WS2812FX::blendSegment(), so effects should produce their per-segment contributions without trying to composite with underlying layers themselves.
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
2 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.
Add support for validating gzipped firmware files by borrowing the JSON buffer for decompression. If a gzipped file is detected, the validation process waits for the upload to complete, then validates the data from flash. As a hedge, the last block is not written to flash until the validation succeeds.
The bad news is that the decompression library must be brought in -- it's not (yet) used anywhere else in our firmware. This comes at the cost of ~3kb of flash. :(
Fixes #5538.
Summary by CodeRabbit
New Features
Improvements