Skip to content

Commit 65fdfd0

Browse files
committed
feat(flight-controller): handle version-dependent PARAM_ERROR replies
Detect the connected ArduPilot firmware version from AUTOPILOT_VERSION and enable PARAM_ERROR acknowledgement handling only for firmware 4.7.0 and newer. Older firmware keeps the original send-only parameter-write behavior without an unnecessary response timeout. Add a local MAVLink-2 PARAM_ERROR compatibility decoder for pymavlink versions that do not define message 345. Preserve unrelated PARAM_VALUE messages received while waiting for an acknowledgement so later parameter operations can consume them safely. Extend the MAVLink connection test double with message filtering, unfiltered message reception, connection lookup, and parameter-send support. Add regression tests covering firmware-version boundaries, PARAM_ERROR decoding, stale replies, message preservation, and legacy write performance.
1 parent 518d444 commit 65fdfd0

9 files changed

Lines changed: 711 additions & 42 deletions

ARCHITECTURE_2_flight_controller_communication.md

Lines changed: 38 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,9 @@ make MAVLink, MAVFTP, or command operations concurrent or non-blocking.
3131
- ✅ Downloads parameters through MAVFTP when supported, with MAVLink `PARAM_REQUEST_LIST` fallback.
3232
- ✅ Downloads parameter defaults only through the MAVFTP path.
3333
- ✅ Validates parameter names and numeric value types before sending a parameter write.
34-
- ⚠️ `set_param()` reports whether the send operation succeeded; MAVLink parameter writes have no command ACK here. Callers must use `fetch_param()` or re-download parameters to verify the controller state.
34+
- ✅ Detects a matching MAVLink-2 `PARAM_ERROR` response from newer ArduPilot firmware and reports the controller's rejection reason.
35+
- ⚠️ Older firmware does not acknowledge `PARAM_SET`; after the short `PARAM_ERROR` response window, `set_param()` can report only that the write was sent.
36+
Callers that need positive confirmation must use `fetch_param()` or re-download parameters.
3537

3638
4. **Protocol Support**
3739
- ✅ Uses pymavlink for MAVLink messages, connection creation, retries, and reconnect support.
@@ -49,13 +51,15 @@ make MAVLink, MAVFTP, or command operations concurrent or non-blocking.
4951

5052
1. **Performance**
5153
- ✅ Uses MAVFTP when the controller advertises support and provides transfer progress callbacks.
52-
- ⚠️ Download, command, and connection operations use polling and blocking waits. They should be invoked from an appropriate UI workflow to avoid blocking the event loop.
54+
- ⚠️ Download, command, and connection operations use polling and blocking waits.
55+
They should be invoked from an appropriate UI workflow to avoid blocking the event loop.
5356
- ⚠️ No performance limit for parameter count or memory consumption is enforced or benchmarked by this component.
5457

5558
2. **Reliability** ⚠️ **PARTIALLY IMPLEMENTED**
5659
- ✅ Checks for a connection before most controller operations and returns structured error messages for many failures.
5760
- ✅ Verifies command results when the MAVLink command protocol provides `COMMAND_ACK`.
58-
- ⚠️ Parameter writes require explicit read-back for verification.
61+
- ✅ Reports explicit rejections from newer firmware through MAVLink-2 `PARAM_ERROR`.
62+
- ⚠️ Parameter writes still require explicit read-back for positive verification, especially with older firmware that sends no response.
5963
-**TODO**: Interrupted operations cannot be resumed from persisted state.
6064

6165
3. **Compatibility**
@@ -67,7 +71,7 @@ make MAVLink, MAVFTP, or command operations concurrent or non-blocking.
6771
- ✅ Validates parameter names and value types before sending parameter writes.
6872
- ⚠️ Message parsing and transport-level validation are delegated to pymavlink; this is not an end-to-end integrity or authorization guarantee.
6973
-**TODO**: MAVLink signing/authentication is not implemented by this component.
70-
- ⚠️ Parameter-write confirmation requires an explicit read-back, not a `set_param()` acknowledgement.
74+
- ⚠️ Newer firmware can explicitly reject writes with `PARAM_ERROR`, but positive parameter-write confirmation still requires an explicit read-back.
7175

7276
## Architecture
7377

@@ -101,13 +105,15 @@ and the parameter dictionary); the ownership and mutation rules above are the re
101105

102106
- **File**: `backend_flightcontroller_connection.py`
103107
- **Classes**: `FlightControllerConnection`, `FakeSerialForTests`
104-
- **Purpose**: Discovers connection choices, establishes and closes MAVLink connections, selects a supported autopilot from heartbeats, and populates `FlightControllerInfo`.
108+
- **Purpose**: Discovers connection choices, establishes and closes MAVLink connections, and selects a supported
109+
autopilot from heartbeats before populating `FlightControllerInfo`.
105110
- **Key methods**:
106111
- `connect()` — connects to an explicit device or tries auto-detected choices.
107112
- `disconnect()` — closes the current connection, clears the banner buffer, and resets controller information.
108113
- `discover_connections(preserved_connections)` — merges locally enumerated serial ports, configured network endpoints, and persisted choices.
109114
- `_register_and_try_connect()` and `create_connection_with_retry()` — internal connection helpers.
110-
- **Connection validation**: `_detect_vehicles_from_heartbeats()` is used during connection establishment; `_retrieve_autopilot_version_and_banner()` then requests controller details.
115+
- **Connection validation**: `_detect_vehicles_from_heartbeats()` is used during connection establishment;
116+
`_retrieve_autopilot_version_and_banner()` then requests controller details.
111117
- **Dependencies**: pymavlink, pyserial port discovery, `FlightControllerInfo`, time, and logging.
112118

113119
#### Parameters Manager
@@ -164,7 +170,8 @@ and the parameter dictionary); the ownership and mutation rules above are the re
164170

165171
- **File**: `data_model_flightcontroller_info.py`
166172
- **Class**: `FlightControllerInfo`
167-
- **Purpose**: Stores and derives flight-controller metadata from heartbeat, `AUTOPILOT_VERSION`, and banner data, including capabilities, board information, firmware details, and vehicle type.
173+
- **Purpose**: Stores and derives flight-controller metadata from heartbeat, `AUTOPILOT_VERSION`, and banner data,
174+
including capabilities, board information, firmware details, and vehicle type.
168175

169176
#### Flight Controller ID Model
170177

@@ -176,7 +183,8 @@ and the parameter dictionary); the ownership and mutation rules above are the re
176183
- **File**: `frontend_tkinter_connection_selection.py`
177184
- **Classes**: `ConnectionSelectionWidgets`, `ConnectionSelectionWindow`
178185
- **Purpose**: Lets the user choose or add a connection and provides progress/status feedback.
179-
- **Key behavior**: `_refresh_ports()` refreshes choices every three seconds while preserving connection history cached from `ProgramSettings`. `reconnect()` persists the user-selected connection string where possible.
186+
- **Key behavior**: `_refresh_ports()` refreshes choices every three seconds while preserving connection history cached
187+
from `ProgramSettings`. `reconnect()` persists the user-selected connection string where possible.
180188

181189
#### Flight Controller Information UI
182190

@@ -201,10 +209,12 @@ and the parameter dictionary); the ownership and mutation rules above are the re
201209
- `FlightController.download_params()` delegates to the parameters manager.
202210
- When `info.is_mavftp_supported` is true, the manager first tries MAVFTP, including defaults when requested; otherwise it uses MAVLink parameter messages.
203211
- If MAVFTP fails, it falls back to MAVLink. An incomplete MAVLink download returns no parameter set.
204-
- `set_param()` only reports send success. A caller needing confirmation follows it with `fetch_param()` or another download.
212+
- `set_param()` waits briefly for newer firmware's `PARAM_ERROR` rejection response. No response preserves compatibility
213+
with older firmware, so a caller needing positive confirmation follows it with `fetch_param()` or another download.
205214

