Mavftp gui - #2075
Open
amilcarlucas wants to merge 6 commits into
Open
Mavftp gui#2075amilcarlucas wants to merge 6 commits into
amilcarlucas wants to merge 6 commits into
Conversation
Log the number of uploaded parameters and elapsed verification time using descriptive, order-independent placeholders. Add regression coverage for the new message.
Contributor
☂️ Code Coverage
Overall Coverage
New Files
Modified Files
|
Contributor
Test Results 4 files 4 suites 36m 19s ⏱️ For more details on these failures, see this check. Results for commit d182127. ♻️ This comment has been updated with latest results. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Critical path-traversal and symlink-safety issues, along with transfer correctness problems, remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a Tkinter MAVFTP/local file browser with recursive transfers, file management, progress handling, documentation, and tests.
Changes:
- Added browsing, navigation, sorting, creation, deletion, renaming, upload, and download workflows.
- Extended MAVFTP backend APIs and restart/reconnect progress handling.
- Added documentation, screenshots, tests, and tooling configuration.
File summaries
| File | Reviewed changes |
|---|---|
USERMANUAL.md |
Links the MAVFTP manual. |
USERMANUAL_MAVFTP.md |
Documents browser usage and troubleshooting. |
ty.toml |
Updates type-checking exclusions. |
tests/test_frontend_tkinter_progress_window.py |
Tests staged progress behavior. |
tests/test_frontend_tkinter_parameter_editor.py |
Tests progress integration. |
tests/test_download_bin_logs.py |
Tests browser navigation and transfers. |
tests/test_data_model_parameter_editor.py |
Tests file and reset workflows. |
tests/test_backend_mavftp.py |
Tests MAVFTP behavior and staging. |
tests/test_backend_mavftp_aux.py |
Updates MAVFTP error-code tests. |
tests/test_backend_flightcontroller.py |
Tests unified reconnect progress. |
tests/test_backend_flightcontroller_sitl.py |
Updates SITL MAVFTP operations. |
tests/test_backend_flightcontroller_files.py |
Tests remote file operations. |
tests/test_backend_flightcontroller_factory_mavftp.py |
Updates MAVFTP factory fixtures. |
tests/test_backend_flightcontroller_connection.py |
Tests reconnect retries. |
tests/plugins/test_data_model_motor_test.py |
Updates motor-test callback tests. |
tests/plugins/acceptance_battery_monitor.py |
Updates progress-window expectations. |
scripts/regenerate_mavftp_screenshots.py |
Generates documentation screenshots. |
pyproject.toml |
Updates lint and type-check configuration. |
ardupilot_methodic_configurator/plugins/frontend_tkinter_motor_test.py |
Uses unified restart progress. |
ardupilot_methodic_configurator/plugins/data_model_motor_test.py |
Propagates restart progress callbacks. |
ardupilot_methodic_configurator/mavftp_example.py |
Updates MAVFTP upload reply handling. |
ardupilot_methodic_configurator/frontend_tkinter_progress_window.py |
Supports staged restart progress. |
ardupilot_methodic_configurator/frontend_tkinter_parameter_editor.py |
Integrates the browser and progress handling. |
ardupilot_methodic_configurator/data_model_parameter_editor.py |
Adds browser-facing file workflows. |
ardupilot_methodic_configurator/backend_flightcontroller.py |
Adds file APIs and reconnect progress. |
ardupilot_methodic_configurator/backend_flightcontroller_protocols.py |
Extends controller protocols. |
ardupilot_methodic_configurator/backend_flightcontroller_params.py |
Uses MAVFTP error types. |
ardupilot_methodic_configurator/backend_flightcontroller_files.py |
Adds remote file operations and path handling. |
ardupilot_methodic_configurator/backend_flightcontroller_connection.py |
Adds reconnect retries and progress reporting. |
ARCHITECTURE.md |
Links the browser architecture. |
ARCHITECTURE_download_bin_logs.md |
Documents browser architecture and contracts. |
.pylintrc |
Updates pylint exclusions. |
Review details
Suppressed comments (7)
ardupilot_methodic_configurator/data_model_parameter_editor.py:1429
- This compatibility download workflow uses the same collision check but only case-folds on Windows. On the supported, commonly case-insensitive macOS filesystem, two remote names that differ only by case are treated as distinct and the later
os.replacecan overwrite the first without the overwrite prompt; include macOS in the case-insensitive key handling here as well.
return target.casefold() if platform.system() == "Windows" else target
ardupilot_methodic_configurator/frontend_tkinter_download_bin_logs.py:1459
- The recursive upload plan is built synchronously for every selected directory before
_start_background_operation()is called. Enumerating a large log/configuration tree (includingiterdir()andstat()for every descendant) blocks Tk's event loop, so the new browser can freeze before its progress/cancel UI appears; build this plan in the background task or otherwise dispatch local traversal off the UI thread.
target = posixpath.join(remote_directory, entry.name)
child_dirs, child_files = self._local_upload_plan(entry, target)
directories.extend(child_dirs)
ardupilot_methodic_configurator/frontend_tkinter_download_bin_logs.py:1414
- Local POSIX filenames may legally contain
\\, but this name is joined directly into a remote POSIX path.make_remote_directorynormalizes backslashes to/, whereasupload_fileand_remote_parent_directoriesuse the raw path, so a directory or file containing a backslash can be created and uploaded under different remote paths. Reject unsupported names or canonicalize them once before building the plan.
if not entry.is_directory:
return [], [(entry.path, remote_target, entry.size_bytes)]
ardupilot_methodic_configurator/frontend_tkinter_download_bin_logs.py:1002
- This collision key is case-insensitive only on Windows. On the supported macOS platform, a case-insensitive volume can map remote entries such as
Fooandfooto the same local path, but the planner treats them as distinct and the later download can overwrite the first without an overwrite prompt. Include macOS (or use filesystem-aware collision detection).
return str(path.absolute()).casefold() if sys.platform == "win32" else str(path.absolute())
ardupilot_methodic_configurator/frontend_tkinter_download_bin_logs.py:58
- The protocol still types
create_progress_windowas accepting exactly four positional arguments, but this window now calls it with theauto_close_on_completekeyword through the injected UI service. That makes the new call incompatible with the declared interface and will be reported by static type checking; update the protocol to include the optional keyword (or use a matching callable alias).
create_progress_window: Callable[[tk.Misc, str, str, bool], ProgressWindow]
ardupilot_methodic_configurator/frontend_tkinter_download_bin_logs.py:1440
- Unlike the download, delete, rename, and refresh paths, upload only checks
_operation_active; it can therefore start while_remote_task_threadis still listing/planning remote data (for example during initial loading). The remote-task completion can then repopulate the tree and re-enable controls while the upload is active, so guard uploads with the same remote-task check.
if getattr(self, "_operation_active", False):
ardupilot_methodic_configurator/frontend_tkinter_download_bin_logs.py:353
- After falling back from
/APM/LOGS/, this closure still formats the label with the originalremote_directorycaptured before the fallback. The panel can therefore show/APM/LOGS/while displaying entries from/APM/; format the label from the currentremote_directory_varvalue.
text=_("Remote files in {remote_directory}").format(remote_directory=remote_directory)
- Files reviewed: 33/36 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
130
to
133
| if not self._ensure_remote_directory_exists(mavftp_instance, remote_filename): | ||
| return False | ||
|
|
||
| put_ret = mavftp_instance.cmd_put([local_filename, remote_filename], progress_callback=put_progress_callback) |
| if directory.is_symlink() or (directory.exists() and not directory.is_dir()): | ||
| failed.append(str(directory)) | ||
| continue | ||
| directory.mkdir(parents=True, exist_ok=True) |
Comment on lines
+383
to
+384
| result = mavftp_instance.cmd_mkdir([normalized_directory.rstrip("/")]) | ||
| return result.error_code in {FtpError.Success, FtpError.FileExists} |
Comment on lines
+1417
to
+1420
| try: | ||
| children = sorted(entry.path.iterdir(), key=lambda path: path.name.casefold()) | ||
| except OSError: | ||
| return directories, files |
| args.save_component_to_system_templates, | ||
| ) | ||
| parameter_editor = ParameterEditor("", flight_controller, filesystem) | ||
| flight_controller.connect(args.device) |
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.
amilcarlucas
force-pushed
the
mavftp_gui
branch
from
September 16, 2026 22:17
a682ea0 to
8ce2911
Compare
Replace separate reset and connection progress callbacks with one progress callback for the complete restart workflow. Emit deterministic restart progress stages: - 10% after the reset command is sent - 30%, 50%, and 70% for reconnect attempts - 90% after MAVLink heartbeat detection - 100% after the controller handshake completes Retry failed USB-port opens, heartbeat detection, and version/banner handshakes so a flight controller that re-enumerates after reboot can reconnect successfully. Use one shared restart/reconnect progress window in the parameter editor and motor-test plugin while keeping parameter-download progress unchanged. Add support for replacing a progress window message between stages and update acceptance and unit tests accordingly. BREAKING CHANGE: reset_progress_callback and connection_progress_callback are replaced by a single progress_callback.
amilcarlucas
force-pushed
the
mavftp_gui
branch
from
September 16, 2026 22:25
be7f183 to
1fa7641
Compare
amilcarlucas
force-pushed
the
mavftp_gui
branch
from
September 16, 2026 23:13
1fa7641 to
d182127
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
A MAVFTP file browser
Requires #2050
Checklist
git commit --signoff)Testing
Describe how you tested these changes: