From 4685a6d6cfdad962e99b99e692c8f5ddb372d387 Mon Sep 17 00:00:00 2001 From: Don Gagne Date: Wed, 12 Aug 2026 12:36:26 -0700 Subject: [PATCH] fix(AnalyzeView): fall back to message transport when vehicle NAKs FTP list-with-time PX4 <= 1.17 NAKs kCmdListDirectoryWithTime so FTP directory listings carry no modification times, leaving all logs with an unknown date. When FTPManager reports the NAK, fall back to the message based transport where LOG_ENTRY reports the log dates. Fixes #14789 (cherry picked from commit f1883f93a5ba0b5e9c1ec88b3815b290e2608697) --- .../OnboardLogs/OnboardLogController.cc | 15 ++- src/Comms/MockLink/MockLink.cc | 81 ++++++++++++--- src/Comms/MockLink/MockLink.h | 3 + src/Comms/MockLink/MockLinkFTP.h | 3 + src/Vehicle/FTPManager.h | 3 + test/AnalyzeView/OnboardLogDownloadTest.cc | 98 +++++++++++++++++++ test/AnalyzeView/OnboardLogDownloadTest.h | 2 + 7 files changed, 188 insertions(+), 17 deletions(-) diff --git a/src/AnalyzeView/OnboardLogs/OnboardLogController.cc b/src/AnalyzeView/OnboardLogs/OnboardLogController.cc index 38bf478db8ea..bb6cbc3a73ed 100644 --- a/src/AnalyzeView/OnboardLogs/OnboardLogController.cc +++ b/src/AnalyzeView/OnboardLogs/OnboardLogController.cc @@ -532,9 +532,12 @@ void OnboardLogController::cancel() if (_transport == Transport::Ftp) { if (_vehicle) { if (_requestingLogEntries) { - _vehicle->ftpManager()->cancelListDirectory(); + // Idle first: cancelListDirectory() completes synchronously and the abort + // completion must not start the fallback-root listing + _ftpListState = FtpListState::Idle; _ftpDirsToList.clear(); - _ftpFinishListing(); + _vehicle->ftpManager()->cancelListDirectory(); + _setListing(false); } if (_ftpDeleting) { @@ -1111,6 +1114,14 @@ void OnboardLogController::_ftpListNextSubdir() void OnboardLogController::_ftpFinishListing() { _ftpListState = FtpListState::Idle; + + // Firmware which NAKs kCmdListDirectoryWithTime (PX4 <= 1.17) reports no modification times + // over FTP. Fall back to the message based transport where LOG_ENTRY reports the dates (issue #14789). + if (_vehicle && _vehicle->ftpManager()->listDirectoryWithTimeUnsupported()) { + _ftpFallbackToMessages(); + return; + } + _setListing(false); } diff --git a/src/Comms/MockLink/MockLink.cc b/src/Comms/MockLink/MockLink.cc index b57475a0d522..f62a088307f3 100644 --- a/src/Comms/MockLink/MockLink.cc +++ b/src/Comms/MockLink/MockLink.cc @@ -2656,6 +2656,29 @@ void MockLink::_handleLogRequestList(const mavlink_message_t &msg) return; } + // When simulated FTP log files are set, LOG_ENTRY responses describe the same logs so + // both transports report a consistent log list (matching PX4 behavior). + const QList logFiles = _mockLinkFTP->logFiles(); + if (!_logsErased && !logFiles.isEmpty()) { + const uint16_t numLogs = static_cast(logFiles.count()); + for (uint16_t id = 0; id < numLogs; id++) { + mavlink_message_t responseMsg{}; + (void) mavlink_msg_log_entry_pack_chan( + _vehicleSystemId, + _vehicleComponentId, + _outgoingMavlinkChannel, + &responseMsg, + id, // log id + numLogs, // num_logs + numLogs - 1, // last_log_num + logFiles[id].mtime, // time_utc + static_cast(logFiles[id].size) // size + ); + respondWithMavlinkMessage(responseMsg); + } + return; + } + const uint16_t numLogs = _logsErased ? 0 : 1; const uint16_t logId = _logsErased ? 0 : _logDownloadLogId; const uint32_t logSize = _logsErased ? 0 : _logDownloadFileSize; @@ -2705,34 +2728,62 @@ QString MockLink::_createRandomFile(uint32_t byteCount) return tempFile.fileName(); } +QString MockLink::_createLogContentsFile(const QString &logName) +{ + QTemporaryFile tempFile; + tempFile.setAutoRemove(false); + if (!tempFile.open()) { + qCWarning(MockLinkLog) << "_createLogContentsFile open failed" << tempFile.errorString(); + return QString(); + } + (void) tempFile.write(_mockLinkFTP->logFileContents(logName)); + tempFile.close(); + return tempFile.fileName(); +} + void MockLink::_handleLogRequestData(const mavlink_message_t &msg) { mavlink_log_request_data_t request{}; mavlink_msg_log_request_data_decode(&msg, &request); + // Serialize with _logDownloadWorker which reads this state every 2ms on the worker thread + QMutexLocker locker(&_logDownloadMutex); + + const QList logFiles = _logsErased ? QList() : _mockLinkFTP->logFiles(); + if (!logFiles.isEmpty()) { + // Serve the simulated FTP log files so LOG_ENTRY/LOG_REQUEST_DATA stay consistent with the FTP transport + if (request.id >= logFiles.count()) { + qCWarning(MockLinkLog) << "_handleLogRequestData id out of range:" << request.id; + return; + } + if (_logDownloadFilename.isEmpty() || (_logDownloadId != request.id)) { + _logDownloadFilename = _createLogContentsFile(logFiles[request.id].name); + _logDownloadId = request.id; + _logDownloadSize = static_cast(logFiles[request.id].size); + } + } else { #ifdef QGC_UNITTEST_BUILD - if (_logDownloadFilename.isEmpty()) { - _logDownloadFilename = _createRandomFile(_logDownloadFileSize); - } + if (_logDownloadFilename.isEmpty()) { + _logDownloadFilename = _createRandomFile(_logDownloadFileSize); + } #endif - - if (request.id != 0) { - qCWarning(MockLinkLog) << "_handleLogRequestData id must be 0"; - return; + if (request.id != _logDownloadLogId) { + qCWarning(MockLinkLog) << "_handleLogRequestData id must be" << _logDownloadLogId; + return; + } + _logDownloadId = _logDownloadLogId; + _logDownloadSize = _logDownloadFileSize; } - if (request.ofs > (_logDownloadFileSize - 1)) { - qCWarning(MockLinkLog) << "_handleLogRequestData offset past end of file request.ofs:size" << request.ofs << _logDownloadFileSize; + if (request.ofs > (_logDownloadSize - 1)) { + qCWarning(MockLinkLog) << "_handleLogRequestData offset past end of file request.ofs:size" << request.ofs << _logDownloadSize; return; } // This will trigger _logDownloadWorker to send data - // Thread-safe access: Main thread writes, worker thread reads every 2ms. Serialize to avoid - // worker reading inconsistent offset/count or using stale values while downloading. - QMutexLocker locker(&_logDownloadMutex); _logDownloadCurrentOffset = request.ofs; - if (request.ofs + request.count > _logDownloadFileSize) { - request.count = _logDownloadFileSize - request.ofs; + if (request.ofs + request.count > _logDownloadSize) { + request.count = _logDownloadSize - request.ofs; } _logDownloadBytesRemaining = request.count; } @@ -2775,7 +2826,7 @@ void MockLink::_logDownloadWorker() _vehicleComponentId, _outgoingMavlinkChannel, &responseMsg, - _logDownloadLogId, + _logDownloadId, _logDownloadCurrentOffset, bytesToRead, &buffer[0] diff --git a/src/Comms/MockLink/MockLink.h b/src/Comms/MockLink/MockLink.h index 240a7aad6d6f..3b6f59f6929c 100644 --- a/src/Comms/MockLink/MockLink.h +++ b/src/Comms/MockLink/MockLink.h @@ -336,6 +336,7 @@ private slots: /// Creates a file with random contents of the specified size. /// @return Fully qualified path to created file static QString _createRandomFile(uint32_t byteCount); + QString _createLogContentsFile(const QString &logName); QThread *_workerThread = nullptr; MockLinkWorker *_worker = nullptr; @@ -426,6 +427,8 @@ private slots: QString _logDownloadFilename; ///< Filename for log download which is in progress bool _logsErased = false; ///< Set by LOG_ERASE, LOG_REQUEST_LIST reports no logs + uint16_t _logDownloadId = 0; ///< Log id being served, echoed in LOG_DATA + uint32_t _logDownloadSize = 0; ///< Size of the log being served uint32_t _logDownloadCurrentOffset = 0; ///< Current offset we are sending from uint32_t _logDownloadBytesRemaining = 0; ///< Number of bytes still to send, 0 = send inactive /// Protects log download state from race conditions between: diff --git a/src/Comms/MockLink/MockLinkFTP.h b/src/Comms/MockLink/MockLinkFTP.h index b1e8ddb5175c..a85d8c17ec32 100644 --- a/src/Comms/MockLink/MockLinkFTP.h +++ b/src/Comms/MockLink/MockLinkFTP.h @@ -35,6 +35,9 @@ class MockLinkFTP : public QObject /// Removes any temp files cached for the previous log set. void setLogFiles(const QList &logFiles); + /// Returns the log files served from the @MAV_LOG virtual directory. + QList logFiles() const { return _logFiles; } + /// Returns the deterministic contents served for the named @MAV_LOG log file. /// Empty if the name is unknown. QByteArray logFileContents(const QString &name) const; diff --git a/src/Vehicle/FTPManager.h b/src/Vehicle/FTPManager.h index 5ba932ef2642..2e1a63b08afa 100644 --- a/src/Vehicle/FTPManager.h +++ b/src/Vehicle/FTPManager.h @@ -48,6 +48,9 @@ class FTPManager : public QObject /// Signals listDirectoryComplete bool listDirectory(uint8_t fromCompId, const QString& fromURI); + /// true when the vehicle NAK'ed kCmdListDirectoryWithTime, i.e. directory listings carry no modification times. + bool listDirectoryWithTimeUnsupported() const { return _listDirWithTimeSupport == WithTimeSupport_t::Unsupported; } + /// Deletes a file on the vehicle. /// @param fromCompId Component id of the component to delete from. If fromCompId is MAV_COMP_ID_ALL, then MAV_COMP_ID_AUTOPILOT1 is used. /// @param fromURI File path to delete on the component. May include mftp:// scheme and optional component id selector. diff --git a/test/AnalyzeView/OnboardLogDownloadTest.cc b/test/AnalyzeView/OnboardLogDownloadTest.cc index 848355cdef23..274cdf7b119a 100644 --- a/test/AnalyzeView/OnboardLogDownloadTest.cc +++ b/test/AnalyzeView/OnboardLogDownloadTest.cc @@ -5,6 +5,7 @@ #include #include +#include "FTPManager.h" #include "OnboardLogController.h" #include "OnboardLogEntry.h" #include "MAVLinkProtocol.h" @@ -250,6 +251,103 @@ void OnboardLogFtpDownloadTest::_ftpListAndDownloadTest() QCOMPARE(file.readAll(), _mockLink->mockLinkFTP()->logFileContents(QStringLiteral("log_1.ulg"))); } +void OnboardLogFtpDownloadTest::_ftpListNoTimeFallbackTest() +{ + _connectMockLink(MAV_AUTOPILOT_PX4, MockConfiguration::FailNone, MockConfiguration::OptionFtpCapability); + if (QTest::currentTestFailed()) return; + + // Simulate firmware (PX4 <= 1.17) which doesn't implement kCmdListDirectoryWithTime: + // the FTP listing has no modification times so the controller must fall back to the + // message based transport where LOG_ENTRY reports the dates. + const QList logFiles = { + { QStringLiteral("log_1.ulg"), 5000, 1700000000 }, + { QStringLiteral("log_2.ulg"), 12345, 1700086400 }, + }; + _mockLink->mockLinkFTP()->setLogFiles(logFiles); + _mockLink->mockLinkFTP()->setListDirectoryWithTimeSupported(false); + + OnboardLogController* const controller = new OnboardLogController(this); + MultiSignalSpy* multiSpy = new MultiSignalSpy(this); + QVERIFY(multiSpy->init(controller)); + + QVERIFY(refreshAndWaitForListComplete(controller, multiSpy)); + + QCOMPARE(controller->transport(), QStringLiteral("messages")); + + QmlObjectListModel* const model = controller->_getModel(); + QVERIFY(model); + QCOMPARE(model->count(), 2); + + QGCOnboardLogEntry *firstLog = nullptr; + QGCOnboardLogEntry *secondLog = nullptr; + for (int i = 0; i < model->count(); i++) { + QGCOnboardLogEntry *const entry = model->value(i); + QVERIFY(entry); + QVERIFY(entry->received()); + if (entry->size() == 5000) { + firstLog = entry; + } else if (entry->size() == 12345) { + secondLog = entry; + } + } + QVERIFY(firstLog); + QVERIFY(secondLog); + + // Dates come from the LOG_ENTRY time_utc values + QCOMPARE(firstLog->time(), QDateTime::fromSecsSinceEpoch(1700000000, QTimeZone::UTC)); + QCOMPARE(secondLog->time(), QDateTime::fromSecsSinceEpoch(1700086400, QTimeZone::UTC)); + + // Message downloads of the advertised logs must serve the matching per-id contents + secondLog->setSelected(true); + QTemporaryDir tempDir; + QVERIFY(tempDir.isValid()); + QVERIFY(downloadAndWaitForComplete(controller, multiSpy, tempDir.path())); + QCOMPARE(secondLog->status(), QStringLiteral("Downloaded")); + + // Filename embeds the local-time formatted log date so locate it by directory scan + const QStringList downloadedFiles = QDir(tempDir.path()).entryList(QDir::Files); + QCOMPARE(downloadedFiles.count(), 1); + QFile file(QDir(tempDir.path()).filePath(downloadedFiles.first())); + QVERIFY(file.open(QIODevice::ReadOnly)); + QCOMPARE(file.readAll(), _mockLink->mockLinkFTP()->logFileContents(QStringLiteral("log_2.ulg"))); +} + +void OnboardLogFtpDownloadTest::_ftpCancelListNoFallbackTest() +{ + _connectMockLink(MAV_AUTOPILOT_PX4, MockConfiguration::FailNone, MockConfiguration::OptionFtpCapability); + if (QTest::currentTestFailed()) return; + + _mockLink->mockLinkFTP()->setLogFiles({ { QStringLiteral("log_1.ulg"), 5000, 1700000000 } }); + _mockLink->mockLinkFTP()->setListDirectoryWithTimeSupported(false); + + // Prime FTPManager's cached NAK of kCmdListDirectoryWithTime with a listing which + // doesn't involve the controller + FTPManager* const ftpManager = _vehicle->ftpManager(); + QSignalSpy listSpy(ftpManager, &FTPManager::listDirectoryComplete); + QVERIFY(ftpManager->listDirectory(MAV_COMP_ID_AUTOPILOT1, QStringLiteral("@MAV_LOG"))); + QVERIFY(listSpy.wait(FTPManager::kTestOperationMaxWaitMs)); + QVERIFY(ftpManager->listDirectoryWithTimeUnsupported()); + + OnboardLogController* const controller = new OnboardLogController(this); + MultiSignalSpy* multiSpy = new MultiSignalSpy(this); + QVERIFY(multiSpy->init(controller)); + + // Canceling while the FTP listing is still in progress must stop the listing + // without triggering the message transport fallback + controller->refresh(); + QVERIFY(controller->_getRequestingList()); + controller->cancel(); + + QVERIFY(!controller->_getRequestingList()); + QCOMPARE(controller->transport(), QStringLiteral("ftp")); + + // The synchronous Abort completion must not spawn a replacement root listing: + // FTPManager must be idle immediately after cancel + QSignalSpy idleCheckSpy(ftpManager, &FTPManager::listDirectoryComplete); + QVERIFY(ftpManager->listDirectory(MAV_COMP_ID_AUTOPILOT1, QStringLiteral("@MAV_LOG"))); + QVERIFY(idleCheckSpy.wait(FTPManager::kTestOperationMaxWaitMs)); +} + void OnboardLogFtpDownloadTest::_ftpListFallbackTest() { _connectMockLink(MAV_AUTOPILOT_PX4, MockConfiguration::FailNone, MockConfiguration::OptionFtpCapability); diff --git a/test/AnalyzeView/OnboardLogDownloadTest.h b/test/AnalyzeView/OnboardLogDownloadTest.h index 41ac83125b54..2c79ca634f55 100644 --- a/test/AnalyzeView/OnboardLogDownloadTest.h +++ b/test/AnalyzeView/OnboardLogDownloadTest.h @@ -26,6 +26,8 @@ class OnboardLogFtpDownloadTest : public VehicleTestManualConnect private slots: void _ftpListAndDownloadTest(); + void _ftpListNoTimeFallbackTest(); + void _ftpCancelListNoFallbackTest(); void _ftpListFallbackTest(); void _ftpMultiDownloadAndDedupTest(); void _ftpDownloadErrorDisablesFtpTest();