Fix OTA controller update status after firmware update (#749)#753
Fix OTA controller update status after firmware update (#749)#753EyJunge1 wants to merge 4 commits into
Conversation
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughWebUIPlugin::updateOTAStatus no longer accepts a version param: it reads controller->getSystemInfo().version once, syncs it to the OTA instance when changed (tracking lastSyncedControllerVersion), and reuses that cached value in the res:ota-settings WebSocket response. Call sites were updated to the parameterless call. ChangesOTA Status Version Caching
Sequence Diagram(s)sequenceDiagram
participant WebUIPlugin
participant Controller
participant OTA
participant WebSocket
WebUIPlugin->>Controller: getSystemInfo().version
WebUIPlugin->>OTA: if version != lastSyncedControllerVersion -> setControllerVersion(controllerVersion)
WebUIPlugin->>WebSocket: send res:ota-settings (controllerVersion, latestVersion, ...)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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. ✨ 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.
🧹 Nitpick comments (2)
src/display/plugins/WebUIPlugin.cpp (2)
819-819: 💤 Low valueThe
versionparameter appears unused.The function signature accepts
const String &version, but the parameter is not referenced in the function body (lines 819-874). Line 828 retrieves the latest version directly viaota->getCurrentVersion()instead. This parameter could be removed to simplify the signature and call sites (lines 102, 428).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/display/plugins/WebUIPlugin.cpp` at line 819, The updateOTAStatus function takes an unused parameter (const String &version); remove the parameter from WebUIPlugin::updateOTAStatus (change signature to no-arg) and update its declaration in the class header, then update every call site that passes a version to instead call updateOTAStatus() with no arguments (also update any function pointer/override/virtual declarations if present); verify the body still uses ota->getCurrentVersion() as before and compile to ensure no remaining references to the removed parameter.
824-825: 💤 Low valueConsider caching and comparing before calling
setControllerVersion().The
setControllerVersion()implementation frees and re-parses the semver on every call. SinceupdateOTAStatus()is invoked periodically (lines 98-103), this parsing happens repeatedly even when the controller version hasn't changed. You could store the last-seen controller version as a member variable and only callsetControllerVersion()when it differs.⚡ Proposed optimization
Add a member variable to cache the last controller version:
class WebUIPlugin { // ... existing members ... String lastControllerVersion; };Then modify the logic:
const String controllerVersion = controller->getSystemInfo().version; -ota->setControllerVersion(controllerVersion); +if (controllerVersion != lastControllerVersion) { + ota->setControllerVersion(controllerVersion); + lastControllerVersion = controllerVersion; +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/display/plugins/WebUIPlugin.cpp` around lines 824 - 825, Cache the last seen controller version in the WebUIPlugin (e.g. add a member String lastControllerVersion) and then in the code path that currently does const String controllerVersion = controller->getSystemInfo().version; ota->setControllerVersion(controllerVersion); (used by updateOTAStatus) compare controllerVersion to lastControllerVersion and only call ota->setControllerVersion(controllerVersion) and update lastControllerVersion when they differ; this avoids repeated semver free/parse work inside setControllerVersion when the version is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/display/plugins/WebUIPlugin.cpp`:
- Line 819: The updateOTAStatus function takes an unused parameter (const String
&version); remove the parameter from WebUIPlugin::updateOTAStatus (change
signature to no-arg) and update its declaration in the class header, then update
every call site that passes a version to instead call updateOTAStatus() with no
arguments (also update any function pointer/override/virtual declarations if
present); verify the body still uses ota->getCurrentVersion() as before and
compile to ensure no remaining references to the removed parameter.
- Around line 824-825: Cache the last seen controller version in the WebUIPlugin
(e.g. add a member String lastControllerVersion) and then in the code path that
currently does const String controllerVersion =
controller->getSystemInfo().version;
ota->setControllerVersion(controllerVersion); (used by updateOTAStatus) compare
controllerVersion to lastControllerVersion and only call
ota->setControllerVersion(controllerVersion) and update lastControllerVersion
when they differ; this avoids repeated semver free/parse work inside
setControllerVersion when the version is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd8c20d7-b975-49a3-bdec-e221560d7ab9
📒 Files selected for processing (1)
src/display/plugins/WebUIPlugin.cpp
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
Sync the cached OTA controller version with the live SystemInfo version before computing controllerUpdateAvailable, so the web UI no longer shows an update prompt for the version already installed. Fixes jniebuhr#749 Co-authored-by: Cursor <cursoragent@cursor.com>
Remove the unused version parameter from updateOTAStatus() and only sync the OTA controller version cache when SystemInfo reports a change. Co-authored-by: Cursor <cursoragent@cursor.com>
8655c97 to
8c25002
Compare
|
1 participant
Summary
SystemInfoversion before computingcontrollerUpdateAvailableTest plan
pio run -e displaySummary by CodeRabbit
Bug Fixes
Refactor