206215
4. **Command and file operations**
207-
- The commands manager sends `COMMAND_LONG` messages and waits synchronously for matching `COMMAND_ACK` messages.
216+
- Command operations that use `send_command_and_wait_ack()` send `COMMAND_LONG` messages and wait synchronously
217+
for matching `COMMAND_ACK` messages. Batched motor-test commands are sent without per-command acknowledgement waits.
208218
- Battery status is read from telemetry and briefly cached.
209219
- The files manager creates a MAVFTP instance for uploads and supported log downloads.
210220

@@ -224,7 +234,8 @@ and the parameter dictionary); the ownership and mutation rules above are the re
224234
#### MAVLink Parameter Protocol
225235

226236
- Uses `PARAM_REQUEST_LIST`/`PARAM_VALUE` for bulk MAVLink downloads.
227-
- Uses pymavlink parameter-send support for writes and `PARAM_VALUE` polling for individual reads.
237+
- Uses pymavlink parameter-send support for writes and locally registers the MAVLink-2 `PARAM_ERROR` decoder required
238+
by the pinned pymavlink version. It polls `PARAM_VALUE` for individual reads.
228239
- Validates parameter names and numeric value types locally before writes.
229240

230241
#### FTP-over-MAVLink
@@ -238,29 +249,33 @@ and the parameter dictionary); the ownership and mutation rules above are the re
238249
- **Connection errors**: Return error messages and, for several serial failures, actionable guidance. Pymavlink receives the configured retry/autoreconnect settings.
239250
- **Timeout errors**: Use operation-specific fixed timeouts and return an error when they expire.
240251
- **Parameter download errors**: Fall back from MAVFTP to MAVLink; reject incomplete MAVLink downloads.
241-
- **Parameter write errors**: Validate locally before sending. Read back the parameter when verification is required.
252+
- **Parameter write errors**: Validate locally before sending and report a matching newer-firmware `PARAM_ERROR`
253+
rejection. Read back the parameter when positive verification is required or when the firmware provides no rejection response.
242254

243255
## Testing Strategy
244256

245257
### Test Organization
246258

247259
The test suite separates manager/facade tests from SITL coverage:
248260

249-
- `test_backend_flightcontroller.py` exercises facade delegation, lifecycle, commands, parameter workflows, and error paths.
261+
- `test_backend_flightcontroller.py` exercises facade delegation, lifecycle, commands, parameter workflows, and error
262+
paths.
250263
- `test_backend_flightcontroller_business_logic.py` exercises pure calculations and validation functions.
251-
- `test_backend_flightcontroller_connection.py`, `test_backend_flightcontroller_params.py`, `test_backend_flightcontroller_commands.py`, and `test_backend_flightcontroller_files.py` exercise the specialized managers.
264+
- `test_backend_flightcontroller_connection.py`, `test_backend_flightcontroller_params.py`,
265+
`test_backend_flightcontroller_commands.py`, and `test_backend_flightcontroller_files.py` exercise the specialized managers.
252266
- `test_backend_flightcontroller_sitl.py` uses a real ArduCopter SITL TCP connection and is marked with both `integration` and `sitl` where applicable.
253267

254-
Test names and marker use vary by test; do not treat BDD-style names or integration markers as universal conventions. Avoid recording fixed test counts here because they change as the suite evolves.
268+
Test names and marker use vary by test; do not treat BDD-style names or integration markers as universal conventions.
269+
Avoid recording fixed test counts here because they change as the suite evolves.
255270

256271
### Running Tests Selectively
257272

