Skip to content

fix(logs): open log files from the UI task (#7513)#7523

Open
raphaelcoeffic wants to merge 3 commits into
mainfrom
fix-7513-log-open-ui-task
Open

fix(logs): open log files from the UI task (#7513)#7523
raphaelcoeffic wants to merge 3 commits into
mainfrom
fix-7513-log-open-ui-task

Conversation

@raphaelcoeffic

@raphaelcoeffic raphaelcoeffic commented Jul 4, 2026

Copy link
Copy Markdown
Member

Fixes #7513.

Problem

On the X9D+ (and other targets), telemetry briefly drops ("Telemetry lost" / "Telemetry recovered") the moment SD-card logging starts. The reporter traced it to the size of the LOGS/ directory: ~2,250 files caused the glitch every time, while an empty LOGS/ did not. It is a regression from 2.10.6.

Root cause

Log files were opened lazily from logsWrite(), which runs on the FreeRTOS timer daemon task. Telemetry frame polling (_poll_frame) is dispatched to that same task via async_call_isr() / xTimerPendFunctionCallFromISR(). When LOGS/ is large, f_open(..., FA_OPEN_ALWAYS | ...) has to scan the whole FAT directory and blocks the timer task long enough to starve telemetry polling.

The _poll_frame coalescing already present does not help here — coalescing prevents queue flooding, but nothing runs while the daemon task is blocked inside f_open().

Fix

Move the log-file open/close into a new logsHandle(), called cyclically from perMain() on the menus/UI task (priority 1). logsWrite() now only appends a line to an already-open file and never blocks on f_open().

Because the timer/telemetry task (priority 2) is higher priority than the UI task, it preempts the directory scan now running on the UI task, so telemetry keeps flowing while the log file is being created.

Concurrency

The timer task (configTIMER_TASK_PRIORITY = 2) cannot be preempted by the UI task (MENUS_TASK_PRIO = 1), so logsClose()/error_displayed cannot race an in-flight logsWrite() on the target. Only the simulator (timer_native host thread) is truly concurrent, and FF_FS_REENTRANT = 1 covers that.

Testing

  • Builds: X7 firmware + color native simulator.
  • Not yet validated on hardware against the original repro (large LOGS/, arm, watch for telemetry drop) — @Fpv191, a test build would help confirm.

Backports for 2.11 and 2.12 are prepared in separate PRs.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved logging reliability with more consistent log start/stop behavior, including proper open/close handling.
    • Eliminated repeated warning popups by showing each logging issue only once until the situation changes.
    • Logging now stops cleanly when storage is unavailable or the card is full, and it avoids further write attempts after failures.

Log files were opened lazily from logsWrite(), which runs on the
FreeRTOS timer daemon task (priority 2). Telemetry frame polling is
dispatched to that same task via async_call_isr(). 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 (priority 1). logsWrite() 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 being created.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: de471740-f155-4824-85cd-3f818ea4d90c

📥 Commits

Reviewing files that changed from the base of the PR and between a30fc58 and d7bbfb4.

📒 Files selected for processing (1)
  • radio/src/logs.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • radio/src/logs.cpp

📝 Walkthrough

Walkthrough

Refactors SD card logging so the UI task owns log file lifecycle and timer control, while the timer task only appends to an already-open file. Adds shared one-time error display handling and updates the caller and header declarations.

Changes

Log lifecycle refactor

Layer / File(s) Summary
Shared error display state
radio/src/logs.cpp
Adds file-scope error state and a helper to suppress repeated logging warnings.
logsHandle() owns lifecycle
radio/src/logs.cpp
Reworks logsHandle() to manage logging activation, close the file when inactive, clear error state, reopen the file when needed, and drive the logging timer period.
logsWrite() append-only path
radio/src/logs.cpp
logsWrite() now returns early when SD is unavailable, no file is open, or an error was already shown, and routes SD-full and write failures through the shared error helper.
Caller and declaration updates
radio/src/main.cpp, radio/src/sdcard.h
perMain() calls logsHandle(), the old timer initializer declaration is removed, and sdcard.h now declares logsHandle() instead of logsWrite().

Estimated code review effort: 3 (Moderate) | ~25 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: moving log file handling to the UI task.
Description check ✅ Passed The description covers the problem, root cause, fix, concurrency, and testing, though it does not use the template heading verbatim.
Linked Issues check ✅ Passed The changes address #7513 by moving log open/close work off the timer task so telemetry polling is no longer blocked when logging starts.
Out of Scope Changes check ✅ Passed The modified files and declarations stay focused on logging control flow and related APIs, with no clear unrelated additions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-7513-log-open-ui-task

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@radio/src/logs.cpp`:
- Around line 104-110: The sticky error_displayed state is preventing
logsWrite() from resuming while logging remains enabled, and logsHandle() is not
closing the open file after write-side failures. Update the logging flow in
logsWrite(), logsHandle(), and the related close/retry paths so any write error
clears or resets the error state, closes g_oLogFile.obj.fs when appropriate, and
allows the timer-driven callback to recover without requiring the user to toggle
logging.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4c9f59a7-dacc-4bda-b2b7-633d0ca7eef7

📥 Commits

Reviewing files that changed from the base of the PR and between 3e88e06 and f943021.

📒 Files selected for processing (3)
  • radio/src/logs.cpp
  • radio/src/main.cpp
  • radio/src/sdcard.h

Comment thread radio/src/logs.cpp
raphaelcoeffic and others added 2 commits July 4, 2026 07:37
After logsWrite() reported a read/write error it set error_displayed and
then no-op'd on every timer tick, leaving the file open and 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, 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 pfeerick added bug/regression ↩️ A new version of EdgeTX broke something firmware General radio firmware issue, not colorlcd or B&W specific labels Jul 4, 2026
@philmoz

philmoz commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator

Isn't this going to potentially break the log update frequency.

The UI task is not guaranteed to run at a consistent 20hz rate - a busy screen layout with complex widgets could cause inconsistent log update.

Log file write is still done in timer callback so update frequency should be the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug/regression ↩️ A new version of EdgeTX broke something firmware General radio firmware issue, not colorlcd or B&W specific

Projects

None yet

Development

Successfully merging this pull request may close these issues.

X9D+ Telemetry lost briefly when SD card starts LOG.

3 participants