Cleanup from #4984 #5128
Cleanup from #4984 #5128
Conversation
Many of these functions are internal, and we can reduce RAM by caching the binary value instead.
WalkthroughRefactors bootloader SHA256 caching from a precomputed hex string to a 32-byte binary cache with a validity flag; narrows three helpers to internal linkage ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 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: 1
🧹 Nitpick comments (1)
wled00/ota_update.cpp (1)
276-277: LGTM!The binary cache with a validity flag is a sensible approach for reducing memory usage compared to the previous string-based cache. The static file-scope is appropriate for internal caching.
Based on learnings about magic numbers: the value
32here represents the SHA256 digest size (256 bits / 8 bytes), which is a well-known cryptographic constant. Consider defining a constant likeconstexpr size_t SHA256_DIGEST_SIZE = 32;for clarity, but this is optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/ota_update.cpp(2 hunks)wled00/ota_update.h(0 hunks)
💤 Files with no reviewable changes (1)
- wled00/ota_update.h
🧰 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/ota_update.cpp
🧠 Learnings (3)
📓 Common learnings
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: 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.
📚 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/ota_update.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/ota_update.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). (18)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32S3_wroom2)
- GitHub Check: wled_build / Build Environments (esp32c3dev_qio)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- 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 (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
🔇 Additional comments (4)
wled00/ota_update.cpp (4)
279-283: LGTM!Making the function
staticcorrectly limits its scope to this translation unit, reducing the public API surface and potentially enabling compiler optimizations.
302-304: LGTM!Writing directly to the cache buffer and setting the validity flag afterward is the correct ordering.
329-335: LGTM!Making this function
staticcorrectly limits its scope, and the implementation simply invalidates the cache flag.
337-345: LGTM!The
staticqualifier correctly restricts linkage. The reference parameters forbufferandlenare documented as potentially being adjusted during validation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wled00/ota_update.cpp (1)
316-326: Signed char causes incorrect hex output for bytes ≥ 0x80.The nibble order and
std::moveissues from the prior review were fixed, but the signedchartype remains. WhenbootloaderSHA256Cache[i] >= 0x80, the arithmetic right-shift sign-extends, producing wrong characters (e.g., byte0xABoutputs"(b"instead of"ab").Apply this diff to fix:
for (int i = 0; i < 32; i++) { - char b1 = bootloaderSHA256Cache[i]; - char b2 = b1 >> 4; - b1 &= 0x0F; - b1 += '0'; b2 += '0'; - if (b1 > '9') b1 += 39; - if (b2 > '9') b2 += 39; - result.concat(b2); - result.concat(b1); + uint8_t byte = bootloaderSHA256Cache[i]; + uint8_t hi = byte >> 4; + uint8_t lo = byte & 0x0F; + hi += (hi > 9) ? ('a' - 10) : '0'; + lo += (lo > 9) ? ('a' - 10) : '0'; + result.concat((char)hi); + result.concat((char)lo); }
🧹 Nitpick comments (1)
wled00/ota_update.cpp (1)
276-277: LGTM — binary cache approach reduces memory footprint as intended.Consider defining a named constant for the SHA256 hash size to improve maintainability and avoid magic numbers. Based on learnings about replacing magic numbers with meaningful constants:
constexpr size_t SHA256_DIGEST_SIZE = 32; // ... static uint8_t bootloaderSHA256Cache[SHA256_DIGEST_SIZE];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/ota_update.cpp(2 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/ota_update.cpp
🧠 Learnings (4)
📓 Common learnings
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: 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.
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: 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.
📚 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/ota_update.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/ota_update.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/ota_update.cpp
🔇 Additional comments (3)
wled00/ota_update.cpp (3)
279-305: LGTM — internal linkage and binary caching are correctly implemented.The
statickeyword appropriately limits visibility to this translation unit, and writing directly to the binary cache avoids the overhead of constructing a hex String during hash computation.
329-335: LGTM — cache invalidation correctly resets the validity flag.The
statickeyword appropriately restricts this helper to internal use within the file.
337-345: LGTM — internal linkage and updated documentation clarify intent.The
staticdeclaration correctly limits visibility, and the documentation accurately describes the reference parameter semantics for buffer/length adjustment.
DedeHai
left a comment
DedeHai
left a comment
There was a problem hiding this comment.
looks good, I added out of diff comments that are more nit-picks, nothing functionally wrong. Only one question: are we sure the code also works if the IDF config used to generate the image does not allow to write the bootloader? i.e. it will not just crash but decline gracefully?
| strip.resetSegments(); | ||
|
|
||
| // Check available heap before attempting allocation | ||
| size_t freeHeap = getFreeHeapSize(); |
There was a problem hiding this comment.
this should be contiguous heap
|
|
||
| context->buffer = (uint8_t*)malloc(context->maxBootloaderSize); | ||
| if (!context->buffer) { | ||
| size_t freeHeapNow = getFreeHeapSize(); |
There was a problem hiding this comment.
dito, there can be enough free heap but not a large enough block, meaning the message can show more heap than required but still fail
|
|
||
| // Re-validate size after adjustment | ||
| if (len < MIN_IMAGE_HEADER_SIZE) { | ||
| *bootloaderErrorMsg = "Bootloader at offset 0x1000 too small - invalid header"; |
There was a problem hiding this comment.
there is quite a lot of long strings with info not really of much use to users, for example here "invalid header" would be enough. would save some flash space. Also I would add "Bootloader: " as a seperate string when sending this error out and omit mentioning bootloader in almost every string.
|
one more cosmetic thing I would change: when updating, both update divs should be hidden for a cleaner appearance while the update is going on. |
2 participants
Some code cleanup from #4984.
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.