Skip to content

Fix bytewise parameter write acknowledgements #14662

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ikalnytskyi wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix bytewise parameter write acknowledgements #14662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Comms/MockLink/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions src/Comms/MockLink/GenericMockLink.params
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Vehicle-Id Component-Id Name Value Type
1 1 TEST_UINT8 1 1
1 1 TEST_UINT16 1 3
2 changes: 2 additions & 0 deletions src/Comms/MockLink/MockLink.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Comment on lines 397 to 404

@ikalnytskyi ikalnytskyi Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... but there shouldn't be any expected parameters set for the Generic autopilot, should there?

Expand Down
8 changes: 4 additions & 4 deletions src/FactSystem/ParameterManager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<quint32>(paramUnion.param_uint8));

@ikalnytskyi ikalnytskyi Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

n.b. Without this cast, QVariant ends up storing an int, not a uint, while the fact system we compare this value to stores a uint. Due to the mismatch, QGC rejects write acknowledgement, retries and eventually shows the write failure.

return true;
case MAV_PARAM_TYPE_INT8:
outValue = QVariant(paramUnion.param_int8);
outValue = QVariant(static_cast<qint32>(paramUnion.param_int8));
return true;
case MAV_PARAM_TYPE_UINT16:
outValue = QVariant(paramUnion.param_uint16);
outValue = QVariant(static_cast<quint32>(paramUnion.param_uint16));
return true;
case MAV_PARAM_TYPE_INT16:
outValue = QVariant(paramUnion.param_int16);
outValue = QVariant(static_cast<qint32>(paramUnion.param_int16));
return true;
case MAV_PARAM_TYPE_UINT32:
outValue = QVariant(paramUnion.param_uint32);
Expand Down
36 changes: 28 additions & 8 deletions test/FactSystem/ParameterManagerTest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Comment thread
ikalnytskyi marked this conversation as resolved.
verifyExpectedLogMessage();
}

Expand All @@ -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");
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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 &paramName, 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);
Expand All @@ -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();
Expand All @@ -261,7 +281,7 @@ void ParameterManagerTest::_setParamWithFailureMode(MockLink::ParamSetFailureMod
: -std::numeric_limits<double>::infinity();
const double maxValue = (metaData && metaData->rawMax().isValid()) ? metaData->rawMax().toDouble()
: std::numeric_limits<double>::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;
Expand Down
5 changes: 4 additions & 1 deletion test/FactSystem/ParameterManagerTest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ private slots:
void _requestListMissingParamFail();
void _paramWriteNoAckRetry();
void _paramWriteNoAckPermanent();
void _paramWriteUInt8();
void _paramWriteUInt16();
void _paramReadFirstAttemptNoResponseRetry();
void _paramReadNoResponse();
void _paramWriteParamError();
Expand All @@ -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 &paramName, MAV_AUTOPILOT autopilot);
};
Loading