258273
```bash
259-
# Run all flight-controller tests
260-
pytest tests/test_*flightcontroller*.py -v
274+
# Run all flight-controller tests, including unit-prefixed modules
275+
pytest tests/test_*flightcontroller*.py tests/unit_backend_flightcontroller*.py -v
261276

262277
# Run flight-controller tests that are not marked SITL
263-
pytest tests/test_*flightcontroller*.py -m "not sitl" -v
278+
pytest tests/test_*flightcontroller*.py tests/unit_backend_flightcontroller*.py -m "not sitl" -v
264279

265280
# Run integration-marked tests
266281
pytest -m integration tests/ -v
@@ -283,11 +298,16 @@ ardupilot_methodic_configurator/
283298
├── backend_flightcontroller_files.py
284299
├── backend_flightcontroller_protocols.py
285300
├── backend_flightcontroller_business_logic.py
301+
├── backend_flightcontroller_factory_mavlink.py
286302
├── backend_flightcontroller_factory_mavftp.py
303+
├── backend_flightcontroller_factory_serial.py
304+
├── backend_mavlink_param_error.py
287305
├── backend_mavftp.py
306+
├── data_model_par_dict.py
288307
├── data_model_flightcontroller_info.py
289308
├── data_model_fc_ids.py
290309
├── frontend_tkinter_connection_selection.py
310+
├── frontend_tkinter_flightcontroller_connection_progress.py
291311
└── frontend_tkinter_flightcontroller_info.py
292312
```
293313

ardupilot_methodic_configurator/backend_flightcontroller_factory_mavlink.py

Lines changed: 103 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,14 @@
88
SPDX-License-Identifier: GPL-3.0-or-later
99
"""
1010

11+
from collections import deque
12+
from time import monotonic
1113
from typing import TYPE_CHECKING, Optional, Protocol
1214

1315
from pymavlink import mavutil
1416

17+
from ardupilot_methodic_configurator.backend_mavlink_param_error import install_param_error_message
18+
1519
if TYPE_CHECKING:
1620
from ardupilot_methodic_configurator.backend_flightcontroller_protocols import MavlinkConnection
1721

@@ -44,14 +48,18 @@ def create( # pylint: disable=too-many-arguments, too-many-positional-arguments
4448
) -> Optional["MavlinkConnection"]:
4549
"""Create connection using actual PyMAVLink library."""
4650
try:
47-
return mavutil.mavlink_connection( # pyright: ignore[reportReturnType]
51+
# PARAM_ERROR has id 345 and is therefore MAVLink-2 only. Select
52+
# the v2 dialect before mavutil constructs its parser.
53+
install_param_error_message()
54+
connection = mavutil.mavlink_connection( # pyright: ignore[reportReturnType]
4855
device=device,
4956
baud=baudrate,
5057
timeout=timeout,
5158
retries=retries,
5259
progress_callback=progress_callback,
5360
autoreconnect=True,
5461
)
62+
return BufferedMavlinkConnection(connection)
5563
except PermissionError:
5664
# PermissionError subclasses OSError; preserve it for permission-specific UI guidance.
5765
raise
@@ -81,13 +89,89 @@ def create( # pylint: disable=too-many-arguments, too-many-positional-arguments
8189
conn = FakeMavlinkConnection(device, baudrate)
8290
conn.retries = retries
8391
conn.progress_callback = progress_callback
92+
self._connections[device] = conn
8493
return conn
8594

8695
def get_connection(self, device: str) -> Optional["FakeMavlinkConnection"]:
8796
"""Get a previously created fake connection."""
8897
return self._connections.get(device)
8998

9099

100+
class BufferedMavlinkConnection:
101+
"""MAVLink connection adapter that preserves messages skipped by filters."""
102+
103+
def __init__(self, connection: object) -> None:
104+
"""Initialize the adapter around a pymavlink connection."""
105+
self._connection = connection
106+
self._pending_messages: deque[object] = deque()
107+
108+
def __getattr__(self, name: str) -> object:
109+
"""Expose the underlying connection's normal MAVLink attributes."""
110+
return getattr(self._connection, name)
111+
112+
def recv_msg(self) -> object | None:
113+
"""Return the next message, including one preserved by a prior filter."""
114+
if self._pending_messages:
115+
return self._pending_messages.popleft()
116+
recv_msg = self._connection.recv_msg # type: ignore[attr-defined]
117+
return recv_msg()
118+
119+
def recv_match(
120+
self,
121+
condition: str | None = None,
122+
type: str | list[str] | set[str] | None = None, # noqa: A002 # pylint: disable=redefined-builtin
123+
blocking: bool = False,
124+
timeout: float | None = None,
125+
) -> object | None:
126+
"""Return a matching message while retaining messages of other types."""
127+
if condition is not None:
128+
error_message = "Buffered receive does not support conditions"
129+
raise NotImplementedError(error_message)
130+
131+
message_types = None if type is None else {type} if isinstance(type, str) else set(type)
132+
start_time = monotonic()
133+
while True:
134+
# A busy telemetry link can always have another unrelated message
135+
# ready. Check the deadline before every read so a filtered wait
136+
# cannot indefinitely drain and buffer that stream.
137+
if timeout is not None and monotonic() - start_time >= timeout:
138+
return None
139+
message = self._pop_pending_matching(message_types)
140+
if message is None:
141+
recv_msg = self._connection.recv_msg # type: ignore[attr-defined]
142+
message = recv_msg()
143+
if message is not None:
144+
if message_types is None or getattr(message, "get_type", lambda: None)() in message_types:
145+
return message
146+
self._pending_messages.append(message)
147+
# A non-blocking call must be bounded even when the transport
148+
# is continuously receiving telemetry. The next poll resumes
149+
# the search, with this message safely retained.
150+
if not blocking:
151+
return None
152+
continue
153+
154+
if not blocking:
155+
return None
156+
if timeout is not None:
157+
remaining = timeout - (monotonic() - start_time)
158+
if remaining <= 0:
159+
return None
160+
else:
161+
remaining = 0.05
162+
select = getattr(self._connection, "select", None)
163+
if callable(select):
164+
select(min(remaining, 0.05))
165+
166+
def _pop_pending_matching(self, message_types: set[str] | None) -> object | None:
167+
"""Remove the first buffered message matching the requested types."""
168+
for index, message in enumerate(self._pending_messages):
169+
if message_types is None or getattr(message, "get_type", lambda: None)() in message_types:
170+
del self._pending_messages[index]
171+
return message
172+
return None
173+
174+
91175
class FakeMavlinkConnection:
92176
"""Fake MAVLink connection for testing."""
93177

