Recovery - #1941
Open
amilcarlucas wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
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.
This PR improves resilience when dealing with MAVFTP parameter transfer failures and ambiguous/invalid frame/vehicle identifiers, preventing hard exits and enabling UI-level recovery paths.
Changes:
- Replace
sys.exit()behavior during MAVFTP param decode/read failures with error returns and fallback handling. - Resolve ambiguous VTOL vehicle type classification using firmware banner parsing.
- Allow motor-test UI/model to remain usable when
FRAME_CLASS/FRAME_TYPEcombos are invalid, including metadata-based recovery and new layouts.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_data_model_flightcontroller_info.py | Updates classification expectations for ambiguous VTOL MAV type. |
| tests/test_backend_mavftp.py | Adds coverage ensuring MAVFTP decode failures return an error instead of exiting. |
| tests/test_backend_flightcontroller_params.py | Adds coverage for fallback behavior when MAVFTP transfer raises. |
| tests/test_backend_flightcontroller_connection.py | Adds tests for early vehicle-type logging and banner-based resolution. |
| tests/plugins/test_data_model_motor_test.py | Adds/updates tests for invalid frame-type correction and motor layout population. |
| ardupilot_methodic_configurator/plugins/frontend_tkinter_motor_test.py | Hardens combobox selection and forces diagram refresh on frame-type changes/activation. |
| ardupilot_methodic_configurator/plugins/data_model_motor_test.py | Refactors motor layout loading; introduces invalid-frame-type recovery and metadata fallback. |
| ardupilot_methodic_configurator/plugins/AP_Motors_test.json | Adds a BiCopter (class 10) layout for motor testing. |
| ardupilot_methodic_configurator/log_analysis/data_model_quality_fft.py | Minor simplification of return path. |
| ardupilot_methodic_configurator/data_model_flightcontroller_info.py | Marks VTOL_DUOROTOR as ambiguous vehicle type. |
| ardupilot_methodic_configurator/backend_mavftp.py | Returns MAVFTP error codes instead of terminating on param read/decode failures. |
| ardupilot_methodic_configurator/backend_flightcontroller_params.py | Converts MAVFTP download exceptions into logged fallback results. |
| ardupilot_methodic_configurator/backend_flightcontroller_connection.py | Extracts firmware type from banner more robustly; resolves ambiguous heartbeat vehicle types. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1038
to
+1041
| class_name = next( | ||
| (str(name).strip().upper() for code, name in class_values.items() if int(code) == frame_class), | ||
| None, | ||
| ) |
Comment on lines
+206
to
+211
| try: | ||
| mavftp = create_mavftp(self.master) | ||
|
|
||
| def get_params_progress_callback(completion: float) -> None: | ||
| if progress_callback is not None and completion is not None: | ||
| progress_callback(int(completion * 100), 100) |
Comment on lines
+236
to
+241
| except Exception as error: # pylint: disable=broad-exception-caught | ||
| logging_warning( | ||
| _("MAVFTP parameter download failed; falling back to MAVLink: %(error)s"), | ||
| {"error": str(error)}, | ||
| ) | ||
| return {}, ParDict() |
Comment on lines
+1456
to
+1463
| data = fh.read() | ||
| except OSError as exp: | ||
| logging.error("FTP: Failed to read file param.pck: %s", exp) | ||
| sys.exit(1) | ||
| return MAVFTPReturn("GetParams", ERR_Fail) | ||
| pdata = MAVFTP.ftp_param_decode(data) | ||
| if pdata is None: | ||
| sys.exit(1) | ||
| logging.error("FTP: Failed to decode parameter file param.pck") | ||
| return MAVFTPReturn("GetParams", ERR_Fail) |
| frame_type_pairs = self.model.get_frame_type_pairs() | ||
| current_selection = self.model.get_current_frame_selection_key() if frame_type_pairs else None | ||
| current_selection_key = self.model.get_current_frame_selection_key() if frame_type_pairs else None | ||
| current_selection = current_selection_key if current_selection_key in {key for key, _ in frame_type_pairs} else None |
Contributor
☂️ Code Coverage
Overall Coverage
New Files
Modified Files
|
Contributor
Test Results 4 files 4 suites 43m 35s ⏱️ Results for commit 19b6c2d. |
… state Keep the motor-test plugin usable when the flight controller reports a valid frame class with an unsupported FRAME_TYPE. Show an informative error dialog while still rendering the plugin and allowing the user to select a valid frame type. Refresh motor controls and diagrams after frame changes and flight-controller resets. Avoid invalid combobox selections and suppress diagram lookups while the frame type is invalid. Add regression coverage for invalid frame recovery, plugin rendering, diagram refresh.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
ardupilot_methodic_configurator/data_model_flightcontroller_info.py:252
- Encoding “ambiguity” as a human-readable string (and then keying logic off
" or "elsewhere) is brittle and mixes display text with decision logic. A more robust approach is to store ambiguity as structured state (e.g.,vehicle_type_candidates: set[str]or a dedicated enum/flag) and derive a user-facing string separately; then banner resolution can update the structured field without relying on substring checks.
mavutil.mavlink.MAV_TYPE_VTOL_DUOROTOR: "ArduPlane or ArduCopter",
ardupilot_methodic_configurator/plugins/frontend_tkinter_motor_test.py:937
- Displaying a modal UI dialog inside a model factory couples model instantiation to the UI layer and makes the factory harder to reuse/test (and can be problematic if invoked before Tk is fully initialized, depending on call site). Prefer returning the model + error status to the view layer (or raising a recoverable exception) and letting the UI decide how/when to notify the user.
model = MotorTestDataModel(context.flight_controller, context.local_filesystem)
if model.invalid_frame_type_error:
showerror(_("Invalid Frame Type"), model.invalid_frame_type_error)
return model
tests/test_backend_flightcontroller_connection.py:1325
- This assertion is likely brittle because it depends on the exact MAVLink enum description formatting (punctuation/capitalization) from
pymavlinkand could change across versions/localization. Consider asserting on more stable substrings (e.g.,"Vehicle type:"and"running ArduPlane firmware") and relying primarily onconnection.info.vehicle_type == "ArduPlane"for correctness.
assert "Vehicle type: Fixed wing aircraft." in caplog.text
| break | ||
| if layout.get("Class") == self._frame_class and layout.get("motors"): | ||
| has_frame_class_configuration = True | ||
| if layout.get("Class") == self._frame_class and layout.get("Type") == self._frame_type: |
Coverage Report for CI Build 32292213333Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage decreased (-0.3%) to 90.654%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions249 previously-covered lines in 9 files lost coverage.
Coverage Stats
💛 - Coveralls |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Recover from invalid FRAME_CLASS, FRAME_TYPE combinations
Checklist
git commit --signoff)Testing
Describe how you tested these changes: