Update mavftp - #2018
Open
amilcarlucas wants to merge 4 commits into
Open
Conversation
Contributor
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.
Pull request overview
This PR updates the project to use the upstream MAVFTP implementation (from pymavlink) and aligns related data-model and UI behavior with ArduPlane-specific frame-class semantics, while also syncing ArduPilot device-ID decoding utilities.
Changes:
- Replace many legacy MAVFTP constants/structures with upstream-aligned enums/types (
FtpError,DirectoryEntry) and update tests/callers accordingly. - Adjust frame-class handling so ArduPlane can treat
"Undefined"as valid and prefersQ_FRAME_*parameters where applicable. - Introduce / sync ArduPilot’s device-id decode script into the repo and update workflows and import sites.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit_data_model_vehicle_components_validation_constants.py | Updates expectations around "Undefined" frame class for ArduPlane only. |
| tests/test_data_model_vehicle_components_validation.py | Adds coverage for accepting "Undefined" frame class on ArduPlane. |
| tests/test_backend_mavftp_aux.py | Migrates tests from integer error constants to FtpError. |
| tests/test_backend_mavftp.py | Updates/extends MAVFTP tests for new error handling and callback behavior. |
| tests/test_backend_flightcontroller_sitl.py | Updates error handling to use FtpError values. |
| tests/test_backend_flightcontroller_files.py | Updates directory listing mocks to use DirectoryEntry. |
| tests/test_backend_flightcontroller_factory_mavftp.py | Improves MAVFTP factory tests by mocking ResetSessions ACK behavior. |
| tests/test_backend_flightcontroller_business_logic.py | Adds ArduPlane-specific preference for Q_FRAME_* in get_frame_info. |
| pyproject.toml | Excludes synced ArduPilot device-id decoder file from Ruff. |
| ardupilot_methodic_configurator/vehicle_templates/ArduPlane/normal_plane/00_default.param | Syncs/updates ArduPlane default params (large template update). |
| ardupilot_methodic_configurator/log_analysis/decode_devid_lib.py | Updates/syncs device-id decoding logic and CLI behavior. |
| ardupilot_methodic_configurator/log_analysis/data_model_vehicle_overview_sensor_rules.py | Switches imports to the new synced decode_devid module and adapts to new return type. |
| ardupilot_methodic_configurator/data_model_vehicle_components_validation.py | Allows "Undefined" frame class only for ArduPlane in UI choices. |
| ardupilot_methodic_configurator/data_model_vehicle_components_import.py | Passes firmware type into get_frame_info for ArduPlane behavior. |
| ardupilot_methodic_configurator/configuration_steps_ArduPlane.json | Adds derived-parameter rules for Q_FRAME_CLASS and autoimport hints. |
| ardupilot_methodic_configurator/backend_mavftp.py | Major MAVFTP refactor to upstream structures (pymavlink.mavftp_op), new error enum, listing entries, and new read/messaging logic. |
| ardupilot_methodic_configurator/backend_flightcontroller_params.py | Updates success checks to use FtpError.Success. |
| ardupilot_methodic_configurator/backend_flightcontroller_files.py | Updates MAVFTP integration for new error codes and directory listing type. |
| ardupilot_methodic_configurator/backend_flightcontroller_commands.py | Ensures get_frame_info receives detected vehicle_type. |
| ardupilot_methodic_configurator/backend_flightcontroller_business_logic.py | Adds ArduPlane-specific Q_FRAME_* preference in get_frame_info. |
| ardupilot_methodic_configurator/main.py | Sets vehicle type/version earlier so frame-class UI choices derive correctly. |
| .github/workflows/update_flightcontroller_ids.yml | Extends sync workflow to copy decode_devid.py and updates commit message/body. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
129
to
130
|
|
||
| def __init__(self) -> None: |
Comment on lines
+606
to
+612
| try: | ||
| size_int = int(size) | ||
| size = int(size_str) | ||
| except (ValueError, TypeError, OverflowError): | ||
| logging.error("Invalid file size: %s", size) | ||
| size_int = 0 | ||
| self.directory_listing[name] = size_int | ||
| self.total_size += size_int | ||
| logging.info(" %s\t%u", name, size_int) | ||
| logging.error("Invalid file size: %s", size_str) | ||
| size = 0 | ||
| self.list_temp_result.append( | ||
| DirectoryEntry(name=name, is_dir=False, size_b=size) |
| self.done = False | ||
|
|
||
| logging.info( | ||
| "Getting %s starting at %u reading %u bytes", |
Comment on lines
718
to
737
| @@ -608,37 +735,69 @@ def cmd_get( | |||
| self.burst_size = int(self.ftp_settings.burst_read_size) | |||
| if self.burst_size < 1 or self.burst_size > 239: | |||
| self.burst_size = 239 | |||
Comment on lines
746
to
+751
|
|
||
| def __handle_open_ro_reply(self, op: FTP_OP, _m: object) -> MAVFTPReturn: | ||
| def __handle_open_ro_reply(self, op: FTP_OP, _m) -> MAVFTPReturn: | ||
| """Handle OP_OpenFileRO reply.""" | ||
| if op.opcode == OP_Ack: | ||
| if self.filename is None: | ||
| return MAVFTPReturn("OpenFileRO", ERR_FileNotFound) | ||
| return MAVFTPReturn("OpenFileRO", FtpError.FileNotFound) |
Comment on lines
+811
to
+815
| return True | ||
| if self.op_start is None: | ||
| return True | ||
| if len(self.read_gaps) == 0 and ( | ||
| self.reached_eof or self.read_total >= self.requested_size |
| op_ret_name = operation_name or operation_name_dict.get(op.req_opcode, "Unknown") | ||
| op_ret_name = operation_name or operation_name_dict.get( | ||
| op.req_opcode, "Unknown" | ||
| ) |
| elif error_code == FtpError.FailErrno: | ||
| error_code = FtpError.NoFilesystemErrorInPayload | ||
| elif error_code not in [ | ||
| FtpError.Fail, |
Comment on lines
+1858
to
+1863
| invalid_error_code = error_code | ||
| error_code = ERR_InvalidErrorCode | ||
| elif op.payload is not None and op.payload[0] == ERR_FailErrno and len_payload == 2: | ||
| error_code = FtpError.InvalidErrorCode | ||
| elif ( | ||
| op.payload is not None | ||
| and op.payload[0] == FtpError.FailErrno | ||
| and len_payload == 2 |
| type=float, | ||
| metavar="setting_value", | ||
| help="MAVFTP internal configuration parameter value.", | ||
| ) |
amilcarlucas
force-pushed
the
update_mavftp
branch
2 times, most recently
from
September 3, 2026 00:04
8691eac to
9fee757
Compare
Contributor
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.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 6 comments.
Comment on lines
+659
to
+664
| logging.info("loop closed, gaps:%u, done: %u", self.read_gaps, self.done) | ||
| if not self.done and self.__has_active_session(): | ||
| self.__terminate_session() | ||
| if len(self.read_gaps) == 0: | ||
| return self.get_result | ||
| logging.error("closed read with %u gaps", self.read_gaps) |
Comment on lines
+740
to
+750
| self.fh.seek(self.requested_offset) | ||
| read = FTP_OP( | ||
| self.seq, | ||
| self.session, | ||
| OP_BurstReadFile, | ||
| self.burst_size, | ||
| 0, | ||
| 0, | ||
| self.requested_offset, | ||
| None, | ||
| ) |
Comment on lines
+759
to
764
| self.requested_size = self.remote_file_size | ||
| else: | ||
| self.remote_file_size = None | ||
| self.remote_file_size = 0 | ||
| read = FTP_OP(self.seq, self.session, OP_BurstReadFile, self.burst_size, 0, 0, 0, None) | ||
| self.last_burst_read = time.time() | ||
| self.__send(read) |
Comment on lines
+613
to
+618
| def read_sector(self, path: str, offset: int, size: int) -> bytes | None: | ||
| logging.info("reading sector %s, offset=%u, size=%u", path, offset, size) | ||
| return self.read(path, size, offset) | ||
|
|
||
| def read(self, path: str, size: int, offset: int = 0) -> bytes | None: | ||
| """Get file.""" |
Comment on lines
1867
to
1883
| @@ -1442,10 +1876,10 @@ def save_params( | |||
| return | |||
| with open(filename, "w", encoding="utf-8") as f: | |||
| parameter_data_types = { | |||
| 1: "8-bit", | |||
| 2: "16-bit", | |||
| 3: "32-bit integer", | |||
| 4: "32-bit float", | |||
| "1": "8-bit", | |||
| "2": "16-bit", | |||
| "3": "32-bit integer", | |||
| "4": "32-bit float", | |||
| } | |||
| """Wait for a heartbeat so we know the target system IDs.""" | ||
| logging.info("Waiting for flight controller heartbeat") | ||
| m.wait_heartbeat(timeout=5) | ||
| logging.info("Heartbeat from system %u, component %u", m.target_system, m.target_component) | ||
| logging.info("Heartbeat from system %u, component %u", m.target_system, m.target_system) |
amilcarlucas
force-pushed
the
update_mavftp
branch
from
September 3, 2026 19:45
aa1687d to
7774e53
Compare
Contributor
☂️ Code Coverage
Overall Coverage
New Files
Modified Files
|
Coverage Report for CI Build 33796539831Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Warning No base build found for commit Coverage: 87.622%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
Replace the local MAVFTP backend with pymavlink's implementation and adapt consumers to the FtpError and DirectoryEntry APIs. Prevent callback-owned downloads from writing virtual remote paths as local files, and update the related tests and fixtures.
- Add modal remote log browser rooted at /APM/LOGS/ - Support refresh, Enter, Ctrl+A, filename and size sorting - Support single-file and batch downloads with progress and overwrite handling - Continue batch downloads after individual failures with per-file summaries - Preserve the existing last-log download workflow - Add architecture documentation and BDD tests Signed-off-by: Dr.-Ing. Amilcar do Carmo Lucas <amilcar.lucas@iav.de>
Add an FTP-style remote/local file browser to the .bin log window with recursive MAVFTP transfers, directory navigation, upload, rename, deletion, sorting, keyboard shortcuts, and progress reporting. Retain the existing last-log download workflow and add pytest coverage plus updated architecture documentation.
amilcarlucas
force-pushed
the
update_mavftp
branch
from
September 4, 2026 09:47
7774e53 to
be289ee
Compare
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
Use upstream mavftp from pymavlink instead of out local forked copy
Thanks to @peterbarker this speeds up parameter download from 4 seconds to 40ms :)
Checklist
git commit --signoff)Testing
Describe how you tested these changes: