fix(logs): open log files from the UI task (#7513) [2.11] - #7525
Open
raphaelcoeffic wants to merge 3 commits into
Open
fix(logs): open log files from the UI task (#7513) [2.11]#7525raphaelcoeffic wants to merge 3 commits into
raphaelcoeffic wants to merge 3 commits into
Conversation
Log files were opened lazily from logsWrite(), which on firmware runs on the FreeRTOS timer daemon task. Telemetry frame polling is dispatched to that same task via xTimerPendFunctionCallFromISR(). When the LOGS directory is large (thousands of files), f_open() with FA_OPEN_ALWAYS has to scan the whole FAT directory and blocks the timer task long enough to starve telemetry polling, producing a brief "Telemetry lost" / "Telemetry recovered" at log start. Move the file open/close into the new logsHandle(), called cyclically from perMain() on the menus/UI task. On firmware the logging timer now only appends a line to an already-open file and never blocks on f_open(); because the timer/telemetry task is higher priority it preempts the directory scan running on the UI task, so telemetry keeps flowing while the log file is created. In the simulator logsHandle() opens the file and calls logsWrite() directly, as before (the open already happened on the UI thread there, so it was never affected). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
1 task
After logsWrite() reported a read/write error it set error_displayed and then no-op'd on every tick, leaving the file open (and, on firmware, the timer firing empty callbacks) until the user toggled logging off and on. The open-error path in logsHandle() already retried automatically, so the two error paths were inconsistent. Handle a pending error in logsHandle() (UI task): stop the timer (on firmware), close the file, and retry a fresh open through the existing open path. A successful (re)open clears error_displayed and resumes logging; while the error persists the timer stays stopped and the file closed, and the popup remains deduplicated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The previous change retried the open on every perMain() cycle after any error. For a persistent hard error (bad card / corrupted FS) that means rescanning a potentially huge LOGS directory indefinitely, which is wasteful and cannot succeed. Only a full SD card is treated as recoverable: sdIsFull() is a cheap check (no directory scan) and the condition clears when the user frees space, so logging resumes automatically. Any other error now stops logging until the user re-enables it (which clears error_displayed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pfeerick
added
bug/regression ↩️
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Backport of #7523 to 2.11. Fixes #7513.
Telemetry briefly drops when SD-card logging starts if the
LOGS/directory is large. Log files were opened lazily fromlogsWrite(), which on firmware runs on the FreeRTOS timer daemon task — the same task that dispatches telemetry frame polling. A largeLOGS/makesf_open(FA_OPEN_ALWAYS)scan the whole FAT directory and blocks that task long enough to starve telemetry.Note on this backport
2.11 predates the
os/timer.habstraction, so this is a hand-port rather than a cherry-pick:xTimer*) inside#if !defined(SIMU).logsWrite()was already called directly fromperMain()(UI thread), so SIMU never had the bug.The unified
logsHandle()opens the file on the UI thread in both configs, then either drives the FreeRTOS timer (firmware) or callslogsWrite()directly (simulator).See #7523 for the full description.
Tested: X7 firmware + simulator build.
🤖 Generated with Claude Code