fix(AnalyzeView): Fix onboard log download path and refactor controller#14194
fix(AnalyzeView): Fix onboard log download path and refactor controller#14194HTRamsey wants to merge 1 commit intomavlink:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the Analyze > Onboard Logs workflow by fixing the download destination bug (especially for deduplicated filenames), tightening download retry behavior, and simplifying MAVLink message sending while adding a select-all control in the UI.
Changes:
- Fixes log download path handling (including duplicate filename generation) and ensures the destination directory is created.
- Adds bounded retry logic for missing log data/list retrieval and refactors log-data handling for clearer control flow.
- Updates the QML page to guard auto-refresh without an active vehicle and adds a header checkbox to select/deselect all logs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/AnalyzeView/OnboardLogs/OnboardLogPage.qml | Guards initial refresh without a connected vehicle; adds select-all checkbox behavior; removes unused import. |
| src/AnalyzeView/OnboardLogs/OnboardLogEntry.cc | Enables ctor/dtor debug logging for onboard log entry/download data objects. |
| src/AnalyzeView/OnboardLogs/OnboardLogController.h | Removes unused compression API surface and adds retry constants + MAVLink send helper declaration. |
| src/AnalyzeView/OnboardLogs/OnboardLogController.cc | Fixes deduped file path, adds retry cap, refactors _logData, adds _sendMavlinkMessage, and ensures download directory exists. |
Comments suppressed due to low confidence (1)
src/AnalyzeView/OnboardLogs/OnboardLogController.cc:63
QDir().mkpath(_downloadPath)return value is ignored. If directory creation fails (permissions/invalid path), the controller still enters downloading mode and later file open/resize will fail. Please check the mkpath result and fail early (set current entry status to Error and stop downloading) with a warning that includes the path.
(void) QDir().mkpath(_downloadPath);
_setDownloading(true);
_receivedAllData();
}
| if (!_downloadData->file.write(reinterpret_cast<const char*>(data), count)) { | ||
| qCWarning(OnboardLogControllerLog) << "Error while writing log file chunk"; | ||
| _downloadData->entry->setStatus(tr("Error")); | ||
| return; | ||
| } |
There was a problem hiding this comment.
On file write failure you set status to Error and return, but the controller stays in downloading mode and keeps retrying, leaving a corrupted/partial file. Treat write failure as fatal: abort the current download and remove the file so subsequent downloads don’t appear to succeed with a bad artifact.
| if (_downloadData->file.exists()) { | ||
| uint32_t numDups = 0; | ||
| const QStringList filename_spl = _downloadData->filename.split('.'); | ||
| do { | ||
| numDups += 1; | ||
| const QString filename = filename_spl[0] + '_' + QString::number(numDups) + '.' + filename_spl[1]; | ||
| _downloadData->file.setFileName(filename); | ||
| _downloadData->file.setFileName(_downloadPath + filename); | ||
| } while ( _downloadData->file.exists()); |
There was a problem hiding this comment.
The deduplicated filename logic is a key behavioral change (ensuring duplicates still land under _downloadPath), but the existing OnboardLogDownloadTest doesn’t exercise the duplicate-download case. Please add/extend a unit/integration test to download the same log twice and assert the second file is created under the selected directory with the expected suffix.
| qCDebug(OnboardLogEntryLog) << this; | ||
| } |
There was a problem hiding this comment.
Re-enabling qCDebug(OnboardLogEntryLog) in these constructors/destructors can create very noisy logs and overhead when the category is enabled. Most similar ctor/dtor debug statements in the codebase are commented out; consider reverting these to commented debug, or include Q_FUNC_INFO/more context so enabling the category yields actionable logs.
Build ResultsPlatform Status
All builds passed. Pre-commit
Pre-commit hooks: 4 passed, 34 failed, 7 skipped. Test Resultslinux-coverage: 67 passed, 0 skipped Code CoverageCoverage: 57.4% No baseline available for comparison Artifact Sizes
Updated: 2026-04-08 12:47:00 UTC • Triggered by: Android |
Fix duplicate filenames losing download directory prefix, causing logs to land in home directory instead of the selected path. Also: - Add retry limit for data downloads (was infinite) - Fix seek failure silently dropping error status - Ensure download directory exists before writing - Extract MAVLink send boilerplate into helper template - Flatten _logData with early returns, removing nested result flag - Remove dead compression API stubs and unused members - Make header checkbox a select-all toggle - Guard auto-refresh on page load behind vehicle check - Fix error logs to print actual file path, not bare filename - Name retry constants, fix typo, remove unused QML import - Simplify duplicate status string in _updateDataRate - Enable debug logging in OnboardLogEntry ctors/dtors
HTRamsey
force-pushed
the
fix/onboard-log-download
branch
from
d98e084 to
cb220d1
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #14194 +/- ##
=========================================
Coverage ? 25.53%
=========================================
Files ? 744
Lines ? 67983
Branches ? 31663
=========================================
Hits ? 17362
Misses ? 37843
Partials ? 12778
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/AnalyzeView/OnboardLogs/OnboardLogController.cc:66
- When starting a download, the next selected entry no longer gets a status update (previously it was set to something like "Waiting"). As a result, the UI can continue to show the old status (e.g. "Available") until enough data arrives to trigger
_updateDataRate(). Consider setting the entry status to a "Waiting"/"Downloading" state before calling_receivedAllData()(for example in_downloadToDirectoryor_prepareLogDownload).
_setDownloading(true);
_receivedAllData();
}
| _downloadData.reset(); | ||
| } | ||
|
|
||
| _resetSelection(true); |
There was a problem hiding this comment.
_abortDownload() calls _resetSelection(true), which marks any still-selected logs as "Canceled" even though the abort path is an error. This can mislead users by showing "Canceled" for logs that were never attempted. Consider clearing selection without the canceled flag (or using a separate error path) so only the current entry is marked "Error" and remaining entries keep their prior status.
| _resetSelection(true); | |
| _resetSelection(); |
| if (_retries++ > kMaxDataRetries) { | ||
| qCWarning(OnboardLogControllerLog) << "Too many errors downloading log data. Giving up."; | ||
| _abortDownload(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Retry limit checks use post-increment with > (e.g. if (_retries++ > kMaxDataRetries)), which allows kMaxDataRetries + 1 attempts before aborting. If the intent is to cap retries at exactly kMaxDataRetries, adjust the logic (for example, increment first or use >=) so the constant matches the actual maximum.
| OnboardLogDownloadData::OnboardLogDownloadData(QGCOnboardLogEntry * const logEntry) | ||
| : ID(logEntry->id()) | ||
| , entry(logEntry) | ||
| { | ||
| // qCDebug(OnboardLogEntryLog) << Q_FUNC_INFO << this; | ||
| qCDebug(OnboardLogEntryLog) << this; | ||
| } | ||
|
|
||
| OnboardLogDownloadData::~OnboardLogDownloadData() | ||
| { | ||
| // qCDebug(OnboardLogEntryLog) << Q_FUNC_INFO << this; | ||
| qCDebug(OnboardLogEntryLog) << this; | ||
| } |
There was a problem hiding this comment.
The new constructor/destructor debug logs only print the object pointer (<< this), which is hard to interpret in logs and adds noise when the category is enabled. Consider including Q_FUNC_INFO (or some identifying fields like log id) or removing these statements if they’re not needed.
Summary
_findMissingDatacould retry indefinitely; now bounded bykMaxDataRetries_sendMavlinkMessagetemplate_logData: Replaced deeply nested logic with early returns, removing theresultflag patterncompressLogFile,cancelCompression, and related stubs/properties/signalsrefresh()without an active vehiclemkpath, name retry constants, fix typo, remove unused QML import, deduplicate status string formattingTest plan