@@ -103,12 +187,26 @@ def __init__(self, device: str, baudrate: int) -> None:
103187

104188
def recv_match(
105189
self,
190+
type: str | None = None, # noqa: A002 # pylint: disable=redefined-builtin
106191
blocking: bool = True, # noqa: ARG002 # pylint: disable=unused-argument
107192
timeout: float | None = None, # noqa: ARG002 # pylint: disable=unused-argument
108193
) -> object | None:
109-
"""Receive a matched message from queue."""
194+
"""Receive the next queued message matching an optional MAVLink type."""
110195
# Note: blocking and timeout parameters are accepted for API compatibility
111196
# but not used in fake implementation
197+
if type is None:
198+
if self._message_queue:
199+
return self._message_queue.pop(0)
200+
return None
201+
202+
for index, message in enumerate(self._message_queue):
203+
get_type = getattr(message, "get_type", None)
204+
if callable(get_type) and get_type() == type:
205+
return self._message_queue.pop(index)
206+
return None
207+
208+
def recv_msg(self) -> object | None:
209+
"""Receive the next queued message without filtering by MAVLink type."""
112210
if self._message_queue:
113211
return self._message_queue.pop(0)
114212
return None
@@ -117,6 +215,9 @@ def mav_send(self, msg: object) -> None:
117215
"""Send a MAVLink message (no-op for fake)."""
118216
# Note: msg parameter is accepted for API compatibility but not used in fake
119217

218+
def param_set_send(self, param_name: str, param_value: float) -> None:
219+
"""Send a parameter write (no-op for fake)."""
220+
120221
def close(self) -> None:
121222
"""Close connection."""
122223
self.connected = False

0 commit comments

Comments
 (0)