feat(AnalyzeView): add MAVLink FTP log download for PX4 .ulg log files#13996
feat(AnalyzeView): add MAVLink FTP log download for PX4 .ulg log files#13996dakejahl wants to merge 1 commit intomavlink:masterfrom
Conversation
When a PX4 vehicle reports MAV_PROTOCOL_CAPABILITY_FTP, the log download page uses MAVFTP to list and download .ulg files from /fs/microsd/log. Legacy LOG_REQUEST protocol remains for ArduPilot and non-FTP vehicles. MAVFTP code is guarded by QGC_ENABLE_MAVFTP_LOG_DOWNLOAD (ON by default) so the legacy path can be isolated or deprecated in the future. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds MAVLink FTP-based log download capability for PX4 vehicles to the AnalyzeView, providing a faster and more reliable alternative to the legacy LOG_REQUEST_DATA protocol. When a PX4 vehicle reports the MAV_PROTOCOL_CAPABILITY_FTP capability, the system automatically uses MAVFTP to list and download .ulg files from /fs/microsd/log. The implementation is guarded by a QGC_ENABLE_MAVFTP_LOG_DOWNLOAD CMake option (ON by default), allowing the feature to be disabled at build time if needed.
Changes:
- Adds MAVFTP-based log listing by walking the PX4 log directory tree (date subdirectories → .ulg files)
- Implements sequential log downloads via FTPManager with progress tracking and download rate display
- Maintains backward compatibility with the legacy LOG_REQUEST protocol for ArduPilot and non-FTP vehicles
- Includes proper state management with cleanup on vehicle switch and cancellation
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/AnalyzeView/LogEntry.h | Adds ftpPath field to QGCLogEntry (guarded by QGC_MAVFTP_LOG_DOWNLOAD) to store remote file path |
| src/AnalyzeView/LogDownloadController.h | Declares MAVFTP methods, state machine enum, and state variables for tracking listing/download progress |
| src/AnalyzeView/LogDownloadController.cc | Implements MAVFTP directory listing, file download, state management, and integrates with existing legacy code paths |
| src/AnalyzeView/CMakeLists.txt | Conditionally defines QGC_MAVFTP_LOG_DOWNLOAD preprocessor macro based on CMake option |
| cmake/CustomOptions.cmake | Adds QGC_ENABLE_MAVFTP_LOG_DOWNLOAD CMake option (default ON) |
| cmake/PrintSummary.cmake | Adds build summary output for the MAVFTP log download option |
|
|
||
| const QString ftpPath = QString(kPx4LogRoot) + QStringLiteral("/") + currentDir + QStringLiteral("/") + fileName; | ||
|
|
||
| QGCLogEntry *logEntry = new QGCLogEntry(_mavftpLogIdCounter++, dateTime, fileSize, true, this); |
There was a problem hiding this comment.
The QGCLogEntry object is created with this as the parent, which differs from the legacy log download path (line 209) where entries are created without a parent. While both approaches work with clearAndDeleteContents() calling deleteLater(), setting the parent could cause the entry to persist longer than intended if the model is cleared while the controller exists. Consider creating the entry without a parent for consistency with the legacy path.
| QGCLogEntry *logEntry = new QGCLogEntry(_mavftpLogIdCounter++, dateTime, fileSize, true, this); | |
| QGCLogEntry *logEntry = new QGCLogEntry(_mavftpLogIdCounter++, dateTime, fileSize, true); |
|
|
||
| qCDebug(LogDownloadControllerLog) << "MAVFTP: listing subdir" << path; | ||
|
|
||
| if (!_vehicle->ftpManager()->listDirectory(MAV_COMP_ID_AUTOPILOT1, path)) { |
There was a problem hiding this comment.
Missing null check for _vehicle before calling _vehicle->ftpManager(). If the vehicle changes while MAVFTP listing is in progress, this could cause a null pointer dereference. Add a check similar to line 880 in _downloadLogMavftp.
| if (!_vehicle->ftpManager()->listDirectory(MAV_COMP_ID_AUTOPILOT1, path)) { | |
| if (!_vehicle) { | |
| qCWarning(LogDownloadControllerLog) << "MAVFTP: no active vehicle, aborting subdir listing for" << path; | |
| _mavftpListState = MavftpIdle; | |
| _mavftpDirsToList.clear(); | |
| _receivedAllEntries(); | |
| return; | |
| } | |
| auto* ftpManager = _vehicle->ftpManager(); | |
| if (!ftpManager) { | |
| qCWarning(LogDownloadControllerLog) << "MAVFTP: no FTP manager, aborting subdir listing for" << path; | |
| _mavftpListState = MavftpIdle; | |
| _mavftpDirsToList.clear(); | |
| _receivedAllEntries(); | |
| return; | |
| } | |
| if (!ftpManager->listDirectory(MAV_COMP_ID_AUTOPILOT1, path)) { |
| if (!_ftpDownloadQueue.isEmpty()) { | ||
| _downloadLogMavftp(_ftpDownloadQueue.dequeue()); | ||
| } else { | ||
| _setDownloading(false); |
There was a problem hiding this comment.
The _setDownloading(false) call could occur after the vehicle has been changed to null (via _setActiveVehicle), since FTPManager signals may arrive asynchronously. While _ftpCurrentDownloadEntry is checked at line 936, _vehicle could still become null before reaching line 954. Consider adding a vehicle null check before calling _setDownloading(false), or ensure _setDownloading handles null _vehicle gracefully.
| _setDownloading(false); | |
| if (_vehicle) { | |
| _setDownloading(false); | |
| } |
| void LogDownloadController::_mavftpListDirComplete(const QStringList &dirList, const QString &errorMsg) | ||
| { | ||
| if (!errorMsg.isEmpty()) { | ||
| qCWarning(LogDownloadControllerLog) << "MAVFTP listing error:" << errorMsg; | ||
| _mavftpListState = MavftpIdle; | ||
| _receivedAllEntries(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The callback _mavftpListDirComplete can be invoked asynchronously from FTPManager after the vehicle has been changed to null via _setActiveVehicle. While signals are disconnected during vehicle change (line 174), there may be a race condition where a signal is already queued. When _receivedAllEntries() is called (line 772), it calls _setListing(false) which dereferences _vehicle without a null check. Consider adding a guard check for _vehicle at the start of this method.
| target_include_directories(${CMAKE_PROJECT_NAME} PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}) | ||
|
|
||
| if(QGC_ENABLE_MAVFTP_LOG_DOWNLOAD) | ||
| target_compile_definitions(${CMAKE_PROJECT_NAME} PRIVATE QGC_MAVFTP_LOG_DOWNLOAD) |
There was a problem hiding this comment.
The CMake option is named QGC_ENABLE_MAVFTP_LOG_DOWNLOAD but the compile definition is QGC_MAVFTP_LOG_DOWNLOAD (missing ENABLE_). For consistency with other features in the codebase (e.g., QGC_ENABLE_BLUETOOTH at src/Comms/CMakeLists.txt:78), consider using the same name for both the option and the definition.
| target_compile_definitions(${CMAKE_PROJECT_NAME} PRIVATE QGC_MAVFTP_LOG_DOWNLOAD) | |
| target_compile_definitions(${CMAKE_PROJECT_NAME} PRIVATE QGC_ENABLE_MAVFTP_LOG_DOWNLOAD) |
| @@ -427,6 +474,23 @@ bool LogDownloadController::_prepareLogDownload() | |||
| void LogDownloadController::refresh() | |||
| { | |||
There was a problem hiding this comment.
Calling refresh() while an MAVFTP download is in progress could cause issues. The clearAndDeleteContents() call at line 476 will delete all log entries, including _ftpCurrentDownloadEntry if it's still referenced. When the download completes and _ftpDownloadComplete tries to access _ftpCurrentDownloadEntry->setStatus(), it could access a deleted object. Consider adding a guard to prevent refresh during active downloads, or ensure ongoing downloads are properly canceled before clearing the model.
| { | |
| { | |
| #ifdef QGC_MAVFTP_LOG_DOWNLOAD | |
| // Prevent clearing the model while a MAVFTP download is in progress, since | |
| // that would delete entries that may still be referenced by the FTP logic. | |
| if (_useMavftp && _downloading) { | |
| qCWarning(LogDownloadControllerLog) << "refresh requested while MAVFTP download is in progress - ignoring refresh"; | |
| return; | |
| } | |
| #endif |
Build ResultsPlatform Status
Some builds failed. Pre-commit
Pre-commit hooks: 12 passed, 59 failed, 3 skipped. Test Resultslinux_gcc_64: 73 passed, 0 skipped Total: 73 passed, 0 skipped Code Coverage
Artifact Sizes
Total size increased by 0.01 MBUpdated: 2026-02-17 09:47:07 UTC • Triggered by: Android |
dakejahl
left a comment
dakejahl
left a comment
There was a problem hiding this comment.
I need to review this more thoroughly. I tested it and it does work. Over USB I am seeing 350KB/s.
|
|
||
| // --- MAVFTP log listing --- | ||
|
|
||
| static constexpr const char *kPx4LogRoot = "/fs/microsd/log"; |
There was a problem hiding this comment.
This needs to be something more generic that the FC can reinterpret as the root log dir
| static constexpr const char *kPx4LogRoot = "/fs/microsd/log"; | |
| static constexpr const char *kPx4LogRoot = "@LOGS_DIR"; |
|
|
||
| #ifdef QGC_MAVFTP_LOG_DOWNLOAD | ||
| if (_useMavftp) { | ||
| // TODO: Implement MAVFTP-based log erasure (FTP file removal) |
There was a problem hiding this comment.
Maybe just continue to use the LOG_ERASE for mass erase. Individual erase is too slow and I don't see the use.
|
I would make it a separate tab with general file access rather than replacing the existing log file tab, as that would generally be good to have. |
|
Is there any value in having that cmake option or just enable it always when PX4 is detected? |
I did think about that. I agree another tab for general file access would be super nice. My thinking was that by replacing the Log Download page with FTP is we could eventually deprecate and remove the non-FTP impl of log download in PX4.
Yeah probably not much value. And yes, this may also work for Ardupilot too and we should ask them if we should prefer it. The primary motivating factor for this PR is to avoid link spam during log download. The LOG_DATA doesn't have target sys/comp ID fields so during bulk download all endpoints on a mavlink-router system get flooded. |
|
@dakejahl are you aware of the fact that we are just discussing this over here? |
|
Thanks I was not aware. Then maybe @peterbarker can comment: would Ardupilot prefer FTP over LOG_DATA as the default mechanism for log download in QGC? |
Transfer via the traditional mechanism is actually marginally faster on ArduPilot - 800kB/s or there-abouts vs 750kB/s on USB. I don't think this really needs to be any form of "default". Ask the vehicle what file it should look at to get a list of logs per #668. In ArduPilot we may synthesise that listing entirely, but at the very least it will allow us to abstract out the actual path to the log files. On stm32-based boards these are in APM/LOGS on every in-tree board. But on Linux the FTP client gets free access to the root of the SD card and the logs actually appear in several different places, depending on the specific Linux board. The contents of the directory are always the same, 'though there's I'm pretty sure the string |
That would indicate something is wrong somewhere wouldn't it. The FTP packets have more room for data than the LOG ones. |
|
On Tue, 17 Feb 2026, Don Gagne wrote:
Transfer via the traditional mechanism is actually marginally faster on ArduPilot - 800kB/s or there-abouts vs 750kB/s on USB.
That would indicate something is wrong somewhere wouldn't it. The FTP packets have more room for data than the LOG ones.
I'm not sure what the limits on the whole burst-read in FTP thing are.
But you can just ask the autopilot to send a log file using the existing
protocol and you just get a stream of messages past that point, no acks
required, so there's never a round-trip overhead for the initial fetch.
In ArduPilot we also make the traditional log transfer from the main
thread (something I've been trying to change for a long time - I have a
branch), so *that* will also give the old mechanism a bit of a leg-up vs
FTP (which is on a thread)
Note that packing FTP packets is a little rather more expensive CPU-wise
than the old messages, too.
In terms of resource allocation it would be far preferable to concentrate
on making FTP faster, for sure!
|
|
A couple of things to note from #13996 and its discussion:
PS FWIW Burst mode should be lots faster. |
|
On Tue, 17 Feb 2026, Hamish Willee wrote:
* If we can't agree a standard virtual drive, then we should add a log location to COMPONENT_METADATA such that a GCS can fetch the generic location of files. This might be on a server
somewhere (suggestion courtesy of @peterbarker)
I think the virtual drive stuff is probably going to work.
... also, it crossed my mind that it's not just "cloud" that means there
might be better ways to get the vehicle than mavftp. You can run a web
server on ArduPilot and apparently it's *much* faster to get the logs over
http than either of ftp or the old protocol; I'll be playing with that
this-afternoon. I think this simply shows that the COMPONENT_METADATA
thing is *complicated* and shouldn't block the virtual filesystem
suggestion here.
|
IMO COMPONENT_METADATA is simpler
But I still want to use virtual drives such as
@dakejahl I'd be interested on your thoughts. Would PX4 be interested in providing an abstraction to a log virtual drive? |
I would think so. |
A little more info. In addition to the above speeds (CubeRedPrimary), I get about the same log-download rates over a 100Mbit TCP connection. OTOH, over http (using a http server written in Lua running on CubeRedPrimary): I've varying reports on how far you can push that up towards the limits implied by the ethernet bitrate. |
|
tridge suggests that giving the client a selection of URLs to download the logs from would be a good idea. e.g. http will likely be fastest but the client may not have a route. |
|
Virtual drive for logs makes sense. I like the idea of using a file to provide the log list and associated URLs. I'd be happy to help work on this both in PX4 and QGC. Can we put together a spec into a doc somewhere? This is my understanding
It's that simple? |
|
On Wed, 25 Feb 2026, Jacob Dahl wrote:
Virtual drive for logs makes sense. I like the idea of using a file to provide the log list and associated URLs. I'd be happy to help work on this both in PX4 and QGC. Can we put
together a spec into a doc somewhere?
This is my understanding
* GCS requests logs lists (via FTP, asking for file @LOG_LIST which is an alias for the actual location?)
* FC sends down the file which contains logs metadata: index, datetime, URL
* GCS FTPs from the URL (which could be onboard the FC or in the cloud somewhere)
It's that simple?
That all sounds about right - but I'd suggest a bog-standard directory
listing of ***@***.***_LOGS is also a reasonable thing, and standard FTP
download of ***@***.***_LOGS/log-20240224-1.BIN would be a thing.
Doc would be good - I'm assuming it would be JSON - and let's make sure to
include a damned json version number in there because it won't be right
the first time :-)
|
|
@dakejahl For the MAVFTP part, we're suggesting This doesn't suggest anything about log lists or whatever, but there is no reason this could not be standardized. Component metadata might further specify the log location for logs that might be exposed via HTTPS - of which this could be one option. |
|
On Thu, 26 Feb 2026, Hamish Willee wrote:
@dakejahl For the MAVFTP part, we're suggesting @MAV_LOG for the virtual directory name. I think we have broad agreement on this mavlink/mavlink-devguide#668 PR - needs prototype if
you're up for it?
For retrieval of logs via FTP, sure.
This doesn't suggest anything about log lists or whatever, but there is no reason this could not be standardized.
Component metadata might further specify the log location for logs that might be exposed via HTTPS - of which this could be one option.
tridge separately suggested something to what @dakejahl was suggesting -
providing a list of alternative fetch mechanisms for each log as it comes
up.
|
Feature Description
Adds MAVLink FTP-based log download for PX4 vehicles, providing faster and more
reliable log transfers than the legacy LOG_REQUEST_DATA protocol. When a PX4
vehicle reports MAV_PROTOCOL_CAPABILITY_FTP, the log download page automatically
uses MAVFTP to list and download .ulg files from
/fs/microsd/log.The legacy LOG_REQUEST protocol remains fully functional for ArduPilot and
non-FTP-capable vehicles.
Implementation Details
MAV_PROTOCOL_CAPABILITY_FTPFTPManager::listDirectory(date subdirs → .ulg files)FTPManager::downloadwith progress/rate displayQGC_ENABLE_MAVFTP_LOG_DOWNLOADcmake option (ON by default)so the legacy protocol can be isolated or deprecated via build flag in the future
MavftpListStateenum tracks listing phase (root vs subdirectory)Testing
Platforms Tested
Flight Stacks Tested
Checklist
Known Limitations
eraseAll()still uses legacyLOG_ERASEMAVLink message when MAVFTP is active(PX4 should still honor this, but FTP-based deletion could be a follow-up)
directory listing support (follow-up)
By submitting this pull request, I confirm that my contribution is made under
the terms of the project's dual license (Apache 2.0 and GPL v3).