Skip to content

feat(flight-controller): improve diagnostics and compass calibration - #2199

Open
amilcarlucas wants to merge 1 commit into
masterfrom
banner_features
Open

amilcarlucas wants to merge 1 commit into
masterfrom
banner_features

Conversation

@amilcarlucas

Copy link
Copy Markdown
Collaborator

Description

Capture and retain the latest 100 STATUSTEXT messages, expose them through the flight-controller model, and display them alongside the boot banner. Add a sortable detected-device table with formatted device names and types.

Treat active MAG_CAL_PROGRESS telemetry as confirmation that compass calibration started when its command acknowledgment is missing, and preserve STATUSTEXT details when reporting command failures.

Add tests for status capture and buffer limits, compass calibration progress, and banner device-table behavior.

Checklist

  • Run pre-commit checks locally
  • Verified by a human programmer
  • All commits are signed off (use git commit --signoff)
  • Code follows our coding standards
  • Documentation updated if needed
  • No breaking changes or properly documented

Testing

Describe how you tested these changes:

  • Unit tests pass
  • Integration tests pass
  • Manual testing performed
  • Tested on flight controller hardware

Copilot AI balanced review requested due to automatic review settings October 11, 2026 17:39
@amilcarlucas amilcarlucas added the AIReview Request an automated AI review; picked up by the reviewprs sweep label Oct 11, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

4 open findings
What changed in this PR

This PR improves flight-controller diagnostics visibility in the UI and hardens compass calibration startup confirmation by capturing more MAVLink telemetry context.

Changes:

  • Capture and retain the latest 100 STATUSTEXT messages (including chunk assembly), expose them via the FC model, and poll them for the banner window.
  • Enhance the FC banner window to show runtime STATUSTEXT and a sortable detected-device table with tooltips and formatted names/types.
  • Improve compass calibration start handling by accepting MAG_CAL_PROGRESS as a start confirmation when ACK is missing, while preserving STATUSTEXT details on failures; add/adjust unit tests accordingly.
File Description
tests/​unit_backend_flightcontroller_commands.py Updates command tests to use the new internal ACK/result helper and validates MAG_CAL_PROGRESS confirmation settings.
tests/​test_frontend_tkinter_parameter_export.py Fixes patch target to match tooltip monitor-bounds helper’s new module location.
tests/​test_frontend_tkinter_parameter_editor.py Updates banner window invocation to pass status text/provider, params, and navigation lock.
tests/​test_frontend_tkinter_fc_banner_window.py Adds unit tests for banner window runtime text refresh, device table setup/sorting, and tooltip behavior.
tests/​test_data_model_fc_diagnostics.py Adds tests for STATUSTEXT chunk assembly and device-row formatting derived from parameters.
tests/​test_backend_flightcontroller_connection.py Refactors banner receive test, adds hook registration test, and adds status text capture/buffer limit test.
tests/​test_backend_fc_diagnostics.py Adds integration-ish tests using real PyMAVLink parsing to validate diagnostics capture, banner exclusion, and buffer limits.
tests/​test_backend_compass_start_confirmation.py Adds behavior tests for compass calibration start ACK vs progress fallback and diagnostic preservation.
ardupilot_methodic_configurator/​plugins/​frontend_tkinter_servo_out.py Switches ServoOutView base to ttk.Frame and adjusts tkinter imports.
ardupilot_methodic_configurator/​frontend_tkinter_show.py Adds shared helper to position tooltips near pointer within monitor bounds.
ardupilot_methodic_configurator/​frontend_tkinter_parameter_export.py Reuses new tooltip positioning helper to avoid duplicated bounds/geometry logic.
ardupilot_methodic_configurator/​frontend_tkinter_parameter_editor.py Passes banner/status/devices context into the updated FlightControllerBannerWindow.
ardupilot_methodic_configurator/​frontend_tkinter_fc_banner_window.py Expands banner window to include status text polling, devices table with sorting, and device tooltips.
ardupilot_methodic_configurator/​data_model_parameter_editor.py Exposes status text getters/poller from the FC model for UI consumption.
ardupilot_methodic_configurator/​data_model_fc_diagnostics.py Introduces STATUSTEXT decoding + chunk assembly and device-row derivation helpers.
ardupilot_methodic_configurator/​backend_flightcontroller_protocols.py Extends connection protocol API with status_text_buffer and poll_status_text.
ardupilot_methodic_configurator/​backend_flightcontroller_factory_mavlink.py Enhances fake mavlink recv_match to dispatch hooks and filter by message type(s).
ardupilot_methodic_configurator/​backend_flightcontroller_connection.py Adds STATUSTEXT hook capture, bounded buffer, banner/runtime separation, and polling support.
ardupilot_methodic_configurator/​backend_flightcontroller_commands.py Adds start-confirmation telemetry fallback and chunk-safe status text extraction for errors/progress.
ardupilot_methodic_configurator/​backend_flightcontroller.py Plumbs status text buffer and poll method through FlightController facade.

