fix(StateMachine): replace isRunning() with queued-start-aware inProgress()

QStateMachine::start() is queued, so isRunning() stays false until the
event loop processes the start. QGCStateMachine::active() returned
isRunning(), so Vehicle::isInitialConnectComplete(), which is !active(),
reported a newly created vehicle as complete before AUTOPILOT_VERSION had
been requested. On loaded macOS CI runners the 20 ms active-vehicle timer
can fire ahead of the queued start, so VehicleTest::waitForInitialConnect()
returned immediately and tests such as OnboardLogFtpDownloadTest failed on
capabilitiesKnown(). This regressed in #13981: the old StateMachine set its
active flag synchronously in start().

Having both active() and isRunning() with subtly different meanings is
what allowed the regression, so QGCStateMachine now has a single notion:

- inProgress() (property, inProgressChanged) is set in start() and cleared
  at the top of the stopped/finished handlers, so later handlers already
  see the machine as no longer in progress.
- active(), the running property and runningChanged are removed.
- isRunning() and setRunning() are deleted on QGCStateMachine, so using
  them fails to compile.
- start() logs qCCritical and returns when already in progress or when
  there is no initial state, rather than relying on Qt's refusal rules.
- Starting without QGCStateMachine::start(), e.g. through a QStateMachine*
  or setRunning(), is logged with qCCritical.

Vehicle, ComponentInformationManager, SubMachineState and the
QGCStateMachine lifecycle helpers use inProgress().
ComponentInformationManager's first progress update no longer reports
100% while its start is queued. StateMachineTest::startAndWaitForFinished()
now starts QGCStateMachines through QGCStateMachine::start().

Author: DonLakeFlyer

src/Utilities/StateMachine/QGCStateMachine.cc

