-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Backport version reporting (PR #5093 and #5111) to 0.15.x #5126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the WalkthroughThis PR backports device identification and bootloader integrity features. It adds platform-specific device fingerprinting via SHA1-based IDs, bootloader SHA256 hashing for ESP32, and expands JSON info serialization with device and repository identifiers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Co-authored-by: netmindz <[email protected]>
Co-authored-by: netmindz <[email protected]>
Co-authored-by: netmindz <[email protected]>
|
@netmindz if you also want the bootloader hash reported, then |
|
@copilot we also need to cherry-pick calculateBootloaderSHA256() from the main branch |
Co-authored-by: netmindz <[email protected]>
Added |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wled00/ota_update.cpp (1)
6-8: ESP32 bootloader offset handling looks correct across variantsThe new
BOOTLOADER_OFFSETselection viaCONFIG_IDF_TARGET_*should fix the earlier S3/C3 mis-hash issue while keeping legacy ESP32/ESP32-S2 at the traditional 0x1000 offset. UsingESP32/CONFIG_IDF_TARGET_* here is consistent with established patterns for variant detection. One small follow-up: the later comment insidecalculateBootloaderSHA256()still claims a fixed 0x1000 offset; consider updating it to reflect the newBOOTLOADER_OFFSETlogic.Also applies to: 15-22
🧹 Nitpick comments (3)
wled00/json.cpp (1)
2-2: JSON info extensions are wired correctly and respect feature guards
- Including
ota_update.his appropriate given the newbootloaderSHA256field and matches the ESP32+OTA guard aroundgetBootloaderSHA256Hex().- Adding
"repo"(repoString) and"deviceId"(getDeviceId()) toserializeInfo()cleanly extends the info schema without breaking existing consumers.- On ESP32,
"bootloaderSHA256"is only emitted when OTA is enabled, which is consistent with the implementation being compiled under the same guard.The deviceId is derived from a salted, hashed fingerprint and is used only for version/upgrade statistics, which aligns with the intended low-privacy-risk usage. Based on learnings, this is acceptable for the opt‑in reporting use case.
Also applies to: 635-637, 740-761
wled00/ota_update.cpp (1)
270-321: Bootloader SHA256 computation is sound, but read errors are silently ignoredThe hashing flow (fixed-size region, 256‑byte chunks, mbedTLS SHA256, cached hex string) is reasonable and guarded correctly by
ARDUINO_ARCH_ESP32 && !WLED_DISABLE_OTA. Two potential improvements:
- If
esp_flash_read/spi_flash_readfails for a chunk, the loop still proceeds and a hash is emitted; that could mask partial‑read issues. Consider bailing out (and leaving the cache empty) or logging an explicit error when a read fails.- The
0x8000bootloader size is currently a magic number; a namedconstexpr size_t BOOTLOADER_SIZE = 0x8000;would make intent clearer and simplify future adjustments.wled00/util.cpp (1)
6-6: Device ID hashing flow is consistent and privacy‑aware
computeSHA1correctly normalizes the SHA1 implementation across ESP8266 (Hash.h) and ESP32 (mbedTLS), returning a 40‑char hex string.getDeviceId()’s “double‑hash and append last 2 chars” approach yields a deterministic 42‑character ID while making it non‑trivial to recover the underlying fingerprint or MAC.- Caching the result in a static
Stringavoids repeated hashing overhead for frequent JSON info requests.Given this ID is only used for opt‑in version/upgrade statistics and not behavioral tracking, the MAC+salt‑based design is aligned with the intended low‑risk usage. Based on learnings, the privacy trade‑off looks acceptable.
Also applies to: 14-16, 752-776, 826-845
📜 Review details
Configuration used: CodeRabbit 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 (6)
.gitignore(1 hunks)wled00/fcn_declare.h(1 hunks)wled00/json.cpp(3 hunks)wled00/ota_update.cpp(2 hunks)wled00/ota_update.h(1 hunks)wled00/util.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/json.cppwled00/ota_update.cppwled00/util.cpp
wled00/**/!(html_*)*.h
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for non-generated C++ header files (.h)
Files:
wled00/ota_update.hwled00/fcn_declare.h
🧠 Learnings (21)
📓 Common learnings
Learnt from: netmindz
Repo: wled/WLED PR: 5093
File: wled00/util.cpp:1159-1182
Timestamp: 2025-11-20T00:04:04.829Z
Learning: In WLED PR #5093, the deviceId feature is designed for opt-in usage reporting that tracks only version/upgrade information (non-behavioral data), not user activity patterns. The deterministic salt approach (MAC + "WLED" + chip model/revision) is acceptable for this limited use case, as correlating MAC addresses to version history represents minimal privacy risk compared to behavioral tracking.
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated embedded web header files (wled00/html_*.h)
Applied to files:
.gitignorewled00/json.cpp
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : Always commit generated html_*.h files along with related source changes
Applied to files:
.gitignore
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/**/!(html_*)*.h : Use 2-space indentation for non-generated C++ header files (.h)
Applied to files:
.gitignore
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/**/*.cpp : Use 2-space indentation for C++ source files (.cpp)
Applied to files:
.gitignore
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/** : For web UI changes, edit files only under wled00/data (not firmware or generated files)
Applied to files:
.gitignore
📚 Learning: 2025-08-29T00:26:15.808Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: WLED uses a vendored ArduinoJson library (version 6) located at "src/dependencies/json/ArduinoJson-v6.h" which is included through wled.h. Usermods should not directly include ArduinoJson headers but instead rely on wled.h for ArduinoJson symbols. The standard pattern is to include wled.h and use JsonObject, JsonArray, DynamicJsonDocument, etc. without additional includes.
Applied to files:
.gitignorewled00/json.cpp
📚 Learning: 2025-08-29T00:26:15.808Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: In WLED projects, ArduinoJson.h is not directly included via #include <ArduinoJson.h> - the ArduinoJson symbols are made available through the WLED build system and wled.h transitive includes, so explicitly adding #include <ArduinoJson.h> is not necessary and may not work.
Applied to files:
wled00/json.cpp
📚 Learning: 2025-10-11T17:56:24.923Z
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.
Applied to files:
wled00/json.cppwled00/ota_update.hwled00/ota_update.cpp
📚 Learning: 2025-11-20T00:04:04.829Z
Learnt from: netmindz
Repo: wled/WLED PR: 5093
File: wled00/util.cpp:1159-1182
Timestamp: 2025-11-20T00:04:04.829Z
Learning: In WLED PR #5093, the deviceId feature is designed for opt-in usage reporting that tracks only version/upgrade information (non-behavioral data), not user activity patterns. The deterministic salt approach (MAC + "WLED" + chip model/revision) is acceptable for this limited use case, as correlating MAC addresses to version history represents minimal privacy risk compared to behavioral tracking.
Applied to files:
wled00/json.cppwled00/fcn_declare.hwled00/util.cpp
📚 Learning: 2025-05-26T16:09:34.325Z
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.
Applied to files:
wled00/json.cppwled00/ota_update.hwled00/ota_update.cpp
📚 Learning: 2025-11-22T20:33:14.800Z
Learnt from: softhack007
Repo: wled/WLED PR: 5107
File: boards/adafruit_matrixportal_esp32s3.json:1-66
Timestamp: 2025-11-22T20:33:14.800Z
Learning: In WLED, board JSON files in the boards/ directory may reference partition files that don't exist or have slightly different names than actual files in tools/. This is intentional because platformio.ini or platformio_override.sample.ini explicitly override the partition file using the board_build.partitions setting, which takes precedence over the board JSON partition reference. This pattern exists in multiple local board definitions and should not be flagged as an issue.
Applied to files:
wled00/json.cpp
📚 Learning: 2025-11-16T19:40:46.260Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
Applied to files:
wled00/json.cpp
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/json.cpp
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: 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. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
wled00/json.cppwled00/util.cpp
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/json.cpp
📚 Learning: 2025-04-26T19:19:07.600Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
Applied to files:
wled00/json.cpp
📚 Learning: 2025-02-19T12:43:34.199Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Applied to files:
wled00/ota_update.cpp
📚 Learning: 2025-09-02T02:27:39.220Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHI.S:8-16
Timestamp: 2025-09-02T02:27:39.220Z
Learning: The ESP32 macro is defined when compiling code on all ESP32 variants (ESP32, ESP32-S2, ESP32-S3, ESP32-C3, etc.) in the Arduino framework, providing consistent library compatibility across the entire ESP32 family. For target-specific detection, CONFIG_IDF_TARGET_* macros should be used instead.
Applied to files:
wled00/ota_update.cppwled00/util.cpp
📚 Learning: 2025-09-18T02:59:33.666Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4928
File: wled00/FX_fcn.cpp:1181-1181
Timestamp: 2025-09-18T02:59:33.666Z
Learning: In WLED, getFreeHeapSize() is a platform-agnostic function defined in fcn_declare.h that provides accurate free heap reporting across ESP32 and ESP8266 platforms. On ESP32, it uses heap_caps_get_free_size(MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT) instead of ESP.getFreeHeap() because the latter can include other memory types. On ESP8266, it uses ESP.getFreeHeap().
Applied to files:
wled00/ota_update.cpp
📚 Learning: 2025-08-29T01:34:34.358Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4853
File: wled00/util.cpp:779-781
Timestamp: 2025-08-29T01:34:34.358Z
Learning: On ESP8266 systems, avoid adding no-op stub functions across translation units due to limited code memory constraints, as the compiler cannot inline away the function calls, resulting in wasteful memory usage.
Applied to files:
wled00/ota_update.cppwled00/util.cpp
🧬 Code graph analysis (3)
wled00/json.cpp (2)
wled00/util.cpp (2)
getDeviceId(828-846)getDeviceId(828-828)wled00/ota_update.cpp (2)
getBootloaderSHA256Hex(313-316)getBootloaderSHA256Hex(313-313)
wled00/ota_update.h (1)
wled00/ota_update.cpp (5)
calculateBootloaderSHA256(274-274)getBootloaderSHA256Hex(313-316)getBootloaderSHA256Hex(313-313)invalidateBootloaderSHA256Cache(319-321)invalidateBootloaderSHA256Cache(319-319)
wled00/fcn_declare.h (1)
wled00/util.cpp (4)
computeSHA1(753-776)computeSHA1(753-753)getDeviceId(828-846)getDeviceId(828-828)
🔇 Additional comments (4)
.gitignore (1)
26-26: Ignoring CodeQL working directory is fineAdding
_codeql_detected_source_rootto.gitignoreis appropriate to keep CodeQL analysis artifacts out of the repo.wled00/fcn_declare.h (1)
403-404: Device ID/SHA1 declarations match implementations
computeSHA1(const String&)andgetDeviceId()are correctly declared here and align with the new definitions inutil.cpp, making them safely usable across the codebase.wled00/ota_update.h (1)
54-72: Bootloader SHA256 public API is cleanly defined and correctly guardedThe three new ESP32-only OTA functions are well-documented, match their implementations in
ota_update.cpp, and are properly hidden whenWLED_DISABLE_OTAis defined.wled00/util.cpp (1)
778-824: The esp_adc_cal_characteristics_t fieldslow_curveandhigh_curveexist in ESP-IDF 3.3.5 and all newer versions; code is safe as-is.The review comment's concern about these fields not existing in older ESP-IDF versions is unfounded. The
low_curveandhigh_curvepointer fields are documented members ofesp_adc_cal_characteristics_tin ESP-IDF 3.3.5 (the minimum version supported via WLED's classic ESP32 platform with framework-arduinoespressif32 @ 1.0.6.4), and they remain present in all newer versions including IDF 4.4.x.The existing runtime NULL pointer checks (
if (ch.low_curve)andif (ch.high_curve)at lines 800 and 805) are the correct approach: these pointers are legitimately nullable when no lookup table correction is available, regardless of IDF version. No additional#if ESP_IDF_VERSIONguards are needed.
not needed yet, but will make maintenance easier in the future, and avoid confusion.
# Conflicts: # wled00/util.cpp
computeSHA1()function declaration tofcn_declare.hgetDeviceId()function declaration tofcn_declare.hdeviceIdfield toserializeInfo()injson.cpprepofield toserializeInfo()injson.cppcomputeSHA1()implementation toutil.cpputil.cppgenerateDeviceFingerprint()function for ESP32generateDeviceFingerprint()function for ESP8266getDeviceId()to use the new fingerprint functioncalculateBootloaderSHA256()function toota_update.cppgetBootloaderSHA256Hex()function toota_update.cppinvalidateBootloaderSHA256Cache()function toota_update.cppota_update.hbootloaderSHA256field to JSON info responsespi_flash_read()for compatibility with ESP-IDF v3Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.