Fix bytewise parameter write acknowledgements#14662
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes parameter write acknowledgements for bytewise MAVLink parameter types by decoding MAV_PARAM_TYPE_UINT8/UINT16 (and similarly INT8/INT16) into QGC’s canonical 32-bit QVariant integer representations, preventing valid PARAM_VALUE echoes from being rejected due to mismatched QVariant types.
Changes:
- Fix
_mavlinkParamUnionToVariantto cast narrow integer types toqint32/quint32before wrapping inQVariant. - Add unit tests covering successful writes of
UINT8andUINT16parameters. - Extend
MockLinkto load a minimal generic parameter set containingUINT8/UINT16test parameters.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/FactSystem/ParameterManagerTest.h | Adds new unit test slots and extends the helper signature to parameterize param name and autopilot type. |
| test/FactSystem/ParameterManagerTest.cc | Implements UINT8/UINT16 write tests and updates the helper to support generic autopilot + integer step sizing. |
| src/FactSystem/ParameterManager.cc | Fixes UINT8/UINT16 (and INT8/INT16) decoding to consistent 32-bit QVariant integer types. |
| src/Comms/MockLink/MockLink.cc | Loads a new generic parameter file when MAV_AUTOPILOT_GENERIC is used. |
| src/Comms/MockLink/GenericMockLink.params | Adds test parameters TEST_UINT8 and TEST_UINT16 with MAVLink types for coverage. |
| src/Comms/MockLink/CMakeLists.txt | Registers GenericMockLink.params as a Qt resource for the mock link. |
| return true; | ||
| case MAV_PARAM_TYPE_UINT8: | ||
| outValue = QVariant(paramUnion.param_uint8); | ||
| outValue = QVariant(static_cast<quint32>(paramUnion.param_uint8)); |
There was a problem hiding this comment.
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14662 +/- ##
==========================================
+ Coverage 25.47% 31.69% +6.21%
==========================================
Files 769 785 +16
Lines 65912 67046 +1134
Branches 30495 31042 +547
==========================================
+ Hits 16788 21247 +4459
+ Misses 37285 31399 -5886
- Partials 11839 14400 +2561
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 455 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
See the Build Results workflow run for details. |
941ac1c to
d03b7da
Compare
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.
d03b7da to
23252a5
Compare
| } 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"); | ||
| } |
There was a problem hiding this comment.
Hmm... but there shouldn't be any expected parameters set for the Generic autopilot, should there?
Description
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
_mavlinkParamUnionToVariantmethod passeduint8_tanduint16_tdirectly to QVariant. C++ selected QVariant'sintconstructor, 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.
Type of Change
Testing
Platforms Tested
Flight Stacks Tested
Screenshots
Checklist
Related Issues
By submitting this pull request, I confirm that my contribution is made under the terms of the project's dual license (Apache 2.0 and GPL v3).