fix(AnalyzeView): fall back to message transport when vehicle NAKs FTP list-with-time (cherry-pick to Stable_V5.1)

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);
 }
 

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<MockLinkFTP::LogFile> logFiles = _mockLinkFTP->logFiles();
+    if (!_logsErased && !logFiles.isEmpty()) {
+        const uint16_t numLogs = static_cast<uint16_t>(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<uint32_t>(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<MockLinkFTP::LogFile> logFiles = _logsErased ? QList<MockLinkFTP::LogFile>() : _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<uint32_t>(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]

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:

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<LogFile> &logFiles);
 
+    /// Returns the log files served from the @MAV_LOG virtual directory.
+    QList<LogFile> 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;

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.

test/AnalyzeView/OnboardLogDownloadTest.cc

@@ -5,6 +5,7 @@
 #include <QtCore/QTimeZone>
 #include <QtCore/QTimer>
 
+#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<MockLinkFTP::LogFile> 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<QGCOnboardLogEntry*>(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);

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();