Safe uid generation #5111
Merged
Safe uid generation #5111
Conversation
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
Contributor
WalkthroughAdded a new public function Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
wled00/util.cpp (1)
1206-1225: Implement proper caching or remove thestatickeyword.The
cachedDeviceIdvariable (line 1209) is declared asstaticbut is never checked before recalculation. The function always recomputes the fingerprint and hashes, making the caching ineffective. Either implement proper caching by checking ifcachedDeviceIdis already populated, or remove thestatickeyword and rename the variable to reflect that it's not cached.Option 1: Implement proper caching (recommended for performance):
String getDeviceId() { static String cachedDeviceId = ""; + if (cachedDeviceId.length() > 0) return cachedDeviceId; + // The device string is deterministic as it needs to be consistent for the same device, even after a full flash erase // MAC is salted with other consistent device info to avoid rainbow table attacks. // If the MAC address is known by malicious actors, they could precompute SHA1 hashes to impersonate devices, // but as WLED developers are just looking at statistics and not authenticating devices, this is acceptable. // If the usage data was exfiltrated, you could not easily determine the MAC from the device ID without brute forcing SHA1 String firstHash = computeSHA1(generateDeviceFingerprint()); // Second hash: SHA1 of the first hash String secondHash = computeSHA1(firstHash); // Concatenate first hash + last 2 chars of second hash cachedDeviceId = firstHash + secondHash.substring(38); return cachedDeviceId; }Option 2: Remove caching if not needed:
String getDeviceId() { - static String cachedDeviceId = ""; // The device string is deterministic as it needs to be consistent for the same device, even after a full flash erase // MAC is salted with other consistent device info to avoid rainbow table attacks. // If the MAC address is known by malicious actors, they could precompute SHA1 hashes to impersonate devices, // but as WLED developers are just looking at statistics and not authenticating devices, this is acceptable. // If the usage data was exfiltrated, you could not easily determine the MAC from the device ID without brute forcing SHA1 String firstHash = computeSHA1(generateDeviceFingerprint()); // Second hash: SHA1 of the first hash String secondHash = computeSHA1(firstHash); // Concatenate first hash + last 2 chars of second hash - cachedDeviceId = firstHash + secondHash.substring(38); + String deviceId = firstHash + secondHash.substring(38); - return cachedDeviceId; + return deviceId; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/util.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/util.cpp
🧠 Learnings (3)
📓 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.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
📚 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/util.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/util.cpp
⏰ 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). (16)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32S3_wroom2)
- GitHub Check: wled_build / Build Environments (esp32c3dev_qio)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
DedeHai
commented
Nov 25, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wled00/util.cpp (1)
1181-1181: Replace hardcoded curve size with a named constant.The hardcoded value
8for the ADC calibration curve size should be replaced with a defined constant for clarity and maintainability. This was flagged in the previous review but only the initialization issue was addressed.Based on coding guidelines, which emphasize replacing magic numbers with meaningful constants.
Apply this diff:
+#define ADC_CAL_CURVE_SIZE 8 + String generateDeviceFingerprint() { uint32_t fp[2] = {0, 0}; // create 64 bit fingerprint esp_chip_info_t chip_info; esp_chip_info(&chip_info); esp_efuse_mac_get_default((uint8_t*)fp); fp[1] ^= ESP.getFlashChipSize(); fp[0] ^= chip_info.full_revision | (chip_info.model << 16); // mix in ADC calibration data: esp_adc_cal_characteristics_t ch; #if SOC_ADC_MAX_BITWIDTH == 13 // S2 has 13 bit ADC #define BIT_WIDTH ADC_WIDTH_BIT_13 #else #define BIT_WIDTH ADC_WIDTH_BIT_12 #endif esp_adc_cal_characterize(ADC_UNIT_1, ADC_ATTEN_DB_11, BIT_WIDTH, 1100, &ch); fp[0] ^= ch.coeff_a; fp[1] ^= ch.coeff_b; if (ch.low_curve) { - for (int i = 0; i < 8; i++) { + for (int i = 0; i < ADC_CAL_CURVE_SIZE; i++) { fp[0] ^= ch.low_curve[i]; } } if (ch.high_curve) { - for (int i = 0; i < 8; i++) { + for (int i = 0; i < ADC_CAL_CURVE_SIZE; i++) { fp[1] ^= ch.high_curve[i]; } } char fp_string[17]; // 16 hex chars + null terminator sprintf(fp_string, "%08X%08X", fp[1], fp[0]); return String(fp_string); }Also applies to: 1186-1186
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/util.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/util.cpp
🧠 Learnings (14)
📓 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.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
📚 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/util.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/util.cpp
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Applied to files:
wled00/util.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/util.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/util.cpp
📚 Learning: 2025-08-21T00:00:17.196Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4859
File: usermods/udp_name_sync/udp_name_sync.cpp:62-70
Timestamp: 2025-08-21T00:00:17.196Z
Learning: In WLED codebase, UDP packet processing code must never rely on null termination being present on the wire and must always validate packet size first. The codebase already has some vulnerabilities where malformed data can crash the software, so avoiding introduction of new ones is a priority.
Applied to files:
wled00/util.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, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Applied to files:
wled00/util.cpp
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Applied to files:
wled00/util.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/util.cpp
📚 Learning: 2025-09-16T18:08:42.848Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Applied to files:
wled00/util.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/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/util.cpp
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.
Applied to files:
wled00/util.cpp
⏰ 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). (6)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
🔇 Additional comments (2)
wled00/util.cpp (2)
1195-1203: LGTM! ESP8266 implementation is clean and correct.The ESP8266 version of
generateDeviceFingerprint()properly addresses all issues from the previous review. The initialization is correct, the entropy sources are appropriate (MAC address combined with flash chip identifiers), and the implementation is straightforward and maintainable.
1206-1225: Refactoring looks good! Minor note on caching.The refactoring of
getDeviceId()to usegenerateDeviceFingerprint()is clean and aligns well with the PR objectives. The deterministic fingerprint generation combined with SHA1 double-hashing provides a good balance between uniqueness and privacy for the opt-in usage reporting use case.Minor observation: The
cachedDeviceIdvariable is declared as static (line 1209) but is overwritten on every call (line 1222), so caching isn't actually implemented. If caching was intended, add a check to return the cached value when non-empty; otherwise, consider removing the static qualifier and comment about caching.Based on learnings, the deterministic salt approach (MAC + chip-specific data) is acceptable for WLED's opt-in usage reporting.
netmindz
reviewed
Nov 26, 2025
Copilot AI
added a commit
that referenced
this pull request
Nov 27, 2025
Co-authored-by: netmindz <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wled00/util.cpp (1)
1172-1176: Critical: Incorrect macro name causes compilation failure or incorrect behavior.Line 1172 uses
SOC_ADC_MAX_BITWIDTH, but this macro does not exist in standard ESP-IDF. The correct macros areSOC_ADC_DIGI_MAX_BITWIDTHandSOC_ADC_RTC_MAX_BITWIDTH. Based on past review discussion, you mentioned thatSOC_ADC_RTC_MAX_BITWIDTHexists inesp_adc_cal.h, but the code uses a different macro name (SOC_ADC_MAX_BITWIDTH).This conditional will either fail to compile (if the macro is undefined) or evaluate incorrectly. Use chip-specific target defines instead for reliable cross-platform compatibility.
Apply this diff to use chip-specific defines:
- #if SOC_ADC_MAX_BITWIDTH == 13 // S2 has 13 bit ADC - #define BIT_WIDTH ADC_WIDTH_BIT_13 - #else - #define BIT_WIDTH ADC_WIDTH_BIT_12 - #endif + #if defined(CONFIG_IDF_TARGET_ESP32S2) + #define BIT_WIDTH ADC_WIDTH_BIT_13 // S2 has 13-bit ADC + #else + #define BIT_WIDTH ADC_WIDTH_BIT_12 // ESP32, S3, C3 use 12-bit + #endifAdditionally, undefine the macro after use to prevent it from leaking to other compilation units:
char fp_string[17]; // 16 hex chars + null terminator sprintf(fp_string, "%08X%08X", fp[1], fp[0]); + #undef BIT_WIDTH return String(fp_string);
🧹 Nitpick comments (1)
wled00/util.cpp (1)
1194-1203: LGTM with minor suggestion.The ESP8266 implementation is correct and properly initializes the fingerprint array to zero, addressing the previous uninitialized memory concern. The use of flash chip ID, size, and vendor ID provides good device-specific entropy.
Minor suggestion: Consider defining a constant for the buffer size instead of the magic number
17(applies to both ESP32 and ESP8266 versions).For example, add at the top of the file or in a header:
#define DEVICE_FINGERPRINT_HEX_SIZE 17 // 16 hex chars + null terminatorThen use:
- char fp_string[17]; // 16 hex chars + null terminator + char fp_string[DEVICE_FINGERPRINT_HEX_SIZE];Based on learnings, replacing magic numbers with named constants improves maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/util.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/util.cpp
🧠 Learnings (17)
📓 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.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
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.
📚 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/util.cpp
📚 Learning: 2025-11-27T06:33:11.401Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.401Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Applied to files:
wled00/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/util.cpp
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Applied to files:
wled00/util.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/util.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/util.cpp
📚 Learning: 2025-08-21T00:00:17.196Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4859
File: usermods/udp_name_sync/udp_name_sync.cpp:62-70
Timestamp: 2025-08-21T00:00:17.196Z
Learning: In WLED codebase, UDP packet processing code must never rely on null termination being present on the wire and must always validate packet size first. The codebase already has some vulnerabilities where malformed data can crash the software, so avoiding introduction of new ones is a priority.
Applied to files:
wled00/util.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/util.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/util.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, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Applied to files:
wled00/util.cpp
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.
Applied to files:
wled00/util.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/util.cpp
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Applied to files:
wled00/util.cpp
📚 Learning: 2025-09-15T19:13:56.469Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4791
File: wled00/util.cpp:737-743
Timestamp: 2025-09-15T19:13:56.469Z
Learning: In WLED's util.cpp, the *_realloc_malloc functions (p_realloc_malloc and d_realloc_malloc) are intentionally designed to free the original buffer on realloc failure and allocate a new buffer, implementing a "replace buffer" semantic rather than traditional realloc behavior. This is documented in the function comments and is the intended design by the author DedeHai.
Applied to files:
wled00/util.cpp
📚 Learning: 2025-09-16T18:08:42.848Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Applied to files:
wled00/util.cpp
📚 Learning: 2025-08-29T19:51:15.511Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/open_weather_map_source.cpp:13-19
Timestamp: 2025-08-29T19:51:15.511Z
Learning: On ESP8266/ESP32 devices with limited heap, using HTTPS for API calls that return large JSON responses may not be feasible due to the combined memory requirements of TLS handshake and JSON parsing. HTTP may be necessary despite security concerns.
Applied to files:
wled00/util.cpp
⏰ 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). (12)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32c3dev_qio)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32S3_wroom2)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
🔇 Additional comments (1)
wled00/util.cpp (1)
1206-1226: LGTM! Excellent improvement to device ID generation.The refactored
getDeviceId()function properly addresses the performance concerns raised in past review comments:
- Caches the result in a static variable to avoid repeated SHA1 computation on frequently polled endpoints
- Uses the new
generateDeviceFingerprint()function for cleaner, more maintainable code- Maintains deterministic generation for consistency across reboots
- Well-documented security and privacy considerations
The double-hashing approach (first hash of fingerprint, second hash for additional entropy) provides good privacy protection while maintaining uniqueness for usage reporting.
netmindz
approved these changes
Nov 27, 2025
Member
|
Has this been tested for 8226, classic, S2, S3 and C3 @DedeHai or do you need help from other for any varient? |
Collaborator
Author
|
I tested the underlying function on all platforms but not the final variant. |
Collaborator
Author
|
If the plan is to put this into 0.15.2 without any beta testing, someone else should confirm this works on all platforms, especially on older silicon revisions which unfortunately I do not own. |
Member
I'd like to get this merged into main, then we will have a few days of nightly builds and then tagging 0.15.2 hopefully this weekend |
Collaborator
Author
|
ok, I will re-run the code as-is on all platforms |
Member
|
I'm currently ready to move house next Wednesday, so I have limited hardware available, but I did keep out a classic, S3 and C3, think I missed keeping an S2 out of the boxes so might need someone else to do that one and 8226 as we actually as I don't use them |
Member
|
As long as at least one person has seen it running on actual hardware for each of the 5 targets, that's ok for merge |
Collaborator
Author
|
can confirm it works on all devices: ESP8266, C3, S2, S3 and vanilla ESP32 |
24 tasks
Copilot AI
added a commit
that referenced
this pull request
Nov 27, 2025
Co-authored-by: netmindz <[email protected]>
softhack007
pushed a commit
to MoonModules/WLED-MM
that referenced
this pull request
Nov 27, 2025
Safe uid generation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
instead of using Strings, this generates a 64bit unique ID from the MAC address, salted with unique chip information:
The 64bit fingerprint is then converted to a string and hashed. I tested the
generateDeviceFingerprint()function on all platforms with various ESPs and always got consistent, unique outputs.edit
forgot the important bit: no more use of "potentially dangerous" direct access of efuses, only standard IDF calls.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.