fix: free TFT sprite before OTA to fix SSL memory failure (#59)#61
Conversation
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds OTA UI hooks to the display interface, implements TFT-specific OTA rendering and resource release, and wires the active display into the web server's OTA task so the display is freed before SSL download, progress is shown, and errors are reported. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Web Client
participant WSM as WebServerManager (OTA task)
participant Display as TFTManager / DisplayI
participant Network as HTTPS / SSL
participant Updater as Update subsystem
User->>Client: Start OTA (POST /update)
Client->>WSM: Trigger OTA task
WSM->>Display: freeForOTA()
activate Display
Display->>Display: Stop TFT task\nDelete sprite\nDraw "Updating..."
deactivate Display
WSM->>Network: Establish TLS and download firmware
loop while downloading
Network-->>WSM: data chunk
WSM->>WSM: compute newPct
alt percent changed
WSM->>Display: updateOTAProgress(newPct)
activate Display
Display->>Display: Fill progress bar\nUpdate percent text
deactivate Display
end
end
alt success
WSM->>Updater: apply update
Updater-->>WSM: success (reboot)
else failure
WSM->>Display: showOTAError(errorMsg)
activate Display
Display->>Display: Draw error UI (red X + text)
deactivate Display
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/TFTManager.cpp`:
- Around line 745-768: TFT task is never restarted after OTA failure so queued
messages on _messageQueue have no consumer; update TFTManager::showOTAError (or
the OTA failure handling path after freeForOTA()) to either restart the TFT task
(the same routine that creates the TFT task, e.g., the start/create task method
used elsewhere) or explicitly document that a manual reboot is required—ensure
the fix reinitializes whatever freeForOTA() torn down so subsequent calls like
showReady() and showSpool() are served, and reference _messageQueue so the
consumer is restored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3c7af056-07eb-4cd3-aade-d548d64bd903
📒 Files selected for processing (6)
src/DisplayI.hsrc/TFTManager.cppsrc/TFTManager.hsrc/WebServerManager.cppsrc/WebServerManager.hsrc/main.cpp
| void TFTManager::showOTAError(const char* error) { | ||
| int cx = _tft.width() / 2; | ||
|
|
||
| // Clear progress area | ||
| _tft.fillRect(0, 60, _tft.width(), _tft.height() - 60, COLOR_BG); | ||
|
|
||
| // Error icon | ||
| _tft.fillCircle(cx, 100, 22, 0xFF4444); | ||
| _tft.setTextColor(COLOR_BG); | ||
| _tft.setTextSize(3); | ||
| _tft.setTextDatum(MC_DATUM); | ||
| _tft.drawString("X", cx, 100); | ||
|
|
||
| // "Update Failed" text | ||
| _tft.setTextColor(COLOR_TEXT); | ||
| _tft.setTextSize(1); | ||
| _tft.drawString("Update Failed", cx, 135); | ||
|
|
||
| // Error detail | ||
| if (error && error[0]) { | ||
| _tft.setTextColor(COLOR_SUBTEXT); | ||
| _tft.drawString(error, cx, 155); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
TFT task is not restarted after OTA failure—is this intentional?
After freeForOTA() deletes the TFT task, if OTA fails the task is never restarted. This means subsequent display calls (e.g., showReady(), showSpool()) will queue messages to _messageQueue with no consumer, leaving the TFT stuck on the error screen.
This is likely acceptable since OTA failure typically requires user intervention (power cycle or retry), but consider documenting this behavior or adding a note that manual reboot is expected after OTA failure.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 746-746: variable 'cx' is not initialized
(cppcoreguidelines-init-variables)
[warning] 746-746: variable name 'cx' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 764-764: implicit conversion 'const char *' -> bool
(readability-implicit-bool-conversion)
[warning] 764-764: implicit conversion 'char' -> bool
(readability-implicit-bool-conversion)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TFTManager.cpp` around lines 745 - 768, TFT task is never restarted after
OTA failure so queued messages on _messageQueue have no consumer; update
TFTManager::showOTAError (or the OTA failure handling path after freeForOTA())
to either restart the TFT task (the same routine that creates the TFT task,
e.g., the start/create task method used elsewhere) or explicitly document that a
manual reboot is required—ensure the fix reinitializes whatever freeForOTA()
torn down so subsequent calls like showReady() and showSpool() are served, and
reference _messageQueue so the consumer is restored.
1 participant
Summary
freeForOTA(),updateOTAProgress(),showOTAError()to DisplayI with default no-ops so LCDManager is unaffectedCloses #59
Test Plan
/updatepage — verify TFT shows "Updating..." with progress barSummary by CodeRabbit
New Features
Reliability