Repository navigation
feat(flight-controller): improve diagnostics and compass calibration - #2199
amilcarlucas wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
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
This new file appears to be committed with CRLF line endings (visible as stray carriage returns in… · New If a chunk sequence is already in progress for a given (system, component, message_id), receiving a… · New Hard-coding the striped-row background color can reduce contrast and may conflict with system… · New The device table column widths are hard-coded pixel values and don’t appear to incorporate the… · New
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.
| """ | ||
| Normalize flight-controller diagnostics and derive device-table rows. | ||
|
|
| previous = b"" | ||
| if key not in self._pending and len(self._pending) >= self.MAX_PENDING: |
| 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") |
| 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, | ||
| ) |
|
Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head 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. 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 Fourth and fifth IMU. macOS. ardupilot_methodic_configurator/frontend_tkinter_fc_banner_window.py:72 calls 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 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 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): 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>
6cab52c to
0b73a15
Compare


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
git commit --signoff)Testing
Describe how you tested these changes: