Add deviceId #284
Add deviceId #284
Conversation
WalkthroughThe PR introduces SHA1 computation and device identification functionality. A platform-agnostic SHA1 API wraps native implementations for ESP8266 and ESP32. A device ID generator creates deterministic identifiers from MAC and platform-specific data, with caching. The generated device ID is now included in JSON serialization. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wled00/util.cpp (2)
678-702: SHA1 implementation is correct but ignores mbedTLS error codesThe ESP8266
sha1(input)delegation and ESP32 mbedTLS-based SHA1 with 20-byte buffer and 40-char hex output look correct and safe from buffer overruns. However, the_starts_ret,_update_ret, and_finish_retreturn values are ignored; if SHA1 were ever disabled or an internal error occurred, callers would silently get whatever happens to be inshaResult.Consider checking these return codes and either:
- Falling back to an empty string or a known constant on failure, or
- Asserting/logging an error in debug builds.
This would improve diagnosability without changing the public API.
704-730: EFUSE block reader is reasonable, but block-size assumption is implicitThe
dump_raw_blockhelper is lightweight and only used as extra entropy forgetDeviceId, but it assumes eachesp_efuse_block_tyou call it with exposes at least 256 bits. On SoCs where a block is smaller or laid out differently,esp_efuse_read_field_blobmay return an error and you silently fall back to an empty string.Given it’s only used for salting the device ID:
- This is acceptable behavior functionally (you still get a deterministic ID from MAC + chip info), but
- It would help to either document the 256‑bit assumption in a comment, or gate the EFUSE blocks per target if you ever add support for chips where some of
EFUSE_BLK0..3are not 256 bits.No changes are strictly required, but documenting the assumption will make future SoC bring‑ups easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wled00/fcn_declare.h(1 hunks)wled00/json.cpp(1 hunks)wled00/util.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/json.cppwled00/util.cppwled00/fcn_declare.h
🧠 Learnings (6)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
📚 Learning: 2025-11-14T13:55:44.372Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.372Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Applied to files:
wled00/util.cpp
📚 Learning: 2025-06-30T17:53:37.232Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 248
File: usermods/audioreactive/audio_reactive.h:199-200
Timestamp: 2025-06-30T17:53:37.232Z
Learning: In WLED AudioReactive usermod, when using ArduinoFFT (enabled via UM_AUDIOREACTIVE_USE_ARDUINO_FFT), it's acceptable and necessary to redefine the global sqrt macro to sqrtf within the conditional compilation block for performance optimization. This is a specific requirement for ArduinoFFT optimization that can provide 10-50% performance improvement on ESP32.
Applied to files:
wled00/util.cppwled00/fcn_declare.h
📚 Learning: 2025-07-02T23:22:57.175Z
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Applied to files:
wled00/util.cpp
📚 Learning: 2025-10-22T21:50:25.839Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Applied to files:
wled00/fcn_declare.h
📚 Learning: 2025-07-12T04:20:14.546Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Applied to files:
wled00/fcn_declare.h
🧬 Code graph analysis (2)
wled00/json.cpp (1)
wled00/util.cpp (2)
getDeviceId(735-767)getDeviceId(735-735)
wled00/fcn_declare.h (1)
wled00/util.cpp (4)
computeSHA1(679-702)computeSHA1(679-679)getDeviceId(735-767)getDeviceId(735-735)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (seeed_esp32c3_4MB_S)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp32S3_8MB_M)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32_16MB_V4_M)
🔇 Additional comments (4)
wled00/fcn_declare.h (1)
403-404: New utility declarations are consistent and well-placedExporting
computeSHA1(const String&)andgetDeviceId()here aligns with otherutil.cpphelpers; no signature or ordering issues spotted.wled00/json.cpp (1)
909-917: AddingdeviceIdto info JSON looks correctHooking
getDeviceId()intoserializeInfo()cleanly exposes a deterministic ID with minimal runtime cost (cached inutil.cpp). The only impact is a slightly larger info JSON, which should be acceptable given current usage.wled00/util.cpp (2)
4-10: Confirm header availability on all build targetsThe ESP8266 (
user_interface.h,<Hash.h>) and ESP32 (mbedtls/sha1.h,esp_efuse.h) includes are appropriate for Arduino ESP cores, but on non-ESP or “native” targets these headers may not exist. Please confirm all configured environments (including any WLED-Native builds) still compile, and if not, wrap the ESP32-specific includes with the same platform guards used elsewhere (e.g.ARDUINO_ARCH_ESP32/WLED_NATIVE) or provide a stub implementation.
733-767: Device ID generation and caching look solid
getDeviceId()builds a deterministic per-device string from MAC + platform-specific data (flash ID or chip model/revision + EFUSE blocks), hashes it twice with SHA1, and caches the 42‑character result. This achieves a stable ID across re-flashes without exposing the raw MAC or EFUSE contents, and the static cache keeps subsequent calls cheap.Behavior with failing EFUSE reads (empty salt) is still deterministic and acceptable for this use case.
|
@netmindz could we make this ID optional, maybe with a checkbox "provide unique device ID" in Wifi settings? |
|
@netmindz I've overlooked the fact that you are just adding something to /json/info - this endpoint was always reporting - among other technical data - the device specific MAC. So no need to opt-anything and stuff, because the doors for tracking were always wide open 🤣 ==> Forget my previous suggestions -> every WLED has always told the world who exactly it is, by telling the MAC to everyone who knocks on the door. So your PR actually adds a new option that's just a less obvious ID compared to using the MAC. |
softhack007
left a comment
softhack007
left a comment
There was a problem hiding this comment.
this is basically the MM port of wled#5093, right?
ok to merge 👍
I'll test it quicky in the next days, should be simple to fix problems if something comes up.
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.