🧠 Review effort: Lite


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +1 to +3
"""
Normalize flight-controller diagnostics and derive device-table rows.

Comment on lines +66 to +67
previous = b""
if key not in self._pending and len(self._pending) >= self.MAX_PENDING:
Comment on lines +122 to +128
def _create_device_table(self, parent: ttk.LabelFrame, fc_parameters: dict[str, float]) -> None:
rows = device_rows(fc_parameters)
container = ttk.Frame(parent)
container.pack(fill=tk.BOTH, expand=True, padx=6, pady=6)
tree = ttk.Treeview(container, columns=self.DEVICE_COLUMNS, show="headings", height=self.DEVICE_TABLE_HEIGHT)
self.device_tree = tree
tree.tag_configure("striped", background="#eeeeee")
Comment on lines +137 to +144
widths = {"DeviceName": 190, "BusType": 100, "Bus": 70, "Address": 90, "DevType": 190}
for column in self.DEVICE_COLUMNS:
tree.heading(column, text=self._heading_text(column), command=partial(self._sort_by_column, column))
tree.column(
column,
width=widths[column],
anchor=tk.W if column in ("DeviceName", "DevType") else tk.CENTER,
)
@AP-Review

Copy link
Copy Markdown

Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting.
Verdict: REQUEST CHANGES

Reviewed at head 6cab52c547.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/methodicconfigurator/2199/1.html#prMethodicConfigurator-2199

Reviewed at 6cab52c, on Linux, with the test files this PR touches, pymavlink's parser over an in-memory transport, and the new window under real Tk. No flight controller, SITL, macOS or Windows.

Needs fixing before merge. test_device_table_shows_vertical_scrollbar_only_when_rows_overflow fails on every run: the fake scrollbar returned at tests/test_frontend_tkinter_fc_banner_window.py:216 is a _FakeWidget with no set, so ardupilot_methodic_configurator/frontend_tkinter_fc_banner_window.py:132 raises AttributeError. Here the 8 touched test files give 1 failed, 300 passed. Adding a no-op set() to the fake makes it pass, and the product code works with a real ttk.Scrollbar. The pytest (windows-latest, 3.14) job is red on this head; I could not read its log, so I can't confirm it is this test.

Worth fixing, not blocking.

Shared receiver. ardupilot_methodic_configurator/backend_flightcontroller_connection.py:223 reads every message type every 200 ms while the window is open, but the grab and the navigation lock (ardupilot_methodic_configurator/frontend_tkinter_fc_banner_window.py:74) only stop user input: the plugin timers behind the window (battery monitor, motor test, RC calibration) keep reading the same link. In a virtual-clock simulation with the real backend and battery model, opening the window made the battery reading go unavailable up to 3 times in 50 s, each time re-requesting the BATTERY_STATUS stream and blocking the GUI thread for about 1 to 2 s. With the window closed it never happened. It recovers by itself. Pausing the active plugin view while the window is open would cover it; the exclusively owns the idle connection docstrings are not true as the code stands.

Fourth and fifth IMU. INS4_ACC_ID, INS4_GYR_ID, INS5_ACC_ID and INS5_GYR_ID never reach the device table, because both ardupilot_methodic_configurator/data_model_fc_diagnostics.py:91 and ardupilot_methodic_configurator/data_model_fc_diagnostics.py:107 test for INS_. Both lines need the change.

macOS. ardupilot_methodic_configurator/frontend_tkinter_fc_banner_window.py:72 calls grab_set() unconditionally. Six other dialogs skip it on darwin, three of them citing issue #1264 (for example frontend_tkinter_parameter_editor.py:1384). I did not run this on macOS; the same guard would be consistent.

Line endings. data_model_fc_diagnostics.py (ardupilot_methodic_configurator/data_model_fc_diagnostics.py:1) and the three new tests test_backend_compass_start_confirmation.py, test_backend_fc_diagnostics.py and test_data_model_fc_diagnostics.py are committed with CRLF. I agree with Copilot on normalising them. It is not what breaks CI (ruff and pytest are fine with them and no hook covers .py), but the three tests are executable and their shebang now ends in a carriage return, so ./tests/test_backend_fc_diagnostics.py fails on Linux with env: 'python3\r': No such file or directory.

Banner length. ardupilot_methodic_configurator/backend_flightcontroller_connection.py:666 still takes one STATUSTEXT per 100 ms for 1 s, so in a 15-line burst only 9 or 10 lines reached the banner and the rest showed up as runtime messages. Reading until recv_match returns None on each iteration would remove the limit, since the hook now captures every packet.

For information, no change requested.

Compass start. With the ACK lost and progress streaming, the start now succeeds after the 5 s timeout; before this PR it failed. Telemetry read during that window is not passed to the progress popup, but ArduPilot keeps sending MAG_CAL_REPORT while a result is held, and a repeated report was picked up in my probe.

Copilot's duplicate first chunk (ardupilot_methodic_configurator/data_model_fc_diagnostics.py:65): order 0,1,0,2 does emit nothing, but ArduPilot sends each chunk once per link, so it needs a link that duplicates packets. Optional.

Any text captured during the banner second is hidden from the runtime list for the rest of the connection (ardupilot_methodic_configurator/backend_flightcontroller_connection.py:212), so a PreArm warning that arrives then shows once in the banner pane and its repeats do not appear below.

The commit also changes the servo-output plugin (ardupilot_methodic_configurator/plugins/frontend_tkinter_servo_out.py:27): ttk.Frame base and the heading label removed. Fine by me, just not mentioned in the description.

Copilot's stripe colour and column width comments match existing practice here (frontend_tkinter_parameter_export.py:272 uses the same stripe and plain-pixel widths), so I have nothing to add on those. I did not check dark-theme or HiDPI rendering.

Capture and retain the latest 100 STATUSTEXT messages, expose them through
the flight-controller model, and display them alongside the boot banner.
Add a sortable detected-device table with formatted device names and types.

Treat active MAG_CAL_PROGRESS telemetry as confirmation that compass
calibration started when its command acknowledgment is missing, and preserve
STATUSTEXT details when reporting command failures.

Add tests for status capture and buffer limits, compass calibration progress,
and banner device-table behavior.

Signed-off-by: Dr.-Ing. Amilcar do Carmo Lucas <amilcar.lucas@iav.de>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AIReview Request an automated AI review; picked up by the reviewprs sweep

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants