From f37bce0c9ce46d74aa93f23dcd58d20a28b5218f Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Fri, 30 Oct 2026 19:27:50 +0100 Subject: [PATCH 1/4] fix(rc_calibration plugin): Fix loading and unloading of the plugin Signed-off-by: Dr.-Ing. Amilcar do Carmo Lucas --- .../plugins/backend_rc_telemetry.py | 85 ++++++++++++++++++ .../plugins/data_model_rc_calibration.py | 87 ++++++++----------- .../plugins/data_model_rc_receiver.py | 51 +++-------- .../frontend_tkinter_rc_calibration.py | 8 +- tests/plugins/conftest.py | 1 + .../plugins/test_data_model_rc_calibration.py | 84 ++++++++++++++++-- tests/plugins/test_data_model_rc_receiver.py | 44 ++++++++++ .../test_frontend_tkinter_rc_calibration.py | 63 +++++++++++++- 8 files changed, 321 insertions(+), 102 deletions(-) create mode 100644 ardupilot_methodic_configurator/plugins/backend_rc_telemetry.py diff --git a/ardupilot_methodic_configurator/plugins/backend_rc_telemetry.py b/ardupilot_methodic_configurator/plugins/backend_rc_telemetry.py new file mode 100644 index 000000000..2a4a18bd5 --- /dev/null +++ b/ardupilot_methodic_configurator/plugins/backend_rc_telemetry.py @@ -0,0 +1,85 @@ +""" +Shared MAVLink telemetry transport for RC monitoring and calibration. + +This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + +SPDX-FileCopyrightText: 2026 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +from collections.abc import Iterator +from logging import debug as logging_debug +from typing import TYPE_CHECKING, Any, cast + +if TYPE_CHECKING: + from ardupilot_methodic_configurator.backend_flightcontroller import FlightController + +_MAX_MESSAGES_PER_POLL = 50 + + +def rc_channel_values(message: Any, channel_count: int) -> list[int]: # noqa: ANN401 + """Decode only advertised RC channels, capped at the consumer's display limit.""" + count = max(0, min(int(message.chancount), channel_count)) + return [int(getattr(message, f"chan{channel}_raw")) for channel in range(1, count + 1)] + + +class RCTelemetryBackend: + """Own one monitor's stream lifecycle and bounded reads from the shared MAVLink link.""" + + def __init__(self, flight_controller: "FlightController") -> None: + self.flight_controller = flight_controller + self.connection: object | None = None + self._stream_requested = False + + def sync_connection(self) -> bool: + """Reset stream ownership on a new connection; tell consumers to clear cached readings.""" + master = self.flight_controller.master + if master is self.connection: + return False + self.connection = master + self._stream_requested = False + return True + + def stop_monitoring(self) -> None: + """Restore defaults once, only on the connection whose intervals we requested.""" + if self._stream_requested and self.flight_controller.master is self.connection: + success, error = self.flight_controller.request_rc_receiver_messages(0) + if not success: + logging_debug("Could not restore RC receiver telemetry intervals: %s", error) + self._stream_requested = False + + def get_rc_telemetry(self) -> Iterator[Any]: + """Yield every packet in a bounded, nonblocking batch, in order.""" + self.sync_connection() + master = cast("Any", self.connection) + if master is None: + return + if not self._stream_requested: + # Older firmware may reject the interval request while already streaming data. + self._stream_requested = True + success, error = self.flight_controller.request_rc_receiver_messages() + if not success: + logging_debug("Could not request RC receiver telemetry intervals: %s", error) + + for _index in range(_MAX_MESSAGES_PER_POLL): + # A type filter can consume an unbounded unrelated backlog inside pymavlink. + message = master.recv_match(blocking=False) + if message is None: + break + yield message + + def is_selected_source(self, message: Any) -> bool: # noqa: ANN401 + """Identify receiver-health packets belonging to the selected flight controller.""" + master = cast("Any", self.connection) + if master is None: + return False + info = self.flight_controller.info + system_id, component_id = info.system_id, info.component_id + target_system = int(system_id) if isinstance(system_id, str) and system_id else master.target_system + target_component = int(component_id) if isinstance(component_id, str) and component_id else master.target_component + if not isinstance(target_system, int) or target_system <= 0 or message.get_srcSystem() != target_system: + return False + return not ( + isinstance(target_component, int) and target_component > 0 and message.get_srcComponent() != target_component + ) diff --git a/ardupilot_methodic_configurator/plugins/data_model_rc_calibration.py b/ardupilot_methodic_configurator/plugins/data_model_rc_calibration.py index 0f4c90085..e369b6ac9 100644 --- a/ardupilot_methodic_configurator/plugins/data_model_rc_calibration.py +++ b/ardupilot_methodic_configurator/plugins/data_model_rc_calibration.py @@ -2,7 +2,9 @@ Data model for the RC calibration plugin. This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + SPDX-FileCopyrightText: 2024-2026 Amilcar do Carmo Lucas + SPDX-License-Identifier: GPL-3.0-or-later """ @@ -22,6 +24,7 @@ ParameterEditor, ParameterValueUpdateStatus, ) +from ardupilot_methodic_configurator.plugins.backend_rc_telemetry import RCTelemetryBackend, rc_channel_values if TYPE_CHECKING: from ardupilot_methodic_configurator.data_model_ardupilot_parameter import ArduPilotParameter @@ -48,7 +51,7 @@ } -class RCCalibrationDataModel: +class RCCalibrationDataModel: # pylint: disable=too-many-instance-attributes """Data model for RC calibration, backed by MAVLink RC_CHANNELS telemetry.""" def __init__(self, flight_controller: FlightController, parameter_editor: ParameterEditor | None = None) -> None: @@ -59,6 +62,11 @@ def __init__(self, flight_controller: FlightController, parameter_editor: Parame self._channel_max: dict[int, int] = {} # 0-based channel index → observed maximum PWM self._last_raw: list[int] = [] self._last_channel_count = 0 + self._telemetry = RCTelemetryBackend(flight_controller) + + def stop_monitoring(self) -> None: + """Restore the receiver stream defaults when this monitor yields the link.""" + self._telemetry.stop_monitoring() def _preview_parameters(self) -> dict[str, float]: """Overlay staged RC values without modifying the downloaded flight-controller cache.""" @@ -371,12 +379,28 @@ def finish_calibration(self) -> tuple[bool, str]: self._channel_max.clear() return success, message + def _read_channels(self, message: Any) -> dict[str, Any]: # noqa: ANN401 + """Record each sample's calibration extremes and build its live display values.""" + raw = rc_channel_values(message, _RC_DISPLAY_CHANNELS) + self._last_raw = raw + self._last_channel_count = len(raw) + if self._is_calibrating: + for index, value in enumerate(raw): + if value not in (0, _RC_INVALID_PWM): + self._channel_min[index] = min(self._channel_min.get(index, value), value) + self._channel_max[index] = max(self._channel_max.get(index, value), value) + + channels = self.get_channel_calibration() + for index, value in enumerate(raw): + if value not in (0, _RC_INVALID_PWM): + channels[index]["value"] = value + return {**self.get_preview_axes(), "channels": channels} + def get_rc_telemetry(self) -> dict[str, Any]: """ Return live RC telemetry from the flight controller. - Reads the MAVLink RC_CHANNELS message (non-blocking) and the most - recent HEARTBEAT (non-blocking) to build the telemetry dict. + Read a bounded batch without blocking, preserving every RC sample and the latest heartbeat. Returns an empty dict when not connected or when no message is available yet, which signals the GUI to keep waiting. @@ -385,59 +409,20 @@ def get_rc_telemetry(self) -> dict[str, Any]: Trim maps to zero for roll/pitch/yaw; the endpoint midpoint does so for throttle. Defaults are CH1=roll, CH2=pitch, CH3=throttle, CH4=yaw and a 1000-2000 us range. """ - if self.flight_controller.master is None: + if self._telemetry.sync_connection(): + self._last_raw = [] + self._last_channel_count = 0 + if self._telemetry.connection is None: return {} - master = self.flight_controller.master telemetry: dict[str, Any] = {} try: - latest_rc_msg = master.recv_match( # pyright: ignore[reportAttributeAccessIssue] - type="RC_CHANNELS", blocking=False - ) - latest_hb = master.recv_match( # pyright: ignore[reportAttributeAccessIssue] - type="HEARTBEAT", blocking=False - ) - - if latest_rc_msg: - n_channels = max(0, min(latest_rc_msg.chancount, _RC_DISPLAY_CHANNELS)) - raw: list[int] = [ - latest_rc_msg.chan1_raw, - latest_rc_msg.chan2_raw, - latest_rc_msg.chan3_raw, - latest_rc_msg.chan4_raw, - latest_rc_msg.chan5_raw, - latest_rc_msg.chan6_raw, - latest_rc_msg.chan7_raw, - latest_rc_msg.chan8_raw, - latest_rc_msg.chan9_raw, - latest_rc_msg.chan10_raw, - latest_rc_msg.chan11_raw, - latest_rc_msg.chan12_raw, - latest_rc_msg.chan13_raw, - latest_rc_msg.chan14_raw, - latest_rc_msg.chan15_raw, - latest_rc_msg.chan16_raw, - ] - - self._last_raw = raw - self._last_channel_count = n_channels - telemetry.update(self.get_preview_axes()) - - if self._is_calibrating: - for i in range(n_channels): - if raw[i] not in (0, _RC_INVALID_PWM): - self._channel_min[i] = min(self._channel_min.get(i, raw[i]), raw[i]) - self._channel_max[i] = max(self._channel_max.get(i, raw[i]), raw[i]) - - channels = self.get_channel_calibration() - for index in range(n_channels): - if raw[index] not in (0, _RC_INVALID_PWM): - channels[index]["value"] = raw[index] - telemetry["channels"] = channels - - if latest_hb: - telemetry["flight_mode"] = str(latest_hb.custom_mode) + for message in self._telemetry.get_rc_telemetry(): + if message.get_type() == "RC_CHANNELS": + telemetry.update(self._read_channels(message)) + elif message.get_type() == "HEARTBEAT": + telemetry["flight_mode"] = str(message.custom_mode) except Exception as exc: # pylint: disable=broad-exception-caught logging_debug(_("Error reading MAVLink telemetry: %(error)s"), {"error": str(exc)}) diff --git a/ardupilot_methodic_configurator/plugins/data_model_rc_receiver.py b/ardupilot_methodic_configurator/plugins/data_model_rc_receiver.py index 6d0ad1dbe..0a8e6a03f 100644 --- a/ardupilot_methodic_configurator/plugins/data_model_rc_receiver.py +++ b/ardupilot_methodic_configurator/plugins/data_model_rc_receiver.py @@ -12,16 +12,17 @@ from dataclasses import dataclass from logging import debug as logging_debug from time import monotonic -from typing import TYPE_CHECKING, Any, cast +from typing import TYPE_CHECKING, Any from pymavlink.dialects.v20.ardupilotmega import MAV_SYS_STATUS_SENSOR_RC_RECEIVER +from ardupilot_methodic_configurator.plugins.backend_rc_telemetry import RCTelemetryBackend, rc_channel_values + if TYPE_CHECKING: from ardupilot_methodic_configurator.backend_flightcontroller import FlightController _CHANNEL_COUNT = 5 _TELEMETRY_TIMEOUT = 3.0 -_MAX_MESSAGES_PER_POLL = 50 _RC_RECEIVER = MAV_SYS_STATUS_SENSOR_RC_RECEIVER @@ -44,8 +45,7 @@ class RCReceiverDataModel: # pylint: disable=too-many-instance-attributes def __init__(self, flight_controller: "FlightController", clock: Callable[[], float] = monotonic) -> None: self.flight_controller = flight_controller self._clock = clock - self._connection: object | None = None - self._stream_requested = False + self._telemetry = RCTelemetryBackend(flight_controller) self._channels: tuple[int | None, ...] = (None,) * _CHANNEL_COUNT self._rssi: int | None = None self._rc_time: float | None = None @@ -65,16 +65,12 @@ def clear_live_data(self) -> None: def stop_monitoring(self) -> None: """Restore default stream rates and invalidate readings when yielding the link.""" - if self._stream_requested and self.flight_controller.master is self._connection: - success, error = self.flight_controller.request_rc_receiver_messages(0) - if not success: - logging_debug("Could not restore RC receiver telemetry intervals: %s", error) - self._stream_requested = False + self._telemetry.stop_monitoring() self.clear_live_data() def _read_channels(self, message: Any, now: float) -> None: # noqa: ANN401 - count = max(0, min(int(message.chancount), _CHANNEL_COUNT)) - raw = [int(getattr(message, f"chan{channel}_raw")) for channel in range(1, count + 1)] + raw = rc_channel_values(message, _CHANNEL_COUNT) + count = len(raw) self._channels = tuple(raw[i] if i < count and 0 < raw[i] < 65535 else None for i in range(_CHANNEL_COUNT)) rssi = int(message.rssi) self._rssi = rssi if 0 <= rssi <= 254 else None @@ -90,41 +86,16 @@ def _read_status(self, message: Any, now: float) -> None: # noqa: ANN401 def get_receiver_state(self) -> RCReceiverState: """Poll without blocking; expire each stream independently and bound work per GUI tick.""" - master = cast("Any", self.flight_controller.master) - if master is not self._connection: + if self._telemetry.sync_connection(): self.clear_live_data() self._failsafe_detected = False - self._connection = master - self._stream_requested = False - if master is None: + if self._telemetry.connection is None: return RCReceiverState() - if not self._stream_requested: - # Request once per activation/connection, even if only one command is accepted. - # Telemetry remains usable when older firmware rejects interval requests. - self._stream_requested = True - success, error = self.flight_controller.request_rc_receiver_messages() - if not success: - logging_debug("Could not request RC receiver telemetry intervals: %s", error) - - info = self.flight_controller.info - system_id, component_id = info.system_id, info.component_id - target_system = int(system_id) if isinstance(system_id, str) and system_id else master.target_system - target_component = int(component_id) if isinstance(component_id, str) and component_id else master.target_component now = self._clock() try: - for _index in range(_MAX_MESSAGES_PER_POLL): - # Count every packet: a type filter can drain an unbounded unrelated backlog. - message = master.recv_match(blocking=False) - if message is None: - break - if not isinstance(target_system, int) or target_system <= 0 or message.get_srcSystem() != target_system: - continue - if ( - isinstance(target_component, int) - and target_component > 0 - and message.get_srcComponent() != target_component - ): + for message in self._telemetry.get_rc_telemetry(): + if not self._telemetry.is_selected_source(message): continue if message.get_type() == "RC_CHANNELS": self._read_channels(message, now) diff --git a/ardupilot_methodic_configurator/plugins/frontend_tkinter_rc_calibration.py b/ardupilot_methodic_configurator/plugins/frontend_tkinter_rc_calibration.py index a5632e4d1..daf7fc777 100644 --- a/ardupilot_methodic_configurator/plugins/frontend_tkinter_rc_calibration.py +++ b/ardupilot_methodic_configurator/plugins/frontend_tkinter_rc_calibration.py @@ -493,6 +493,11 @@ def __init__(self, parent: tk.Widget, model: RCCalibrationDataModel) -> None: self._timer_id = self.root.after(100, self._check_telemetry) logging_debug(_("RC calibration progress popup created and polling scheduled.")) + def destroy(self) -> None: + """Release the requested stream when the monitor closes.""" + self.model.stop_monitoring() + super().destroy() + def _setup_ui(self) -> None: content_frame = self._create_framed_ui(_("Live Monitor")) content_frame.pack_configure(padx=_RC_FRAME_PADDING, pady=_RC_FRAME_PADDING) @@ -709,6 +714,7 @@ def on_activate(self) -> None: def on_deactivate(self) -> None: """Yield the shared receiver while hidden or while diagnostics is open.""" self._stop_polling() + self.model.stop_monitoring() def _check_telemetry(self) -> None: telemetry = self.model.get_rc_telemetry() @@ -736,7 +742,7 @@ def _check_telemetry(self) -> None: def destroy(self) -> None: """Stop the polling loop before destroying the widget.""" try: - self._stop_polling() + self.on_deactivate() if self._calibration_active: self.model.cancel_calibration() self._calibration_active = False diff --git a/tests/plugins/conftest.py b/tests/plugins/conftest.py index f81eb0cb4..c9ad34fb9 100755 --- a/tests/plugins/conftest.py +++ b/tests/plugins/conftest.py @@ -69,6 +69,7 @@ def rc_staging_context() -> tuple[MagicMock, ParameterEditor]: """Use real parameter objects and editor APIs; mock only filesystem and FC I/O.""" controller = MagicMock() controller.master.recv_match.return_value = None + controller.request_rc_receiver_messages.return_value = (True, "") # pylint: disable=duplicate-code controller.fc_parameters = { "RCMAP_ROLL": 1.0, diff --git a/tests/plugins/test_data_model_rc_calibration.py b/tests/plugins/test_data_model_rc_calibration.py index 7fec6192c..2e37cebb6 100755 --- a/tests/plugins/test_data_model_rc_calibration.py +++ b/tests/plugins/test_data_model_rc_calibration.py @@ -40,6 +40,7 @@ def _make_rc_channels_msg(raw: list[int], chancount: int | None = None) -> Magic """ padded = list(raw) + [_RC_INVALID_PWM] * (18 - len(raw)) msg = MagicMock() + msg.get_type.return_value = "RC_CHANNELS" for i in range(18): setattr(msg, f"chan{i + 1}_raw", padded[i]) msg.chancount = chancount if chancount is not None else len(raw) @@ -52,6 +53,7 @@ def connected_flight_controller() -> MagicMock: flight_controller = MagicMock() flight_controller.master = MagicMock() flight_controller.master.recv_match.return_value = None + flight_controller.request_rc_receiver_messages.return_value = (True, "") flight_controller.fc_parameters = {} return flight_controller @@ -315,6 +317,74 @@ def test_standalone_finish_cannot_bypass_the_upload_workflow(self, connected_fli class TestRCCalibrationDataModelTelemetry: """Test live RC telemetry parsing from MAVLink RC_CHANNELS messages.""" + def test_batch_preserves_extremes_and_displays_latest_sample(self, rc_staging_context) -> None: + """Queued extremes are staged even when the latest sample has returned to center.""" + controller, editor = rc_staging_context + model = RCCalibrationDataModel(controller, editor) + model.start_calibration() + controller.master.recv_match.side_effect = [ + _make_rc_channels_msg([1100]), + _make_rc_channels_msg([1900]), + _make_rc_channels_msg([1500]), + None, + ] + + telemetry = model.get_rc_telemetry() + + assert telemetry["channels"][0]["value"] == 1500 + assert model.finish_calibration()[0] + assert editor.current_step_parameters["RC1_MIN"].get_new_value() == 1100 + assert editor.current_step_parameters["RC1_MAX"].get_new_value() == 1900 + + def test_poll_bounds_unrelated_packets(self, connected_flight_controller: MagicMock) -> None: + """Unrelated traffic cannot cause an unbounded poll or hide queued RC data forever.""" + controller = connected_flight_controller + unrelated = MagicMock() + unrelated.get_type.return_value = "SYS_STATUS" + controller.master.recv_match.side_effect = [unrelated] * 50 + [_make_rc_channels_msg([1200]), None] + model = RCCalibrationDataModel(controller) + + assert not model.get_rc_telemetry() + assert controller.master.recv_match.call_count == 50 + assert model.get_rc_telemetry()["channels"][0]["value"] == 1200 + + def test_monitor_rerequests_stream_after_reconnection(self, connected_flight_controller: MagicMock) -> None: + """ + Monitoring resumes after a controller reconnects. + + GIVEN a monitor that requested telemetry on an earlier connection, + WHEN the flight controller reconnects with a different link, + THEN the stream is requested on the new connection and old stick readings are discarded. + """ + controller = connected_flight_controller + model = RCCalibrationDataModel(controller) + controller.master.recv_match.side_effect = [_make_rc_channels_msg([1900]), None] + assert model.get_rc_telemetry()["roll"] == 800 + controller.master = None + assert not model.get_rc_telemetry() + controller.master = MagicMock() + controller.master.recv_match.return_value = None + assert not model.get_rc_telemetry() + assert model.get_preview_axes()["roll"] == 0 + assert controller.request_rc_receiver_messages.call_count == 2 + + def test_monitor_uses_existing_stream_when_interval_request_is_rejected( + self, connected_flight_controller: MagicMock + ) -> None: + """ + Existing telemetry remains usable when interval commands are unsupported. + + GIVEN a controller that rejects interval requests but already sends RC data, + WHEN the monitor polls, + THEN it still shows received values and does not repeatedly request the stream. + """ + controller = connected_flight_controller + controller.request_rc_receiver_messages.return_value = (False, "unsupported") + controller.master.recv_match.side_effect = [_make_rc_channels_msg([1200]), _make_rc_channels_msg([1800]), None] + model = RCCalibrationDataModel(controller) + assert model.get_rc_telemetry()["channels"][0]["value"] == 1800 + controller.request_rc_receiver_messages.assert_called_once_with() + def test_telemetry_is_empty_when_disconnected(self, disconnected_flight_controller) -> None: """ Telemetry is empty without a link so the GUI keeps waiting. @@ -358,10 +428,8 @@ def test_telemetry_maps_first_four_channels_to_axes(self, connected_flight_contr assert telemetry["pitch"] == 0.0 assert telemetry["throttle"] == 1000.0 assert telemetry["yaw"] == 0.0 - assert [entry.kwargs for entry in connected_flight_controller.master.recv_match.call_args_list] == [ - {"type": "RC_CHANNELS", "blocking": False}, - {"type": "HEARTBEAT", "blocking": False}, - ] + assert connected_flight_controller.master.recv_match.call_count == 2 + connected_flight_controller.master.recv_match.assert_called_with(blocking=False) def test_telemetry_keeps_all_channels_and_marks_invalid_values_unavailable(self, connected_flight_controller) -> None: """ @@ -453,19 +521,21 @@ def test_recv_match_exception_is_swallowed(self, connected_flight_controller, ca def test_telemetry_includes_flight_mode_from_heartbeat(self, connected_flight_controller) -> None: """ - A HEARTBEAT following the RC_CHANNELS read populates the flight mode. + A HEARTBEAT following an RC_CHANNELS message populates the flight mode. GIVEN: An RC_CHANNELS message followed by a HEARTBEAT with custom_mode=3 - WHEN: get_rc_telemetry reads both messages + WHEN: get_rc_telemetry reads both messages in one poll THEN: The telemetry carries flight_mode="3" """ model = RCCalibrationDataModel(connected_flight_controller) rc_msg = _make_rc_channels_msg([1500], chancount=1) hb = MagicMock() hb.custom_mode = 3 - connected_flight_controller.master.recv_match.side_effect = [rc_msg, hb] + hb.get_type.return_value = "HEARTBEAT" + connected_flight_controller.master.recv_match.side_effect = [rc_msg, hb, None] telemetry = model.get_rc_telemetry() + assert telemetry["channels"][0]["value"] == 1500 assert telemetry["flight_mode"] == "3" diff --git a/tests/plugins/test_data_model_rc_receiver.py b/tests/plugins/test_data_model_rc_receiver.py index bc3d7834c..36315c734 100755 --- a/tests/plugins/test_data_model_rc_receiver.py +++ b/tests/plugins/test_data_model_rc_receiver.py @@ -23,6 +23,7 @@ from ardupilot_methodic_configurator import data_model_parameter_editor as editor_module from ardupilot_methodic_configurator.data_model_parameter_editor import ParameterEditor from ardupilot_methodic_configurator.plugins import frontend_tkinter_rc_receiver as frontend +from ardupilot_methodic_configurator.plugins.data_model_rc_calibration import RCCalibrationDataModel from ardupilot_methodic_configurator.plugins.data_model_rc_receiver import RCReceiverDataModel, RCReceiverState from ardupilot_methodic_configurator.plugins.frontend_tkinter_rc_receiver import register_rc_receiver_plugin from ardupilot_methodic_configurator.plugins.plugin_constants import PLUGIN_RC_RECEIVER @@ -86,6 +87,49 @@ def test_user_can_monitor_first_five_channels_without_writing_parameters(receive receiver_context.controller.request_rc_receiver_messages.assert_called_once_with() +def test_controller_step_restarts_stream_and_preserves_interleaved_rc_samples(receiver_context: SimpleNamespace) -> None: + """ + Channel monitoring continues after the receiver step. + + GIVEN the receiver step has enabled and then restored its telemetry stream, + WHEN the controller step monitors multiple RC samples before a heartbeat, + THEN it requests the stream again, records both extremes and displays the latest sample. + """ + controller = receiver_context.controller + controller.fc_parameters = {} + receiver_context.messages.append(rc_message((1500,))) + assert receiver_context.model.get_receiver_state().channels[0] == 1500 + receiver_context.model.stop_monitoring() + + # Mimic pymavlink's filter: unmatched packets are consumed, not queued. + def recv_match(*, blocking: bool, type: list[str] | None = None) -> Any: # noqa: ANN401,A002 # pylint: disable=redefined-builtin + assert not blocking + while receiver_context.messages: + message = receiver_context.messages.popleft() + if type is None or message.get_type() in type: + return message + return None + + controller.master.recv_match.side_effect = recv_match + heartbeat = mavutil.mavlink.MAVLink_heartbeat_message(2, 3, 0, 7, 4, 3) + receiver_context.messages.extend([rc_message((1100,)), rc_message((1900,)), heartbeat]) + model = RCCalibrationDataModel(controller) + assert model.start_calibration()[0] + telemetry = model.get_rc_telemetry() + assert telemetry["channels"][0]["value"] == 1900 + assert telemetry["flight_mode"] == "7" + assert model._channel_min[0] == 1100 + assert model._channel_max[0] == 1900 + assert [call.args for call in controller.request_rc_receiver_messages.call_args_list] == [(), (0,), ()] + model.stop_monitoring() + model.stop_monitoring() + assert controller.request_rc_receiver_messages.call_args_list[-1].args == (0,) + assert controller.request_rc_receiver_messages.call_count == 4 + model.get_rc_telemetry() + assert controller.request_rc_receiver_messages.call_args_list[-1].args == () + controller.set_param.assert_not_called() + + @pytest.mark.parametrize(("rssi", "expected"), [(0, 0), (254, 254), (255, None)]) def test_rssi_distinguishes_zero_strength_from_unavailable( receiver_context: SimpleNamespace, rssi: int, expected: int | None diff --git a/tests/plugins/test_frontend_tkinter_rc_calibration.py b/tests/plugins/test_frontend_tkinter_rc_calibration.py index edcdd9d08..4046bf644 100755 --- a/tests/plugins/test_frontend_tkinter_rc_calibration.py +++ b/tests/plugins/test_frontend_tkinter_rc_calibration.py @@ -126,6 +126,52 @@ def while_popup_open(*_args: object, **_kwargs: object) -> MagicMock: rc_navigation_view.controller.set_param.assert_not_called() +def test_rc_monitor_restores_stream_after_pause(rc_navigation_view: SimpleNamespace) -> None: + """ + Monitoring resumes after yielding the connection. + + GIVEN the controller step is receiving telemetry, + WHEN another operation pauses and resumes the monitor, + THEN the stream is released while paused and requested again on the next poll. + """ + view = rc_navigation_view.view + controller = rc_navigation_view.controller + view._stop_polling() + view._check_telemetry() + controller.request_rc_receiver_messages.assert_called_once_with() + view.on_deactivate() + assert controller.request_rc_receiver_messages.call_args.args == (0,) + view.on_activate() + view._stop_polling() + view._check_telemetry() + assert [call.args for call in controller.request_rc_receiver_messages.call_args_list] == [(), (0,), ()] + + +def test_controller_step_refreshes_bars_for_consecutive_samples(rc_navigation_view: SimpleNamespace) -> None: + """ + Consecutive receiver samples update the real controller-step widgets. + + GIVEN the controller step has opened after the receiver stream was restored, + WHEN the user moves a channel twice, + THEN each new PWM value is displayed and the telemetry stream is requested. + """ + view = rc_navigation_view.view + controller = rc_navigation_view.controller + messages = [] + for pwm in (1100, 1900): + message = MagicMock(chancount=1) + message.get_type.return_value = "RC_CHANNELS" + for number in range(1, 17): + setattr(message, f"chan{number}_raw", pwm) + messages.extend((message, None)) + controller.master.recv_match.side_effect = messages + for expected in ("1100 µs", "1900 µs"): + view._stop_polling() + view._check_telemetry() + assert view.channel_bars._value_labels[1].cget("text") == expected + controller.request_rc_receiver_messages.assert_called_once_with() + + class TestRCChannelBars: """Test the user's live channel-value and calibration-marker display.""" @@ -576,6 +622,7 @@ def test_embedded_monitor_initialises_all_markers_and_preserves_channels_on_hear """ controller = MagicMock() controller.fc_parameters = {"RC1_MIN": 990.0, "RC1_MAX": 2010.0, "RC1_TRIM": 1490.0} + controller.request_rc_receiver_messages.return_value = (True, "") controller.master.recv_match.return_value = None parent = tk.Frame(tk_root) view = RCCalibrationView(parent, RCCalibrationDataModel(controller), MagicMock(spec=BaseWindow)) @@ -586,6 +633,7 @@ def test_embedded_monitor_initialises_all_markers_and_preserves_channels_on_hear canvas = view.channel_bars._bar_canvases[1] assert len(canvas.find_withtag("calibration_marker")) == 3 message = MagicMock(chancount=4) + message.get_type.return_value = "RC_CHANNELS" for channel_number in range(1, 17): setattr(message, f"chan{channel_number}_raw", 1600 if channel_number == 1 else 1500) controller.master.recv_match.side_effect = [message, None] @@ -593,7 +641,8 @@ def test_embedded_monitor_initialises_all_markers_and_preserves_channels_on_hear view._check_telemetry() before = view.channel_bars._value_labels[1].cget("text") heartbeat = MagicMock(custom_mode=3) - controller.master.recv_match.side_effect = [None, heartbeat] + heartbeat.get_type.return_value = "HEARTBEAT" + controller.master.recv_match.side_effect = [heartbeat] view._stop_polling() view._check_telemetry() assert before == "1600 µs" @@ -651,7 +700,7 @@ def test_embedded_monitor_aligns_sections_with_documentation_and_moves_scrollbar host.destroy() -def test_floating_monitor_shares_the_stick_boxes_and_three_channel_markers(tk_root: tk.Tk) -> None: +def test_floating_monitor_shares_the_stick_boxes_and_three_channel_markers(tk_root: tk.Tk) -> None: # noqa: PLR0915 # pylint: disable=too-many-statements """ The floating live monitor presents the same markers and stick preview as the embedded view. @@ -661,6 +710,7 @@ def test_floating_monitor_shares_the_stick_boxes_and_three_channel_markers(tk_ro """ controller = MagicMock() controller.fc_parameters = {"RC1_MIN": 990.0, "RC1_MAX": 2010.0, "RC1_TRIM": 1490.0} + controller.request_rc_receiver_messages.return_value = (True, "") controller.master.recv_match.return_value = None root = tk.Toplevel(tk_root) root.withdraw() @@ -696,13 +746,16 @@ def init_hidden_base(window: BaseWindow, _root_tk: tk.Toplevel) -> None: assert tuple(int(str(value)) for value in section.cget("padding")) == (4,) assert section.pack_info()["pady"] == (0, 4) message = MagicMock(chancount=4) + message.get_type.return_value = "RC_CHANNELS" for number in range(1, 17): setattr(message, f"chan{number}_raw", 1700 if number == 3 else 1500) controller.master.recv_match.side_effect = [message, None] popup._stop_polling() popup._check_telemetry() assert popup.channel_bars._value_labels[3].cget("text") == "1700 µs" - controller.master.recv_match.side_effect = [None, MagicMock(custom_mode=5)] + heartbeat = MagicMock(custom_mode=5) + heartbeat.get_type.return_value = "HEARTBEAT" + controller.master.recv_match.side_effect = [heartbeat] popup._stop_polling() popup._check_telemetry() assert popup.channel_bars._value_labels[3].cget("text") == "1700 µs" @@ -728,6 +781,7 @@ def test_cancel_calibration_restores_configured_markers_immediately(tk_root: tk. """ controller = MagicMock() controller.fc_parameters = {"RC1_MIN": 1000.0, "RC1_MAX": 2000.0, "RC1_TRIM": 1490.0} + controller.request_rc_receiver_messages.return_value = (True, "") controller.master.recv_match.return_value = None parent = tk.Frame(tk_root) view = RCCalibrationView(parent, RCCalibrationDataModel(controller), MagicMock(spec=BaseWindow)) @@ -735,6 +789,7 @@ def test_cancel_calibration_restores_configured_markers_immediately(tk_root: tk. view._start_btn.invoke() assert str(view._cancel_btn.cget("state")) == "normal" message = MagicMock(chancount=1) + message.get_type.return_value = "RC_CHANNELS" for number in range(1, 17): setattr(message, f"chan{number}_raw", 1300) controller.master.recv_match.side_effect = [message, None] @@ -764,6 +819,7 @@ def test_mode_combobox_stages_mapping_and_refreshes_new_values_without_upload( controller, editor = rc_staging_context model = RCCalibrationDataModel(controller, editor) message = MagicMock(chancount=4) + message.get_type.return_value = "RC_CHANNELS" for number in range(1, 17): setattr(message, f"chan{number}_raw", 1800 if number == 3 else 1500) controller.master.recv_match.side_effect = [message, None] @@ -1147,6 +1203,7 @@ def test_rc_monitor_warns_once_for_a_telemetry_gap_then_recovers( assert view._timer_id is not None # When: Fresh receiver telemetry arrives. message = MagicMock(chancount=16) + message.get_type.return_value = "RC_CHANNELS" for number in range(1, 17): setattr(message, f"chan{number}_raw", 1700 if number == 3 else 1500) context.controller.master.recv_match.side_effect = [message, None] From 0e633dd661956b27e38323c10a579adfff63aadd Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Fri, 30 Oct 2026 19:49:53 +0100 Subject: [PATCH 2/4] fix(ESC telemetry plugin): fix startup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixed and verified on the connected Cube: step 08 now shows ESC 9–12. 0* means zero RPM or unavailable individual telemetry; electrical values remain unavailable. Signed-off-by: Dr.-Ing. Amilcar do Carmo Lucas --- .../backend_esc_telemetry.py | 41 +++-- .../data_model_esc_telemetry.py | 28 ++- .../frontend_tkinter_esc_telemetry.py | 4 +- tests/test_esc_telemetry.py | 173 ++++++++++++++++++ 4 files changed, 229 insertions(+), 17 deletions(-) diff --git a/ardupilot_methodic_configurator/backend_esc_telemetry.py b/ardupilot_methodic_configurator/backend_esc_telemetry.py index 81d354675..831d2fb0c 100644 --- a/ardupilot_methodic_configurator/backend_esc_telemetry.py +++ b/ardupilot_methodic_configurator/backend_esc_telemetry.py @@ -27,6 +27,7 @@ from ardupilot_methodic_configurator.backend_flightcontroller import FlightController ESC_MESSAGE_OFFSETS = {f"ESC_TELEMETRY_{start}_TO_{start + 3}": start - 1 for start in range(1, MAX_ESCS + 1, 4)} +_UNKNOWN_ESC_MESSAGES = {f"UNKNOWN_{getattr(mavlink, f'MAVLINK_MSG_ID_{name}')}" for name in ESC_MESSAGE_OFFSETS} class EscTelemetryBackend: @@ -38,6 +39,7 @@ def __init__(self, flight_controller: "FlightController", clock: Callable[[], fl self._master: Any = None self._last_request: float | None = None self._readings: dict[int, EscTelemetryReading] = {} + self._decoder = mavlink.MAVLink(None) def close(self) -> None: """Remove only this observer and reset request timing.""" @@ -56,6 +58,17 @@ def _target_ids(self, master: Any) -> tuple[int, int]: # noqa: ANN401 ) def _capture(self, master: Any, message: Any) -> None: # noqa: ANN401 + if message.get_type() in _UNKNOWN_ESC_MESSAGES: + # A connection that initially detected MAVLink 1 can still receive + # MAVLink 2 frames, but its v1 dialect does not know ESC message IDs. + # Decode and validate the original packet without replacing the + # shared parser or disturbing other connection consumers. + try: + self._decoder.signing = master.mav.signing + message = self._decoder.decode(bytearray(message.get_msgbuf())) + except mavlink.MAVError as exc: + logging_debug("Invalid ESC telemetry packet: %s", exc) + return offset = ESC_MESSAGE_OFFSETS.get(str(message.get_type())) if offset is None: return @@ -88,18 +101,22 @@ def poll(self) -> EscTelemetrySnapshot: if self._last_request is None or now - self._last_request >= 1.0: # ArduPilot maps this first group to its shared sender for ALL ESC groups. system_id, component_id = self._target_ids(master) - master.mav.command_long_send( - system_id, - component_id, - mavlink.MAV_CMD_REQUEST_MESSAGE, - 0, - mavlink.MAVLINK_MSG_ID_ESC_TELEMETRY_1_TO_4, - 0, - 0, - 0, - 0, - 0, - 0, + # ESC_TELEMETRY_* IDs require MAVLink 2. Encode this request + # as v2 even if the link initially negotiated MAVLink 1. + master.mav.send( + mavlink.MAVLink_command_long_message( + system_id, + component_id, + mavlink.MAV_CMD_REQUEST_MESSAGE, + 0, + mavlink.MAVLINK_MSG_ID_ESC_TELEMETRY_1_TO_4, + 0, + 0, + 0, + 0, + 0, + 0, + ) ) self._last_request = now deadline = self._clock() + 0.01 diff --git a/ardupilot_methodic_configurator/data_model_esc_telemetry.py b/ardupilot_methodic_configurator/data_model_esc_telemetry.py index 172628acc..109e9dad1 100644 --- a/ardupilot_methodic_configurator/data_model_esc_telemetry.py +++ b/ardupilot_methodic_configurator/data_model_esc_telemetry.py @@ -46,7 +46,7 @@ def __init__(self, flight_controller: "FlightController") -> None: self.flight_controller = flight_controller self._connection: object | None = None self._telemetry: dict[int, tuple[float, EscTelemetryReading]] = {} - self._rpm: dict[int, tuple[float, int]] = {} + self._rpm: dict[int, tuple[float, int | str]] = {} @property def visible_esc_indices(self) -> tuple[int, ...]: @@ -71,15 +71,35 @@ def row_labels() -> tuple[str, ...]: _("Telemetry packets"), ) - def update(self, snapshot: EscTelemetrySnapshot) -> tuple[tuple[str, ...], ...]: + def _configured_bdshot_output(self, index: int) -> bool: + """Identify configured outputs, accounting for the ESC MAVLink offset.""" + params = self.flight_controller.fc_parameters + if not isinstance(params, dict): + return False + try: + mask = int(params.get("SERVO_BLH_BDMASK", 0)) + offset = int(params.get("ESC_TLM_MAV_OFS", 0)) + except (TypeError, ValueError, OverflowError): + return False + output = index + offset + return 0 <= offset < MAX_ESCS and 0 <= output < MAX_ESCS and bool(mask & (1 << output)) + + def update(self, snapshot: EscTelemetrySnapshot) -> tuple[tuple[str, ...], ...]: # pylint: disable=too-many-branches """Return measurement rows; zeros remain valid for an ESC reporting telemetry.""" if snapshot.connection is not self._connection or snapshot.error or snapshot.connection is None: self.clear_live_data() self._connection = snapshot.connection if not snapshot.error and snapshot.connection is not None: for index, reading in snapshot.readings.items(): - if not 0 <= index < MAX_ESCS or len(reading.values) != len(ESC_FIELDS) or not any(reading.values): - continue # ArduPilot pads unused members of a four-ESC group with zeros. + if not 0 <= index < MAX_ESCS or len(reading.values) != len(ESC_FIELDS): + continue + if not any(reading.values): + # The wire format cannot distinguish a stopped BDShot motor from + # an unavailable member of an otherwise active group. + if self._configured_bdshot_output(index): + received_at = snapshot.time if reading.received_at is None else reading.received_at + self._rpm[index] = (received_at, "0*") + continue old = self._telemetry.get(index) count = reading.values[-1] received_at = snapshot.time if reading.received_at is None else reading.received_at diff --git a/ardupilot_methodic_configurator/frontend_tkinter_esc_telemetry.py b/ardupilot_methodic_configurator/frontend_tkinter_esc_telemetry.py index 44a5046ba..fcc0df129 100644 --- a/ardupilot_methodic_configurator/frontend_tkinter_esc_telemetry.py +++ b/ardupilot_methodic_configurator/frontend_tkinter_esc_telemetry.py @@ -48,7 +48,9 @@ def __init__(self, parent: tk.Frame | ttk.Frame, model: EscTelemetryDataModel, b self.table, _( "Live ESC readings. Missing or stale data is shown as '-'. " - "ESC numbers follow the flight controller's ESC telemetry offset." + "ESC numbers follow the flight controller's ESC telemetry offset. " + "For configured bidirectional DShot outputs, 0* means the group reported zero RPM; " + "the individual output may instead be unavailable." ), ) self.scroll_tooltip = Tooltip(scrollbar, _("Scroll to view additional ESC columns.")) diff --git a/tests/test_esc_telemetry.py b/tests/test_esc_telemetry.py index 2e5b80ec4..bc2c8fe0e 100755 --- a/tests/test_esc_telemetry.py +++ b/tests/test_esc_telemetry.py @@ -17,6 +17,7 @@ import jsonschema import pytest +from pymavlink.dialects.v10 import ardupilotmega as mavlink1 from pymavlink.dialects.v20 import ardupilotmega as mavlink from telemetry_test_helpers import create_telemetry_link, feed @@ -93,6 +94,119 @@ def test_received_esc_groups_appear_in_the_correct_columns( assert not link.master.message_hooks +@pytest.mark.parametrize("start", [1, 5, 9, 13, 17, 21, 25, 29]) +def test_esc_telemetry_is_displayed_when_connection_initially_detected_mavlink1( + link: SimpleNamespace, model: EscTelemetryDataModel, start: int +) -> None: + """ + Decode ESC messages even when the shared connection began in MAVLink 1. + + GIVEN a MAVLink 1 parser and an FC that sends ESC telemetry only after a MAVLink 2 request, + WHEN the monitor requests and receives an ESC group, + THEN the correct readings appear without replacing the connection's parser. + """ + master = link.master + master.mav = mavlink1.MAVLink(master, srcSystem=255, srcComponent=190) + master.mav.robust_parsing = True + master.WIRE_PROTOCOL_VERSION = "1.0" + parser = master.mav + message_type = getattr(mavlink, f"MAVLink_esc_telemetry_{start}_to_{start + 3}_message") + expected = packet() + + def respond_to_request(data: bytes) -> None: + command = mavlink.MAVLink(None).parse_char(data) + assert data[0] == 0xFD + assert command is not None + assert command.command == mavlink.MAV_CMD_REQUEST_MESSAGE + feed( + link, + message_type( + *[getattr(expected, name) for name in ("temperature", "voltage", "current", "totalcurrent", "rpm", "count")] + ), + ) + + master.write.side_effect = respond_to_request + backend = EscTelemetryBackend(link.controller, lambda: link.now) + try: + rows = model.update(backend.poll()) + assert [row[start - 1] for row in rows] == ["1200", "24.50", "1.23", "42", "77", "10"] + assert model.visible_esc_indices == (start - 1,) + assert master.mav is parser + finally: + backend.close() + + +def test_invalid_unknown_esc_packet_does_not_display_telemetry(link: SimpleNamespace) -> None: + """ + Validate packets before using telemetry unknown to a MAVLink 1 parser. + + GIVEN a corrupted MAVLink 2 ESC packet received by a MAVLink 1 parser, + WHEN the monitor polls, + THEN the invalid reading is ignored without stopping the observer. + """ + master = link.master + master.mav = mavlink1.MAVLink(master, srcSystem=255, srcComponent=190) + master.mav.robust_parsing = True + encoded = bytearray(packet().pack(link.sender)) + encoded[-1] ^= 1 + link.incoming.write(encoded) + link.incoming.seek(0) + backend = EscTelemetryBackend(link.controller, lambda: link.now) + try: + snapshot = backend.poll() + assert not snapshot.error + assert not snapshot.readings + assert backend._capture in master.message_hooks + finally: + backend.close() + + +@pytest.mark.parametrize("source", [(2, 1), (1, 2)]) +def test_mavlink1_parser_does_not_accept_esc_packets_from_other_sources( + link: SimpleNamespace, source: tuple[int, int] +) -> None: + """ + Filter decoded ESC packets using the actual MAVLink 2 source. + + GIVEN a MAVLink 1 parser and an ESC packet from another system or component, + WHEN the monitor decodes it, + THEN the unrelated ESC is not displayed. + """ + link.master.mav = mavlink1.MAVLink(link.master, srcSystem=255, srcComponent=190) + link.master.mav.robust_parsing = True + feed(link, packet(), *source) + backend = EscTelemetryBackend(link.controller, lambda: link.now) + try: + assert not backend.poll().readings + finally: + backend.close() + + +@pytest.mark.parametrize("valid", [True, False]) +def test_mavlink1_unknown_esc_packets_respect_connection_signing(link: SimpleNamespace, valid: bool) -> None: + """ + Preserve the connection's authentication policy while decoding ESC packets. + + GIVEN a MAVLink 1 parser with a signing key and a signed MAVLink 2 ESC packet, + WHEN the monitor decodes the packet, + THEN only a packet with a valid signature is accepted. + """ + link.master.mav = mavlink1.MAVLink(link.master, srcSystem=255, srcComponent=190) + link.master.mav.robust_parsing = True + link.master.mav.signing.secret_key = b"a" * 32 + link.sender.signing.secret_key = (b"a" if valid else b"b") * 32 + link.sender.signing.sign_outgoing = True + link.sender.signing.timestamp = 100 + feed(link, packet()) + backend = EscTelemetryBackend(link.controller, lambda: link.now) + try: + snapshot = backend.poll() + assert not snapshot.error + assert bool(snapshot.readings) is valid + finally: + backend.close() + + def test_missing_and_stale_escs_are_unavailable(model: EscTelemetryDataModel) -> None: """ An empty table and one reporting ESC. @@ -127,6 +241,45 @@ def test_rpm_only_telemetry_does_not_invent_electrical_readings(model: EscTeleme assert model.visible_esc_indices == (0,) +def test_stopped_bdshot_outputs_from_real_controller_packet_are_visible(link: SimpleNamespace) -> None: + """ + Show zero-filled packets from the connected controller. + + GIVEN the connected controller's four configured BDShot outputs and an all-zero group 9-12 + WHEN the encoded packet is received and later stops arriving + THEN ambiguous zero RPMs are shown, electrical data stays unavailable, and RPM expires. + """ + link.controller.fc_parameters = {"SERVO_BLH_BDMASK": 3840.0, "ESC_TLM_MAV_OFS": 0.0} + model = EscTelemetryDataModel(link.controller) + backend = EscTelemetryBackend(link.controller, lambda: link.now) + feed(link, mavlink.MAVLink_esc_telemetry_9_to_12_message(*([[0] * 4] * 6))) + try: + rows = model.update(backend.poll()) + assert model.visible_esc_indices == (8, 9, 10, 11) + assert rows[0][8:12] == ("0*",) * 4 + assert all(row[8:12] == ("-",) * 4 for row in rows[1:]) + link.now = 3.0 + assert model.update(backend.poll())[0][8:12] == ("-",) * 4 + finally: + backend.close() + + +def test_bdshot_zero_padding_respects_mask_and_mavlink_offset(link: SimpleNamespace) -> None: + """ + Avoid treating unconfigured padding as a BDShot output. + + GIVEN a group containing configured and unconfigured zero-filled outputs and a MAVLink offset + WHEN the group arrives + THEN only configured outputs become visible and their RPM is explicitly ambiguous. + """ + link.controller.fc_parameters = {"SERVO_BLH_BDMASK": float(1 << 9), "ESC_TLM_MAV_OFS": 4.0} + model = EscTelemetryDataModel(link.controller) + readings = {index: EscTelemetryReading((0, 0, 0, 0, 0, 0)) for index in range(4, 8)} + rows = model.update(EscTelemetrySnapshot(object(), 0, readings)) + assert model.visible_esc_indices == (5,) + assert rows[0][4:8] == ("-", "0*", "-", "-") + + def test_counter_wrap_and_connection_boundaries(model: EscTelemetryDataModel) -> None: """ A reporting ESC near counter wrap. @@ -346,6 +499,26 @@ def test_treeview_refreshes_and_releases_resources(view: frontend.EscTelemetryVi assert resumed not in view.tk.call("after", "info") +def test_esc_step_shows_values_from_mavlink1_initial_connection( + view: frontend.EscTelemetryView, link: SimpleNamespace +) -> None: + """ + Show the decoded ESC values in the real step table. + + GIVEN a connection that initially selected the MAVLink 1 dialect, + WHEN an ESC telemetry packet arrives in MAVLink 2, + THEN the ESC column and its voltage become visible. + """ + link.master.mav = mavlink1.MAVLink(link.master, srcSystem=255, srcComponent=190) + link.master.mav.robust_parsing = True + feed(link, packet()) + poll_view(view) + assert view.table.cget("displaycolumns") == ("1",) + values = view.table.item("1", "values") + assert isinstance(values, tuple) + assert values[0] == "24.50" + + def test_upload_lock_and_modal_dialog_pause_reads(view: frontend.EscTelemetryView, link: SimpleNamespace) -> None: """ An exclusive FC operation or modal tool. From f9fa81a31d0adf40d28e6f7813e073d6e691bff3 Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Sat, 31 Oct 2026 15:23:24 +0100 Subject: [PATCH 3/4] refactor(telemetry): share polling and MAVLink observer lifecycle Consolidate telemetry transport and RC view polling to keep connection handling, bounded reads, rendering, and warnings consistent. - Add MavlinkTelemetryBackend for shared observer registration, connection changes, cleanup, and the 200-packet/10-ms polling limit. - Reuse the backend in ESC and data telemetry while preserving their message decoding and request behavior. - Share polling, rendering, warning counters, and timer scheduling between the embedded RC calibration view and floating monitor. - Apply the existing selected-controller source filter to calibration so foreign RC samples and heartbeats cannot affect its results. - Remove the unused heartbeat reader that could discard RC samples. - Reuse source-aware MAVLink messages and controller fixtures in tests. - Cover foreign-source rejection, polling limits, reconnects, and preservation of other observers. - Include the shared backend in mutation-testing targets. Validation: - 482 relevant tests pass. - Two known GUI layout failures remain excluded. - Pre-commit, Pylint, mypy, and Pyright pass. Signed-off-by: Dr.-Ing. Amilcar do Carmo Lucas --- .../backend_data_telemetry.py | 27 +--- .../backend_esc_telemetry.py | 37 ++--- .../backend_mavlink_telemetry.py | 53 +++++++ .../plugins/backend_rc_telemetry.py | 7 +- .../plugins/data_model_rc_calibration.py | 14 -- .../plugins/data_model_rc_receiver.py | 2 - .../frontend_tkinter_rc_calibration.py | 98 ++++++------ pyproject.toml | 1 + tests/plugins/conftest.py | 16 +- .../plugins/test_data_model_rc_calibration.py | 148 ++++++++---------- tests/plugins/test_data_model_rc_receiver.py | 19 +-- .../test_frontend_tkinter_rc_calibration.py | 49 +++--- tests/telemetry_test_helpers.py | 13 +- tests/test_esc_telemetry.py | 35 ++++- .../test_frontend_tkinter_navigation_lock.py | 3 +- 15 files changed, 272 insertions(+), 250 deletions(-) create mode 100644 ardupilot_methodic_configurator/backend_mavlink_telemetry.py diff --git a/ardupilot_methodic_configurator/backend_data_telemetry.py b/ardupilot_methodic_configurator/backend_data_telemetry.py index 68fece636..26ce7ba04 100644 --- a/ardupilot_methodic_configurator/backend_data_telemetry.py +++ b/ardupilot_methodic_configurator/backend_data_telemetry.py @@ -11,10 +11,11 @@ from collections.abc import Callable from logging import debug as logging_debug from time import monotonic -from typing import TYPE_CHECKING, Any, cast +from typing import TYPE_CHECKING, Any from serial import SerialException +from ardupilot_methodic_configurator.backend_mavlink_telemetry import MavlinkTelemetryBackend from ardupilot_methodic_configurator.data_model_data_telemetry import ( MessageKey, MessageSample, @@ -28,25 +29,21 @@ _MAX_MESSAGE_TYPES = 512 -class DataTelemetryBackend: +class DataTelemetryBackend(MavlinkTelemetryBackend): """Observe parsed packets without replacing hooks or reading from a second connection.""" def __init__(self, flight_controller: "FlightController", clock: Callable[[], float] = monotonic) -> None: - self.flight_controller = flight_controller - self._clock = clock - self._master: Any = None + super().__init__(flight_controller, clock) self._messages: dict[MessageKey, MessageSample] = {} self._radio: RadioSample | None = None def close(self) -> None: """Detach only our observer and discard data at lifecycle boundaries.""" - if self._master is not None and self._capture in self._master.message_hooks: - self._master.message_hooks.remove(self._capture) - self._master = None + super().close() self._messages.clear() self._radio = None - def _capture(self, _master: object, message: Any) -> None: # noqa: ANN401 + def _capture(self, _master: object, message: Any, /) -> None: # noqa: ANN401 """Count all sources independently, including modem-injected RADIO messages.""" name = str(message.get_type()) if name == "BAD_DATA": @@ -83,20 +80,12 @@ def _radio_value(value: int, maximum: int) -> int | None: def poll(self) -> TelemetrySample: """Enable connection-owned heartbeats and drain at most 200 packets/10 ms.""" - master = cast("Any", self.flight_controller.master) - if master is not self._master: - self.close() - self._master = master - if master is not None: - master.message_hooks.append(self._capture) + master = self._sync_connection() if master is None: return TelemetrySample(None, self._clock()) try: self.flight_controller.start_gcs_heartbeat() - deadline = self._clock() + 0.01 - for _index in range(200): - if self._clock() >= deadline or master.recv_match(blocking=False) is None: - break + self._drain_messages() return TelemetrySample( connection=master, time=self._clock(), diff --git a/ardupilot_methodic_configurator/backend_esc_telemetry.py b/ardupilot_methodic_configurator/backend_esc_telemetry.py index 831d2fb0c..bc3f0c653 100644 --- a/ardupilot_methodic_configurator/backend_esc_telemetry.py +++ b/ardupilot_methodic_configurator/backend_esc_telemetry.py @@ -11,11 +11,12 @@ from collections.abc import Callable from logging import debug as logging_debug from time import monotonic -from typing import TYPE_CHECKING, Any, cast +from typing import TYPE_CHECKING, Any from pymavlink.dialects.v20 import ardupilotmega as mavlink from serial import SerialException +from ardupilot_methodic_configurator.backend_mavlink_telemetry import MavlinkTelemetryBackend from ardupilot_methodic_configurator.data_model_esc_telemetry import ( ESC_FIELDS, MAX_ESCS, @@ -28,24 +29,21 @@ ESC_MESSAGE_OFFSETS = {f"ESC_TELEMETRY_{start}_TO_{start + 3}": start - 1 for start in range(1, MAX_ESCS + 1, 4)} _UNKNOWN_ESC_MESSAGES = {f"UNKNOWN_{getattr(mavlink, f'MAVLINK_MSG_ID_{name}')}" for name in ESC_MESSAGE_OFFSETS} +_UNUSED_REQUEST_PARAMETERS = (0, 0, 0, 0, 0, 0) -class EscTelemetryBackend: +class EscTelemetryBackend(MavlinkTelemetryBackend): """Observe ESC packets without opening another connection or changing stream rates.""" def __init__(self, flight_controller: "FlightController", clock: Callable[[], float] = monotonic) -> None: - self.flight_controller = flight_controller - self._clock = clock - self._master: Any = None + super().__init__(flight_controller, clock) self._last_request: float | None = None self._readings: dict[int, EscTelemetryReading] = {} self._decoder = mavlink.MAVLink(None) def close(self) -> None: """Remove only this observer and reset request timing.""" - if self._master is not None and self._capture in self._master.message_hooks: - self._master.message_hooks.remove(self._capture) - self._master = None + super().close() self._last_request = None self._readings.clear() @@ -57,7 +55,7 @@ def _target_ids(self, master: Any) -> tuple[int, int]: # noqa: ANN401 int(info.component_id) if info.component_id else int(master.target_component), ) - def _capture(self, master: Any, message: Any) -> None: # noqa: ANN401 + def _capture(self, master: Any, message: Any, /) -> None: # noqa: ANN401 if message.get_type() in _UNKNOWN_ESC_MESSAGES: # A connection that initially detected MAVLink 1 can still receive # MAVLink 2 frames, but its v1 dialect does not know ESC message IDs. @@ -86,14 +84,7 @@ def _capture(self, master: Any, message: Any) -> None: # noqa: ANN401 def poll(self) -> EscTelemetrySnapshot: """Request all groups once per second and drain at most 200 packets/10 ms.""" - # This observer lifecycle and MAVLink command layout intentionally match other backends. - # pylint: disable=duplicate-code - master = cast("Any", self.flight_controller.master) - if master is not self._master: - self.close() - self._master = master - if master is not None: - master.message_hooks.append(self._capture) + master = self._sync_connection() if master is None: return EscTelemetrySnapshot(None, self._clock()) try: @@ -110,19 +101,11 @@ def poll(self) -> EscTelemetrySnapshot: mavlink.MAV_CMD_REQUEST_MESSAGE, 0, mavlink.MAVLINK_MSG_ID_ESC_TELEMETRY_1_TO_4, - 0, - 0, - 0, - 0, - 0, - 0, + *_UNUSED_REQUEST_PARAMETERS, ) ) self._last_request = now - deadline = self._clock() + 0.01 - for _index in range(200): - if self._clock() >= deadline or master.recv_match(blocking=False) is None: - break + self._drain_messages() readings, self._readings = self._readings, {} return EscTelemetrySnapshot(master, self._clock(), readings) except (OSError, SerialException, AttributeError, TypeError, ValueError) as exc: diff --git a/ardupilot_methodic_configurator/backend_mavlink_telemetry.py b/ardupilot_methodic_configurator/backend_mavlink_telemetry.py new file mode 100644 index 000000000..511eaaa02 --- /dev/null +++ b/ardupilot_methodic_configurator/backend_mavlink_telemetry.py @@ -0,0 +1,53 @@ +""" +Shared observer lifecycle and bounded reads for MAVLink telemetry backends. + +This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + +SPDX-FileCopyrightText: 2026 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +from abc import ABC, abstractmethod +from collections.abc import Callable +from time import monotonic +from typing import TYPE_CHECKING, Any, cast + +if TYPE_CHECKING: + from ardupilot_methodic_configurator.backend_flightcontroller import FlightController + + +class MavlinkTelemetryBackend(ABC): # pylint: disable=too-few-public-methods + """Attach one observer to the shared connection and keep each poll bounded.""" + + def __init__(self, flight_controller: "FlightController", clock: Callable[[], float] = monotonic) -> None: + self.flight_controller = flight_controller + self._clock = clock + self._master: Any = None + + def close(self) -> None: + """Detach only this backend's hook, preserving other connection consumers.""" + if self._master is not None and self._capture in self._master.message_hooks: + self._master.message_hooks.remove(self._capture) + self._master = None + + def _sync_connection(self) -> Any: # noqa: ANN401 + """Reset observations through close() before attaching to a replacement connection.""" + master = cast("Any", self.flight_controller.master) + if master is not self._master: + self.close() + self._master = master + if master is not None: + master.message_hooks.append(self._capture) + return master + + def _drain_messages(self) -> None: + """Read at most 200 packets or 10 ms, counting unrelated traffic toward both limits.""" + deadline = self._clock() + 0.01 + for _index in range(200): + if self._clock() >= deadline or self._master.recv_match(blocking=False) is None: + break + + @abstractmethod + def _capture(self, master: Any, message: Any, /) -> None: # noqa: ANN401 + """Handle one parsed message according to the consumer's telemetry policy.""" diff --git a/ardupilot_methodic_configurator/plugins/backend_rc_telemetry.py b/ardupilot_methodic_configurator/plugins/backend_rc_telemetry.py index 2a4a18bd5..bfdbb5495 100644 --- a/ardupilot_methodic_configurator/plugins/backend_rc_telemetry.py +++ b/ardupilot_methodic_configurator/plugins/backend_rc_telemetry.py @@ -50,7 +50,7 @@ def stop_monitoring(self) -> None: self._stream_requested = False def get_rc_telemetry(self) -> Iterator[Any]: - """Yield every packet in a bounded, nonblocking batch, in order.""" + """Yield packets from the selected controller in a bounded, nonblocking batch.""" self.sync_connection() master = cast("Any", self.connection) if master is None: @@ -67,10 +67,11 @@ def get_rc_telemetry(self) -> Iterator[Any]: message = master.recv_match(blocking=False) if message is None: break - yield message + if self.is_selected_source(message): + yield message def is_selected_source(self, message: Any) -> bool: # noqa: ANN401 - """Identify receiver-health packets belonging to the selected flight controller.""" + """Identify RC monitoring and calibration packets from the selected flight controller.""" master = cast("Any", self.connection) if master is None: return False diff --git a/ardupilot_methodic_configurator/plugins/data_model_rc_calibration.py b/ardupilot_methodic_configurator/plugins/data_model_rc_calibration.py index e369b6ac9..2d59e2704 100644 --- a/ardupilot_methodic_configurator/plugins/data_model_rc_calibration.py +++ b/ardupilot_methodic_configurator/plugins/data_model_rc_calibration.py @@ -427,17 +427,3 @@ def get_rc_telemetry(self) -> dict[str, Any]: logging_debug(_("Error reading MAVLink telemetry: %(error)s"), {"error": str(exc)}) return telemetry - - def get_flight_mode(self) -> str: - """Return the current flight mode string from the most recent HEARTBEAT message.""" - if self.flight_controller.master is None: - return _("Not connected") - try: - hb = self.flight_controller.master.recv_match( # pyright: ignore[reportAttributeAccessIssue] - type="HEARTBEAT", blocking=False - ) - if hb: - return str(hb.custom_mode) - except Exception as exc: # pylint: disable=broad-exception-caught - logging_debug(_("Error reading HEARTBEAT: %(error)s"), {"error": str(exc)}) - return _("No Data") diff --git a/ardupilot_methodic_configurator/plugins/data_model_rc_receiver.py b/ardupilot_methodic_configurator/plugins/data_model_rc_receiver.py index 0a8e6a03f..e55e42e4d 100644 --- a/ardupilot_methodic_configurator/plugins/data_model_rc_receiver.py +++ b/ardupilot_methodic_configurator/plugins/data_model_rc_receiver.py @@ -95,8 +95,6 @@ def get_receiver_state(self) -> RCReceiverState: now = self._clock() try: for message in self._telemetry.get_rc_telemetry(): - if not self._telemetry.is_selected_source(message): - continue if message.get_type() == "RC_CHANNELS": self._read_channels(message, now) elif message.get_type() == "SYS_STATUS": diff --git a/ardupilot_methodic_configurator/plugins/frontend_tkinter_rc_calibration.py b/ardupilot_methodic_configurator/plugins/frontend_tkinter_rc_calibration.py index daf7fc777..dc4c09c90 100644 --- a/ardupilot_methodic_configurator/plugins/frontend_tkinter_rc_calibration.py +++ b/ardupilot_methodic_configurator/plugins/frontend_tkinter_rc_calibration.py @@ -476,21 +476,62 @@ def _pwm_to_x(value: int, width: int) -> int: return 1 + round(fraction * (width - 2)) -class RCCalibrationPopup(CalibrationPopupBase["RCCalibrationDataModel"]): +class _RCTelemetryPolling: # pylint: disable=too-few-public-methods + """Share rendering, telemetry-gap warnings and scheduling between the two RC monitors.""" + + model: RCCalibrationDataModel + craft_preview: RCCraftPreview + stick_preview: RCStickPreview + channel_bars: RCChannelBars + _polling_widget: tk.Misc + _timer_id: str | None + _polls_without_updates: int + _no_telemetry_warning_emitted: bool + + def _schedule_rc_poll(self) -> None: + self._timer_id = self._polling_widget.after(100, self._check_telemetry) + + def _check_telemetry(self) -> None: + telemetry = self.model.get_rc_telemetry() + self.craft_preview.update_telemetry(telemetry) + + if not telemetry: + self._polls_without_updates += 1 + if self._polls_without_updates == 50 and not self._no_telemetry_warning_emitted: + self._no_telemetry_warning_emitted = True + logging_warning( + _("No RC telemetry has arrived after %(polls)d polls."), {"polls": self._polls_without_updates} + ) + self._schedule_rc_poll() + return + + self._polls_without_updates = 0 + self._no_telemetry_warning_emitted = False + + self.stick_preview.update_axes(telemetry) + if "channels" in telemetry: + self.channel_bars.update_channels(telemetry["channels"]) + + self._schedule_rc_poll() + + +class RCCalibrationPopup(_RCTelemetryPolling, CalibrationPopupBase["RCCalibrationDataModel"]): """A modern, borderless popup window with a custom draggable title bar for RC monitoring.""" _MIN_WIDTH = 600 _MIN_HEIGHT = 500 + _timer_id: str | None def __init__(self, parent: tk.Widget, model: RCCalibrationDataModel) -> None: super().__init__(parent, model) + self._polling_widget = self.root self._setup_style() self._setup_ui() self._resize_and_center() self.root.lift() self.root.focus_force() - self._timer_id = self.root.after(100, self._check_telemetry) + self._schedule_rc_poll() logging_debug(_("RC calibration progress popup created and polling scheduled.")) def destroy(self) -> None: @@ -524,31 +565,8 @@ def _refresh_preview(self) -> None: self.craft_preview.update_telemetry(axes) self.channel_bars.update_channels(self.model.get_channel_calibration()) - def _check_telemetry(self) -> None: - telemetry = self.model.get_rc_telemetry() - self.craft_preview.update_telemetry(telemetry) - if not telemetry: - self._polls_without_updates += 1 - if self._polls_without_updates == 50 and not self._no_telemetry_warning_emitted: - self._no_telemetry_warning_emitted = True - logging_warning( - _("No RC telemetry has arrived after %(polls)d polls."), {"polls": self._polls_without_updates} - ) - self._timer_id = self.root.after(100, self._check_telemetry) - return - - self._polls_without_updates = 0 - self._no_telemetry_warning_emitted = False - - self.stick_preview.update_axes(telemetry) - if "channels" in telemetry: - self.channel_bars.update_channels(telemetry["channels"]) - - self._timer_id = self.root.after(100, self._check_telemetry) - - -class RCCalibrationView(ttk.Frame): # pylint: disable=too-many-ancestors, too-many-instance-attributes +class RCCalibrationView(_RCTelemetryPolling, ttk.Frame): # pylint: disable=too-many-ancestors, too-many-instance-attributes """Main GUI view for the RC calibration plugin inside AMC.""" def __init__( @@ -558,6 +576,7 @@ def __init__( base_window: BaseWindow, ) -> None: super().__init__(parent) + self._polling_widget = self self.model = model self.base_window = base_window self._timer_id: str | None = None @@ -566,7 +585,7 @@ def __init__( self._no_telemetry_warning_emitted = False self._setup_style() self._setup_ui() - self._timer_id = self.after(100, self._check_telemetry) + self._schedule_rc_poll() def _setup_style(self) -> None: style = ttk.Style(self) @@ -709,36 +728,13 @@ def _stop_polling(self) -> None: def on_activate(self) -> None: """Resume live RC monitoring without changing collected or staged calibration values.""" if self._timer_id is None: - self._timer_id = self.after(100, self._check_telemetry) + self._schedule_rc_poll() def on_deactivate(self) -> None: """Yield the shared receiver while hidden or while diagnostics is open.""" self._stop_polling() self.model.stop_monitoring() - def _check_telemetry(self) -> None: - telemetry = self.model.get_rc_telemetry() - self.craft_preview.update_telemetry(telemetry) - - if not telemetry: - self._polls_without_updates += 1 - if self._polls_without_updates == 50 and not self._no_telemetry_warning_emitted: - self._no_telemetry_warning_emitted = True - logging_warning( - _("No RC telemetry has arrived after %(polls)d polls."), {"polls": self._polls_without_updates} - ) - self._timer_id = self.after(100, self._check_telemetry) - return - - self._polls_without_updates = 0 - self._no_telemetry_warning_emitted = False - - self.stick_preview.update_axes(telemetry) - if "channels" in telemetry: - self.channel_bars.update_channels(telemetry["channels"]) - - self._timer_id = self.after(100, self._check_telemetry) - def destroy(self) -> None: """Stop the polling loop before destroying the widget.""" try: diff --git a/pyproject.toml b/pyproject.toml index 1e9e912db..35178369e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -203,6 +203,7 @@ only_mutate = [ # Shared implementations used by telemetry and calibration plugins. "ardupilot_methodic_configurator/backend_data_telemetry.py", "ardupilot_methodic_configurator/backend_esc_telemetry.py", + "ardupilot_methodic_configurator/backend_mavlink_telemetry.py", "ardupilot_methodic_configurator/backend_radio_status.py", "ardupilot_methodic_configurator/backend_flightcontroller_commands.py", "ardupilot_methodic_configurator/data_model_data_telemetry.py", diff --git a/tests/plugins/conftest.py b/tests/plugins/conftest.py index c9ad34fb9..05d6a8c61 100755 --- a/tests/plugins/conftest.py +++ b/tests/plugins/conftest.py @@ -65,11 +65,23 @@ def fire(view: tk.Misc, timer_id: str) -> None: @pytest.fixture -def rc_staging_context() -> tuple[MagicMock, ParameterEditor]: - """Use real parameter objects and editor APIs; mock only filesystem and FC I/O.""" +def rc_controller() -> MagicMock: + """Provide a selected controller with RC stream requests and transport I/O mocked.""" controller = MagicMock() + controller.info.system_id = "1" + controller.info.component_id = "1" + controller.master.target_system = 1 + controller.master.target_component = 0 controller.master.recv_match.return_value = None controller.request_rc_receiver_messages.return_value = (True, "") + controller.fc_parameters = {} + return controller + + +@pytest.fixture +def rc_staging_context(rc_controller: MagicMock) -> tuple[MagicMock, ParameterEditor]: # pylint: disable=redefined-outer-name + """Use real parameter objects and editor APIs; mock only filesystem and FC I/O.""" + controller = rc_controller # pylint: disable=duplicate-code controller.fc_parameters = { "RCMAP_ROLL": 1.0, diff --git a/tests/plugins/test_data_model_rc_calibration.py b/tests/plugins/test_data_model_rc_calibration.py index 2e37cebb6..0a1c2da10 100755 --- a/tests/plugins/test_data_model_rc_calibration.py +++ b/tests/plugins/test_data_model_rc_calibration.py @@ -15,6 +15,7 @@ from unittest.mock import MagicMock import pytest +from telemetry_test_helpers import rc_channels_message as _make_rc_channels_msg from ardupilot_methodic_configurator.data_model_ardupilot_parameter import ArduPilotParameter from ardupilot_methodic_configurator.data_model_par_dict import Par @@ -30,32 +31,10 @@ # pylint: disable=protected-access,redefined-outer-name,too-many-lines -def _make_rc_channels_msg(raw: list[int], chancount: int | None = None) -> MagicMock: - """ - Build a mock MAVLink RC_CHANNELS message. - - ``raw`` holds the desired chan1_raw..chanN_raw values; the list is padded - with the invalid-PWM sentinel up to 18 channels so every ``chanN_raw`` - attribute the data model reads is present. - """ - padded = list(raw) + [_RC_INVALID_PWM] * (18 - len(raw)) - msg = MagicMock() - msg.get_type.return_value = "RC_CHANNELS" - for i in range(18): - setattr(msg, f"chan{i + 1}_raw", padded[i]) - msg.chancount = chancount if chancount is not None else len(raw) - return msg - - @pytest.fixture -def connected_flight_controller() -> MagicMock: - """Fixture providing a mock flight controller that reports a live MAVLink link.""" - flight_controller = MagicMock() - flight_controller.master = MagicMock() - flight_controller.master.recv_match.return_value = None - flight_controller.request_rc_receiver_messages.return_value = (True, "") - flight_controller.fc_parameters = {} - return flight_controller +def connected_flight_controller(rc_controller: MagicMock) -> MagicMock: + """Provide a selected flight controller with mocked transport I/O.""" + return rc_controller @pytest.fixture @@ -314,6 +293,63 @@ def test_standalone_finish_cannot_bypass_the_upload_workflow(self, connected_fli connected_flight_controller.set_param.assert_not_called() +class TestRCCalibrationDataModelSources: + """Keep other systems and components out of the selected controller calibration.""" + + @pytest.mark.parametrize("source", [(2, 1), (1, 42)]) + def test_calibration_stages_only_selected_controller_extremes( + self, rc_staging_context: tuple[MagicMock, ParameterEditor], source: tuple[int, int] + ) -> None: + """ + Foreign receiver samples cannot change the selected controller's calibration. + + GIVEN another system or component sends wider extremes on the same link + WHEN the user calibrates and finishes using the selected controller's samples + THEN only the selected controller's endpoints are displayed and staged + """ + controller, editor = rc_staging_context + model = RCCalibrationDataModel(controller, editor) + model.start_calibration() + controller.master.recv_match.side_effect = [ + _make_rc_channels_msg([1000], source=source), + _make_rc_channels_msg([1400]), + _make_rc_channels_msg([2000], source=source), + _make_rc_channels_msg([1600]), + None, + ] + + assert model.get_rc_telemetry()["channels"][0]["value"] == 1600 + assert model.finish_calibration()[0] + assert editor.current_step_parameters["RC1_MIN"].get_new_value() == 1400 + assert editor.current_step_parameters["RC1_MAX"].get_new_value() == 1600 + controller.set_param.assert_not_called() + + @pytest.mark.parametrize("source", [(2, 1), (1, 42)]) + def test_foreign_heartbeat_and_channels_cannot_establish_calibration( + self, connected_flight_controller: MagicMock, source: tuple[int, int] + ) -> None: + """ + Unrelated telemetry cannot supply a flight mode or calibration data. + + GIVEN only another system or component sends RC channels and a heartbeat + WHEN the selected controller's calibration monitor polls + THEN no telemetry is shown and Finish reports that no calibration data was recorded + """ + heartbeat = MagicMock(custom_mode=7) + heartbeat.get_type.return_value = "HEARTBEAT" + heartbeat.get_srcSystem.return_value, heartbeat.get_srcComponent.return_value = source + connected_flight_controller.master.recv_match.side_effect = [ + _make_rc_channels_msg([1100], source=source), + heartbeat, + None, + ] + model = RCCalibrationDataModel(connected_flight_controller) + model.start_calibration() + + assert not model.get_rc_telemetry() + assert not model.finish_calibration()[0] + + class TestRCCalibrationDataModelTelemetry: """Test live RC telemetry parsing from MAVLink RC_CHANNELS messages.""" @@ -532,6 +568,8 @@ def test_telemetry_includes_flight_mode_from_heartbeat(self, connected_flight_co hb = MagicMock() hb.custom_mode = 3 hb.get_type.return_value = "HEARTBEAT" + hb.get_srcSystem.return_value = 1 + hb.get_srcComponent.return_value = 1 connected_flight_controller.master.recv_match.side_effect = [rc_msg, hb, None] telemetry = model.get_rc_telemetry() @@ -734,66 +772,6 @@ def test_extremes_are_not_recorded_when_not_calibrating(self, connected_flight_c assert not model._channel_max -class TestRCCalibrationDataModelFlightMode: - """Test flight-mode reporting from HEARTBEAT messages.""" - - def test_flight_mode_reports_not_connected_without_link(self, disconnected_flight_controller) -> None: - """ - Flight mode is reported as not-connected without a link. - - GIVEN: A disconnected flight controller - WHEN: get_flight_mode is called - THEN: A non-empty not-connected string is returned - """ - model = RCCalibrationDataModel(disconnected_flight_controller) - - assert model.get_flight_mode() == "Not connected" - - def test_flight_mode_returns_custom_mode_from_heartbeat(self, connected_flight_controller) -> None: - """ - The custom_mode from a HEARTBEAT is surfaced as the flight mode. - - GIVEN: A HEARTBEAT message with custom_mode=5 - WHEN: get_flight_mode is called - THEN: The string "5" is returned - """ - model = RCCalibrationDataModel(connected_flight_controller) - hb = MagicMock() - hb.custom_mode = 5 - connected_flight_controller.master.recv_match.return_value = hb - - assert model.get_flight_mode() == "5" - connected_flight_controller.master.recv_match.assert_called_once_with(type="HEARTBEAT", blocking=False) - - def test_flight_mode_returns_no_data_without_heartbeat(self, connected_flight_controller) -> None: - """ - Absence of a HEARTBEAT yields a no-data marker rather than an error. - - GIVEN: A connected FC whose recv_match returns None - WHEN: get_flight_mode is called - THEN: A non-empty no-data string is returned - """ - model = RCCalibrationDataModel(connected_flight_controller) - connected_flight_controller.master.recv_match.return_value = None - - assert model.get_flight_mode() == "No Data" - - def test_flight_mode_swallows_recv_match_exception(self, connected_flight_controller, caplog) -> None: - """ - A HEARTBEAT read error never propagates out of get_flight_mode. - - GIVEN: recv_match raises an exception - WHEN: get_flight_mode is called - THEN: A non-empty fallback string is returned instead of raising - """ - model = RCCalibrationDataModel(connected_flight_controller) - connected_flight_controller.master.recv_match.side_effect = RuntimeError("link lost") - - with caplog.at_level("DEBUG"): - assert model.get_flight_mode() == "No Data" - assert caplog.messages == ["Error reading HEARTBEAT: link lost"] - - class TestTransmitterStickModes: """Check each transmitter mode with distinct axis values rather than identical stick movement.""" diff --git a/tests/plugins/test_data_model_rc_receiver.py b/tests/plugins/test_data_model_rc_receiver.py index 36315c734..ec773e699 100755 --- a/tests/plugins/test_data_model_rc_receiver.py +++ b/tests/plugins/test_data_model_rc_receiver.py @@ -19,6 +19,7 @@ import pytest from jsonschema import validate from pymavlink import mavutil +from telemetry_test_helpers import rc_channels_message from ardupilot_methodic_configurator import data_model_parameter_editor as editor_module from ardupilot_methodic_configurator.data_model_parameter_editor import ParameterEditor @@ -37,12 +38,7 @@ def rc_message(channels: tuple[int, ...] = (1000, 1200, 1500, 1800, 2000), rssi: int = 127) -> Any: # noqa: ANN401 """Use real MAVLink fields, including unused channels and the source address.""" - message = mavutil.mavlink.MAVLink_rc_channels_message( - 0, len(channels), *(channels + (65535,) * (18 - len(channels))), rssi - ) - message._header.srcSystem = 1 - message._header.srcComponent = 1 - return message + return rc_channels_message(channels, rssi=rssi) def status_message(present: int = _RC_BIT, enabled: int = _RC_BIT, health: int = _RC_BIT) -> Any: # noqa: ANN401 @@ -54,15 +50,10 @@ def status_message(present: int = _RC_BIT, enabled: int = _RC_BIT, health: int = @pytest.fixture -def receiver_context() -> SimpleNamespace: +def receiver_context(rc_controller: MagicMock) -> SimpleNamespace: """Mock only transport and time; execute the real model and MAVLink messages.""" messages: deque[Any] = deque() - controller = MagicMock() - controller.master.target_system = 1 - controller.master.target_component = 0 - controller.info.system_id = "1" - controller.info.component_id = "1" - controller.request_rc_receiver_messages.return_value = (True, "") + controller = rc_controller controller.master.recv_match.side_effect = lambda **_kwargs: messages.popleft() if messages else None clock = MagicMock(return_value=100.0) return SimpleNamespace(controller=controller, clock=clock, messages=messages, model=RCReceiverDataModel(controller, clock)) @@ -112,6 +103,8 @@ def recv_match(*, blocking: bool, type: list[str] | None = None) -> Any: # noqa controller.master.recv_match.side_effect = recv_match heartbeat = mavutil.mavlink.MAVLink_heartbeat_message(2, 3, 0, 7, 4, 3) + heartbeat._header.srcSystem = 1 + heartbeat._header.srcComponent = 1 receiver_context.messages.extend([rc_message((1100,)), rc_message((1900,)), heartbeat]) model = RCCalibrationDataModel(controller) assert model.start_calibration()[0] diff --git a/tests/plugins/test_frontend_tkinter_rc_calibration.py b/tests/plugins/test_frontend_tkinter_rc_calibration.py index 4046bf644..0cbbacbe7 100755 --- a/tests/plugins/test_frontend_tkinter_rc_calibration.py +++ b/tests/plugins/test_frontend_tkinter_rc_calibration.py @@ -19,6 +19,7 @@ import pytest from PIL import ImageChops, ImageTk from pytest_mock import MockerFixture +from telemetry_test_helpers import rc_channels_message from ardupilot_methodic_configurator.data_model_ardupilot_parameter import ArduPilotParameter from ardupilot_methodic_configurator.data_model_par_dict import Par @@ -159,10 +160,7 @@ def test_controller_step_refreshes_bars_for_consecutive_samples(rc_navigation_vi controller = rc_navigation_view.controller messages = [] for pwm in (1100, 1900): - message = MagicMock(chancount=1) - message.get_type.return_value = "RC_CHANNELS" - for number in range(1, 17): - setattr(message, f"chan{number}_raw", pwm) + message = rc_channels_message([pwm]) messages.extend((message, None)) controller.master.recv_match.side_effect = messages for expected in ("1100 µs", "1900 µs"): @@ -612,7 +610,9 @@ def test_heartbeat_only_update_preserves_stick_positions(self, stick_preview: RC assert canvas.coords(canvas.find_withtag("stick")[0]) == before -def test_embedded_monitor_initialises_all_markers_and_preserves_channels_on_heartbeat(tk_root: tk.Tk) -> None: +def test_embedded_monitor_initialises_all_markers_and_preserves_channels_on_heartbeat( + tk_root: tk.Tk, rc_controller: MagicMock +) -> None: """ The real embedded view displays configured markers immediately and keeps RC state on heartbeat-only polls. @@ -620,7 +620,7 @@ def test_embedded_monitor_initialises_all_markers_and_preserves_channels_on_hear WHEN: The view opens, receives RC telemetry, and then receives only a heartbeat THEN: Markers, sticks and live values are retained without displaying a flight-mode readout """ - controller = MagicMock() + controller = rc_controller controller.fc_parameters = {"RC1_MIN": 990.0, "RC1_MAX": 2010.0, "RC1_TRIM": 1490.0} controller.request_rc_receiver_messages.return_value = (True, "") controller.master.recv_match.return_value = None @@ -632,16 +632,15 @@ def test_embedded_monitor_initialises_all_markers_and_preserves_channels_on_hear assert view.craft_preview.cget("text") == "Remote controller stick and vehicle attitude preview" canvas = view.channel_bars._bar_canvases[1] assert len(canvas.find_withtag("calibration_marker")) == 3 - message = MagicMock(chancount=4) - message.get_type.return_value = "RC_CHANNELS" - for channel_number in range(1, 17): - setattr(message, f"chan{channel_number}_raw", 1600 if channel_number == 1 else 1500) + message = rc_channels_message([1600, 1500, 1500, 1500]) controller.master.recv_match.side_effect = [message, None] view._stop_polling() view._check_telemetry() before = view.channel_bars._value_labels[1].cget("text") heartbeat = MagicMock(custom_mode=3) heartbeat.get_type.return_value = "HEARTBEAT" + heartbeat.get_srcSystem.return_value = 1 + heartbeat.get_srcComponent.return_value = 1 controller.master.recv_match.side_effect = [heartbeat] view._stop_polling() view._check_telemetry() @@ -700,7 +699,7 @@ def test_embedded_monitor_aligns_sections_with_documentation_and_moves_scrollbar host.destroy() -def test_floating_monitor_shares_the_stick_boxes_and_three_channel_markers(tk_root: tk.Tk) -> None: # noqa: PLR0915 # pylint: disable=too-many-statements +def test_floating_monitor_shares_the_stick_boxes_and_three_channel_markers(tk_root: tk.Tk, rc_controller: MagicMock) -> None: # noqa: PLR0915 # pylint: disable=too-many-statements """ The floating live monitor presents the same markers and stick preview as the embedded view. @@ -708,7 +707,7 @@ def test_floating_monitor_shares_the_stick_boxes_and_three_channel_markers(tk_ro WHEN: The popup is created using its real UI construction THEN: Compact sections show sticks and markers without a flight-mode readout, and polling is cancelled on destroy """ - controller = MagicMock() + controller = rc_controller controller.fc_parameters = {"RC1_MIN": 990.0, "RC1_MAX": 2010.0, "RC1_TRIM": 1490.0} controller.request_rc_receiver_messages.return_value = (True, "") controller.master.recv_match.return_value = None @@ -745,16 +744,15 @@ def init_hidden_base(window: BaseWindow, _root_tk: tk.Toplevel) -> None: for section in (popup.craft_preview, popup.channel_bars): assert tuple(int(str(value)) for value in section.cget("padding")) == (4,) assert section.pack_info()["pady"] == (0, 4) - message = MagicMock(chancount=4) - message.get_type.return_value = "RC_CHANNELS" - for number in range(1, 17): - setattr(message, f"chan{number}_raw", 1700 if number == 3 else 1500) + message = rc_channels_message([1500, 1500, 1700, 1500]) controller.master.recv_match.side_effect = [message, None] popup._stop_polling() popup._check_telemetry() assert popup.channel_bars._value_labels[3].cget("text") == "1700 µs" heartbeat = MagicMock(custom_mode=5) heartbeat.get_type.return_value = "HEARTBEAT" + heartbeat.get_srcSystem.return_value = 1 + heartbeat.get_srcComponent.return_value = 1 controller.master.recv_match.side_effect = [heartbeat] popup._stop_polling() popup._check_telemetry() @@ -771,7 +769,7 @@ def init_hidden_base(window: BaseWindow, _root_tk: tk.Toplevel) -> None: parent.destroy() -def test_cancel_calibration_restores_configured_markers_immediately(tk_root: tk.Tk) -> None: +def test_cancel_calibration_restores_configured_markers_immediately(tk_root: tk.Tk, rc_controller: MagicMock) -> None: """ Calibration controls update the markers immediately rather than leaving stale observed values. @@ -779,7 +777,7 @@ def test_cancel_calibration_restores_configured_markers_immediately(tk_root: tk. WHEN: The user starts calibration, moves the channel and then presses Cancel THEN: Observed MIN/MAX are discarded, configured MIN/MAX/TRIM return, and no parameters are written """ - controller = MagicMock() + controller = rc_controller controller.fc_parameters = {"RC1_MIN": 1000.0, "RC1_MAX": 2000.0, "RC1_TRIM": 1490.0} controller.request_rc_receiver_messages.return_value = (True, "") controller.master.recv_match.return_value = None @@ -788,10 +786,7 @@ def test_cancel_calibration_restores_configured_markers_immediately(tk_root: tk. try: view._start_btn.invoke() assert str(view._cancel_btn.cget("state")) == "normal" - message = MagicMock(chancount=1) - message.get_type.return_value = "RC_CHANNELS" - for number in range(1, 17): - setattr(message, f"chan{number}_raw", 1300) + message = rc_channels_message([1300]) controller.master.recv_match.side_effect = [message, None] view._stop_polling() view._check_telemetry() @@ -818,10 +813,7 @@ def test_mode_combobox_stages_mapping_and_refreshes_new_values_without_upload( """ controller, editor = rc_staging_context model = RCCalibrationDataModel(controller, editor) - message = MagicMock(chancount=4) - message.get_type.return_value = "RC_CHANNELS" - for number in range(1, 17): - setattr(message, f"chan{number}_raw", 1800 if number == 3 else 1500) + message = rc_channels_message([1500, 1500, 1800, 1500]) controller.master.recv_match.side_effect = [message, None] model.get_rc_telemetry() base_window = MagicMock() @@ -1202,10 +1194,7 @@ def test_rc_monitor_warns_once_for_a_telemetry_gap_then_recovers( ] assert view._timer_id is not None # When: Fresh receiver telemetry arrives. - message = MagicMock(chancount=16) - message.get_type.return_value = "RC_CHANNELS" - for number in range(1, 17): - setattr(message, f"chan{number}_raw", 1700 if number == 3 else 1500) + message = rc_channels_message([1500, 1500, 1700] + [1500] * 13) context.controller.master.recv_match.side_effect = [message, None] view._stop_polling() view._check_telemetry() diff --git a/tests/telemetry_test_helpers.py b/tests/telemetry_test_helpers.py index 496f0be26..ccf49cc1f 100755 --- a/tests/telemetry_test_helpers.py +++ b/tests/telemetry_test_helpers.py @@ -9,7 +9,7 @@ SPDX-License-Identifier: GPL-3.0-or-later """ -from collections.abc import Generator +from collections.abc import Generator, Sequence from io import BytesIO from types import SimpleNamespace from unittest.mock import MagicMock @@ -22,6 +22,17 @@ from ardupilot_methodic_configurator.data_model_flightcontroller_info import FlightControllerInfo +def rc_channels_message( + raw: Sequence[int], chancount: int | None = None, *, rssi: int = 127, source: tuple[int, int] = (1, 1) +) -> mavlink.MAVLink_rc_channels_message: + """Build actual RC telemetry, including its advertised channel count and source address.""" + message = mavlink.MAVLink_rc_channels_message( + 0, len(raw) if chancount is None else chancount, *(list(raw) + [65535] * (18 - len(raw))), rssi=rssi + ) + message._header.srcSystem, message._header.srcComponent = source # pylint: disable=protected-access + return message + + def connected_telemetry_link() -> Generator[SimpleNamespace, None, None]: """Provide a real parser and connection-owned sender, stopping it after each test.""" link = create_telemetry_link() diff --git a/tests/test_esc_telemetry.py b/tests/test_esc_telemetry.py index bc2c8fe0e..e36293f64 100755 --- a/tests/test_esc_telemetry.py +++ b/tests/test_esc_telemetry.py @@ -680,7 +680,10 @@ def test_sparse_esc_columns_keep_their_numbers_and_ignore_unused_slots(view: fro assert view.table.column("8", "stretch") == 0 -def test_busy_links_have_bounded_reads_and_preserve_other_observers(link: SimpleNamespace) -> None: +@pytest.mark.parametrize("backend_type", [EscTelemetryBackend, DataTelemetryBackend]) +def test_busy_links_have_bounded_reads_and_preserve_other_observers( + link: SimpleNamespace, backend_type: type[EscTelemetryBackend] | type[DataTelemetryBackend] +) -> None: """ Keep telemetry responsive on a busy shared connection. @@ -692,7 +695,7 @@ def test_busy_links_have_bounded_reads_and_preserve_other_observers(link: Simple link.master.message_hooks.append(observer) for _index in range(250): feed(link, mavlink.MAVLink_heartbeat_message(2, 3, 0, 0, 3, 3)) - backend = EscTelemetryBackend(link.controller, lambda: link.now) + backend = backend_type(link.controller, lambda: link.now) backend.poll() assert observer.call_count == 200 replacement = create_telemetry_link() @@ -704,6 +707,34 @@ def test_busy_links_have_bounded_reads_and_preserve_other_observers(link: Simple assert not replacement.master.message_hooks +@pytest.mark.parametrize("backend_type", [EscTelemetryBackend, DataTelemetryBackend]) +def test_slow_packets_stop_both_telemetry_polls_at_the_time_budget( + link: SimpleNamespace, backend_type: type[EscTelemetryBackend] | type[DataTelemetryBackend] +) -> None: + """ + Bound elapsed work even when packet processing is slow. + + GIVEN a backlog whose observer takes six milliseconds to process each packet + WHEN the telemetry backend polls with a ten-millisecond budget + THEN only two packets are read and closing the backend preserves the other observer + """ + + def advance_clock(_master: object, _message: object) -> None: + link.now += 0.006 + + observer = MagicMock(side_effect=advance_clock) + link.master.message_hooks.append(observer) + for _index in range(10): + feed(link, mavlink.MAVLink_heartbeat_message(2, 3, 0, 0, 3, 3)) + backend = backend_type(link.controller, lambda: link.now) + try: + backend.poll() + assert observer.call_count == 2 + finally: + backend.close() + assert link.master.message_hooks == [observer] + + def test_reusable_esc_table_labels_identify_every_measurement_and_its_units() -> None: """ Keep measurement labels and engineering units consistent in every ESC table host. diff --git a/tests/test_frontend_tkinter_navigation_lock.py b/tests/test_frontend_tkinter_navigation_lock.py index 298439af5..933961eac 100755 --- a/tests/test_frontend_tkinter_navigation_lock.py +++ b/tests/test_frontend_tkinter_navigation_lock.py @@ -610,7 +610,8 @@ def test_destroying_a_calibration_view_releases_its_navigation_lock(calibration_ view._calibration_popup = None view._instructions_popup = None fixture.host.navigation_lock.acquire(view) - parent_destroy = mocker.patch.object(view_type.__bases__[0], "destroy") + widget_base = next(base for base in view_type.__bases__ if hasattr(base, "destroy")) + parent_destroy = mocker.patch.object(widget_base, "destroy") if view_type is AccelerometerCalibrationView: mocker.patch.object(view, "_stop_polling", side_effect=OSError("cleanup unavailable")) elif view_type is LevelCalibrationView: From 36f31410d4513bc1f6b5dfea85fe70a20b0dc69c Mon Sep 17 00:00:00 2001 From: Amilcar Lucas Date: Sun, 1 Nov 2026 20:55:43 +0100 Subject: [PATCH 4/4] test(heartbeat): fix GCS heartbeat side effect in telemetry test Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Amilcar Lucas --- tests/test_esc_telemetry.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_esc_telemetry.py b/tests/test_esc_telemetry.py index e36293f64..163768353 100755 --- a/tests/test_esc_telemetry.py +++ b/tests/test_esc_telemetry.py @@ -695,6 +695,7 @@ def test_busy_links_have_bounded_reads_and_preserve_other_observers( link.master.message_hooks.append(observer) for _index in range(250): feed(link, mavlink.MAVLink_heartbeat_message(2, 3, 0, 0, 3, 3)) + link.controller.start_gcs_heartbeat.side_effect = None backend = backend_type(link.controller, lambda: link.now) backend.poll() assert observer.call_count == 200