AnalyzeView: unify onboard-log download into single page with transport facade#14331
Open
HTRamsey wants to merge 1 commit into
Open
AnalyzeView: unify onboard-log download into single page with transport facade#14331HTRamsey wants to merge 1 commit into
HTRamsey wants to merge 1 commit into
Conversation
…rt facade Collapses the parallel "Onboard Logs" / "Onboard Logs (FTP)" pages and controllers into one. A new OnboardLogController facade picks the LOG protocol or MAVLink-FTP transport per active vehicle based on MAV_PROTOCOL_CAPABILITY_FTP, eliminating duplicate UI and entry types. Architecture - New abstract OnboardLogTransport base in src/AnalyzeView/OnboardLogs/, owning model/requestingList/ downloadingLogs Q_PROPERTYs plus the selection/erasure surface. - LogProtocolTransport (was OnboardLogController) and FtpTransport (was OnboardLogFtpController) are demoted from QML singletons to facade-owned children. - OnboardLogController is now a thin QML_SINGLETON facade. It delegates refresh/download/cancel/eraseAll to the active transport, re-emits its signals, and swaps transports on vehicle change, on late-arriving Vehicle::capabilityBitsChanged, and when the user toggles the override Fact. - OnboardLogEntry (was QGCOnboardLogEntry) is the shared row item; LOG-protocol-only OnboardLogDownloadData split into its own pair. - OnboardLogTable.qml is shared; OnboardLogPage.qml is the single AnalyzePage entry, hiding Erase All when the active transport reports canErase=false. - OnboardLogsFtp/ subdirectory and the second analyzePages() entry removed. FTP erase parity - FtpTransport overrides canErase()=true and eraseAll(), processing the model sequentially via FTPManager::deleteFile(). Successful deletes drop entries from the model; failures leave them with an Error status. cancel() also tears down in-flight delete state. Settings override - New MavlinkSettings.onboardLogTransport Fact (Auto/LOG/FTP enum, default Auto) for forcing a transport when capability detection is unreliable. Folder cleanup - All OnboardLog sources live in src/AnalyzeView/OnboardLogs/. - FtpTransport's logging-category declaration moved out of the header (no external consumer). - Stale class QThread; forward decl removed from LogProtocolTransport. - Drive-by null-check fix in LogProtocolTransport's PX4 SYS_LOGGER parameter lookup.
Collaborator
Author
|
@julianoes just merging your FTP log stuff in to one tab |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR consolidates the AnalyzeView “Onboard Logs” UI and logic by introducing a single OnboardLogController facade which selects between the MAVLink LOG protocol and MAVLink FTP transports per active vehicle (or via a new user override setting), removing the duplicate FTP-specific Analyze page/controller.
Changes:
- Introduces an
OnboardLogTransportabstraction with concreteLogProtocolTransportandFtpTransportimplementations, selected/forwarded by a singletonOnboardLogController. - Replaces the separate FTP Analyze page with a shared
OnboardLogTable.qml, and updates the singleOnboardLogPage.qmlto adapt messaging and “Erase All” visibility by active transport. - Adds a new
MavlinkSettings.onboardLogTransportenum setting (Auto/LOG protocol/MAVLink FTP) and updates the integration test to use the facade’s public API.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/AnalyzeView/OnboardLogDownloadTest.cc | Updates the integration test to use the facade’s public accessors and renamed entry type. |
| src/Settings/MavlinkSettings.h | Adds the onboardLogTransport SettingFact declaration. |
| src/Settings/MavlinkSettings.cc | Registers the new onboardLogTransport SettingFact. |
| src/Settings/Mavlink.SettingsGroup.json | Defines the new onboard-log transport override setting (enum). |
| src/API/QGCCorePlugin.cc | Removes the separate “Onboard Logs (FTP)” analyze page entry. |
| src/AnalyzeView/OnboardLogsFtp/OnboardLogFtpPage.qml | Removes the dedicated FTP onboard-log page (deleted). |
| src/AnalyzeView/OnboardLogsFtp/OnboardLogFtpEntry.{h,cc} | Removes FTP-specific log entry type (deleted). |
| src/AnalyzeView/OnboardLogsFtp/OnboardLogFtpController.h | Removes FTP-specific controller singleton header (deleted). |
| src/AnalyzeView/OnboardLogsFtp/CMakeLists.txt | Removes the FTP submodule build file (deleted). |
| src/AnalyzeView/OnboardLogs/OnboardLogTransport.{h,cc} | Adds common transport interface/base state shared by transports and facade. |
| src/AnalyzeView/OnboardLogs/OnboardLogTable.qml | Adds shared QML table UI used by the unified onboard logs page. |
| src/AnalyzeView/OnboardLogs/OnboardLogPage.qml | Switches to shared table component and adapts description/Erase button to active transport. |
| src/AnalyzeView/OnboardLogs/OnboardLogEntry.{h,cc} | Renames/unifies the log entry type and moves LOG-only download tracking out. |
| src/AnalyzeView/OnboardLogs/OnboardLogDownloadData.{h,cc} | Extracts LOG-protocol chunk tracking into its own helper. |
| src/AnalyzeView/OnboardLogs/OnboardLogController.{h,cc} | Implements the singleton facade selecting/forwarding to the active transport + override setting integration. |
| src/AnalyzeView/OnboardLogs/LogProtocolTransport.{h,cc} | Adds LOG-protocol transport implementation (previous controller logic moved here). |
| src/AnalyzeView/OnboardLogs/FtpTransport.{h,cc} | Adds FTP transport implementation (previous FTP controller logic moved here) including “erase all” support. |
| src/AnalyzeView/OnboardLogs/CMakeLists.txt | Updates the module sources to include new transport/facade classes and helper. |
| src/AnalyzeView/CMakeLists.txt | Removes FTP subdir and registers OnboardLogTable.qml in the AnalyzeView QML module. |
Comments suppressed due to low confidence (2)
src/AnalyzeView/OnboardLogs/FtpTransport.cc:53
- On vehicle change, this disconnects list/download/progress signals but does not disconnect FTPManager::deleteComplete. If an erase is in progress and the vehicle changes, deleteComplete can still invoke _deleteComplete on this transport while its internal state (_currentEraseEntry/_eraseQueue/_vehicle) is being reset, which can lead to use-after-free or acting on the wrong vehicle. Disconnect deleteComplete here as well and/or call cancel() before swapping vehicles.
src/AnalyzeView/OnboardLogs/FtpTransport.cc:60 - On vehicle change this resets listing/download queues, but it does not reset the public busy flags (_requestingLogEntries/_downloadingLogs) nor the erase state (_erasing/_eraseQueue/_currentEraseEntry/_eraseFailureCount). If the user switches vehicles mid-refresh/download/erase, QML can remain stuck in a busy state and stale pointers may remain. Consider calling cancel() (which already cancels download/delete) and explicitly clearing erase state + calling _setListing(false)/_setDownloading(false) as part of the vehicle switch teardown.
| } | ||
|
|
||
| _receivedAllEntries(); | ||
| qCWarning(LogProtocolTransportLog) << "Too many errors retreiving log list. Giving up."; |
Comment on lines
+135
to
+139
| if (_vehicle) { | ||
| _logEntriesModel->clearAndDeleteContents(); | ||
| (void)disconnect(_vehicle, &Vehicle::logEntry, this, &LogProtocolTransport::_logEntry); | ||
| (void)disconnect(_vehicle, &Vehicle::logData, this, &LogProtocolTransport::_logData); | ||
| } |
| @@ -0,0 +1,93 @@ | |||
| #pragma once | |||
|
|
|||
Contributor
Build ResultsPlatform Status
All builds passed. Pre-commit
Pre-commit hooks: 4 passed, 36 failed, 7 skipped. Test Resultslinux-coverage: 88 passed, 0 skipped Code CoverageCoverage: 59.5% No baseline available for comparison Artifact Sizes
Updated: 2026-05-05 20:21:32 UTC • Triggered by: Android |
julianoes
requested changes
May 5, 2026
Contributor
julianoes
left a comment
julianoes
left a comment
There was a problem hiding this comment.
picks the LOG protocol or MAVLink-FTP transport per active vehicle based on MAV_PROTOCOL_CAPABILITY_FTP
You can't necessarily do that. I was told that ArduPilot boards without SD card but logs in flash, won't support this. We have to check whether they are setting MAV_PROTOCOL_CAPABILITY_FTP or not!
Also, download via FTP does not have date and time for ArduPilot (see discussion in ArduPilot/ardupilot#32860), so this feature needs to be opt-in or at least manually selectable which is the reason why I made it as a separate tab.
You can of course move it to the same tab and show a selector instead to select the backend.
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
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.
Summary
Collapses the parallel Onboard Logs and Onboard Logs (FTP) AnalyzePages and their controllers into one. A new
OnboardLogControllerfacade picks the LOG protocol or MAVLink-FTP transport per active vehicle based onMAV_PROTOCOL_CAPABILITY_FTP, eliminating duplicate UI, controllers, and entry types.Why
The two pages did the same job — list and download flight logs from the vehicle — over different transports. Users had to know which page their firmware supported. The QML pages and entry classes were ~95% duplicate; the controllers shared an identical state machine (refresh → list → download → cancel) and diverged only in transport.
Changes
New layout (everything under
src/AnalyzeView/OnboardLogs/)Architecture
OnboardLogControlleris now a thinQML_SINGLETONfacade. It owns oneLogProtocolTransportand oneFtpTransportas children, picks one as_active, and forwardsrefresh/download/cancel/eraseAllplus the QML-facing properties (model,requestingList,downloadingLogs,canErase,transportName).MultiVehicleManager::activeVehicleChanged, onVehicle::capabilityBitsChanged(handles late-arriving caps), and when the user toggles the override Fact at runtime.LogProtocolTransport(wasOnboardLogController) andFtpTransport(wasOnboardLogFtpController) are demoted from QML singletons. Their bodies are unchanged except for inheriting fromOnboardLogTransportinstead ofQObject.OnboardLogEntry(wasQGCOnboardLogEntry) is the shared model item; FTP path stays empty for LOG-protocol entries.OnboardLogPage.qmlshows Erase All only when the active transport advertisescanErase. Description text switches between LOG-protocol and MAVLink-FTP framings viatransportName.analyzePages()entry inQGCCorePlugin.ccand theOnboardLogsFtp/subdirectory are removed.FTP erase parity
FtpTransportoverridescanErase()=trueand implementseraseAll()viaFTPManager::deleteFile(), processing the model sequentially. Successful deletes drop the entry from the model; failures leave it with an Error status.cancel()also tears down in-flight delete state.Settings override
New
MavlinkSettings.onboardLogTransportenum Fact (Auto/LOG protocol/MAVLink FTP, defaultAuto). The facade re-evaluates on Fact change so users can force a transport when capability detection is unreliable.Folder cleanup
FtpTransportlogging-category declaration moved out of the header (no external consumer).class QThread;forward decl removed fromLogProtocolTransport.h.LogProtocolTransport's PX4SYS_LOGGERparameter lookup (was already unsafe before this PR).Compatibility
OnboardLogControllerretains its singleton name; existing QML bindings continue to work, now resolved against the facade.QGC_CUSTOM_BUILDplugins that listed both AnalyzePages or referencedOnboardLogFtpControllerwill need a one-line update — the FTP class is nowFtpTransportand is no longer a QML singleton.Test plan
AnalyzeViewtests pass (ctest -L AnalyzeView)OnboardLogDownloadTestintegration test exercises the facade → LOG-transport path on a non-FTP mock vehiclepageDescriptionswaps text on transport changeFollow-ups (intentionally out of scope)
tr()strings ("Log Refresh" / "Onboard Log Refresh", etc.) need consolidation inqgc_*.ts.LogProtocolTransportstill exposes thecompressLogs/compressing/compressionProgressQ_PROPERTYs, but the facade doesn't forward them. Wire them up if/when the UI lands.