From bea8d30eea1d10dbc486ae2fddc8996871153b49 Mon Sep 17 00:00:00 2001 From: Ihor Kalnytskyi Date: Mon, 20 Jul 2026 23:44:43 +0300 Subject: [PATCH] Fix bytewise parameter write acknowledgements When QGC writes a parameter, it waits for the vehicle to echo the new value in PARAM_VALUE. The acknowledgement is accepted only when both the value and the QVariant type match QGC's stored Fact value. For 8- & 16-bit parameters, FactMetaData stores the value in a QVariant: Qt Int (32-bit) for signed values and 32-bit Qt UInt (32-bit) for unsigned values. This preserves the parameter's signedness in QGC's internal representation. The `_mavlinkParamUnionToVariant` method passed `uint8_t` and `uint16_t` directly to QVariant. C++ selected QVariant's `int` constructor, so an unsigned PARAM_VALUE was decoded as signed Qt Int. The type check then rejected the otherwise valid acknowledgement and the write timed out. Decode narrow values into QGC's canonical Int or UInt representations. The bug affects only UINT8 and UINT16 parameters; the other supported types coincidentally work. It flew below the radar because ArduPilot uses float-based parameter encoding, while PX4 does not expose UINT8 or UINT16 parameters for which the problem exhibits itself. --- src/Comms/MockLink/CMakeLists.txt | 1 + src/Comms/MockLink/GenericMockLink.params | 3 ++ src/Comms/MockLink/MockLink.cc | 2 ++ src/FactSystem/ParameterManager.cc | 8 ++--- test/FactSystem/ParameterManagerTest.cc | 36 ++++++++++++++++++----- test/FactSystem/ParameterManagerTest.h | 5 +++- 6 files changed, 42 insertions(+), 13 deletions(-) create mode 100644 src/Comms/MockLink/GenericMockLink.params diff --git a/src/Comms/MockLink/CMakeLists.txt b/src/Comms/MockLink/CMakeLists.txt index 39830f21450b..4832441c05b2 100644 --- a/src/Comms/MockLink/CMakeLists.txt +++ b/src/Comms/MockLink/CMakeLists.txt @@ -29,6 +29,7 @@ set_source_files_properties(MockLink.Parameter.MetaData.json PROPERTIES QT_RESOU qt_add_resources(${CMAKE_PROJECT_NAME} mocklink_resources PREFIX "/MockLink" FILES + GenericMockLink.params PX4MockLink.params MockLink.General.MetaData.json MockLink.General.MetaData.json.xz diff --git a/src/Comms/MockLink/GenericMockLink.params b/src/Comms/MockLink/GenericMockLink.params new file mode 100644 index 000000000000..bed39dd5b1f7 --- /dev/null +++ b/src/Comms/MockLink/GenericMockLink.params @@ -0,0 +1,3 @@ +# Vehicle-Id Component-Id Name Value Type +1 1 TEST_UINT8 1 1 +1 1 TEST_UINT16 1 3 diff --git a/src/Comms/MockLink/MockLink.cc b/src/Comms/MockLink/MockLink.cc index aa048cf85233..4d2f26ff96f1 100644 --- a/src/Comms/MockLink/MockLink.cc +++ b/src/Comms/MockLink/MockLink.cc @@ -397,6 +397,8 @@ void MockLink::_loadParams() } else { paramFile.setFileName(":/FirmwarePlugin/APM/Copter.OfflineEditing.params"); } + } else if (_firmwareType == MAV_AUTOPILOT_GENERIC) { + paramFile.setFileName(":/MockLink/GenericMockLink.params"); } else { paramFile.setFileName(":/MockLink/PX4MockLink.params"); } diff --git a/src/FactSystem/ParameterManager.cc b/src/FactSystem/ParameterManager.cc index 6a1de425db69..cdf269ba21df 100644 --- a/src/FactSystem/ParameterManager.cc +++ b/src/FactSystem/ParameterManager.cc @@ -489,16 +489,16 @@ bool ParameterManager::_mavlinkParamUnionToVariant(const mavlink_param_union_t & outValue = QVariant(paramUnion.param_float); return true; case MAV_PARAM_TYPE_UINT8: - outValue = QVariant(paramUnion.param_uint8); + outValue = QVariant(static_cast(paramUnion.param_uint8)); return true; case MAV_PARAM_TYPE_INT8: - outValue = QVariant(paramUnion.param_int8); + outValue = QVariant(static_cast(paramUnion.param_int8)); return true; case MAV_PARAM_TYPE_UINT16: - outValue = QVariant(paramUnion.param_uint16); + outValue = QVariant(static_cast(paramUnion.param_uint16)); return true; case MAV_PARAM_TYPE_INT16: - outValue = QVariant(paramUnion.param_int16); + outValue = QVariant(static_cast(paramUnion.param_int16)); return true; case MAV_PARAM_TYPE_UINT32: outValue = QVariant(paramUnion.param_uint32); diff --git a/test/FactSystem/ParameterManagerTest.cc b/test/FactSystem/ParameterManagerTest.cc index 13297321c0ab..123aba4b89cf 100644 --- a/test/FactSystem/ParameterManagerTest.cc +++ b/test/FactSystem/ParameterManagerTest.cc @@ -135,7 +135,8 @@ void ParameterManagerTest::_paramWriteNoAckRetry() // BAT1_V_CHARGED requires a vehicle reboot, so writing it pops the reboot // app message (debounce is reset per-test by the framework) expectAppMessage(QRegularExpression("Reboot vehicle for changes to take effect")); - _setParamWithFailureMode(MockLink::FailParamSetFirstAttemptNoAck, true /* expectSuccess */); + _setParamWithFailureMode(MockLink::FailParamSetFirstAttemptNoAck, true /* expectSuccess */, + QStringLiteral("BAT1_V_CHARGED"), MAV_AUTOPILOT_PX4); verifyExpectedLogMessage(); } @@ -145,11 +146,24 @@ void ParameterManagerTest::_paramWriteNoAckPermanent() // setRawValue), then the write-failed message (fires after retries exhaust) expectAppMessage(QRegularExpression("Reboot vehicle for changes to take effect")); expectAppMessage(QRegularExpression("Parameter write failed")); - _setParamWithFailureMode(MockLink::FailParamSetNoAck, false /* expectSuccess */); + _setParamWithFailureMode(MockLink::FailParamSetNoAck, false /* expectSuccess */, + QStringLiteral("BAT1_V_CHARGED"), MAV_AUTOPILOT_PX4); verifyExpectedLogMessage(); verifyExpectedLogMessage(); } +void ParameterManagerTest::_paramWriteUInt8() +{ + _setParamWithFailureMode(MockLink::FailParamSetNone, true /* expectSuccess */, + QStringLiteral("TEST_UINT8"), MAV_AUTOPILOT_GENERIC); +} + +void ParameterManagerTest::_paramWriteUInt16() +{ + _setParamWithFailureMode(MockLink::FailParamSetNone, true /* expectSuccess */, + QStringLiteral("TEST_UINT16"), MAV_AUTOPILOT_GENERIC); +} + void ParameterManagerTest::_paramReadFirstAttemptNoResponseRetry() { QVERIFY2(!_mockLink, "MockLink already connected"); @@ -206,7 +220,8 @@ void ParameterManagerTest::_paramWriteParamError() // setRawValue), then the write-failed message (fires on the PARAM_ERROR ack) expectAppMessage(QRegularExpression("Reboot vehicle for changes to take effect")); expectAppMessage(QRegularExpression("Parameter write failed")); - _setParamWithFailureMode(MockLink::FailParamSetParamError, false /* expectSuccess */); + _setParamWithFailureMode(MockLink::FailParamSetParamError, false /* expectSuccess */, + QStringLiteral("BAT1_V_CHARGED"), MAV_AUTOPILOT_PX4); verifyExpectedLogMessage(); verifyExpectedLogMessage(); } @@ -237,11 +252,17 @@ void ParameterManagerTest::_paramReadParamError() _disconnectMockLink(); } -void ParameterManagerTest::_setParamWithFailureMode(MockLink::ParamSetFailureMode_t failureMode, bool expectSuccess) +void ParameterManagerTest::_setParamWithFailureMode(MockLink::ParamSetFailureMode_t failureMode, bool expectSuccess, + const QString ¶mName, MAV_AUTOPILOT autopilot) { QVERIFY2(!_mockLink, "MockLink already connected"); + if (autopilot == MAV_AUTOPILOT_GENERIC) { + // Generic mock link has no metadata source; this warning is expected for generic autopilot + ignoreLogMessage("ComponentInformation.RequestMetaDataTypeStateMachine", QtWarningMsg, + QRegularExpression("failed to load metadata")); + } // Bring up a clean mock vehicle for each run - _connectMockLink(); + _connectMockLink(autopilot); QVERIFY(_mockLink); QVERIFY(_vehicle); _mockLink->setParamSetFailureMode(failureMode); @@ -250,8 +271,7 @@ void ParameterManagerTest::_setParamWithFailureMode(MockLink::ParamSetFailureMod ParameterManager* const paramManager = _vehicle->parameterManager(); QVERIFY(paramManager); QVERIFY(!_vehicle->parameterManager()->pendingWrites()); - // Use a parameter that exists in the mock PX4 set and has floating point range - Fact* const fact = paramManager->getParameter(MAV_COMP_ID_AUTOPILOT1, QStringLiteral("BAT1_V_CHARGED")); + Fact* const fact = paramManager->getParameter(MAV_COMP_ID_AUTOPILOT1, paramName); QVERIFY(fact); QSignalSpy rawValueChangedSpy(fact, &Fact::rawValueChanged); const QVariant originalValue = fact->rawValue(); @@ -261,7 +281,7 @@ void ParameterManagerTest::_setParamWithFailureMode(MockLink::ParamSetFailureMod : -std::numeric_limits::infinity(); const double maxValue = (metaData && metaData->rawMax().isValid()) ? metaData->rawMax().toDouble() : std::numeric_limits::infinity(); - const double step = 0.1; + const double step = fact->type() == FactMetaData::valueTypeFloat ? 0.1 : 1.0; auto adjustedValue = [&](double candidate) -> double { if (candidate > maxValue) { candidate = originalDouble - step; diff --git a/test/FactSystem/ParameterManagerTest.h b/test/FactSystem/ParameterManagerTest.h index e181867a9283..b3cdedd1b0b0 100644 --- a/test/FactSystem/ParameterManagerTest.h +++ b/test/FactSystem/ParameterManagerTest.h @@ -15,6 +15,8 @@ private slots: void _requestListMissingParamFail(); void _paramWriteNoAckRetry(); void _paramWriteNoAckPermanent(); + void _paramWriteUInt8(); + void _paramWriteUInt16(); void _paramReadFirstAttemptNoResponseRetry(); void _paramReadNoResponse(); void _paramWriteParamError(); @@ -29,5 +31,6 @@ private slots: private: void _noFailureWorker(MockConfiguration::FailureMode_t failureMode); - void _setParamWithFailureMode(MockLink::ParamSetFailureMode_t failureMode, bool expectSuccess); + void _setParamWithFailureMode(MockLink::ParamSetFailureMode_t failureMode, bool expectSuccess, + const QString ¶mName, MAV_AUTOPILOT autopilot); };