@@ -20,16 +20,24 @@ QGCStateMachine::QGCStateMachine(const QString& machineName, Vehicle *vehicle, Q
 
     connect(this, &QGCStateMachine::started, this, [this] () {
         qCDebug(QGCStateMachineLog) << "State machine started:" << objectName();
-        emit runningChanged();
+        if (!_inProgress) {
+            qCCritical(QGCStateMachineLog)
+                << objectName()
+                << "started without QGCStateMachine::start() - QStateMachine::start() or setRunning() was used";
+            _setInProgress(true);
+        }
     });
-    connect(this, &QGCStateMachine::stopped, this, [this] () {
+    // Cleared before anything else reacts to stopped/finished so handlers, including the final progress update,
+    // already see the machine as no longer in progress
+    connect(this, &QGCStateMachine::stopped, this, [this]() {
+        _setInProgress(false);
         qCDebug(QGCStateMachineLog) << "State machine stopped:" << objectName();
-        emit runningChanged();
         if (_deleteOnStop) {
             deleteLater();
         }
     });
-    connect(this, &QGCStateMachine::finished, this, [this] () {
+    connect(this, &QGCStateMachine::finished, this, [this]() {
+        _setInProgress(false);
         qCDebug(QGCStateMachineLog) << "State machine finished:" << objectName();
         if (_progressTotalWeight > 0 && _progressLastEmitted < 1.0f) {
             _progressLastEmitted = 1.0f;
@@ -61,10 +69,27 @@ QString QGCStateMachine::currentStateName() const
 
 void QGCStateMachine::start()
 {
-    if (isRunning()) {
-        qCCritical(QGCStateMachineLog) << objectName() << "start() called but already running - check signal connections";
+    if (_inProgress) {
+        qCCritical(QGCStateMachineLog) << objectName()
+                                       << "start() called but already in progress - check signal connections";
+        return;
+    }
+    if ((childMode() == QState::ExclusiveStates) && !initialState()) {
+        qCCritical(QGCStateMachineLog) << objectName() << "start() called without an initial state";
+        return;
     }
+    _inProgress = true;
     QStateMachine::start();
+    // Emitted after Qt's start so a stop() from a listener is honored
+    emit inProgressChanged();
+}
+
+void QGCStateMachine::_setInProgress(bool inProgress)
+{
+    if (_inProgress != inProgress) {
+        _inProgress = inProgress;
+        emit inProgressChanged();
+    }
 }
 
 void QGCStateMachine::setGlobalErrorState(QAbstractState* errorState)
@@ -330,7 +355,7 @@ bool QGCStateMachine::resetToState(QAbstractState* state)
     qCDebug(QGCStateMachineLog) << objectName() << "resetting to state:" << state->objectName();
 
     // Stop the machine, change initial state, and restart
-    if (isRunning()) {
+    if (inProgress()) {
         auto conn = std::make_shared<QMetaObject::Connection>();
         *conn = connect(this, &QStateMachine::stopped, this, [this, state, conn]() {
             disconnect(*conn);
@@ -377,7 +402,7 @@ bool QGCStateMachine::attemptRecovery()
 
 void QGCStateMachine::ensureRunning()
 {
-    if (!isRunning()) {
+    if (!inProgress()) {
         start();
     }
 }
@@ -390,7 +415,7 @@ void QGCStateMachine::stopMachine(bool deleteOnStop)
 
 void QGCStateMachine::restart()
 {
-    if (isRunning()) {
+    if (inProgress()) {
         // stop() is asynchronous, so connect to stopped signal to start again
         auto conn = std::make_shared<QMetaObject::Connection>();
         *conn = connect(this, &QStateMachine::stopped, this, [this, conn]() {
@@ -490,7 +515,7 @@ QString QGCStateMachine::dumpCurrentState() const
 {
     QString result = objectName() + QStringLiteral(": ");
 
-    if (!isRunning()) {
+    if (!inProgress()) {
         result += QStringLiteral("(not running)");
         return result;
     }

src/Utilities/StateMachine/QGCStateMachine.h

@@ -70,8 +70,8 @@ class QGCStateMachine : public QStateMachine
     /// Current progress from 0.0 to 1.0 (for QML binding)
     Q_PROPERTY(float progress READ progress NOTIFY progressUpdate)
 
-    /// Whether the machine is currently running (for QML binding)
-    Q_PROPERTY(bool running READ isRunning NOTIFY runningChanged)
+    /// Whether the machine is in progress (for QML binding)
+    Q_PROPERTY(bool inProgress READ inProgress NOTIFY inProgressChanged)
 
     /// List of recent state names for history display (for QML binding)
     Q_PROPERTY(QStringList stateHistory READ stateHistory NOTIFY stateHistoryChanged)
@@ -91,9 +91,15 @@ class QGCStateMachine : public QStateMachine
     StateContext& context() { return _context; }
     const StateContext& context() const { return _context; }
 
-    /// @return true if the state machine is currently running
-    /// Compatibility method matching old StateMachine::active()
-    bool active() const { return isRunning(); }
+    /// @return true from start() until the machine emits stopped or finished
+    /// Unlike QStateMachine::isRunning(), this is already true before Qt processes the queued start.
+    bool inProgress() const { return _inProgress; }
+
+    /// Use inProgress() instead: isRunning() is false until Qt processes the queued start
+    bool isRunning() const = delete;
+
+    /// Use start() and stop() instead: setRunning() calls QStateMachine::start() and bypasses start()
+    void setRunning(bool running) = delete;
 
     /// Get the name of the currently active state
     /// @return State name or empty string if not running
@@ -370,7 +376,7 @@ class QGCStateMachine : public QStateMachine
     // Lifecycle Helpers
     // -------------------------------------------------------------------------
 
-    /// Start the machine if not already running
+    /// Start the machine if not already in progress
     void ensureRunning();
 
     /// Stop the machine and optionally delete it
@@ -685,8 +691,8 @@ public slots:
     /// Emitted when the current state changes (for QML binding)
     void currentStateNameChanged();
 
-    /// Emitted when the running state changes (for QML binding)
-    void runningChanged();
+    /// Emitted when inProgress() changes (for QML binding)
+    void inProgressChanged();
 
     /// Emitted when the state history changes (for QML binding)
     void stateHistoryChanged();
@@ -705,12 +711,14 @@ public slots:
 
 private:
     void _onStateEntered();
+    void _setInProgress(bool inProgress);
     float _calculateProgress() const;
 
     Vehicle* _vehicle = nullptr;
     QAbstractState* _globalErrorState = nullptr;
     QAbstractState* _recoveryState = nullptr;
     bool _deleteOnStop = false;
+    bool _inProgress = false;
     EntryCallback _entryCallback;
     ExitCallback _exitCallback;
     EventHandler _eventHandler;

src/Utilities/StateMachine/States/SubMachineState.cc

@@ -35,7 +35,7 @@ void SubMachineState::onEntry(QEvent* event)
 
 void SubMachineState::onExit(QEvent* event)
 {
-    if (_childMachine && _childMachine->isRunning()) {
+    if (_childMachine && _childMachine->inProgress()) {
         qCDebug(QGCStateMachineLog) << stateName() << "stopping child machine on exit";
         _childMachine->stop();
     }

src/Vehicle/ComponentInformation/ComponentInformationManager.cc

@@ -166,7 +166,7 @@ void ComponentInformationManager::_updateProgress()
 
 float ComponentInformationManager::progress() const
 {
-    if (!isRunning()) {
+    if (!inProgress()) {
         return 1.0f;
     }
     return static_cast<float>(_currentStateIndex) / static_cast<float>(_stateCount);
@@ -181,7 +181,8 @@ void ComponentInformationManager::requestAllComponentInformation(RequestAllCompl
 
     // Guard against double-start: a request while already running just updates the
     // callback pointers; the running machine still emits requestAllComplete at the end.
-    if (!isRunning()) {
+    if (!inProgress()) {
+        _currentStateIndex = 0;
         start();
     }
     emit progressUpdate(progress());

src/Vehicle/Vehicle.cc

@@ -711,7 +711,7 @@ void Vehicle::_mavlinkMessageReceived(LinkInterface* link, mavlink_message_t mes
         case MAVLINK_MSG_ID_AVAILABLE_MODES_MONITOR:
     {
         // Avoid duplicate requests during initial connection setup
-        if (!_initialConnectStateMachine || !_initialConnectStateMachine->active()) {
+        if (!_initialConnectStateMachine || !_initialConnectStateMachine->inProgress()) {
             mavlink_available_modes_monitor_t availableModesMonitor;
             mavlink_msg_available_modes_monitor_decode(&message, &availableModesMonitor);
             _standardModes->availableModesMonitorReceived(availableModesMonitor.seq);
@@ -1655,10 +1655,10 @@ void Vehicle::_updateFlightTime()
 
 void Vehicle::_gotProgressUpdate(float progressValue)
 {
-    if (sender() != _initialConnectStateMachine && _initialConnectStateMachine->active()) {
+    if (sender() != _initialConnectStateMachine && _initialConnectStateMachine->inProgress()) {
         return;
     }
-    if (sender() == _initialConnectStateMachine && !_initialConnectStateMachine->active()) {
+    if (sender() == _initialConnectStateMachine && !_initialConnectStateMachine->inProgress()) {
         progressValue = 0.f;
     }
     _loadProgress = progressValue;
@@ -2872,7 +2872,7 @@ QString Vehicle::_formatMavCommand(MAV_CMD command, float param1)
 
 bool Vehicle::isInitialConnectComplete() const
 {
-    return !_initialConnectStateMachine->active();
+    return !_initialConnectStateMachine->inProgress();
 }
 
 void Vehicle::_initializeCsv()

test/UnitTestFramework/BaseClasses/StateMachineTest.cc

@@ -4,6 +4,8 @@
 #include <QtStateMachine/QStateMachine>
 #include <QtTest/QSignalSpy>
 
+#include "QGCStateMachine.h"
+
 void StateMachineTest::ignoreTimeoutWarnings()
 {
     ignoreLogMessage("Utilities.QGCStateMachine", QtWarningMsg, QRegularExpression(QStringLiteral("^Timeout \"")));
@@ -13,7 +15,11 @@ void StateMachineTest::ignoreTimeoutWarnings()
 bool StateMachineTest::startAndWaitForFinished(QStateMachine* machine, int timeoutMs)
 {
     QSignalSpy finishedSpy(machine, &QStateMachine::finished);
-    machine->start();
+    if (auto* qgcMachine = qobject_cast<QGCStateMachine*>(machine)) {
+        qgcMachine->start();
+    } else {
+        machine->start();
+    }
     return spyTriggered(finishedSpy, timeoutMs);
 }
 

test/Utilities/StateMachine/QGCStateMachineTest.cc

@@ -157,6 +157,138 @@ void QGCStateMachineTest::_testLocalErrorState()
     QVERIFY(localErrorHandled);    // Local error should be called
 }
 
+void QGCStateMachineTest::_testInProgressSpansQueuedStart_data()
+{
+    QTest::addColumn<bool>("hasInitialState");
+    QTest::addColumn<bool>("stopBeforeStartRuns");
+
+    QTest::newRow("runsToFinish") << true << false;
+    QTest::newRow("stoppedBeforeStartRuns") << true << true;
+    QTest::newRow("refusedWithoutInitialState") << false << false;
+}
+
+void QGCStateMachineTest::_testInProgressSpansQueuedStart()
+{
+    QFETCH(bool, hasInitialState);
+    QFETCH(bool, stopBeforeStartRuns);
+
+    QGCStateMachine machine(QStringLiteral("InProgressSpansQueuedStartTest"), nullptr);
+    auto* state = new QGCState(QStringLiteral("State"), &machine);
+    auto* finalState = machine.addFinalState();
+    machine.addEventTransition(state, QStringLiteral("finish"), finalState);
+    QSignalSpy inProgressSpy(&machine, &QGCStateMachine::inProgressChanged);
+
+    if (!hasInitialState) {
+        expectLogMessage("Utilities.QGCStateMachine", QtCriticalMsg,
+                         QRegularExpression(QStringLiteral("start\\(\\) called without an initial state")));
+        machine.start();
+        verifyExpectedLogMessage();
+        QVERIFY(!machine.inProgress());
+        QCOMPARE(inProgressSpy.count(), 0);
+        return;
+    }
+
+    machine.setInitialState(state);
+    QSignalSpy startedSpy(&machine, &QStateMachine::started);
+    QSignalSpy stoppedSpy(&machine, &QStateMachine::stopped);
+    QSignalSpy finishedSpy(&machine, &QStateMachine::finished);
+
+    // Listeners must already see the machine as not in progress, e.g. to restart it or treat it as complete
+    int listenerCalls = 0;
+    bool inProgressInListener = true;
+    const auto recordInProgress = [&machine, &listenerCalls, &inProgressInListener]() {
+        listenerCalls++;
+        inProgressInListener = machine.inProgress();
+    };
+    (void) connect(&machine, &QStateMachine::stopped, this, recordInProgress);
+    (void) connect(&machine, &QStateMachine::finished, this, recordInProgress);
+
+    machine.start();
+    QVERIFY(!machine.QStateMachine::isRunning());
+    QVERIFY(machine.inProgress());
+    QCOMPARE(inProgressSpy.count(), 1);
+
+    if (stopBeforeStartRuns) {
+        machine.stop();
+        QVERIFY(machine.inProgress());
+    } else {
+        QVERIFY(spyTriggered(startedSpy));
+        QVERIFY(machine.inProgress());
+        machine.postEvent(QStringLiteral("finish"));
+    }
+
+    QVERIFY(spyTriggered(stopBeforeStartRuns ? stoppedSpy : finishedSpy));
+    QCOMPARE(listenerCalls, 1);
+    QVERIFY(!inProgressInListener);
+    QVERIFY(!machine.inProgress());
+    QCOMPARE(inProgressSpy.count(), 2);
+}
+
+void QGCStateMachineTest::_testStopFromInProgressListenerIsHonored()
+{
+    QGCStateMachine machine(QStringLiteral("StopFromInProgressListenerTest"), nullptr);
+    auto* state = new QGCState(QStringLiteral("State"), &machine);
+    machine.setInitialState(state);
+    QSignalSpy stoppedSpy(&machine, &QStateMachine::stopped);
+
+    (void) connect(&machine, &QGCStateMachine::inProgressChanged, this, [&machine]() {
+        if (machine.inProgress()) {
+            machine.stop();
+        }
+    });
+
+    machine.start();
+
+    QVERIFY(spyTriggered(stoppedSpy));
+    QVERIFY(!machine.inProgress());
+}
+
+void QGCStateMachineTest::_testStartWhileInProgressIsCritical()
+{
+    QGCStateMachine machine(QStringLiteral("StartWhileInProgressTest"), nullptr);
+    auto* state = new QGCState(QStringLiteral("State"), &machine);
+    auto* finalState = machine.addFinalState();
+    machine.addEventTransition(state, QStringLiteral("finish"), finalState);
+    machine.setInitialState(state);
+    QSignalSpy startedSpy(&machine, &QStateMachine::started);
+    QSignalSpy finishedSpy(&machine, &QStateMachine::finished);
+
+    machine.start();
+    expectLogMessage("Utilities.QGCStateMachine", QtCriticalMsg,
+                     QRegularExpression(QStringLiteral("already in progress")));
+    machine.start();
+    verifyExpectedLogMessage();
+
+    QVERIFY(spyTriggered(startedSpy));
+    machine.postEvent(QStringLiteral("finish"));
+    QVERIFY(spyTriggered(finishedSpy));
+    QCOMPARE(startedSpy.count(), 1);
+    QVERIFY(!machine.inProgress());
+}
+
+void QGCStateMachineTest::_testStartBypassIsCritical()
+{
+    QGCStateMachine machine(QStringLiteral("StartBypassTest"), nullptr);
+    auto* state = new QGCState(QStringLiteral("State"), &machine);
+    auto* finalState = machine.addFinalState();
+    machine.addEventTransition(state, QStringLiteral("finish"), finalState);
+    machine.setInitialState(state);
+    QSignalSpy startedSpy(&machine, &QStateMachine::started);
+    QSignalSpy finishedSpy(&machine, &QStateMachine::finished);
+
+    expectLogMessage("Utilities.QGCStateMachine", QtCriticalMsg,
+                     QRegularExpression(QStringLiteral("started without QGCStateMachine::start\\(\\)")));
+    static_cast<QStateMachine&>(machine).start();
+    QVERIFY(!machine.inProgress());
+    QVERIFY(spyTriggered(startedSpy));
+    verifyExpectedLogMessage();
+    QVERIFY(machine.inProgress());
+
+    machine.postEvent(QStringLiteral("finish"));
+    QVERIFY(spyTriggered(finishedSpy));
+    QVERIFY(!machine.inProgress());
+}
+
 void QGCStateMachineTest::_testPropertyAssignment()
 {
     QGCStateMachine machine(QStringLiteral("PropTest"), nullptr);

test/Utilities/StateMachine/QGCStateMachineTest.h

@@ -12,6 +12,11 @@ private slots:
     void _testGlobalErrorState();
     void _testGlobalErrorStateWithAbstractState();
     void _testLocalErrorState();
+    void _testInProgressSpansQueuedStart_data();
+    void _testInProgressSpansQueuedStart();
+    void _testStopFromInProgressListenerIsHonored();
+    void _testStartWhileInProgressIsCritical();
+    void _testStartBypassIsCritical();
 
     // Property assignment
     void _testPropertyAssignment();