diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index feaf5102d..f4ef91cb0 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -101,6 +101,7 @@ Each sub-application has detailed architecture documentation covering requiremen - upload them to the flight controller, - save them to file - [`frontend_tkinter_parameter_editor.py`](ardupilot_methodic_configurator/frontend_tkinter_parameter_editor.py) + - [Flight-controller `.bin` log download architecture](ARCHITECTURE_download_bin_logs.md) - [External parameter file upload architecture](ARCHITECTURE_parameter_upload.md) - [`frontend_tkinter_parameter_editor_documentation_frame.py`](ardupilot_methodic_configurator/frontend_tkinter_parameter_editor_documentation_frame.py) - [`frontend_tkinter_parameter_editor_table.py`](ardupilot_methodic_configurator/frontend_tkinter_parameter_editor_table.py) diff --git a/ARCHITECTURE_download_bin_logs.md b/ARCHITECTURE_download_bin_logs.md new file mode 100644 index 000000000..151b1777d --- /dev/null +++ b/ARCHITECTURE_download_bin_logs.md @@ -0,0 +1,184 @@ +# Flight-controller file browser + +**Status:** Implemented +**Entry point:** Parameter Editor -> **Download .bin log file(s)** +**Implemented on:** September 3, 2026 + +## Purpose + +The Parameter Editor opens a modal, two-panel FTP-style browser for the +connected flight controller. The remote MAVFTP panel is on the left and the +local filesystem panel is on the right. The default remote location is +`/APM/LOGS/`. The browser lists all valid regular remote files, not only +numbered `.BIN` logs, and also displays directories. The existing +**Download last .bin log file** workflow remains available. + +## User interface + +`DownloadBinLogsWindow` inherits from `BaseWindow`, is transient to the +Parameter Editor, and uses a modal grab except on macOS. + +Each panel has a location entry, an icon-only `↖` parent-directory button, +refresh/open controls, a Select all button, and a sortable `ttk.Treeview` with +**Name**, **Type**, and **Size** columns. +The remote entry is initialized to `/APM/LOGS/`; Enter in that field performs +the same operation as Open/Refresh. Double-clicking a directory navigates into +it. The local panel additionally provides Browse and Refresh controls. +The local panel initially opens in the current Parameter Editor vehicle +directory, falling back to the process working directory if that directory is +unavailable. +Backspace navigates to the parent directory of the panel that was most +recently focused or had a selection change. +Delete removes all selected files and empty directories in the last selected +panel. F2 starts an inline rename for the last selected entry, with a name +dialog fallback when inline editing is unavailable. + +Filename sorting is case-insensitive. Size sorting uses the numeric byte count, +not the formatted display text. Treeview rows retain stable entry identifiers, +so sorting does not change which file is selected. Ctrl+A and Command+A select +all entries in the focused panel. + +## Operations + +The action bar provides: + +- **Download selected**: recursively copies selected remote files/directories + into the current local directory; +- **Upload selected**: recursively copies selected local files/directories into + the current remote directory; +- **Delete remote** and **Rename remote**; +- **Delete local** and **Rename local**; +- **Download last .bin log file**; and +- **Cancel**. + +Rename requires exactly one selected entry and accepts only one safe path +component. Parent navigation is performed only through the `↖` button or the +Backspace shortcut; it is not represented as a selectable tree row. + +Remote downloads enumerate selected directories recursively, create the local +directory tree, and download regular files. Local uploads create remote +directories parent-first and upload regular files. Local symlinks are skipped. +Deletion is deliberately non-recursive: remote and local directories must be +empty. A non-empty directory is reported as failed while other selected files +and empty directories continue to be processed. + +Batch operations continue after individual transfer or management failures. +The modal reports succeeded and failed paths separately after each operation. + +## Layered architecture + +```text +ParameterEditorWindow + | + v +DownloadBinLogsWindow(BaseWindow) + | injected dialogs/messages/ProgressWindow + v +ParameterEditor + | + v +FlightController + | + v +FlightControllerFiles + | + v +MAVFTP +``` + +The Tkinter window owns modal lifecycle, panel navigation, selection, sorting, +local enumeration, recursive operation planning, dialogs, confirmations, +progress updates, and summaries. It does not call MAVFTP directly. + +`ParameterEditor` delegates these operations: + +```python +get_bin_log_files(remote_directory) +get_remote_files(remote_directory) +upload_file_to_fc(local_filename, remote_filename, progress_callback) +download_remote_file(remote_path, local_filename, progress_callback) +make_remote_directory(remote_directory) +delete_remote_path(remote_path, is_directory=False) +rename_remote_path(remote_path, new_remote_path) +download_selected_bin_logs_workflow(...) +download_last_flight_log_workflow(...) +``` + +`DownloadBinLogsUiServices` injects save-file, directory, and rename selectors, +confirmation/message callbacks, and the standard application ProgressWindow +factory. The legacy single/multiple regular-file download workflow is retained: +one file uses a save-file selector, multiple files use a directory selector, +and existing targets receive one overwrite confirmation. + +## Backend model and MAVFTP + +`FlightControllerFiles.list_remote_files()` returns files and directories; +`list_bin_log_files()` retains its historical name and filters that listing to +regular files. The manager also implements explicit upload/download, directory +creation, deletion, and rename operations, while retaining the existing +LASTLOG/fallback last-log workflow. + +```python +@dataclass(frozen=True) +class FlightControllerLogFile: + name: str + remote_path: str + size_bytes: int + is_directory: bool = False +``` + +The local panel uses an equivalent `LocalFileEntry` containing `name`, `path`, +`size_bytes`, and `is_directory`. + +Remote paths are normalized to absolute POSIX paths and parent segments are +rejected. Directory entry names must be a single component without `/` or +`\\`; embedded, leading, and trailing spaces are preserved, and malformed +listing names are skipped. Paths are passed to MAVFTP as argument values, not +through a shell, so filenames do not need quoting. This prevents malformed +MAVFTP entries from escaping the selected directory during recursive +operations. + +The current MAVFTP `DirectoryEntry` exposes `name`, `is_dir`, and `size_b`, but +no modification timestamp. Therefore modification dates cannot currently be +displayed; that requires MAVFTP and firmware support for timestamp metadata. + +## Progress and error behavior + +Remote transfers run in a worker thread so the Tk event loop remains responsive. +The browser uses the standard ProgressWindow and adapts each MAVFTP percentage +callback into aggregate byte-oriented batch progress. The Cancel button signals +the active transfer to stop at the next MAVFTP progress boundary and failed +transfers terminate their MAVFTP session. Zero-byte files count as one progress +unit. Empty directories are created without file progress units. + +Missing local destinations and invalid remote paths are reported before work +starts. Existing local download targets prompt once. Upload confirmation is +conservative because the browser does not pre-query every remote target. +Later entries continue after a transfer failure. Local symlinks are never +included in recursive transfer or destructive operations. + +Known limitations are that listing errors and empty directories both appear as +an empty listing, recursive directory planning can briefly block the UI, +explicit remote methods do not use capability-based directory scoping, and +there is no modification-date metadata. Mutations outside `/APM/LOGS/` display +an additional warning and require a separate confirmation. + +## Testing + +`tests/test_download_bin_logs.py` covers generic listing and directory metadata, +MAVFTP management delegation, recursive planning and execution for downloads +and uploads, recursive deletion, rename, batch continuation after failure, +filename and numeric-size sorting, Select all/Ctrl+A, parent-directory button +and Backspace navigation, remote refresh/Enter behavior, legacy destination +selectors, and modal integration. + +```text +.venv\Scripts\python.exe -m pytest tests/test_download_bin_logs.py -q -p no:cacheprovider +``` + +## Deferred hardening + +- modification-date display pending MAVFTP metadata support; +- structured listing errors instead of an empty-list error channel; +- preflight upload conflict discovery; and +- real-Tk/end-to-end MAVFTP integration tests. diff --git a/ardupilot_methodic_configurator/backend_flightcontroller.py b/ardupilot_methodic_configurator/backend_flightcontroller.py index efbd0d069..98604be22 100644 --- a/ardupilot_methodic_configurator/backend_flightcontroller.py +++ b/ardupilot_methodic_configurator/backend_flightcontroller.py @@ -32,7 +32,7 @@ SUPPORTED_BAUDRATES, FlightControllerConnection, ) -from ardupilot_methodic_configurator.backend_flightcontroller_files import FlightControllerFiles +from ardupilot_methodic_configurator.backend_flightcontroller_files import FlightControllerFiles, FlightControllerLogFile from ardupilot_methodic_configurator.backend_flightcontroller_params import FlightControllerParams from ardupilot_methodic_configurator.backend_flightcontroller_protocols import ( FlightControllerCommandsProtocol, @@ -585,6 +585,44 @@ def download_last_flight_log( """Download the last flight log from the flight controller - delegates to files manager.""" return self._files_manager.download_last_flight_log(local_filename, progress_callback) + def list_bin_log_files(self, remote_directory: str = "/APM/LOGS/") -> list[FlightControllerLogFile]: + """List regular files in a remote directory - delegates to files manager.""" + return self._files_manager.list_bin_log_files(remote_directory) + + def list_remote_files(self, remote_directory: str = "/APM/LOGS/") -> list[FlightControllerLogFile]: + """List files and directories in a remote directory - delegates to files manager.""" + return self._files_manager.list_remote_files(remote_directory) + + def download_bin_log_file( + self, + remote_path: str, + local_filename: str, + progress_callback: Callable[[int, int], None] | None = None, + ) -> bool: + """Download an explicitly selected remote file - delegates to files manager.""" + return self._files_manager.download_bin_log_file(remote_path, local_filename, progress_callback) + + def download_remote_file( + self, + remote_path: str, + local_filename: str, + progress_callback: Callable[[int, int], None] | None = None, + ) -> bool: + """Download one remote file - delegates to files manager.""" + return self._files_manager.download_remote_file(remote_path, local_filename, progress_callback) + + def make_remote_directory(self, remote_directory: str) -> bool: + """Create a remote directory - delegates to files manager.""" + return self._files_manager.make_remote_directory(remote_directory) + + def delete_remote_path(self, remote_path: str, is_directory: bool = False) -> bool: + """Delete a remote file or directory - delegates to files manager.""" + return self._files_manager.delete_remote_path(remote_path, is_directory) + + def rename_remote_path(self, remote_path: str, new_remote_path: str) -> bool: + """Rename a remote file or directory - delegates to files manager.""" + return self._files_manager.rename_remote_path(remote_path, new_remote_path) + # Static methods and properties @staticmethod diff --git a/ardupilot_methodic_configurator/backend_flightcontroller_files.py b/ardupilot_methodic_configurator/backend_flightcontroller_files.py index e67809c24..d3b58d911 100644 --- a/ardupilot_methodic_configurator/backend_flightcontroller_files.py +++ b/ardupilot_methodic_configurator/backend_flightcontroller_files.py @@ -11,6 +11,7 @@ import os import posixpath from collections.abc import Callable +from dataclasses import dataclass from logging import debug as logging_debug from logging import error as logging_error from logging import info as logging_info @@ -28,16 +29,17 @@ ) from ardupilot_methodic_configurator.backend_mavftp import MAVFTP as MAVFTPType # noqa: N811 -# Conditionally import MAVFTP if available -try: - from ardupilot_methodic_configurator.backend_mavftp import MAVFTP, ERR_FileExists, ERR_None +from ardupilot_methodic_configurator.backend_mavftp import MAVFTP, FtpError - # from pymavlink import mavftp - # MAVFTP = mavftp.MAVFTP -except ImportError: - ERR_None = 0 - ERR_FileExists = 8 - MAVFTP = None # type: ignore[assignment,misc] + +@dataclass(frozen=True) +class FlightControllerLogFile: + """A regular file exposed by a flight-controller directory listing.""" + + name: str + remote_path: str + size_bytes: int + is_directory: bool = False class FlightControllerFiles: @@ -54,6 +56,7 @@ class FlightControllerFiles: # MAVFTP timeout constants MAVFTP_FILE_OPERATION_TIMEOUT: ClassVar[int] = 10 MAVFTP_FILE_OPERATION_TIMEOUT_SHORT: ClassVar[int] = 5 + DEFAULT_LOG_DIRECTORY: ClassVar[str] = "/APM/LOGS/" def __init__( self, @@ -118,12 +121,12 @@ def put_progress_callback(completion: float) -> None: return False put_ret = mavftp_instance.cmd_put([local_filename, remote_filename], progress_callback=put_progress_callback) - if put_ret.error_code != ERR_None: + if put_ret.error_code != FtpError.Success: put_ret.display_message() return False ret = mavftp_instance.process_ftp_reply("CreateFile", timeout=self.MAVFTP_FILE_OPERATION_TIMEOUT) - if ret.error_code != 0: + if ret.error_code != FtpError.Success: ret.display_message() return False logging_info( @@ -131,6 +134,10 @@ def put_progress_callback(completion: float) -> None: ) return True except Exception as e: # pylint: disable=broad-exception-caught + try: + mavftp_instance.cmd_cancel() + except Exception: # pylint: disable=broad-exception-caught + logging_debug("Could not cancel failed MAVFTP upload", exc_info=True) logging_error(_("Failed to upload file: %(error)s"), {"error": str(e)}) return False @@ -148,7 +155,7 @@ def _ensure_remote_directory_exists(self, mavftp_instance: "MAVFTPType", remote_ for current_dir in parent_directories: ret = mavftp_instance.cmd_mkdir([current_dir]) - if ret.error_code not in {ERR_None, ERR_FileExists}: + if ret.error_code not in {FtpError.Success, FtpError.FileExists}: ret.display_message() logging_error(_("Failed to create remote directory %(directory)s"), {"directory": current_dir}) return False @@ -215,6 +222,220 @@ def get_progress_callback(completion: float) -> None: logging_error(_("Error during flight log download: %(error)s"), {"error": str(e)}) return False + @classmethod + def _normalize_remote_path(cls, remote_path: str, *, directory: bool = False) -> str: + """Normalize and validate an absolute MAVFTP path.""" + if not isinstance(remote_path, str) or not remote_path.strip(): + msg = _("Remote path must not be empty") + raise ValueError(msg) + + # Do not strip the path itself: leading/trailing spaces can be valid + # filename characters on the flight controller. Only whitespace-only + # paths are rejected above. + path = remote_path.replace("\\", "/") + if not path.startswith("/"): + msg = _("Remote path must be absolute") + raise ValueError(msg) + + if ".." in path.split("/"): + msg = _("Remote path must not contain parent-directory segments") + raise ValueError(msg) + + normalized = posixpath.normpath(path) + if not normalized.startswith("/"): + msg = _("Remote path must be absolute") + raise ValueError(msg) + + if directory: + return normalized if normalized == "/" else f"{normalized}/" + return normalized + + @classmethod + def _remote_child_path(cls, remote_directory: str, filename: str) -> str: + """Build a safe remote path for a direct child of a remote directory.""" + if ( + not filename + or filename in {".", ".."} + or "/" in filename + or "\\" in filename + or posixpath.basename(filename) != filename + ): + msg = _("Remote directory entries must be regular file names") + raise ValueError(msg) + directory = cls._normalize_remote_path(remote_directory, directory=True) + return posixpath.join(directory, filename) + + def list_remote_files(self, remote_directory: str = DEFAULT_LOG_DIRECTORY) -> list[FlightControllerLogFile]: # noqa: PLR0911 + """List regular files and directories in a remote directory.""" + if self.master is None: + logging_error(_("No flight controller connected")) + return [] + if not self.info.is_mavftp_supported: + logging_error(_("MAVFTP is not supported by the flight controller")) + return [] + + try: + normalized_directory = self._normalize_remote_path(remote_directory, directory=True) + except ValueError as error: + logging_error(_("Invalid remote directory: %(error)s"), {"error": str(error)}) + return [] + + mavftp_instance = create_mavftp_safe(self.master) + if mavftp_instance is None: + logging_error(_("MAVFTP is not available for file listing")) + return [] + + try: + result = mavftp_instance.cmd_list([normalized_directory]) + listing = getattr(result, "directory_listing", None) + if not isinstance(listing, list): + logging_error(_("No directory listing found in MAVFTPReturn")) + return [] + + entries: list[FlightControllerLogFile] = [] + for entry in listing: + try: + remote_path = self._remote_child_path(normalized_directory, entry.name) + except ValueError: + logging_warning(_("Skipping invalid remote directory entry: %(name)s"), {"name": entry.name}) + continue + entries.append( + FlightControllerLogFile( + entry.name, + remote_path, + max(0, int(entry.size_b)), + bool(entry.is_dir), + ) + ) + return entries + except Exception as error: # pylint: disable=broad-exception-caught + logging_error(_("Failed to list remote directory: %(error)s"), {"error": str(error)}) + return [] + + def list_bin_log_files(self, remote_directory: str = DEFAULT_LOG_DIRECTORY) -> list[FlightControllerLogFile]: + """ + List all regular files in a remote directory. + + The historical method name is retained because this operation is launched + by the .bin-log UI, but the listing intentionally accepts every regular + file returned by MAVFTP. + """ + return [entry for entry in self.list_remote_files(remote_directory) if not entry.is_directory] + + def make_remote_directory(self, remote_directory: str) -> bool: + """Create a remote directory, treating an existing directory as success.""" + if self.master is None or not self.info.is_mavftp_supported: + return False + try: + normalized_directory = self._normalize_remote_path(remote_directory, directory=True) + except ValueError as error: + logging_error(_("Invalid remote directory: %(error)s"), {"error": str(error)}) + return False + if normalized_directory == "/": + return True + + mavftp_instance = create_mavftp_safe(self.master) + if mavftp_instance is None: + return False + try: + result = mavftp_instance.cmd_mkdir([normalized_directory.rstrip("/")]) + return result.error_code in {FtpError.Success, FtpError.FileExists} + except Exception as error: # pylint: disable=broad-exception-caught + logging_error(_("Failed to create remote directory: %(error)s"), {"error": str(error)}) + return False + + def delete_remote_path(self, remote_path: str, is_directory: bool = False) -> bool: + """Delete a remote file or an empty remote directory.""" + if self.master is None or not self.info.is_mavftp_supported: + return False + try: + normalized_path = self._normalize_remote_path(remote_path) + if normalized_path == "/": + raise ValueError(_("The remote root cannot be deleted")) + except ValueError as error: + logging_error(_("Invalid remote path: %(error)s"), {"error": str(error)}) + return False + + mavftp_instance = create_mavftp_safe(self.master) + if mavftp_instance is None: + return False + try: + result = ( + mavftp_instance.cmd_rmdir([normalized_path]) if is_directory else mavftp_instance.cmd_rm([normalized_path]) + ) + return result.error_code == FtpError.Success + except Exception as error: # pylint: disable=broad-exception-caught + logging_error(_("Failed to delete remote path: %(error)s"), {"error": str(error)}) + return False + + def rename_remote_path(self, remote_path: str, new_remote_path: str) -> bool: + """Rename a remote file or directory.""" + if self.master is None or not self.info.is_mavftp_supported: + return False + try: + normalized_old_path = self._normalize_remote_path(remote_path) + normalized_new_path = self._normalize_remote_path(new_remote_path) + if normalized_old_path == "/" or normalized_new_path == "/": + raise ValueError(_("The remote root cannot be renamed")) + except ValueError as error: + logging_error(_("Invalid remote path: %(error)s"), {"error": str(error)}) + return False + + mavftp_instance = create_mavftp_safe(self.master) + if mavftp_instance is None: + return False + try: + result = mavftp_instance.cmd_rename([normalized_old_path, normalized_new_path]) + return result.error_code == FtpError.Success + except Exception as error: # pylint: disable=broad-exception-caught + logging_error(_("Failed to rename remote path: %(error)s"), {"error": str(error)}) + return False + + def download_bin_log_file( + self, + remote_path: str, + local_filename: str, + progress_callback: Callable[[int, int], None] | None = None, + ) -> bool: + """Download one explicitly selected remote file.""" + if self.master is None: + logging_error(_("No flight controller connected")) + return False + if not self.info.is_mavftp_supported: + logging_error(_("MAVFTP is not supported by the flight controller")) + return False + + try: + normalized_remote_path = self._normalize_remote_path(remote_path) + except ValueError as error: + logging_error(_("Invalid remote file: %(error)s"), {"error": str(error)}) + return False + + mavftp_instance = create_mavftp_safe(self.master) + if mavftp_instance is None: + logging_error(_("MAVFTP is not available for file download")) + return False + + def get_progress_callback(completion: float) -> None: + if progress_callback is not None and completion is not None: + progress_callback(int(completion * 100), 100) + + return self._download_remote_file( + mavftp_instance, + normalized_remote_path, + local_filename, + get_progress_callback, + ) + + def download_remote_file( + self, + remote_path: str, + local_filename: str, + progress_callback: Callable[[int, int], None] | None = None, + ) -> bool: + """Download one explicitly selected remote regular file.""" + return self.download_bin_log_file(remote_path, local_filename, progress_callback) + def _get_last_log_number(self, mavftp_instance: "MAVFTP") -> int | None: # pyright: ignore[reportInvalidTypeForm] """ Get the last log number using multiple fallback methods. @@ -263,7 +484,7 @@ def _get_log_number_from_lastlog_txt( temp_lastlog_file = "temp_lastlog.txt" mavftp_instance.cmd_get(["/APM/LOGS/LASTLOG.TXT", temp_lastlog_file]) ret = mavftp_instance.process_ftp_reply("OpenFileRO", timeout=self.MAVFTP_FILE_OPERATION_TIMEOUT) - if ret.error_code != 0: + if ret.error_code != FtpError.Success: logging_warning(_("LASTLOG.TXT not available, trying alternative methods")) return None @@ -289,11 +510,13 @@ def _get_log_number_from_directory_listing( logging_info(_("Trying to get log number from directory listing")) try: result = mavftp_instance.cmd_list(["/APM/LOGS/"]) - if not hasattr(result, "directory_listing") or not isinstance(result.directory_listing, dict): + listing = getattr(result, "directory_listing", None) + if not isinstance(listing, list): logging_error(_("No directory listing found in MAVFTPReturn")) return None highest = -1 - for name in result.directory_listing: + for entry in listing: + name = entry.name # Typical log file names: 00000036.BIN, 00000037.BIN, etc. if name.endswith(".BIN") and name[:8].isdigit(): try: @@ -345,7 +568,7 @@ def _get_log_number_by_scanning( if os.path.exists(temp_test_file): os.remove(temp_test_file) - if ret.error_code == 0: + if ret.error_code == FtpError.Success: # File exists, search in upper half last_found = mid low = mid + 1 @@ -387,13 +610,28 @@ def _download_log_file( """ remote_filename = f"/APM/LOGS/{remote_filenumber:08}.BIN" + return self._download_remote_file( + mavftp_instance, + remote_filename, + local_filename, + get_progress_callback, + ) + + def _download_remote_file( + self, + mavftp_instance: "MAVFTP", # pyright: ignore[reportInvalidTypeForm] + remote_filename: str, + local_filename: str, + get_progress_callback: Callable, + ) -> bool: + """Download an explicitly named remote file through MAVFTP.""" logging_info(_("Downloading flight log %(remote)s to %(local)s"), {"remote": remote_filename, "local": local_filename}) try: # Download the actual log file mavftp_instance.cmd_get([remote_filename, local_filename], progress_callback=get_progress_callback) ret = mavftp_instance.process_ftp_reply("OpenFileRO", timeout=0) # No timeout for large log files - if ret.error_code != 0: + if ret.error_code != FtpError.Success: logging_error(_("Failed to download flight log %(remote)s"), {"remote": remote_filename}) ret.display_message() return False @@ -401,6 +639,10 @@ def _download_log_file( logging_info(_("Successfully downloaded flight log to %(local)s"), {"local": local_filename}) return True except Exception as e: # pylint: disable=broad-exception-caught + try: + mavftp_instance.cmd_cancel() + except Exception: # pylint: disable=broad-exception-caught + logging_debug("Could not cancel failed MAVFTP download", exc_info=True) logging_error(_("Failed to download log file: %(error)s"), {"error": str(e)}) return False diff --git a/ardupilot_methodic_configurator/backend_flightcontroller_params.py b/ardupilot_methodic_configurator/backend_flightcontroller_params.py index 255e784ef..e680270ea 100644 --- a/ardupilot_methodic_configurator/backend_flightcontroller_params.py +++ b/ardupilot_methodic_configurator/backend_flightcontroller_params.py @@ -22,6 +22,7 @@ from ardupilot_methodic_configurator import _ from ardupilot_methodic_configurator.backend_flightcontroller_connection import DEVICE_FC_PARAM_FROM_FILE from ardupilot_methodic_configurator.backend_flightcontroller_factory_mavftp import create_mavftp +from ardupilot_methodic_configurator.backend_mavftp import FtpError from ardupilot_methodic_configurator.data_model_flightcontroller_info import FlightControllerInfo from ardupilot_methodic_configurator.data_model_par_dict import Par, ParDict, validate_param_name @@ -256,7 +257,7 @@ def get_params_progress_callback(completion: float) -> None: # Add a file sync operation to ensure the file is completely written. time_sleep(self.FILE_SYNC_DELAY) - if ret.error_code == 0: + if ret.error_code == FtpError.Success: par_dict = ParDict.from_file(complete_param_filename) pdict = {name: data.value for name, data in par_dict.items()} defdict = ParDict.from_file(default_param_filename) diff --git a/ardupilot_methodic_configurator/backend_flightcontroller_protocols.py b/ardupilot_methodic_configurator/backend_flightcontroller_protocols.py index 159860d32..af84f3545 100644 --- a/ardupilot_methodic_configurator/backend_flightcontroller_protocols.py +++ b/ardupilot_methodic_configurator/backend_flightcontroller_protocols.py @@ -35,6 +35,7 @@ if TYPE_CHECKING: from ardupilot_methodic_configurator.backend_flightcontroller_commands import CompassCalibrationUpdate + from ardupilot_methodic_configurator.backend_flightcontroller_files import FlightControllerLogFile # Type alias for MAVLink connection to avoid type checker issues # We define MavlinkConnection as a protocol-like type to represent any MAVLink connection object @@ -294,3 +295,27 @@ def upload_file( ) -> bool: ... def download_last_flight_log(self, local_filename: str, progress_callback: Callable[[int, int], None] | None) -> bool: ... + + def list_bin_log_files(self, remote_directory: str = "/APM/LOGS/") -> list["FlightControllerLogFile"]: ... + + def list_remote_files(self, remote_directory: str = "/APM/LOGS/") -> list["FlightControllerLogFile"]: ... + + def download_bin_log_file( + self, + remote_path: str, + local_filename: str, + progress_callback: Callable[[int, int], None] | None, + ) -> bool: ... + + def download_remote_file( + self, + remote_path: str, + local_filename: str, + progress_callback: Callable[[int, int], None] | None, + ) -> bool: ... + + def make_remote_directory(self, remote_directory: str) -> bool: ... + + def delete_remote_path(self, remote_path: str, is_directory: bool = False) -> bool: ... + + def rename_remote_path(self, remote_path: str, new_remote_path: str) -> bool: ... diff --git a/ardupilot_methodic_configurator/backend_mavftp.py b/ardupilot_methodic_configurator/backend_mavftp.py index aa6219ade..f7611f9cb 100755 --- a/ardupilot_methodic_configurator/backend_mavftp.py +++ b/ardupilot_methodic_configurator/backend_mavftp.py @@ -2,159 +2,120 @@ # PYTHON_ARGCOMPLETE_OK """ -MAVLink File Transfer Protocol support - https://mavlink.io/en/services/ftp.html - duplicated from MAVProxy code. +MAVLink File Transfer Protocol support - https://mavlink.io/en/services/ftp.html. -Original from MAVProxy/MAVProxy/modules/mavproxy_ftp.py +Original from MAVProxy/MAVProxy/modules/mavproxy_ftp.py. -This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator - -SPDX-FileCopyrightText: 2011-2024 Andrew Tridgell, 2024-2026 Amilcar Lucas +SPDX-FileCopyrightText: 2011-2024 Andrew Tridgell, 2024-2025 Amilcar Lucas SPDX-License-Identifier: GPL-3.0-or-later """ +# FLAKE_CLEAN + +import contextlib import logging import os import random import struct import sys +import tempfile import time from argparse import ArgumentParser -from collections.abc import Callable, Generator +from collections.abc import Callable +from dataclasses import dataclass from datetime import datetime -from io import BufferedReader, BufferedWriter +from enum import IntEnum +from io import BufferedRandom, BufferedReader, BufferedWriter from io import BytesIO as SIO # noqa: N814 -from typing import TYPE_CHECKING, Any +from typing import Any, BinaryIO, cast + +try: + import argcomplete + from argcomplete.completers import FilesCompleter + + _ARGCOMPLETE_AVAILABLE = True +except ImportError: + _ARGCOMPLETE_AVAILABLE = False -import argcomplete -from argcomplete.completers import FilesCompleter -from pymavlink import mavutil + # Dummy class to avoid errors when argcomplete is not available + class FilesCompleter: # type: ignore[no-redef] # pylint: disable=too-few-public-methods,missing-class-docstring + """Fallback completer used when argcomplete is unavailable.""" -if TYPE_CHECKING: - from ardupilot_methodic_configurator.backend_flightcontroller_protocols import MavlinkConnection + def __init__(self, *args, **kwargs) -> None: + pass + + +MavlinkObject = Any +Callback = Callable[..., Any] +FileHandle = Any + +from pymavlink import mavutil # noqa: E402 # pylint: disable=too-many-lines +# mypy: disable-error-code="union-attr,arg-type" +from pymavlink.mavftp_op import ( # noqa: E402 + FTP_OP, + OP_Ack, + OP_BurstReadFile, + OP_CalcFileCRC32, + OP_CreateDirectory, + OP_CreateFile, + OP_ListDirectory, + OP_Nack, + OP_None, + OP_OpenFileRO, + OP_OpenFileWO, + OP_ReadFile, + OP_RemoveDirectory, + OP_RemoveFile, + OP_Rename, + OP_ResetSessions, + OP_TerminateSession, + OP_TruncateFile, + OP_WriteFile, +) + + # pylint: disable=invalid-name -# mypy: disable-error-code="union-attr,operator, arg-type" - -# opcodes -OP_None = 0 -OP_TerminateSession = 1 -OP_ResetSessions = 2 -OP_ListDirectory = 3 -OP_OpenFileRO = 4 -OP_ReadFile = 5 -OP_CreateFile = 6 -OP_WriteFile = 7 -OP_RemoveFile = 8 -OP_CreateDirectory = 9 -OP_RemoveDirectory = 10 -OP_OpenFileWO = 11 -OP_TruncateFile = 12 -OP_Rename = 13 -OP_CalcFileCRC32 = 14 -OP_BurstReadFile = 15 -OP_Ack = 128 -OP_Nack = 129 - -# error codes -ERR_None = 0 -ERR_Fail = 1 -ERR_FailErrno = 2 -ERR_InvalidDataSize = 3 -ERR_InvalidSession = 4 -ERR_NoSessionsAvailable = 5 -ERR_EndOfFile = 6 -ERR_UnknownCommand = 7 -ERR_FileExists = 8 -ERR_FileProtected = 9 -ERR_FileNotFound = 10 - -ERR_NoErrorCodeInPayload = 64 -ERR_NoErrorCodeInNack = 65 -ERR_NoFilesystemErrorInPayload = 66 -ERR_InvalidErrorCode = 67 -ERR_PayloadTooLarge = 68 -ERR_InvalidOpcode = 69 -ERR_InvalidArguments = 70 -ERR_PutAlreadyInProgress = 71 -ERR_FailToOpenLocalFile = 72 -ERR_RemoteReplyTimeout = 73 +class FtpError(IntEnum): + """error codes.""" + + Success = 0 + Fail = 1 + FailErrno = 2 + InvalidDataSize = 3 + InvalidSession = 4 + NoSessionsAvailable = 5 + EndOfFile = 6 + UnknownCommand = 7 + FileExists = 8 + FileProtected = 9 + FileNotFound = 10 + NoErrorCodeInPayload = 64 + NoErrorCodeInNack = 65 + NoFilesystemErrorInPayload = 66 + InvalidErrorCode = 67 + PayloadTooLarge = 68 + InvalidOpcode = 69 + InvalidArguments = 70 + PutAlreadyInProgress = 71 + FailToOpenLocalFile = 72 + RemoteReplyTimeout = 73 + HDR_Len = 12 MAX_Payload = 239 -PARAM_HEADER_STRUCT = struct.Struct(" None: - self.seq = seq # Sequence number for the operation. - self.session = session # Session identifier. - self.opcode = opcode # Operation code indicating the type of FTP operation. - self.size = size # Size of the operation. - self.req_opcode = req_opcode # Request operation code. - self.burst_complete: bool = burst_complete # (bool) Flag indicating if the burst transfer is complete. - self.offset: int = offset # Offset for read/write operations. - self.payload = payload # (bytes) Payload for the operation. - - def pack(self) -> bytearray: - """Pack message.""" - ret = struct.pack( - " str: - """String representation of the FTP_OP.""" - plen = 0 - if self.payload is not None: - plen = len(self.payload) - ret = ( - f"OP seq:{self.seq} sess:{self.session} opcode:{self.opcode} req_opcode:{self.req_opcode}" - f" size:{self.size} bc:{self.burst_complete} ofs:{self.offset} plen={plen}" - ) - if plen > 0 and self.payload is not None: - ret += f" [{self.payload[0]}]" - return ret - - def items(self) -> Generator[tuple[str, int | bool | bytes | bytearray | None]]: - """Yield each attribute and its value for the FTP_OP instance. For debugging purposes.""" - yield "seq", self.seq - yield "session", self.session - yield "opcode", self.opcode - yield "size", self.size - yield "req_opcode", self.req_opcode - yield "burst_complete", self.burst_complete - yield "offset", self.offset - yield "payload", self.payload + name: str + is_dir: bool + size_b: int class WriteQueue: # pylint: disable=too-few-public-methods @@ -174,13 +135,13 @@ class ParamData: """A class to manage parameter values and defaults for ArduPilot configuration.""" def __init__(self) -> None: - self.params: list[tuple[bytes, float, int]] = [] # params as (name, value, ptype) - self.defaults: list[tuple[bytes, float, int]] | None = None # defaults as (name, value, ptype) + self.params: list[tuple[bytes, float, type]] = [] # params as (name, value, ptype) + self.defaults: list[tuple[bytes, float, type]] | None = None # defaults as (name, value, ptype) - def add_param(self, name: bytes, value: float, ptype: int) -> None: + def add_param(self, name: bytes, value: float, ptype: type) -> None: self.params.append((name, value, ptype)) - def add_default(self, name: bytes, value: float, ptype: int) -> None: + def add_default(self, name: bytes, value: float, ptype: type) -> None: if self.defaults is None: self.defaults = [] self.defaults.append((name, value, ptype)) @@ -199,12 +160,12 @@ def __init__(self, name: str, s_type: type, default: float) -> None: class MAVFTPSettings: """A collection of MAVFTP settings.""" - def __init__(self, s_vars: list) -> None: + def __init__(self, s_vars: list[MAVFTPSetting | tuple[str, type, float]]) -> None: self._vars: dict[str, MAVFTPSetting] = {} for v in s_vars: self.append(v) - def append(self, v: MAVFTPSetting | tuple) -> None: + def append(self, v: MAVFTPSetting | tuple[str, type, float]) -> None: if isinstance(v, MAVFTPSetting): setting = v else: @@ -212,11 +173,14 @@ def append(self, v: MAVFTPSetting | tuple) -> None: setting = MAVFTPSetting(name, s_type, default) self._vars[setting.name] = setting + def has_setting(self, name: str) -> bool: + """Return whether a setting exists.""" + return name in self._vars + def __getattr__(self, name: str) -> int | float: """Get attribute.""" try: - result: int | float = self._vars[name].type(self._vars[name].value) - return result + return cast("int | float", self._vars[name].type(self._vars[name].value)) except Exception as exc: raise AttributeError from exc @@ -230,54 +194,11 @@ def __setattr__(self, name: str, value: float) -> None: return raise AttributeError - def command(self, args: list[str]) -> None: - """ - Parse and apply 'set NAME VALUE' style arguments to update a setting. - - args is expected to be [NAME, VALUE]. - """ - if len(args) != 2: - logging.error("Usage: ftp set ") - return - - name, value_str = args - - if name not in self._vars: - logging.error("Unknown FTP setting '%s'", name) - return - - setting = self._vars[name] - s_type = setting.type - - # Only numeric (int/float) setting types are supported here, as documented. - if s_type not in (int, float): - logging.error( - "FTP setting '%s' has unsupported type '%s' for CLI update (only int and float are supported)", - name, - getattr(s_type, "__name__", str(s_type)), - ) - return - - try: - # Convert string to the appropriate type (int or float) - converted = s_type(value_str) - except (ValueError, TypeError): - logging.error( - "Invalid value '%s' for FTP setting '%s' (expected %s)", - value_str, - name, - getattr(s_type, "__name__", str(s_type)), - ) - return - - setting.value = converted - logging.info("FTP setting '%s' set to %s", name, converted) - class MAVFTPReturn: """The result of a MAVFTP operation.""" - def __init__( # pylint: disable=too-many-arguments, too-many-positional-arguments + def __init__( # pylint: disable=too-many-arguments self, operation_name: str, error_code: int, @@ -285,7 +206,7 @@ def __init__( # pylint: disable=too-many-arguments, too-many-positional-argumen invalid_error_code: int = 0, invalid_opcode: int = 0, invalid_payload_size: int = 0, - directory_listing: dict[str, int] | None = None, + directory_listing: list[DirectoryEntry] | None = None, ) -> None: self.operation_name = operation_name self.error_code = error_code @@ -296,56 +217,70 @@ def __init__( # pylint: disable=too-many-arguments, too-many-positional-argumen self.directory_listing = directory_listing def display_message(self) -> None: # pylint: disable=too-many-branches, too-many-statements # noqa: C901, PLR0912, PLR0915 - if self.error_code == ERR_None: + if self.error_code == FtpError.Success: logging.info("%s succeeded", self.operation_name) - elif self.error_code == ERR_Fail: + elif self.error_code == FtpError.Fail: logging.error("%s failed, generic error", self.operation_name) - elif self.error_code == ERR_FailErrno: + elif self.error_code == FtpError.FailErrno: logging.error("%s failed, system error %u", self.operation_name, self.system_error) - elif self.error_code == ERR_InvalidDataSize: + elif self.error_code == FtpError.InvalidDataSize: logging.error("%s failed, invalid data size", self.operation_name) - elif self.error_code == ERR_InvalidSession: + elif self.error_code == FtpError.InvalidSession: logging.error("%s failed, session is not currently open", self.operation_name) - elif self.error_code == ERR_NoSessionsAvailable: + elif self.error_code == FtpError.NoSessionsAvailable: logging.error("%s failed, no sessions available", self.operation_name) - elif self.error_code == ERR_EndOfFile: + elif self.error_code == FtpError.EndOfFile: logging.error("%s failed, offset past end of file", self.operation_name) - elif self.error_code == ERR_UnknownCommand: + elif self.error_code == FtpError.UnknownCommand: logging.error("%s failed, unknown command", self.operation_name) - elif self.error_code == ERR_FileExists: + elif self.error_code == FtpError.FileExists: logging.warning("%s failed, file/directory already exists", self.operation_name) - elif self.error_code == ERR_FileProtected: + elif self.error_code == FtpError.FileProtected: logging.warning("%s failed, file/directory is protected", self.operation_name) - elif self.error_code == ERR_FileNotFound: + elif self.error_code == FtpError.FileNotFound: logging.warning("%s failed, file/directory not found", self.operation_name) - elif self.error_code == ERR_NoErrorCodeInPayload: + elif self.error_code == FtpError.NoErrorCodeInPayload: logging.error("%s failed, payload contains no error code", self.operation_name) - elif self.error_code == ERR_NoErrorCodeInNack: + elif self.error_code == FtpError.NoErrorCodeInNack: logging.error("%s failed, no error code", self.operation_name) - elif self.error_code == ERR_NoFilesystemErrorInPayload: + elif self.error_code == FtpError.NoFilesystemErrorInPayload: logging.error("%s failed, file-system error missing in payload", self.operation_name) - elif self.error_code == ERR_InvalidErrorCode: - logging.error("%s failed, invalid error code %u", self.operation_name, self.invalid_error_code) - elif self.error_code == ERR_PayloadTooLarge: - logging.error("%s failed, payload is too long %u", self.operation_name, self.invalid_payload_size) - elif self.error_code == ERR_InvalidOpcode: + elif self.error_code == FtpError.InvalidErrorCode: + logging.error( + "%s failed, invalid error code %u", + self.operation_name, + self.invalid_error_code, + ) + elif self.error_code == FtpError.PayloadTooLarge: + logging.error( + "%s failed, payload is too long %u", + self.operation_name, + self.invalid_payload_size, + ) + elif self.error_code == FtpError.InvalidOpcode: logging.error("%s failed, invalid opcode %u", self.operation_name, self.invalid_opcode) - elif self.error_code == ERR_InvalidArguments: + elif self.error_code == FtpError.InvalidArguments: logging.error("%s failed, invalid arguments", self.operation_name) - elif self.error_code == ERR_PutAlreadyInProgress: + elif self.error_code == FtpError.PutAlreadyInProgress: logging.error("%s failed, put already in progress", self.operation_name) - elif self.error_code == ERR_FailToOpenLocalFile: + elif self.error_code == FtpError.FailToOpenLocalFile: logging.error("%s failed, failed to open local file", self.operation_name) - elif self.error_code == ERR_RemoteReplyTimeout: + elif self.error_code == FtpError.RemoteReplyTimeout: logging.error("%s failed, remote reply timeout", self.operation_name) else: - logging.error("%s failed, unknown error %u in display_message()", self.operation_name, self.error_code) + logging.error( + "%s failed, unknown error %u in display_message()", + self.operation_name, + self.error_code, + ) - if self.directory_listing is not None: + if self.directory_listing is not None and len(self.directory_listing) > 0: total_size = 0 - for name, size in self.directory_listing.items(): - if size == -1: # directories are defined by a size of -1 + for entry in self.directory_listing: + name = entry.name + size = entry.size_b + if entry.is_dir: logging.info(" %s/", name) else: logging.info(" %s\t%u", name, size) @@ -364,9 +299,9 @@ class MAVFTP: # pylint: disable=too-many-instance-attributes Handles file operations such as reading, writing, listing directories, and managing sessions. """ - def __init__( + def __init__( # noqa: PLR0915 pylint: disable=too-many-statements self, - master: "MavlinkConnection", + master: MavlinkObject, target_system: int, target_component: int, settings: MAVFTPSettings | None = None, @@ -391,29 +326,50 @@ def __init__( self.session = 0 self.network = 0 self.last_op: FTP_OP | None = None - self.fh: SIO | BufferedReader | BufferedWriter | None = None + self.fh: SIO | BufferedReader | BufferedWriter | BufferedRandom | None = None self.filename: str | None = None - self.callback: Callable[..., Any] | None = None + self.callback: Callback | None = None self.callback_failure: MAVFTPReturn | None = None - self.callback_progress: Callable[..., Any] | None = None - self.put_callback: Callable[..., Any] | None = None - self.put_callback_progress: Callable[..., Any] | None = None + self.callback_progress: Callback | None = None + self.put_callback: Callback | None = None + self.put_callback_progress: Callback | None = None self.total_size = 0 self.read_gaps: list[tuple[int, int]] = [] self.read_gap_times: dict[tuple[int, int], float] = {} + # FTP permits several ReadFile requests in flight. Track their + # expected response sequences so delayed replies from a prior request + # cannot be dispatched as a current gap repair. + self.pending_read_replies: dict[int, tuple[int, int]] = {} + self.pending_read_requests: dict[int, FTP_OP] = {} self.last_gap_send = 0.0 self.read_retries = 0 self.read_total = 0 - self.remote_file_size: int | None = None + self.remote_file_size: int = 0 self.duplicates = 0 self.last_read = None self.last_burst_read: float | None = None + # The start offset of the active burst. Burst packets are streamed + # with advancing sequence numbers, so their offsets identify whether + # they belong to the current burst after a new burst is requested. + self.pending_burst_offset: int | None = None + self.pending_burst_request: FTP_OP | None = None self.op_start: float | None = None self.dir_offset = 0 self.last_op_time = time.time() self.last_send_time = time.time() self.rtt = 0.5 self.reached_eof = False + self.read_complete = False + # Explicit terminal reply, identified by (request opcode, reply + # sequence). A boolean here lets a delayed reply from an earlier + # operation complete whichever command is currently waiting. + self.completed_reply: tuple[int, int] | None = None + # sequence numbers of in-flight terminate/reset requests, None + # when nothing is outstanding: replies are correlated by + # sequence so a stale or duplicated reply from an earlier + # request cannot mark the current one complete + self.pending_terminate_seq: int | None = None + self.pending_reset_seq: int | None = None self.backlog = 0 self.burst_size: int = int(self.ftp_settings.burst_read_size) self.write_list: set[int] | None = None @@ -424,29 +380,46 @@ def __init__( self.write_idx = 0 self.write_recv_idx = -1 self.write_pending = 0 + # Uploads have several WriteFile requests in flight. Map each + # response sequence to its requested offset. + self.pending_write_replies: dict[int, int] = {} + self.pending_write_requests: dict[int, FTP_OP] = {} self.write_last_send: float | None = None self.open_retries = 0 - self.directory_listing: dict[str, int] = {} + self.list_result: list[DirectoryEntry] = [] + self.list_temp_result: list[DirectoryEntry] = [] + self.requested_size: int = 0 + self.requested_offset: int = 0 + # set per-download by __handle_open_ro_reply: a securely + # created unique staging file, so concurrent MAVFTP clients on + # one host (e.g. parallel simulator test runners) cannot share + # a staging file and its name is not predictable + self.temp_filename: str | None = None + # only close file handles this instance opened itself; cmd_put + # stores a caller-owned handle in self.fh + self.fh_owned = False self.master = master self.target_system = target_system self.target_component = target_component + self.get_result: bytes | None = None + self.done = False # Reset the flight controller FTP state-machine + self.pending_reset_seq = self.seq self.__send(FTP_OP(self.seq, self.session, OP_ResetSessions, 0, 0, 0, 0, None)) self.process_ftp_reply("ResetSessions") - def cmd_ftp(self, args: list[str]) -> MAVFTPReturn: # noqa: PLR0911 pylint: disable=too-many-return-statements, too-many-branches + def cmd_ftp(self, args: list[str]) -> MAVFTPReturn: # noqa: PLR0911 pylint: disable=too-many-branches,too-many-return-statements """FTP operations.""" usage = "Usage: ftp " if len(args) < 1: logging.error(usage) - return MAVFTPReturn("FTP command", ERR_InvalidArguments) + return MAVFTPReturn("FTP command", FtpError.InvalidArguments) if args[0] == "list": return self.cmd_list(args[1:]) if args[0] == "set": - self.ftp_settings.command(args[1:]) - return MAVFTPReturn("FTP command", ERR_None) + return self.cmd_set(args[1:]) if args[0] == "get": return self.cmd_get(args[1:]) if args[0] == "getparams": @@ -468,17 +441,29 @@ def cmd_ftp(self, args: list[str]) -> MAVFTPReturn: # noqa: PLR0911 pylint: dis if args[0] == "cancel": return self.cmd_cancel() logging.error(usage) - return MAVFTPReturn("FTP command", ERR_InvalidArguments) + return MAVFTPReturn("FTP command", FtpError.InvalidArguments) - def __send(self, op: FTP_OP) -> None: - """Send a request.""" - op.seq = self.seq + def __send(self, op: FTP_OP, retry: bool = False) -> None: + """Send a request, preserving its sequence number on retransmission.""" + if not retry: + op.seq = self.seq payload = op.pack() plen = len(payload) if plen < MAX_Payload + HDR_Len: payload.extend(bytearray([0] * ((HDR_Len + MAX_Payload) - plen))) self.master.mav.file_transfer_protocol_send(self.network, self.target_system, self.target_component, payload) - self.seq = (self.seq + 1) % 256 + expected_reply_seq = (op.seq + 1) % 65536 + if op.opcode == OP_BurstReadFile: + self.pending_burst_offset = op.offset + self.pending_burst_request = op + elif op.opcode == OP_ReadFile: + self.pending_read_replies[expected_reply_seq] = (op.offset, op.size) + self.pending_read_requests[expected_reply_seq] = op + elif op.opcode == OP_WriteFile: + self.pending_write_replies[expected_reply_seq] = op.offset + self.pending_write_requests[expected_reply_seq] = op + if not retry: + self.seq = (self.seq + 1) % 65536 self.last_op = op now = time.time() if self.ftp_settings.debug > 1: @@ -486,17 +471,28 @@ def __send(self, op: FTP_OP) -> None: self.last_op_time = time.time() self.last_send_time = now + def __release_staging(self) -> None: + """ + Close and remove this instance's own staging resources. + + Caller-owned handles (cmd_put's fh argument) are left alone. + + """ + if self.fh is not None and self.fh_owned: + with contextlib.suppress(OSError): + self.fh.close() + self.fh_owned = False + if self.temp_filename is not None: + with contextlib.suppress(OSError): + os.unlink(self.temp_filename) + self.temp_filename = None + def __terminate_session(self) -> None: """Terminate current session.""" + self.pending_terminate_seq = self.seq self.__send(FTP_OP(self.seq, self.session, OP_TerminateSession, 0, 0, 0, 0, None)) - # Ensure any open local file handle is properly closed to release OS resources - try: - if self.fh is not None: - self.fh.close() - except Exception as ex: # pylint: disable=broad-except - logging.error("FTP: error closing local file handle: %s", ex) - finally: - self.fh = None + self.__release_staging() + self.fh = None self.filename = None self.write_list = None if self.callback is not None: @@ -516,81 +512,188 @@ def __terminate_session(self) -> None: self.read_gaps = [] self.read_total = 0 self.read_gap_times = {} + self.pending_read_replies = {} + self.pending_read_requests = {} self.last_read = None self.last_burst_read = None + self.pending_burst_offset = None + self.pending_burst_request = None self.reached_eof = False self.backlog = 0 self.duplicates = 0 + self.pending_write_replies = {} + self.pending_write_requests = {} if self.ftp_settings.debug > 0: logging.info("FTP: Terminated session") self.process_ftp_reply("TerminateSession") self.session = (self.session + 1) % 256 + def __has_active_session(self) -> bool: + """Return whether a file operation may have opened a remote session.""" + if self.fh is not None or self.write_list is not None: + return True + return ( + self.filename is not None + and self.last_op is not None + and self.last_op.opcode + in { + OP_OpenFileRO, + OP_BurstReadFile, + OP_ReadFile, + OP_CreateFile, + OP_WriteFile, + } + ) + def cmd_list(self, args: list[str]) -> MAVFTPReturn: """List files.""" + self.list_result = [] + self.list_temp_result = [] if len(args) == 0: dname = "/" elif len(args) == 1: dname = args[0] else: logging.error("Usage: list [directory]") - return MAVFTPReturn("ListDirectory", ERR_InvalidArguments) + return MAVFTPReturn("ListDirectory", FtpError.InvalidArguments) logging.info("Listing %s", dname) enc_dname = bytearray(dname, "ascii") self.total_size = 0 self.dir_offset = 0 - op = FTP_OP(self.seq, self.session, OP_ListDirectory, len(enc_dname), 0, 0, self.dir_offset, enc_dname) + op = FTP_OP( + self.seq, + self.session, + OP_ListDirectory, + len(enc_dname), + 0, + 0, + self.dir_offset, + enc_dname, + ) self.__send(op) - self.directory_listing = {} return self.process_ftp_reply("ListDirectory") - def __handle_list_reply(self, op: FTP_OP, _m: object) -> MAVFTPReturn: + def __handle_list_reply(self, op: FTP_OP, _m: MavlinkObject) -> MAVFTPReturn: """Handle OP_ListDirectory reply.""" if op.opcode == OP_Ack and op.payload is not None: dentries = sorted(op.payload.split(b"\x00")) - # logging.info(dentries) for d in dentries: if len(d) == 0: continue self.dir_offset += 1 try: - d_str = str(d, "ascii") - except (TypeError, UnicodeDecodeError): + dir_entry = str(d, "ascii") + except UnicodeDecodeError as error: + logging.debug(error) continue - if d_str[0] == "D": - name = d_str[1:] - self.directory_listing[name] = -1 # directories are defined by a size of -1 - logging.info(" D %s", name) - elif d_str[0] == "F": - (name, size) = d_str[1:].split("\t") + if dir_entry[0] == "D": + self.list_temp_result.append(DirectoryEntry(name=dir_entry[1:], is_dir=True, size_b=0)) + elif dir_entry[0] == "F": + (name, size_str) = dir_entry[1:].split("\t") 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)) else: - logging.info(d_str) + logging.info(d) # ask for more more = self.last_op more.offset = self.dir_offset self.__send(more) - elif op.opcode == OP_Nack and op.payload is not None and len(op.payload) == 1 and op.payload[0] == ERR_EndOfFile: - logging.info("Total size %.2f kByte", self.total_size / 1024.0) - self.total_size = 0 + elif op.opcode == OP_Nack and op.payload is not None and len(op.payload) == 1 and op.payload[0] == FtpError.EndOfFile: + self.list_result = self.list_temp_result + self.completed_reply = (op.req_opcode, op.seq) + return MAVFTPReturn("ListDirectory", FtpError.Success, directory_listing=self.list_result) else: return self.__decode_ftp_ack_and_nack(op) - return MAVFTPReturn("ListDirectory", ERR_None, directory_listing=self.directory_listing) + return MAVFTPReturn("ListDirectory", FtpError.Success) + + 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.""" + self.get_result = None + self.requested_offset = offset + self.requested_size = size + self.filename = path + self.done = False + + logging.info( + "Getting %s starting at %u reading %u bytes", + path, + self.requested_offset, + self.requested_size, + ) + + self.op_start = time.time() + self.read_total = 0 + self.reached_eof = False + self.burst_size = int(self.ftp_settings.burst_read_size) + if self.burst_size < 1 or self.burst_size > 239: + self.burst_size = 239 + enc_fname = bytearray(path, "ascii") + self.open_retries = 0 + op = FTP_OP(self.seq, self.session, OP_OpenFileRO, len(enc_fname), 0, 0, 0, enc_fname) + self.__send(op) + timeout = time.time() + 5 + while not self.done and time.time() < timeout: + try: + m = self.master.recv_match( + type="FILE_TRANSFER_PROTOCOL", + blocking=True, + timeout=1.0, + ) + if m is None: + self.__idle_task() + continue + timeout = time.time() + 5 + self.__mavlink_packet(m) + except TypeError as e: + logging.error(e) + self.__idle_task() + time.sleep(0.0001) + 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) + return None + + def cmd_set(self, args: list[str]) -> MAVFTPReturn: + """Set a MAVFTP configuration parameter.""" + if len(args) != 2: + logging.error("Usage: set PARAMETERNAME PARAMETERVALUE") + return MAVFTPReturn("Set", FtpError.InvalidArguments) + + setting_name = args[0] + + # Check if parameter exists in settings + if not self.ftp_settings.has_setting(setting_name): + logging.error("Invalid parameter name: %s", setting_name) + return MAVFTPReturn("Set", FtpError.InvalidArguments) + + try: + setting_value = float(args[1]) + except (ValueError, TypeError): + logging.error("Invalid parameter value: %s", args[1]) + return MAVFTPReturn("Set", FtpError.InvalidArguments) + + setattr(self.ftp_settings, setting_name, setting_value) + logging.info("Set %s = %s", setting_name, setting_value) + return MAVFTPReturn("Set", FtpError.Success) def cmd_get( - self, args: list[str], callback: Callable[..., Any] | None = None, progress_callback: Callable[..., Any] | None = None + self, args: list[str], callback: Callback | None = None, progress_callback: Callback | None = None ) -> MAVFTPReturn: """Get file.""" if len(args) == 0 or len(args) > 2: logging.error("Usage: get [FILENAME ]") - return MAVFTPReturn("OpenFileRO", ERR_InvalidArguments) + return MAVFTPReturn("OpenFileRO", FtpError.InvalidArguments) fname = args[0] if len(args) > 1: self.filename = args[1] @@ -608,37 +711,58 @@ 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 - self.remote_file_size = None + self.remote_file_size = 0 enc_fname = bytearray(fname, "ascii") self.open_retries = 0 op = FTP_OP(self.seq, self.session, OP_OpenFileRO, len(enc_fname), 0, 0, 0, enc_fname) self.__send(op) - return MAVFTPReturn("OpenFileRO", ERR_None) + return MAVFTPReturn("OpenFileRO", FtpError.Success) - def __handle_open_ro_reply(self, op: FTP_OP, _m: object) -> MAVFTPReturn: + def __handle_open_ro_reply(self, op: FTP_OP, _m: MavlinkObject) -> 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) + self.session = op.session try: if self.callback is not None or self.filename == "-": self.fh = SIO() else: - self.fh = open(self.filename, "wb") # noqa: SIM115 pylint: disable=consider-using-with + self.__release_staging() + (temp_fd, self.temp_filename) = tempfile.mkstemp(prefix="mavftp_") + try: + self.fh = os.fdopen(temp_fd, "wb+") + except OSError: + os.close(temp_fd) + raise + self.fh_owned = True + self.fh.truncate(0) + self.fh.seek(self.requested_offset) + read = FTP_OP( + self.seq, + self.session, + OP_BurstReadFile, + self.burst_size, + 0, + 0, + self.requested_offset, + None, + ) except Exception as ex: # pylint: disable=broad-except logging.error("FTP: Failed to open local file %s: %s", self.filename, ex) self.__terminate_session() - return MAVFTPReturn("OpenFileRO", ERR_FileNotFound) + return MAVFTPReturn("OpenFileRO", FtpError.FileNotFound) if op.size == 4 and op.payload is not None and len(op.payload) >= 4: self.remote_file_size = op.payload[0] + (op.payload[1] << 8) + (op.payload[2] << 16) + (op.payload[3] << 24) if self.ftp_settings.debug > 0: logging.info("Remote file size: %u", self.remote_file_size) + 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) - return MAVFTPReturn("OpenFileRO", ERR_None) + return MAVFTPReturn("OpenFileRO", FtpError.Success) ret = self.__decode_ftp_ack_and_nack(op) if self.callback is None or self.ftp_settings.debug > 0: @@ -648,24 +772,65 @@ def __handle_open_ro_reply(self, op: FTP_OP, _m: object) -> MAVFTPReturn: def __check_read_finished(self) -> bool: """Check if download has completed.""" - # logging.debug("FTP: check_read_finished: %s %s", self.reached_eof, self.read_gaps) - if self.reached_eof and len(self.read_gaps) == 0 and self.fh is not None and self.op_start is not None: + if self.fh is None: + 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): ofs = self.fh.tell() dt = time.time() - self.op_start rate = (ofs / dt) / 1024.0 + publish_result = True if self.callback is not None: + # The callback owns the downloaded data. This is also used + # for virtual MAVFTP paths such as param.pck?withdefaults=1, + # which must never be treated as local filenames. + publish_result = False self.fh.seek(0) callback_result = self.callback(self.fh) - if isinstance(callback_result, MAVFTPReturn) and callback_result.error_code != ERR_None: + if isinstance(callback_result, MAVFTPReturn) and callback_result.error_code != FtpError.Success: self.callback_failure = callback_result + publish_result = False self.callback = None elif self.filename == "-": self.fh.seek(0) - print(self.fh.read().decode("utf-8")) # noqa: T201 else: - logging.info("Got %u bytes from %s in %.2fs %.1fkByte/s", ofs, self.filename, dt, rate) - self.remote_file_size = None - self.__terminate_session() + logging.info( + "Wrote %u/%u bytes to %s in %.2fs %.1fkByte/s", + self.read_total, + self.requested_size, + self.temp_filename, + dt, + rate, + ) + logging.info( + "terminating with %u out of %u (ofs=%u)", + self.read_total, + self.requested_size, + ofs, + ) + self.done = True + + assert self.fh is not None # noqa: S101 + self.fh.seek(0) + result = self.fh.read() + self.get_result = result[self.requested_offset : self.requested_offset + self.requested_size] + assert self.get_result is not None # noqa: S101 + if len(self.get_result) < self.requested_size: + logging.warning("expected %u, got %u", self.requested_size, len(self.get_result)) + logging.info("read %u bytes", len(self.get_result)) + self.fh.flush() + try: + if publish_result and self.filename and self.filename != "-": + # Move the result to the final location + logging.info("Moving %s to %s", self.temp_filename, self.filename) + with open(self.filename, "wb") as final_file: + final_file.write(self.get_result) + finally: + # terminate the remote session and release the staging + # file even when the destination cannot be written + self.__terminate_session() + self.read_complete = True return True return False @@ -677,19 +842,19 @@ def __write_payload(self, op: FTP_OP) -> None: if self.callback_progress is not None and self.remote_file_size: self.callback_progress(self.read_total / self.remote_file_size) - def __handle_burst_read(self, op: FTP_OP, _m: object) -> MAVFTPReturn: # noqa: C901, PLR0911, PLR0912, PLR0915 pylint: disable=too-many-branches, too-many-statements, too-many-return-statements + def __handle_burst_read(self, op: FTP_OP, _m: MavlinkObject) -> MAVFTPReturn: # noqa: C901, PLR0911, PLR0912, PLR0915 pylint: disable=too-many-statements,too-many-branches,too-many-return-statements """Handle OP_BurstReadFile reply.""" if self.ftp_settings.pkt_loss_tx > 0 and random.uniform(0, 100) < self.ftp_settings.pkt_loss_tx: # noqa: S311 if self.ftp_settings.debug > 0: logging.warning("FTP: dropping TX") - return MAVFTPReturn("BurstReadFile", ERR_Fail) + return MAVFTPReturn("BurstReadFile", FtpError.Fail) if self.fh is None or self.filename is None: if op.session != self.session: # old session - return MAVFTPReturn("BurstReadFile", ERR_InvalidSession) + return MAVFTPReturn("BurstReadFile", FtpError.InvalidSession) logging.warning("FTP: Unexpected burst read reply. Will be discarded") logging.info(op) - return MAVFTPReturn("BurstReadFile", ERR_Fail) + return MAVFTPReturn("BurstReadFile", FtpError.Fail) self.last_burst_read = time.time() size = len(op.payload) if op.payload is not None else 0 if size > self.burst_size: @@ -706,16 +871,26 @@ def __handle_burst_read(self, op: FTP_OP, _m: object) -> MAVFTPReturn: # noqa: self.read_gaps.remove(gap) self.read_gap_times.pop(gap) if self.ftp_settings.debug > 0: - logging.info("FTP: removed gap %u, %u, %u", gap, self.reached_eof, len(self.read_gaps)) + logging.info( + "FTP: removed gap %u, %u, %u", + gap, + self.reached_eof, + len(self.read_gaps), + ) else: if self.ftp_settings.debug > 0: - logging.info("FTP: dup read reply at %u of len %u ofs=%u", op.offset, op.size, self.fh.tell()) + logging.info( + "FTP: dup read reply at %u of len %u ofs=%u", + op.offset, + op.size, + self.fh.tell(), + ) self.duplicates += 1 - return MAVFTPReturn("BurstReadFile", ERR_Fail) + return MAVFTPReturn("BurstReadFile", FtpError.Fail) self.__write_payload(op) self.fh.seek(ofs) if self.__check_read_finished(): - return MAVFTPReturn("BurstReadFile", ERR_None) + return MAVFTPReturn("BurstReadFile", FtpError.Success) elif op.offset > ofs: # we have a gap gap = (ofs, op.offset - ofs) @@ -736,7 +911,7 @@ def __handle_burst_read(self, op: FTP_OP, _m: object) -> MAVFTPReturn: # noqa: if op.size > 0 and op.size < self.burst_size: # a burst complete with non-zero size and less than burst packet size # means EOF - if not self.reached_eof and self.ftp_settings.debug > 0 and self.op_start is not None: + if not self.reached_eof and self.op_start and self.ftp_settings.debug > 0: logging.info( "FTP: EOF at %u with %u gaps t=%.2f", self.fh.tell(), @@ -744,45 +919,60 @@ def __handle_burst_read(self, op: FTP_OP, _m: object) -> MAVFTPReturn: # noqa: time.time() - self.op_start, ) self.reached_eof = True + self.pending_burst_offset = None + self.pending_burst_request = None if self.__check_read_finished(): - return MAVFTPReturn("BurstReadFile", ERR_None) + return MAVFTPReturn("BurstReadFile", FtpError.Success) self.__check_read_send() - return MAVFTPReturn("BurstReadFile", ERR_None) - more = self.last_op + return MAVFTPReturn("BurstReadFile", FtpError.Success) + more = self.pending_burst_request + if more is None: + return MAVFTPReturn("BurstReadFile", FtpError.Fail) more.offset = op.offset + op.size if self.ftp_settings.debug > 0: logging.info("FTP: burst continue at %u %u", more.offset, self.fh.tell()) self.__send(more) - return MAVFTPReturn("BurstReadFile", ERR_None) - if op.opcode == OP_Nack and op.payload is not None: - ecode = op.payload[0] - if ecode in {ERR_EndOfFile, 0}: + # A valid burst reply may be only one part of the transfer. + # It is successful even when it does not complete the read. + return MAVFTPReturn("BurstReadFile", FtpError.Success) + if op.opcode == OP_Nack: + nack_result = self.__decode_ftp_ack_and_nack(op) + if nack_result.error_code == FtpError.EndOfFile: if not self.reached_eof and op.offset > self.fh.tell(): # we lost the last part of the burst if self.ftp_settings.debug > 0: logging.error("FTP: burst lost EOF %u %u", self.fh.tell(), op.offset) - return MAVFTPReturn("BurstReadFile", ERR_Fail) - if not self.reached_eof and self.ftp_settings.debug > 0 and self.op_start is not None: + return MAVFTPReturn("BurstReadFile", FtpError.Fail) + if not self.reached_eof and self.op_start and self.ftp_settings.debug > 0: logging.info( - "FTP: EOF at %u with %u gaps t=%.2f", self.fh.tell(), len(self.read_gaps), time.time() - self.op_start + "FTP: EOF at %u with %u gaps t=%.2f", + self.fh.tell(), + len(self.read_gaps), + time.time() - self.op_start, ) self.reached_eof = True + self.pending_burst_offset = None + self.pending_burst_request = None if self.__check_read_finished(): - return MAVFTPReturn("BurstReadFile", ERR_None) + return MAVFTPReturn("BurstReadFile", FtpError.Success) self.__check_read_send() + return MAVFTPReturn("BurstReadFile", FtpError.Fail) if self.ftp_settings.debug > 0: - logging.info("FTP: burst Nack (ecode:%u): %s", ecode, op) - else: - logging.warning("FTP: burst error: %s", op) - return MAVFTPReturn("BurstReadFile", ERR_Fail) + logging.error("FTP: burst nack: %s", op) + self.__terminate_session() + return nack_result + logging.warning("FTP: burst error: %s", op) + return MAVFTPReturn("BurstReadFile", FtpError.Fail) - def __handle_reply_read(self, op: FTP_OP, _m: object) -> MAVFTPReturn: + def __handle_reply_read(self, op: FTP_OP, _m: MavlinkObject) -> MAVFTPReturn: """Handle OP_ReadFile reply.""" + self.pending_read_replies.pop(op.seq, None) + self.pending_read_requests.pop(op.seq, None) if self.fh is None or self.filename is None: if self.ftp_settings.debug > 0: logging.warning("FTP: Unexpected read reply") logging.warning(op) - return MAVFTPReturn("ReadFile", ERR_Fail) + return MAVFTPReturn("ReadFile", FtpError.Fail) if self.backlog > 0: self.backlog -= 1 if op.opcode == OP_Ack and self.fh is not None: @@ -790,13 +980,26 @@ def __handle_reply_read(self, op: FTP_OP, _m: object) -> MAVFTPReturn: if gap in self.read_gaps: self.read_gaps.remove(gap) self.read_gap_times.pop(gap) + self.pending_read_replies = { + seq: pending_gap for seq, pending_gap in self.pending_read_replies.items() if pending_gap != gap + } + self.pending_read_requests = { + seq: pending_read + for seq, pending_read in self.pending_read_requests.items() + if (pending_read.offset, pending_read.size) != gap + } ofs = self.fh.tell() self.__write_payload(op) self.fh.seek(ofs) if self.ftp_settings.debug > 0: - logging.info("FTP: removed gap %u, %u, %u", gap, self.reached_eof, len(self.read_gaps)) + logging.info( + "FTP: removed gap %u, %u, %u", + gap, + self.reached_eof, + len(self.read_gaps), + ) if self.__check_read_finished(): - return MAVFTPReturn("ReadFile", ERR_None) + return MAVFTPReturn("ReadFile", FtpError.Success) elif op.size < self.burst_size: logging.info("FTP: file size changed to %u", op.offset + op.size) self.__terminate_session() @@ -806,32 +1009,36 @@ def __handle_reply_read(self, op: FTP_OP, _m: object) -> MAVFTPReturn: logging.info("FTP: no gap read %u, %u", gap, len(self.read_gaps)) elif op.opcode == OP_Nack: logging.info("FTP: Read failed with %u gaps %s", len(self.read_gaps), str(op)) + ret = self.__decode_ftp_ack_and_nack(op) self.__terminate_session() + return ret self.__check_read_send() - return MAVFTPReturn("ReadFile", ERR_None) + return MAVFTPReturn("ReadFile", FtpError.Success) def cmd_put( self, args: list[str], - fh: SIO | BufferedReader | BufferedWriter | None = None, - callback: Callable[..., Any] | None = None, - progress_callback: Callable[..., Any] | None = None, + fh: FileHandle = None, + callback: Callback | None = None, + progress_callback: Callback | None = None, ) -> MAVFTPReturn: """Put file.""" if len(args) == 0 or len(args) > 2: logging.error("Usage: put [FILENAME ]") - return MAVFTPReturn("CreateFile", ERR_InvalidArguments) + return MAVFTPReturn("CreateFile", FtpError.InvalidArguments) if self.write_list is not None: logging.error("FTP: put already in progress") - return MAVFTPReturn("CreateFile", ERR_PutAlreadyInProgress) + return MAVFTPReturn("CreateFile", FtpError.PutAlreadyInProgress) fname = args[0] self.fh = fh + self.fh_owned = False if self.fh is None: try: self.fh = open(fname, "rb") # noqa: SIM115 pylint: disable=consider-using-with + self.fh_owned = True except Exception as ex: # pylint: disable=broad-exception-caught logging.error("FTP: Failed to open %s: %s", fname, ex) - return MAVFTPReturn("CreateFile", ERR_FailToOpenLocalFile) + return MAVFTPReturn("CreateFile", FtpError.FailToOpenLocalFile) if len(args) > 1: self.filename = args[1] else: @@ -867,7 +1074,7 @@ def cmd_put( enc_fname = bytearray(self.filename, "ascii") op = FTP_OP(self.seq, self.session, OP_CreateFile, len(enc_fname), 0, 0, 0, enc_fname) self.__send(op) - return MAVFTPReturn("CreateFile", ERR_None) + return MAVFTPReturn("CreateFile", FtpError.Success) def __put_finished(self, flen: int) -> None: """Finish a put.""" @@ -877,30 +1084,42 @@ def __put_finished(self, flen: int) -> None: if self.put_callback is not None: self.put_callback(flen) self.put_callback = None - elif self.op_start is not None: + elif self.op_start: dt = time.time() - self.op_start rate = (flen / dt) / 1024.0 - logging.info("Put %u bytes to %s file in %.2fs %.1fkByte/s", flen, self.filename, dt, rate) + logging.info( + "Put %u bytes to %s file in %.2fs %.1fkByte/s", + flen, + self.filename, + dt, + rate, + ) - def __handle_create_file_reply(self, op: FTP_OP, _m: object) -> MAVFTPReturn: + def __handle_create_file_reply(self, op: FTP_OP, _m: MavlinkObject) -> MAVFTPReturn: """Handle OP_CreateFile reply.""" if self.fh is None: self.__terminate_session() - return MAVFTPReturn("CreateFile", ERR_FileNotFound) + return MAVFTPReturn("CreateFile", FtpError.FileNotFound) if op.opcode == OP_Ack: - self.__send_more_writes() + self.session = op.session + self.__send_more_writes(op) else: ret = self.__decode_ftp_ack_and_nack(op) self.__terminate_session() return ret - return MAVFTPReturn("CreateFile", ERR_None) + return MAVFTPReturn("CreateFile", FtpError.Success) - def __send_more_writes(self) -> None: + def __send_more_writes(self, completed_reply: FTP_OP | None = None) -> None: """Send some more writes.""" if self.write_list is None or len(self.write_list) == 0: # all done self.__put_finished(self.write_file_size) self.__terminate_session() + if completed_reply is not None: + self.completed_reply = ( + completed_reply.req_opcode, + completed_reply.seq, + ) return now = time.time() @@ -908,8 +1127,6 @@ def __send_more_writes(self) -> None: # we seem to have lost a block of replies self.write_pending = max(0, self.write_pending - 1) - if self.write_list is None: - return n = min(self.ftp_settings.write_qsize - self.write_pending, len(self.write_list)) for _i in range(n): # send in round-robin, skipping any that have been acked @@ -917,30 +1134,55 @@ def __send_more_writes(self) -> None: while idx not in self.write_list: idx = (idx + 1) % self.write_total ofs = idx * self.write_block_size - self.fh.seek(ofs) - data = self.fh.read(self.write_block_size) - write = FTP_OP(self.seq, self.session, OP_WriteFile, len(data), 0, 0, ofs, bytearray(data)) - self.__send(write) + write = next( + (pending_write for pending_write in self.pending_write_requests.values() if pending_write.offset == ofs), + None, + ) + if write is None: + self.fh.seek(ofs) + data = self.fh.read(self.write_block_size) + write = FTP_OP( + self.seq, + self.session, + OP_WriteFile, + len(data), + 0, + 0, + ofs, + bytearray(data), + ) + self.__send(write) + else: + self.__send(write, retry=True) self.write_idx = (idx + 1) % self.write_total self.write_pending += 1 self.write_last_send = now - def __handle_write_reply(self, op: FTP_OP, _m: object) -> MAVFTPReturn: + def __handle_write_reply(self, op: FTP_OP, _m: MavlinkObject) -> MAVFTPReturn: """Handle OP_WriteFile reply.""" + expected_offset = self.pending_write_replies.pop(op.seq, None) + self.pending_write_requests.pop(op.seq, None) + if expected_offset is not None: + self.pending_write_replies = { + seq: offset for seq, offset in self.pending_write_replies.items() if offset != expected_offset + } + self.pending_write_requests = { + seq: pending_write + for seq, pending_write in self.pending_write_requests.items() + if pending_write.offset != expected_offset + } if self.fh is None: self.__terminate_session() - return MAVFTPReturn("WriteFile", ERR_FileNotFound) + return MAVFTPReturn("WriteFile", FtpError.FileNotFound) if op.opcode != OP_Ack: logging.error("FTP: Write failed") + ret = self.__decode_ftp_ack_and_nack(op) self.__terminate_session() - return MAVFTPReturn("WriteFile", ERR_FileProtected) + return ret # assume the FTP server processes the blocks sequentially. This means # when we receive an ack that any blocks between the last ack and this # one have been lost - if self.write_total == 0: - self.__terminate_session() - return MAVFTPReturn("WriteFile", ERR_None) idx = op.offset // self.write_block_size count = (idx - self.write_recv_idx) % self.write_total @@ -950,14 +1192,14 @@ def __handle_write_reply(self, op: FTP_OP, _m: object) -> MAVFTPReturn: self.write_acks += 1 if self.put_callback_progress: self.put_callback_progress(self.write_acks / float(self.write_total)) - self.__send_more_writes() - return MAVFTPReturn("WriteFile", ERR_None) + self.__send_more_writes(op) + return MAVFTPReturn("WriteFile", FtpError.Success) def cmd_rm(self, args: list[str]) -> MAVFTPReturn: """Remove file.""" if len(args) != 1: logging.error("Usage: rm [FILENAME]") - return MAVFTPReturn("RemoveFile", ERR_InvalidArguments) + return MAVFTPReturn("RemoveFile", FtpError.InvalidArguments) fname = args[0] logging.info("Removing file %s", fname) enc_fname = bytearray(fname, "ascii") @@ -969,15 +1211,24 @@ def cmd_rmdir(self, args: list[str]) -> MAVFTPReturn: """Remove directory.""" if len(args) != 1: logging.error("Usage: rmdir [DIRECTORYNAME]") - return MAVFTPReturn("RemoveDirectory", ERR_InvalidArguments) + return MAVFTPReturn("RemoveDirectory", FtpError.InvalidArguments) dname = args[0] logging.info("Removing directory %s", dname) enc_dname = bytearray(dname, "ascii") - op = FTP_OP(self.seq, self.session, OP_RemoveDirectory, len(enc_dname), 0, 0, 0, enc_dname) + op = FTP_OP( + self.seq, + self.session, + OP_RemoveDirectory, + len(enc_dname), + 0, + 0, + 0, + enc_dname, + ) self.__send(op) return self.process_ftp_reply("RemoveDirectory") - def __handle_remove_reply(self, op: FTP_OP, _m: object) -> MAVFTPReturn: + def __handle_remove_reply(self, op: FTP_OP, _m: MavlinkObject) -> MAVFTPReturn: """Handle remove reply.""" return self.__decode_ftp_ack_and_nack(op) @@ -985,7 +1236,7 @@ def cmd_rename(self, args: list[str]) -> MAVFTPReturn: """Rename file or directory.""" if len(args) < 2: logging.error("Usage: rename [OLDNAME NEWNAME]") - return MAVFTPReturn("Rename", ERR_InvalidArguments) + return MAVFTPReturn("Rename", FtpError.InvalidArguments) name1 = args[0] name2 = args[1] logging.info("Renaming %s to %s", name1, name2) @@ -996,7 +1247,7 @@ def cmd_rename(self, args: list[str]) -> MAVFTPReturn: self.__send(op) return self.process_ftp_reply("Rename") - def __handle_rename_reply(self, op: FTP_OP, _m: object) -> MAVFTPReturn: + def __handle_rename_reply(self, op: FTP_OP, _m: MavlinkObject) -> MAVFTPReturn: """Handle rename reply.""" return self.__decode_ftp_ack_and_nack(op) @@ -1004,7 +1255,7 @@ def cmd_mkdir(self, args: list[str]) -> MAVFTPReturn: """Make directory.""" if len(args) != 1: logging.error("Usage: mkdir NAME") - return MAVFTPReturn("CreateDirectory", ERR_InvalidArguments) + return MAVFTPReturn("CreateDirectory", FtpError.InvalidArguments) name = args[0] logging.info("Creating directory %s", name) enc_name = bytearray(name, "ascii") @@ -1012,7 +1263,7 @@ def cmd_mkdir(self, args: list[str]) -> MAVFTPReturn: self.__send(op) return self.process_ftp_reply("CreateDirectory") - def __handle_mkdir_reply(self, op: FTP_OP, _m: object) -> MAVFTPReturn: + def __handle_mkdir_reply(self, op: FTP_OP, _m: MavlinkObject) -> MAVFTPReturn: """Handle mkdir reply.""" return self.__decode_ftp_ack_and_nack(op) @@ -1020,33 +1271,42 @@ def cmd_crc(self, args: list[str]) -> MAVFTPReturn: """Get file crc.""" if len(args) != 1: logging.error("Usage: crc [NAME]") - return MAVFTPReturn("CalcFileCRC32", ERR_InvalidArguments) + return MAVFTPReturn("CalcFileCRC32", FtpError.InvalidArguments) name = args[0] self.filename = name self.op_start = time.time() logging.info("Getting CRC for %s", name) enc_name = bytearray(name, "ascii") - op = FTP_OP(self.seq, self.session, OP_CalcFileCRC32, len(enc_name), 0, 0, 0, bytearray(enc_name)) + op = FTP_OP( + self.seq, + self.session, + OP_CalcFileCRC32, + len(enc_name), + 0, + 0, + 0, + bytearray(enc_name), + ) self.__send(op) return self.process_ftp_reply("CalcFileCRC32") - def __handle_crc_reply(self, op: FTP_OP, _m: object) -> MAVFTPReturn: + def __handle_crc_reply(self, op: FTP_OP, _m: MavlinkObject) -> MAVFTPReturn: """Handle crc reply.""" if op.opcode == OP_Ack and op.size == 4: (crc,) = struct.unpack(" MAVFTPReturn: """Cancel any pending op.""" self.__terminate_session() - return MAVFTPReturn("TerminateSession", ERR_None) + return MAVFTPReturn("TerminateSession", FtpError.Success) def cmd_status(self) -> MAVFTPReturn: """Show status.""" - if self.fh is None or self.op_start is None: + if self.fh is None: logging.info("No transfer in progress") else: ofs = self.fh.tell() @@ -1060,45 +1320,70 @@ def cmd_status(self) -> MAVFTPReturn: self.read_retries, rate, ) - return MAVFTPReturn("Status", ERR_None) + return MAVFTPReturn("Status", FtpError.Success) - def __op_parse(self, m: Any) -> FTP_OP: # noqa: ANN401 + def __op_parse(self, m: MavlinkObject) -> FTP_OP: """Parse a FILE_TRANSFER_PROTOCOL msg.""" hdr = bytearray(m.payload[0:12]) (seq, session, opcode, size, req_opcode, burst_complete, _pad, offset) = struct.unpack(" MAVFTPReturn: # noqa: ANN401, PLR0911 pylint: disable=too-many-branches, too-many-return-statements + def __reply_matches_active_request(self, op: FTP_OP) -> bool: + """Return whether a reply can safely be dispatched to the active operation.""" + if op.req_opcode == OP_BurstReadFile: + return self.pending_burst_offset is not None and op.offset >= self.pending_burst_offset + if op.req_opcode == OP_ReadFile: + return op.seq in self.pending_read_replies + if op.req_opcode == OP_WriteFile: + return op.seq in self.pending_write_replies + + return bool( + self.last_op is not None and op.req_opcode == self.last_op.opcode and op.seq == (self.last_op.seq + 1) % 65536 + ) + + def __mavlink_packet(self, m: MavlinkObject) -> MAVFTPReturn: # noqa: C901, PLR0911 pylint: disable=too-many-branches, too-many-return-statements """Handle a mavlink packet.""" operation_name = "mavlink_packet" mtype = m.get_type() if mtype != "FILE_TRANSFER_PROTOCOL": logging.error("FTP: Unexpected MAVLink message type %s", mtype) - return MAVFTPReturn(operation_name, ERR_Fail) + return MAVFTPReturn(operation_name, FtpError.Fail) if m.target_system != self.master.source_system or m.target_component != self.master.source_component: logging.info( - "FTP: wrong MAVLink target %u component %u. Will discard message", m.target_system, m.target_component + "FTP: wrong MAVLink target %u component %u. Will discard message", + m.target_system, + m.target_component, ) - return MAVFTPReturn(operation_name, ERR_Fail) + return MAVFTPReturn(operation_name, FtpError.Fail) op = self.__op_parse(m) now = time.time() dt = now - self.last_op_time if self.ftp_settings.debug > 1: logging.info("FTP: < %s dt=%.2f", op, dt) - if op.session != self.session: + allocated_session_reply = op.opcode == OP_Ack and op.req_opcode in {OP_OpenFileRO, OP_CreateFile} + if op.session != self.session and not allocated_session_reply: if self.ftp_settings.debug > 0: - logging.warning("FTP: wrong session replied %u expected %u. Will discard message", op.session, self.session) - return MAVFTPReturn(operation_name, ERR_InvalidSession) + logging.warning( + "FTP: wrong session replied %u expected %u. Will discard message", + op.session, + self.session, + ) + return MAVFTPReturn(operation_name, FtpError.InvalidSession) self.last_op_time = now if self.ftp_settings.pkt_loss_rx > 0 and random.uniform(0, 100) < self.ftp_settings.pkt_loss_rx: # noqa: S311 if self.ftp_settings.debug > 1: logging.warning("FTP: dropping packet RX") - return MAVFTPReturn(operation_name, ERR_Fail) + return MAVFTPReturn(operation_name, FtpError.Fail) + + if not self.__reply_matches_active_request(op): + if self.ftp_settings.debug > 0: + logging.warning("FTP: stale reply. Will discard message: %s", op) + return MAVFTPReturn(operation_name, FtpError.Fail) - if op.req_opcode == self.last_op.opcode and op.seq == (self.last_op.seq + 1) % 256: + if self.last_op is not None and op.req_opcode == self.last_op.opcode and op.seq == (self.last_op.seq + 1) % 65536: self.rtt = max(min(self.rtt, dt), 0.01) if op.req_opcode == OP_ListDirectory: @@ -1110,7 +1395,15 @@ def __mavlink_packet(self, m: Any) -> MAVFTPReturn: # noqa: ANN401, PLR0911 pyl if op.req_opcode == OP_ResetSessions: return self.__handle_reset_sessions_reply(op, m) if op.req_opcode in {OP_None, OP_TerminateSession}: - return MAVFTPReturn(operation_name, ERR_None) # ignore reply + if ( + op.req_opcode == OP_TerminateSession + and self.pending_terminate_seq is not None + and op.seq == (self.pending_terminate_seq + 1) % 65536 + ): + # Ack or Nack (InvalidSession means it was already + # closed): the handshake has been answered + self.pending_terminate_seq = None + return MAVFTPReturn(operation_name, FtpError.Success) # ignore reply if op.req_opcode == OP_CreateFile: return self.__handle_create_file_reply(op, m) if op.req_opcode == OP_WriteFile: @@ -1127,15 +1420,32 @@ def __mavlink_packet(self, m: Any) -> MAVFTPReturn: # noqa: ANN401, PLR0911 pyl return self.__handle_crc_reply(op, m) logging.info("FTP Unknown %s", str(op)) - return MAVFTPReturn(operation_name, ERR_InvalidOpcode) + return MAVFTPReturn(operation_name, FtpError.InvalidOpcode) def __send_gap_read(self, g: tuple[int, int]) -> None: """Send a read for a gap.""" (offset, length) = g if self.ftp_settings.debug > 0: - logging.info("FTP: Gap read of %u at %u rem=%u blog=%u", length, offset, len(self.read_gaps), self.backlog) - read = FTP_OP(self.seq, self.session, OP_ReadFile, length, 0, 0, offset, None) - self.__send(read) + logging.info( + "FTP: Gap read of %u at %u rem=%u blog=%u", + length, + offset, + len(self.read_gaps), + self.backlog, + ) + read = next( + ( + pending_read + for pending_read in self.pending_read_requests.values() + if (pending_read.offset, pending_read.size) == g + ), + None, + ) + if read is None: + read = FTP_OP(self.seq, self.session, OP_ReadFile, length, 0, 0, offset, None) + self.__send(read) + else: + self.__send(read, retry=True) self.read_gaps.remove(g) self.read_gaps.append(g) self.last_gap_send = time.time() @@ -1193,11 +1503,7 @@ def __idle_task(self) -> bool: return False # Not idle yet if self.ftp_settings.debug > 0: logging.info("FTP: retry open") - send_op = self.last_op - self.__send(FTP_OP(self.seq, self.session, OP_TerminateSession, 0, 0, 0, 0, None)) - self.session = (self.session + 1) % 256 - send_op.session = self.session - self.__send(send_op) + self.__send(self.last_op, retry=True) if len(self.read_gaps) == 0 and self.last_burst_read is None and self.write_list is None: return self.__last_send_time_was_more_than_idle_detection_time_ago(now) @@ -1214,8 +1520,14 @@ def __idle_task(self) -> bool: dt = now - self.last_burst_read self.last_burst_read = now if self.ftp_settings.debug > 0: - logging.info("FTP: Retry read at %u rtt=%.2f dt=%.2f", self.fh.tell(), self.rtt, dt) - self.__send(FTP_OP(self.seq, self.session, OP_BurstReadFile, self.burst_size, 0, 0, self.fh.tell(), None)) + logging.info( + "FTP: Retry read at %u rtt=%.2f dt=%.2f", + self.fh.tell(), + self.rtt, + dt, + ) + if self.pending_burst_request is not None: + self.__send(self.pending_burst_request, retry=True) self.read_retries += 1 # see if we can fill gaps @@ -1229,37 +1541,133 @@ def __idle_task(self) -> bool: def __last_send_time_was_more_than_idle_detection_time_ago(self, now: float) -> bool: return self.last_send_time is not None and now - self.last_send_time > float(self.ftp_settings.idle_detection_time) - def __handle_reset_sessions_reply(self, op: FTP_OP, _m: object) -> MAVFTPReturn: + def __handle_reset_sessions_reply(self, op: FTP_OP, _m: MavlinkObject) -> MAVFTPReturn: """Handle reset sessions reply.""" + if self.pending_reset_seq is not None and op.seq == (self.pending_reset_seq + 1) % 65536: + # Ack or Nack, the handshake has been answered; the decoded + # result below still reports a Nack to the caller + self.pending_reset_seq = None return self.__decode_ftp_ack_and_nack(op) - def process_ftp_reply(self, operation_name: str, timeout: float = 5) -> MAVFTPReturn: + def process_ftp_reply( # noqa: PLR0915 pylint: disable=too-many-branches, too-many-locals + self, operation_name: str, timeout: float = 5 + ) -> MAVFTPReturn: """Execute an FTP operation that requires processing a MAVLink response.""" start_time = time.time() - ret = MAVFTPReturn(operation_name, ERR_Fail) + ret = MAVFTPReturn(operation_name, FtpError.Fail) recv_timeout = 0.1 assert ( # noqa: S101 timeout == 0 or timeout > float(self.ftp_settings.idle_detection_time) ), "timeout must be > settings.idle_detection_time" - assert recv_timeout < self.ftp_settings.retry_time, "recv_timeout must be < settings.retry_time" # noqa: S101 - + if not recv_timeout < self.ftp_settings.retry_time: + msg = "recv_timeout must be < settings.retry_time" + raise ValueError(msg) + + # A read operation reports its completion positively: EOF seen + # with no gaps outstanding (__check_read_finished). Return as + # soon as the transfer and its session-terminate handshake are + # both done instead of waiting out idle_detection_time of link + # silence - on a fast or simulated link the quiet-period tail + # dwarfs the transfer itself. The flag is only ever set while a + # reply-processing loop runs (this one, or read()'s own driver + # loop, which does not use this method), so clearing it here + # cannot lose a completion; idle detection remains the fallback for a lost + # terminate reply and for operations with no positive + # completion signal. + self.read_complete = False + self.completed_reply = None while True: # an FTP operation can have multiple responses m = self.master.recv_match(type=["FILE_TRANSFER_PROTOCOL"], timeout=recv_timeout) if m is not None: if operation_name == "TerminateSession": - # self.silently_discard_terminate_session_reply() - ret = MAVFTPReturn(operation_name, ERR_None) + # consume only the terminate reply itself: stale + # replies from the aborted transfer must not reach + # their handlers, which can re-enter + # __terminate_session from this very wait + op = self.__op_parse(m) + if ( + m.target_system == self.master.source_system # pylint: disable=too-many-boolean-expressions + and m.target_component == self.master.source_component + and op.session == self.session + and op.req_opcode == OP_TerminateSession + and self.pending_terminate_seq is not None + and op.seq == (self.pending_terminate_seq + 1) % 65536 + ): + self.pending_terminate_seq = None + ret = MAVFTPReturn(operation_name, FtpError.Success) else: - ret = self.__mavlink_packet(m) - if self.callback_failure is not None: - ret = self.callback_failure - break + # Keep a result only from the request that was current + # when this reply arrived. Packet handlers must still + # see stale replies so they can maintain their own + # state, but retaining their result would make idle + # fallback return a previous operation's outcome. + op = self.__op_parse(m) + reply_matches_last_op = ( + self.last_op is not None + and op.req_opcode == self.last_op.opcode + and op.seq == (self.last_op.seq + 1) % 65536 + and op.session == self.session + ) + reply_matches_active_request = op.session == self.session and self.__reply_matches_active_request(op) + packet_ret = self.__mavlink_packet(m) + # An upload's final CreateFile/WriteFile reply starts a + # TerminateSession request before returning here. Its + # result is therefore valid even though last_op is now + # the terminate request. + completed_upload = ( + operation_name.lower() == "put" + and self.completed_reply is not None + and self.completed_reply[0] in {OP_CreateFile, OP_WriteFile} + ) + if ( + reply_matches_last_op + or completed_upload + or (reply_matches_active_request and packet_ret.error_code != FtpError.Success) + ): + ret = packet_ret + if self.callback_failure is not None and operation_name != "TerminateSession": + callback_failure = self.callback_failure + self.callback_failure = None + return callback_failure + # Completion is scoped to the terminal reply that produced it. + # This prevents a delayed ListDirectory EOF from completing a + # following RemoveFile or a subsequent list operation. + reply_complete = False + if self.completed_reply is not None and self.last_op is not None: + completed_opcode, completed_seq = self.completed_reply + reply_complete = completed_opcode == self.last_op.opcode and completed_seq == (self.last_op.seq + 1) % 65536 + # A completed upload sends TerminateSession immediately after + # its final CreateFile/WriteFile reply. It is explicitly + # scoped to the upload reply type, rather than being a global + # latch that any packet handler can set. + if ( + not reply_complete + and completed_opcode in {OP_CreateFile, OP_WriteFile} + and operation_name.lower() == "put" + ): + reply_complete = True + if not reply_complete and self.pending_terminate_seq is None: + reply_complete = self.read_complete + if not reply_complete: + reply_complete = operation_name == "TerminateSession" + if not reply_complete and operation_name == "ResetSessions": + reply_complete = self.pending_reset_seq is None + if reply_complete: + break if self.__idle_task(): + if self.last_burst_read is not None and not self.read_complete: + ret = MAVFTPReturn(operation_name, FtpError.RemoteReplyTimeout) break if timeout > 0 and time.time() - start_time > timeout: # pylint: disable=chained-comparison logging.error("FTP: timed out after %f seconds", time.time() - start_time) - ret = MAVFTPReturn(operation_name, ERR_RemoteReplyTimeout) + ret = MAVFTPReturn(operation_name, FtpError.RemoteReplyTimeout) break + if ( + ret.error_code == FtpError.RemoteReplyTimeout + and operation_name != "TerminateSession" + and self.__has_active_session() + ): + self.__terminate_session() return ret def __decode_ftp_ack_and_nack(self, op: FTP_OP, operation_name: str = "") -> MAVFTPReturn: @@ -1287,36 +1695,39 @@ def __decode_ftp_ack_and_nack(self, op: FTP_OP, operation_name: str = "") -> MAV op_ret_name = operation_name or operation_name_dict.get(op.req_opcode, "Unknown") len_payload = len(op.payload) if op.payload is not None else 0 if op.opcode == OP_Ack: - error_code = ERR_None + error_code = FtpError.Success elif op.opcode == OP_Nack: if len_payload <= 0: - error_code = ERR_NoErrorCodeInPayload - elif len_payload == 1 and op.payload is not None: - error_code = op.payload[0] - if error_code == ERR_None: - error_code = ERR_NoErrorCodeInNack - elif error_code == ERR_FailErrno: - error_code = ERR_NoFilesystemErrorInPayload - elif error_code not in { - ERR_Fail, - ERR_InvalidDataSize, - ERR_InvalidSession, - ERR_NoSessionsAvailable, - ERR_EndOfFile, - ERR_UnknownCommand, - ERR_FileExists, - ERR_FileProtected, - ERR_FileNotFound, - }: + error_code = FtpError.NoErrorCodeInPayload + elif op.payload is not None and len_payload == 1: + try: + error_code = FtpError(op.payload[0]) + except ValueError: + error_code = op.payload[0] + if error_code == FtpError.Success: + error_code = FtpError.NoErrorCodeInNack + elif error_code == FtpError.FailErrno: + error_code = FtpError.NoFilesystemErrorInPayload + elif error_code not in [ + FtpError.Fail, + FtpError.InvalidDataSize, + FtpError.InvalidSession, + FtpError.NoSessionsAvailable, + FtpError.EndOfFile, + FtpError.UnknownCommand, + FtpError.FileExists, + FtpError.FileProtected, + FtpError.FileNotFound, + ]: 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: system_error = op.payload[1] - error_code = ERR_FailErrno + error_code = FtpError.FailErrno else: - error_code = ERR_PayloadTooLarge + error_code = FtpError.PayloadTooLarge else: - error_code = ERR_InvalidOpcode + error_code = FtpError.InvalidOpcode return MAVFTPReturn( op_ret_name, error_code, @@ -1327,79 +1738,102 @@ def __decode_ftp_ack_and_nack(self, op: FTP_OP, operation_name: str = "") -> MAV ) @staticmethod - def __decode_param_record( # pylint: disable=too-many-locals - data: bytes, - offset: int, - last_name: bytes, - with_defaults: bool, - pdata: ParamData, - ) -> tuple[bytes, int] | None: - """Decode one packed-parameter record and append it to ``pdata``.""" - if len(data) - offset < 2: - logging.error("paramftp: truncated parameter header") - return None - - ptype, plen = struct.unpack_from("> 4) & 0x0F - has_default = with_defaults and (flags & 1) != 0 - ptype &= 0x0F - if ptype not in PARAM_TYPE_FORMATS: - logging.error("paramftp: bad type 0x%x", ptype) - return None - - type_len, type_format = PARAM_TYPE_FORMATS[ptype] - name_len = ((plen >> 4) & 0x0F) + 1 - common_len = plen & 0x0F - value_len = type_len * (2 if has_default else 1) - record_len = 2 + name_len + value_len - record_end = offset + record_len - if len(data) < record_end: - logging.error("paramftp: truncated parameter record") - return None - if common_len > len(last_name): - logging.error("paramftp: invalid shared parameter name prefix length %u", common_len) - return None - - name_start = offset + 2 - value_start = name_start + name_len - name = last_name[:common_len] + data[name_start:value_start] - unpack_format = "<" + type_format - if has_default: - value, default = struct.unpack_from("<" + type_format * 2, data, value_start) - pdata.add_param(name, value, ptype) - pdata.add_default(name, default, ptype) - else: - (value,) = struct.unpack_from(unpack_format, data, value_start) - pdata.add_param(name, value, ptype) - if with_defaults: - pdata.add_default(name, value, ptype) - return name, record_end - - @staticmethod - def ftp_param_decode(data: bytes) -> ParamData | None: + def ftp_param_decode(data: bytes) -> ParamData | None: # noqa: PLR0911, PLR0915 pylint: disable=too-many-locals,too-many-statements,too-many-branches,too-many-return-statements """Decode parameter data, returning ParamData.""" - if len(data) < PARAM_HEADER_STRUCT.size: + pdata = ParamData() + + magic = 0x671B + magic_defaults = 0x671C + if len(data) < 6: logging.error("paramftp: Not enough data do decode, only %u bytes", len(data)) return None - magic, num_params, _total_params = PARAM_HEADER_STRUCT.unpack_from(data) - if magic not in {PARAM_MAGIC, PARAM_MAGIC_WITH_DEFAULTS}: - logging.error("paramftp: bad magic 0x%x expected 0x%x", magic, PARAM_MAGIC) + magic2, num_params, total_params = struct.unpack(" total_params: + logging.error( + "paramftp: parameter count %u exceeds total count %u", + num_params, + total_params, + ) + return None + with_defaults = magic2 == magic_defaults + data = data[6:] + + # mapping of data type to type length and format + data_types = { + 1: (1, "b"), + 2: (2, "h"), + 3: (4, "i"), + 4: (4, "f"), + } count = 0 + pad_byte = 0 last_name = b"" - offset = PARAM_HEADER_STRUCT.size - while offset < len(data): - while offset < len(data) and data[offset] == 0: - offset += 1 - if offset == len(data): + while True: + while len(data) > 0 and data[0] == pad_byte: + data = data[1:] # skip pad bytes + + if len(data) == 0: break - decoded = MAVFTP.__decode_param_record(data, offset, last_name, with_defaults, pdata) - if decoded is None: + if len(data) < 2: + logging.error("paramftp: truncated parameter header") return None - last_name, offset = decoded + + ptype, plen = struct.unpack("> 4) & 0x0F + has_default = with_defaults and (flags & 1) != 0 + ptype &= 0x0F + + if ptype not in data_types: + logging.error("paramftp: bad type 0x%x", ptype) + return None + + (type_len, type_format) = data_types[ptype] + default_len = type_len if has_default else 0 + + name_len = ((plen >> 4) & 0x0F) + 1 + common_len = plen & 0x0F + value_len = type_len + default_len + record_len = 2 + name_len + value_len + if len(data) < record_len: + logging.error("paramftp: truncated parameter record") + return None + if common_len > len(last_name): + logging.error( + "paramftp: invalid shared parameter name prefix length %u", + common_len, + ) + return None + name = last_name[0:common_len] + data[2 : 2 + name_len] + if len(name) > 16: + logging.error("paramftp: parameter name is too long (%u bytes)", len(name)) + return None + try: + name.decode("utf-8") + except UnicodeDecodeError: + logging.error("paramftp: parameter name is not valid UTF-8") + return None + vdata = data[2 + name_len : record_len] + last_name = name + data = data[record_len:] + if with_defaults: + if has_default: + ( + v1, + v2, + ) = struct.unpack("<" + type_format + type_format, vdata) + pdata.add_param(name, v1, ptype) + pdata.add_default(name, v2, ptype) + else: + (v,) = struct.unpack("<" + type_format, vdata) + pdata.add_param(name, v, ptype) + pdata.add_default(name, v, ptype) + else: + (v,) = struct.unpack("<" + type_format, vdata) + pdata.add_param(name, v, ptype) count += 1 if count != num_params: @@ -1414,7 +1848,7 @@ def missionplanner_sort(item: str) -> tuple[str, ...]: return tuple(item.split("_")) @staticmethod - def extract_params(pdata: list[tuple[bytes, float, int]] | None, sort_type: str) -> dict[str, tuple[float, int]]: + def extract_params(pdata: list[tuple[bytes, float, type]], sort_type: str) -> dict[str, tuple[float, type]]: """Extract parameter values to an optionally sorted dictionary of name->(value, type).""" pdict = {} if pdata: @@ -1431,8 +1865,8 @@ def extract_params(pdata: list[tuple[bytes, float, int]] | None, sort_type: str) @staticmethod def save_params( - pdict: dict[str, tuple[float, int]], - filename: str | None, + pdict: dict[str, tuple[float, str]], + filename: str, sort_type: str, add_datatype_comments: bool, add_timestamp_comment: bool, @@ -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", } if add_timestamp_comment: f.write(f"# Parameters saved at {datetime.now(tz=None).strftime('%Y-%m-%d %H:%M:%S')}\n") @@ -1460,46 +1894,63 @@ def save_params( f.write("\n") logging.info("Outputted %u parameters to %s", len(pdict), filename) - def cmd_getparams( # pylint: disable=too-many-arguments, too-many-positional-arguments + def cmd_getparams( self, args: list[str], - progress_callback: Callable[..., Any] | None = None, + progress_callback: Callback | None = None, sort_type: str = "missionplanner", add_datatype_comments: bool = False, add_timestamp_comment: bool = False, ) -> MAVFTPReturn: """Decode the parameter file and save the values and defaults to disk.""" - def decode_and_save_params(fh: SIO | None) -> MAVFTPReturn: + def decode_and_save_params(fh: BinaryIO | None) -> MAVFTPReturn: if fh is None: logging.error("FTP: no parameter file handler") - return MAVFTPReturn("GetParams", ERR_Fail) + return MAVFTPReturn("GetParams", FtpError.Fail) try: data = fh.read() except OSError as exp: logging.error("FTP: Failed to read file param.pck: %s", exp) - return MAVFTPReturn("GetParams", ERR_Fail) + return MAVFTPReturn("GetParams", FtpError.Fail) pdata = MAVFTP.ftp_param_decode(data) if pdata is None: logging.error("FTP: Failed to decode parameter file param.pck") - return MAVFTPReturn("GetParams", ERR_Fail) + return MAVFTPReturn("GetParams", FtpError.Fail) param_values = MAVFTP.extract_params(pdata.params, sort_type) param_defaults = MAVFTP.extract_params(pdata.defaults, sort_type) param_values_path = args[0] - param_defaults_path = args[1] if len(args) > 1 else None - - MAVFTP.save_params(param_values, param_values_path, sort_type, add_datatype_comments, add_timestamp_comment) - MAVFTP.save_params(param_defaults, param_defaults_path, sort_type, add_datatype_comments, add_timestamp_comment) + param_defaults_path = args[1] if len(args) > 1 else "" + + MAVFTP.save_params( + param_values, + param_values_path, + sort_type, + add_datatype_comments, + add_timestamp_comment, + ) + MAVFTP.save_params( + param_defaults, + param_defaults_path, + sort_type, + add_datatype_comments, + add_timestamp_comment, + ) if self.ftp_settings.debug > 0: for name, (value, _param_type) in param_values.items(): if name in param_defaults: - logging.info("%-16s %f (default %f)", name, value, param_defaults[name][0]) + logging.info( + "%-16s %f (default %f)", + name, + value, + param_defaults[name][0], + ) else: logging.info("%-16s %f", name, value) - return MAVFTPReturn("GetParams", ERR_None) + return MAVFTPReturn("GetParams", FtpError.Success) return self.cmd_get( ["@PARAM/param.pck?withdefaults=1" if len(args) > 1 else "@PARAM/param.pck"], @@ -1524,16 +1975,24 @@ def create_argument_parser() -> ArgumentParser: " A tool to do file operations between a ground control station and a drone using the MAVLink" " protocol." ) - parser.add_argument("--baudrate", type=int, default=115200, help="master port baud rate. Default is %(default)s") + parser.add_argument( + "--baudrate", + type=int, + default=115200, + help="master port baud rate. Default is %(default)s", + ) parser.add_argument( # type: ignore[attr-defined] "--device", type=str, default="", help="serial device. For windows use COMx where x is the port number. " "For Unix use /dev/ttyUSBx where x is the port number. Default is autodetection", - ).completer = FilesCompleter(directories=False, allowednames=[".port"]) # type: ignore[no-untyped-call] + ).completer = FilesCompleter(directories=False, allowednames=[".port"]) parser.add_argument( - "--source-system", type=int, default=250, help="MAVLink source system for this GCS. Default is %(default)s" + "--source-system", + type=int, + default=250, + help="MAVLink source system for this GCS. Default is %(default)s", ) parser.add_argument("--loglevel", default="INFO", help="log level. Default is %(default)s") @@ -1545,37 +2004,99 @@ def create_argument_parser() -> ArgumentParser: choices=[0, 1, 2], help="Debug level 0 for none, 2 for max verbosity. Default is %(default)s", ) - parser.add_argument("--pkt_loss_tx", type=int, default=0, help="Packet loss on TX. Default is %(default)s") - parser.add_argument("--pkt_loss_rx", type=int, default=0, help="Packet loss on RX. Default is %(default)s") + parser.add_argument( + "--pkt_loss_tx", + type=int, + default=0, + help="Packet loss on TX. Default is %(default)s", + ) + parser.add_argument( + "--pkt_loss_rx", + type=int, + default=0, + help="Packet loss on RX. Default is %(default)s", + ) parser.add_argument("--max_backlog", type=int, default=5, help="Max backlog. Default is %(default)s") - parser.add_argument("--burst_read_size", type=int, default=80, help="Burst read size. Default is %(default)s") + parser.add_argument( + "--burst_read_size", + type=int, + default=80, + help="Burst read size. Default is %(default)s", + ) parser.add_argument("--write_size", type=int, default=80, help="Write size. Default is %(default)s") - parser.add_argument("--write_qsize", type=int, default=5, help="Write queue size. Default is %(default)s") - parser.add_argument("--idle_detection_time", type=float, default=1.2, help="Idle detection time. Default is %(default)s") - parser.add_argument("--read_retry_time", type=float, default=1.0, help="Read retry time. Default is %(default)s") - parser.add_argument("--retry_time", type=float, default=0.5, help="Retry time. Default is %(default)s") + parser.add_argument( + "--write_qsize", + type=int, + default=5, + help="Write queue size. Default is %(default)s", + ) + parser.add_argument( + "--idle_detection_time", + type=float, + default=3.7, + help="Idle detection time. Default is %(default)s", + ) + parser.add_argument( + "--read_retry_time", + type=float, + default=1.0, + help="Read retry time. Default is %(default)s", + ) + parser.add_argument( + "--retry_time", + type=float, + default=0.5, + help="Retry time. Default is %(default)s", + ) subparsers = parser.add_subparsers(dest="command", required=True) + # Set command + parser_set = subparsers.add_parser("set", help="Set a MAVFTP internal configuration parameter.") + parser_set.add_argument( + "arg1", + type=str, + metavar="setting_name", + help="MAVFTP internal configuration parameter name.", + ) + parser_set.add_argument( + "arg2", + type=float, + metavar="setting_value", + help="MAVFTP internal configuration parameter value.", + ) + # Get command parser_get = subparsers.add_parser("get", help="Get a file from the remote flight controller.") - parser_get.add_argument("arg1", type=str, metavar="remote_path", help="Path to the file on the remote flight controller.") + parser_get.add_argument( + "arg1", + type=str, + metavar="remote_path", + help="Path to the file on the remote flight controller.", + ) parser_get.add_argument( # type: ignore[attr-defined] - "arg2", nargs="?", type=str, metavar="local_path", help="Optional local path to save the file." - ).completer = FilesCompleter() # type: ignore[no-untyped-call] + "arg2", + nargs="?", + type=str, + metavar="local_path", + help="Optional local path to save the file.", + ).completer = FilesCompleter() # Getparams command parser_getparams = subparsers.add_parser("getparams", help="Get and decode parameters from the remote flight controller.") parser_getparams.add_argument( # type: ignore[attr-defined] - "arg1", type=str, metavar="param_values_path", help="Local path to save the parameter values file to." - ).completer = FilesCompleter() # type: ignore[no-untyped-call] + "arg1", + type=str, + metavar="param_values_path", + help="Local path to save the parameter values file to.", + ).completer = FilesCompleter() parser_getparams.add_argument( # type: ignore[attr-defined] "arg2", nargs="?", type=str, metavar="param_defaults_path", help="Optional local path to save the parameter defaults file to.", - ).completer = FilesCompleter() # type: ignore[no-untyped-call] + ).completer = FilesCompleter() parser_getparams.add_argument( "-s", "--sort", @@ -1601,8 +2122,11 @@ def create_argument_parser() -> ArgumentParser: # Put command parser_put = subparsers.add_parser("put", help="Put a file to the remote flight controller.") parser_put.add_argument( # type: ignore[attr-defined] - "arg1", type=str, metavar="local_path", help="Local path to the file to upload to the flight controller." - ).completer = FilesCompleter() # type: ignore[no-untyped-call] + "arg1", + type=str, + metavar="local_path", + help="Local path to the file to upload to the flight controller.", + ).completer = FilesCompleter() parser_put.add_argument( "arg2", nargs="?", @@ -1613,7 +2137,13 @@ def create_argument_parser() -> ArgumentParser: # List command parser_list = subparsers.add_parser("list", help="List files in a directory on the remote flight controller.") - parser_list.add_argument("arg1", nargs="?", type=str, metavar="remote_path", help="Optional path to list files from.") + parser_list.add_argument( + "arg1", + nargs="?", + type=str, + metavar="remote_path", + help="Optional path to list files from.", + ) # Mkdir command parser_mkdir = subparsers.add_parser("mkdir", help="Create a directory on the remote flight controller.") @@ -1629,15 +2159,31 @@ def create_argument_parser() -> ArgumentParser: # Rename command parser_rename = subparsers.add_parser("rename", help="Rename a file or directory on the remote flight controller.") - parser_rename.add_argument("arg1", type=str, metavar="old_remote_path", help="Current path of the file/directory.") - parser_rename.add_argument("new_remote_path", type=str, metavar="arg2", help="New path for the file/directory.") + parser_rename.add_argument( + "arg1", + type=str, + metavar="old_remote_path", + help="Current path of the file/directory.", + ) + parser_rename.add_argument( + "new_remote_path", + type=str, + metavar="arg2", + help="New path for the file/directory.", + ) # CRC command parser_crc = subparsers.add_parser("crc", help="Calculate the CRC of a file on the remote flight controller.") - parser_crc.add_argument("arg1", type=str, metavar="remote_path", help="Path to the file to calculate the CRC of.") + parser_crc.add_argument( + "arg1", + type=str, + metavar="remote_path", + help="Path to the file to calculate the CRC of.", + ) # Add other subparsers commands as needed - argcomplete.autocomplete(parser) + if _ARGCOMPLETE_AVAILABLE: + argcomplete.autocomplete(parser) return parser @@ -1671,7 +2217,7 @@ def auto_detect_serial() -> list[mavutil.SerialPort]: return serial_list -def auto_connect(device: str) -> mavutil.SerialPort: +def auto_connect(device: str | None) -> mavutil.SerialPort: comport = None if device: comport = mavutil.SerialPort(device=device, description=device) @@ -1698,11 +2244,11 @@ def auto_connect(device: str) -> mavutil.SerialPort: return comport -def wait_heartbeat(m: "MavlinkConnection") -> None: +def wait_heartbeat(m: MavlinkObject) -> None: """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) def main() -> None: @@ -1734,7 +2280,10 @@ def main() -> None: ) mav_ftp = MAVFTP( - master, target_system=master.target_system, target_component=master.target_component, settings=ftp_settings + master, + target_system=master.target_system, + target_component=master.target_component, + settings=ftp_settings, ) cmd_ftp_args = [args.command] @@ -1748,17 +2297,20 @@ def main() -> None: if args.command in {"get", "put", "getparams"}: ret = mav_ftp.process_ftp_reply(args.command, timeout=500) + exit_code = 1 if isinstance(ret, str): logging.error("Command returned: %s, but it should return a MAVFTPReturn instead", ret) elif isinstance(ret, MAVFTPReturn): - if ret.error_code: + if ret.error_code or args.command == "list": ret.display_message() + exit_code = 0 if ret.error_code == FtpError.Success else 1 elif ret is None: logging.error("Command returned: None, but it should return a MAVFTPReturn instead") else: logging.error("Command returned: something strange, but it should return a MAVFTPReturn instead") master.close() + sys.exit(exit_code) if __name__ == "__main__": diff --git a/ardupilot_methodic_configurator/data_model_parameter_editor.py b/ardupilot_methodic_configurator/data_model_parameter_editor.py index 2fae426e7..e0f4f6ddf 100644 --- a/ardupilot_methodic_configurator/data_model_parameter_editor.py +++ b/ardupilot_methodic_configurator/data_model_parameter_editor.py @@ -15,7 +15,7 @@ import contextlib import platform import subprocess -from collections.abc import Callable +from collections.abc import Callable, Sequence from copy import deepcopy from csv import writer as csv_writer from dataclasses import dataclass @@ -33,6 +33,7 @@ from ardupilot_methodic_configurator.backend_filesystem import LocalFilesystem from ardupilot_methodic_configurator.backend_filesystem_configuration_steps import PhaseData from ardupilot_methodic_configurator.backend_flightcontroller import FlightController +from ardupilot_methodic_configurator.backend_flightcontroller_files import FlightControllerLogFile from ardupilot_methodic_configurator.backend_internet import download_file_from_url, webbrowser_open_url from ardupilot_methodic_configurator.data_model_ardupilot_parameter import ( ArduPilotParameter, @@ -77,6 +78,15 @@ class LogAnalysisInputs: apm_doc: APMDoc | None +@dataclass(frozen=True) +class BinLogDownloadResult: + """Outcome of downloading one or more selected flight-controller files.""" + + successful: tuple[str, ...] = () + failed: tuple[str, ...] = () + cancelled: bool = False + + # Type aliases for callback functions used in workflow methods AskConfirmationCallback = Callable[[str, str], bool] # (title, message) -> bool SelectFileCallback = Callable[[str, list[str]], str | None] # (title, filetypes) -> Optional[filename] @@ -1394,6 +1404,138 @@ def download_last_flight_log_workflow( else: show_error(_("Error"), _("Failed to download flight log. Check the console for details.")) + def get_bin_log_files(self, remote_directory: str = "/APM/LOGS/") -> list[FlightControllerLogFile]: + """Return regular files in the selected remote directory.""" + return self._flight_controller.list_bin_log_files(remote_directory) + + def get_remote_files(self, remote_directory: str = "/APM/LOGS/") -> list[FlightControllerLogFile]: + """Return files and directories in the selected remote directory.""" + return self._flight_controller.list_remote_files(remote_directory) + + def download_last_flight_log( + self, + local_filename: str, + progress_callback: Callable[[int, int], None] | None = None, + ) -> bool: + """Download the last flight log through the flight-controller facade.""" + return self._flight_controller.download_last_flight_log(local_filename, progress_callback) + + def upload_file_to_fc( + self, + local_filename: str, + remote_filename: str, + progress_callback: Callable[[int, int], None] | None = None, + ) -> bool: + """Upload one local file to the flight controller.""" + return self._flight_controller.upload_file(local_filename, remote_filename, progress_callback) + + def download_remote_file( + self, + remote_path: str, + local_filename: str, + progress_callback: Callable[[int, int], None] | None = None, + ) -> bool: + """Download one explicitly selected remote file.""" + return self._flight_controller.download_remote_file(remote_path, local_filename, progress_callback) + + def make_remote_directory(self, remote_directory: str) -> bool: + """Create one remote directory.""" + return self._flight_controller.make_remote_directory(remote_directory) + + def delete_remote_path(self, remote_path: str, is_directory: bool = False) -> bool: + """Delete one remote file or directory.""" + return self._flight_controller.delete_remote_path(remote_path, is_directory) + + def rename_remote_path(self, remote_path: str, new_remote_path: str) -> bool: + """Rename one remote file or directory.""" + return self._flight_controller.rename_remote_path(remote_path, new_remote_path) + + def download_selected_bin_logs_workflow( + self, + selected_files: Sequence[FlightControllerLogFile], + destination: str, + destination_is_directory: bool, + ask_overwrite: AskConfirmationCallback, + show_error: ShowErrorCallback, + show_info: ShowInfoCallback, + progress_callback: Callable[[int, int], None] | None = None, + ) -> BinLogDownloadResult: + """ + Download selected remote files to one local file or a local directory. + + The caller chooses the destination using the GUI. This workflow validates + the destination, asks once before a conflicting batch overwrite, and + delegates each transfer to the flight-controller facade. + """ + files = tuple(selected_files) + if not files: + return BinLogDownloadResult() + + if destination_is_directory: + destination_path = Path(destination) + if not destination_path.is_dir(): + show_error(_("Download Error"), _("The selected destination directory does not exist.")) + return BinLogDownloadResult(failed=tuple(file.name for file in files)) + targets = tuple(destination_path / file.name for file in files) + elif len(files) == 1: + targets = (Path(destination),) + else: + show_error(_("Download Error"), _("A directory destination is required for multiple files.")) + return BinLogDownloadResult(failed=tuple(file.name for file in files)) + + existing_targets = tuple(target for target in targets if target.exists()) + if existing_targets: + existing_names = "\n".join(target.name for target in existing_targets) + if not ask_overwrite( + _("Overwrite existing files?"), + _("The following local file(s) already exist:\n\n%s\n\nOverwrite them?") % existing_names, + ): + return BinLogDownloadResult( + failed=tuple(file.name for file in files), + cancelled=True, + ) + + units = tuple(max(file.size_bytes, 1) for file in files) + total_units = sum(units) + completed_units = 0 + successful: list[str] = [] + failed: list[str] = [] + + for file, target, file_units in zip(files, targets, units, strict=True): + + def update_progress(current: int, total: int, *, offset: int = completed_units, size: int = file_units) -> None: + if progress_callback is None: + return + fraction = current / total if total else 0.0 + progress_callback(min(total_units, int(offset + size * fraction)), total_units) + + transfer_callback = update_progress if progress_callback is not None else None + if self._flight_controller.download_bin_log_file(file.remote_path, str(target), transfer_callback): + successful.append(file.name) + else: + failed.append(file.name) + completed_units += file_units + if progress_callback is not None: + progress_callback(completed_units, total_units) + + result = BinLogDownloadResult(tuple(successful), tuple(failed)) + summary_lines = [ + *(_("Downloaded: %s") % filename for filename in successful), + *(_("Failed: %s") % filename for filename in failed), + ] + summary = "\n".join(summary_lines) + if failed: + show_error( + _("Download summary"), + summary, + ) + else: + show_info( + _("Download summary"), + summary, + ) + return result + def is_configuration_step_optional(self, file_name: str | None = None, threshold_pct: int = 20) -> bool: """ Check if the configuration step for the given file is optional. diff --git a/ardupilot_methodic_configurator/frontend_tkinter_download_bin_logs.py b/ardupilot_methodic_configurator/frontend_tkinter_download_bin_logs.py new file mode 100644 index 000000000..9c5ca01f8 --- /dev/null +++ b/ardupilot_methodic_configurator/frontend_tkinter_download_bin_logs.py @@ -0,0 +1,1375 @@ +""" +Modal two-panel MAVFTP/local-file browser. + +This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + +SPDX-FileCopyrightText: 2026 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +from __future__ import annotations + +import posixpath +import queue +import sys +import tkinter as tk +from dataclasses import dataclass +from pathlib import Path +from threading import Event, Thread +from tkinter import ttk +from typing import TYPE_CHECKING, Literal, Protocol, cast + +from ardupilot_methodic_configurator import _ +from ardupilot_methodic_configurator.backend_flightcontroller_files import FlightControllerLogFile +from ardupilot_methodic_configurator.formatting import format_filesize +from ardupilot_methodic_configurator.frontend_tkinter_base_window import BaseWindow +from ardupilot_methodic_configurator.frontend_tkinter_show import show_tooltip + +if TYPE_CHECKING: + from collections.abc import Callable, Sequence + + from ardupilot_methodic_configurator.data_model_parameter_editor import ParameterEditor + from ardupilot_methodic_configurator.frontend_tkinter_progress_window import ProgressWindow + + +class DownloadBinLogsUiServices(Protocol): + """UI callbacks required by the two-panel file browser.""" + + asksaveasfilename: Callable[..., str] + askdirectory: Callable[..., str] + askstring: Callable[..., str | None] + ask_yesno: Callable[[str, str], bool] + show_warning: Callable[[str, str], None] + show_error: Callable[[str, str], None] + show_info: Callable[[str, str], None] + + create_progress_window: Callable[[tk.Misc, str, str, bool], ProgressWindow] + + +@dataclass(frozen=True) +class LocalFileEntry: + """A local file-system entry displayed in the local panel.""" + + name: str + path: Path + size_bytes: int + is_directory: bool = False + + +class _TransferCancelledError(Exception): + """Raised by a transfer progress callback when the user cancels.""" + + +class DownloadBinLogsWindow(BaseWindow): # pylint: disable=too-many-instance-attributes + """Browse remote and local files and transfer or manage selected entries.""" + + DEFAULT_REMOTE_DIRECTORY = "/APM/LOGS/" + sort_column: str + download_button: ttk.Button + empty_state_label: ttk.Label + + def __init__( + self, + parent: tk.Tk | tk.Toplevel, + parameter_editor: ParameterEditor, + ui_services: DownloadBinLogsUiServices, + ) -> None: + super().__init__(parent) + self.parent = parent + self.parameter_editor = parameter_editor + self.ui = ui_services + self.remote_entries: list[FlightControllerLogFile] = [] + self.remote_files: list[FlightControllerLogFile] = [] # Compatibility with the original file-only tests/API. + self.local_entries: list[LocalFileEntry] = [] + self.remote_sort_column = "" + self.local_sort_column = "" + self.remote_sort_reverse = False + self.local_sort_reverse = False + self.last_selected_panel = "remote" + self.last_selected_items: dict[str, str | None] = {"remote": None, "local": None} + self._operation_queue: queue.Queue[tuple[str, object]] = queue.Queue() + self._operation_thread: Thread | None = None + self._operation_cancel_event: Event | None = None + self._operation_progress: ProgressWindow | None = None + self._operation_active = False + self.remote_directory_var = tk.StringVar(master=self.root, value=self.DEFAULT_REMOTE_DIRECTORY) + self.local_directory_var = tk.StringVar( + master=self.root, + value=self._default_local_directory(parameter_editor), + ) + + self.root.title(_("Download .bin log files")) + self.root.geometry(self.calculate_scaled_geometry(1100, 620)) + self.center_window(self.root, parent) + self.root.resizable(width=True, height=True) + self.root.transient(parent) + self.root.protocol("WM_DELETE_WINDOW", self._on_cancel_or_close) + if sys.platform != "darwin": + self.root.grab_set() + + self._build_widgets() + self.refresh_remote_panel() + self.refresh_local_panel() + + def _build_widgets(self) -> None: + """Create the two file panels and action buttons.""" + panels = ttk.PanedWindow(self.main_frame, orient=tk.HORIZONTAL) + panels.pack(side=tk.TOP, fill=tk.BOTH, expand=True, padx=8, pady=(8, 4)) + + remote_panel = ttk.Frame(panels) + local_panel = ttk.Frame(panels) + panels.add(remote_panel, weight=1) + panels.add(local_panel, weight=1) + + self._build_remote_panel(remote_panel) + self._build_local_panel(local_panel) + + action_frame = ttk.Frame(self.main_frame) + action_frame.pack(side=tk.BOTTOM, fill=tk.X, padx=8, pady=(4, 8)) + + download_button = ttk.Button( + action_frame, + text=_("Download selected →"), + command=self.download_selected_remote_entries, + ) + download_button.pack(side=tk.LEFT) + show_tooltip( + download_button, _("Download selected files and directories from the flight controller to the local computer") + ) + + upload_button = ttk.Button( + action_frame, + text=_("← Upload selected"), + command=self.upload_selected_local_entries, + ) + upload_button.pack(side=tk.LEFT, padx=(6, 0)) + show_tooltip( + upload_button, _("Upload selected files and directories from the local computer to the flight controller") + ) + + last_log_button = ttk.Button( + action_frame, + text=_("Download last .bin log file"), + command=self.download_last_flight_log, + ) + last_log_button.pack(side=tk.LEFT, padx=(14, 0)) + show_tooltip(last_log_button, _("Download the last flight-controller .bin log file")) + + cancel_button = ttk.Button(action_frame, text=_("Cancel"), command=self._on_cancel_or_close) + cancel_button.pack(side=tk.RIGHT) + show_tooltip(cancel_button, _("Close the file browser")) + self.cancel_button = cancel_button + + @staticmethod + def _default_local_directory(parameter_editor: ParameterEditor) -> str: + """Return the current vehicle directory, falling back to the working directory.""" + try: + vehicle_directory = parameter_editor.get_vehicle_directory() + except (AttributeError, OSError, TypeError): + vehicle_directory = "" + if isinstance(vehicle_directory, str) and vehicle_directory: + path = Path(vehicle_directory).expanduser() + if path.is_dir(): + return str(path) + return str(Path.cwd()) + + def _build_remote_panel(self, parent: ttk.Frame) -> None: + """Create the remote destination selector and remote Treeview.""" + selector = ttk.Frame(parent) + selector.pack(side=tk.TOP, fill=tk.X, pady=(0, 4)) + ttk.Label(selector, text=_("Remote destination:")).pack(side=tk.LEFT, padx=(0, 6)) + self.remote_directory_entry = ttk.Entry(selector, textvariable=self.remote_directory_var) + self.remote_directory_entry.pack(side=tk.LEFT, fill=tk.X, expand=True) + self.remote_directory_entry.bind("", self._on_remote_directory_return) + self.remote_parent_button = ttk.Button( + selector, + text="↖", + width=3, + command=self.navigate_remote_parent, + ) + self.remote_parent_button.pack(side=tk.LEFT, padx=(6, 0)) + show_tooltip(self.remote_parent_button, _("Go to the parent directory on the flight controller")) + + open_remote_button = ttk.Button(selector, text=_("Open/Refresh"), command=self.refresh_remote_panel) + open_remote_button.pack(side=tk.LEFT, padx=(6, 0)) + show_tooltip(open_remote_button, _("Open the remote directory and refresh its contents")) + + select_remote_button = ttk.Button(selector, text=_("Select all"), command=self.select_all_remote_entries) + select_remote_button.pack(side=tk.LEFT, padx=(6, 0)) + show_tooltip(select_remote_button, _("Select all remote files and directories")) + + self.remote_directory_label = ttk.Label(parent, text="") + self.remote_directory_label.pack(side=tk.TOP, anchor=tk.W, pady=(0, 4)) + self.remote_tree = self._create_tree(parent, remote=True) + self.tree = self.remote_tree # Compatibility alias retained for existing callers/tests. + + def _build_local_panel(self, parent: ttk.Frame) -> None: + """Create the local directory selector and local Treeview.""" + selector = ttk.Frame(parent) + selector.pack(side=tk.TOP, fill=tk.X, pady=(0, 4)) + ttk.Label(selector, text=_("Local directory:")).pack(side=tk.LEFT, padx=(0, 6)) + self.local_directory_entry = ttk.Entry(selector, textvariable=self.local_directory_var) + self.local_directory_entry.pack(side=tk.LEFT, fill=tk.X, expand=True) + self.local_directory_entry.bind("", self._on_local_directory_return) + self.local_parent_button = ttk.Button( + selector, + text="↖", + width=3, + command=self.navigate_local_parent, + ) + self.local_parent_button.pack(side=tk.LEFT, padx=(6, 0)) + show_tooltip(self.local_parent_button, _("Go to the parent directory on the local computer")) + + browse_local_button = ttk.Button(selector, text=_("Browse"), command=self.choose_local_directory) + browse_local_button.pack(side=tk.LEFT, padx=(6, 0)) + show_tooltip(browse_local_button, _("Choose a local directory")) + + refresh_local_button = ttk.Button(selector, text=_("Refresh"), command=self.refresh_local_panel) + refresh_local_button.pack(side=tk.LEFT, padx=(6, 0)) + show_tooltip(refresh_local_button, _("Refresh the local directory contents")) + + select_local_button = ttk.Button(selector, text=_("Select all"), command=self.select_all_local_entries) + select_local_button.pack(side=tk.LEFT, padx=(6, 0)) + show_tooltip(select_local_button, _("Select all local files and directories")) + + self.local_directory_label = ttk.Label(parent, text="") + self.local_directory_label.pack(side=tk.TOP, anchor=tk.W, pady=(0, 4)) + self.local_tree = self._create_tree(parent, remote=False) + + def _create_tree(self, parent: ttk.Frame, *, remote: bool) -> ttk.Treeview: + """Create one panel Treeview with sorting and navigation bindings.""" + tree = ttk.Treeview( + parent, + columns=("name", "type", "size"), + show="headings", + selectmode="extended", + ) + sort_prefix = "remote" if remote else "local" + columns: tuple[tuple[str, str, Literal["w", "e"], int], ...] = ( + ("name", _("Name"), "w", 180), + ("type", _("Type"), "w", 90), + ("size", _("Size"), "e", 90), + ) + for column, title, anchor, width in columns: + tree.heading( + column, + text=title, + command=lambda col=column, prefix=sort_prefix: self._on_sort_heading(prefix, col), + ) + tree.column(column, anchor=anchor, width=width, stretch=column == "name") + tree.pack(side=tk.LEFT, fill=tk.BOTH, expand=True) + tree.bind("", self._on_remote_double_click if remote else self._on_local_double_click) + tree.bind("", self._on_select_all_key) + tree.bind("", self._on_select_all_key) + tree.bind("", self._on_delete_key) + tree.bind("", self._on_rename_key) + tree.bind("", self._on_backspace) + tree.bind("", lambda _event, panel=sort_prefix: self._remember_panel(panel)) + tree.bind("", lambda event, panel=sort_prefix: self._remember_panel_from_click(panel, event)) + tree.bind("<>", lambda _event, panel=sort_prefix: self._remember_panel_from_selection(panel)) + scrollbar = ttk.Scrollbar(parent, orient=tk.VERTICAL, command=tree.yview) + scrollbar.pack(side=tk.RIGHT, fill=tk.Y) + tree.configure(yscrollcommand=scrollbar.set) + return tree + + def refresh_remote_panel(self) -> None: + """Refresh the remote panel, including directory entries.""" + remote_directory = self.remote_directory_var.get() + if not remote_directory.strip(): + self.ui.show_error(_("Remote directory error"), _("The remote destination must not be empty.")) + return + try: + entries = list(self.parameter_editor.get_remote_files(remote_directory)) + except Exception as error: # pylint: disable=broad-exception-caught + self.ui.show_error(_("Remote directory error"), str(error)) + return + + self.remote_entries = entries + self._populate_remote_tree() + self._update_parent_navigation_buttons() + self.remote_directory_label.configure( + text=_("Remote files in {remote_directory}").format(remote_directory=remote_directory) + ) + + def refresh_remote_files(self) -> None: + """Compatibility method that refreshes the original file-only listing.""" + remote_directory = self.remote_directory_var.get() + if not remote_directory.strip(): + self.ui.show_error(_("Remote directory error"), _("The remote destination must not be empty.")) + return + try: + self.remote_files = list(self.parameter_editor.get_bin_log_files(remote_directory)) + except Exception as error: # pylint: disable=broad-exception-caught + self.remote_files = [] + self.ui.show_error(_("Remote directory error"), str(error)) + return + self.remote_entries = list(self.remote_files) + if hasattr(self, "remote_tree"): + self._populate_remote_tree() + else: + self._populate_tree() + self._update_parent_navigation_buttons() + self.remote_directory_label.configure(text=_("Files in {remote_directory}").format(remote_directory=remote_directory)) + + def refresh_local_panel(self) -> None: + """Refresh the local panel from the selected directory.""" + directory = Path(self.local_directory_var.get()).expanduser() + if not directory.is_dir(): + self.ui.show_error(_("Local directory error"), _("The selected local directory does not exist.")) + return + entries: list[LocalFileEntry] = [] + try: + children = sorted(directory.iterdir(), key=lambda path: path.name.casefold()) + for child in children: + if child.is_symlink(): + continue + try: + is_directory = child.is_dir() + size_bytes = 0 if is_directory else child.stat().st_size + except OSError: + continue + entries.append(LocalFileEntry(child.name, child, size_bytes, is_directory)) + except OSError as error: + self.ui.show_error(_("Local directory error"), str(error)) + return + self.local_entries = entries + self._populate_local_tree() + self._update_parent_navigation_buttons() + self.local_directory_label.configure(text=_("Local files in {directory}").format(directory=directory)) + + def _update_parent_navigation_buttons(self) -> None: + """Enable parent buttons only when the corresponding panel has a parent.""" + remote_button = getattr(self, "remote_parent_button", None) + if remote_button is not None: + remote_directory = self.remote_directory_var.get().rstrip("/") + remote_button.configure(state="normal" if remote_directory and remote_directory != "/" else "disabled") + + local_button = getattr(self, "local_parent_button", None) + if local_button is not None: + local_directory = Path(self.local_directory_var.get()).expanduser() + local_button.configure(state="normal" if local_directory.parent != local_directory else "disabled") + + def navigate_remote_parent(self) -> None: + """Navigate the remote panel to its parent directory.""" + remote_directory = self.remote_directory_var.get() + without_trailing_slashes = remote_directory.rstrip("/") + if not remote_directory.strip() or not without_trailing_slashes or without_trailing_slashes == "/": + return + self.remote_directory_var.set(posixpath.dirname(without_trailing_slashes) or "/") + self.refresh_remote_panel() + + def navigate_local_parent(self) -> None: + """Navigate the local panel to its parent directory.""" + local_directory = Path(self.local_directory_var.get()).expanduser() + if local_directory.parent != local_directory: + self.local_directory_var.set(str(local_directory.parent)) + self.refresh_local_panel() + + def choose_local_directory(self) -> None: + """Choose and open a local directory.""" + directory = self.ui.askdirectory( + title=_("Select local directory"), + initialdir=self.local_directory_var.get(), + ) + if directory: + self.local_directory_var.set(directory) + self.refresh_local_panel() + + def _populate_remote_tree(self) -> None: + """Populate the remote Treeview.""" + self.last_selected_items["remote"] = None + self.remote_sort_column = "" + self._reset_tree_headings(self.remote_tree, "remote") + for item_id in self.remote_tree.get_children(): + self.remote_tree.delete(item_id) + for index, entry in enumerate(self.remote_entries): + self.remote_tree.insert( + "", + tk.END, + iid=str(index), + values=( + entry.name, + _("Directory") if entry.is_directory else _("File"), + "" if entry.is_directory else format_filesize(entry.size_bytes), + ), + ) + self._update_empty_state(self.remote_entries, self.remote_directory_label) + + def _populate_local_tree(self) -> None: + """Populate the local Treeview.""" + self.last_selected_items["local"] = None + self.local_sort_column = "" + self._reset_tree_headings(self.local_tree, "local") + for item_id in self.local_tree.get_children(): + self.local_tree.delete(item_id) + for index, entry in enumerate(self.local_entries): + self.local_tree.insert( + "", + tk.END, + iid=str(index), + values=( + entry.name, + _("Directory") if entry.is_directory else _("File"), + "" if entry.is_directory else format_filesize(entry.size_bytes), + ), + ) + self._update_empty_state(self.local_entries, self.local_directory_label) + + def _populate_tree(self) -> None: + """Compatibility population method for the original file-only Treeview.""" + if hasattr(self, "remote_tree"): + self._populate_remote_tree() + return + for item_id in self.tree.get_children(): + self.tree.delete(item_id) + for index, remote_file in enumerate(self.remote_files): + self.tree.insert("", tk.END, iid=str(index), values=(remote_file.name, format_filesize(remote_file.size_bytes))) + self._on_tree_selection_change() + + @staticmethod + def _update_empty_state(entries: Sequence[FlightControllerLogFile | LocalFileEntry], label: ttk.Label) -> None: + """Show a simple empty state when a panel contains no rows.""" + if not entries: + label.configure(text=_("No entries found in this directory.")) + + def _reset_tree_headings(self, tree: ttk.Treeview, prefix: str) -> None: + """Reset translated headings and initial sort commands.""" + sort_state = "remote_sort_column" if prefix == "remote" else "local_sort_column" + reverse_state = "remote_sort_reverse" if prefix == "remote" else "local_sort_reverse" + setattr(self, sort_state, "") + setattr(self, reverse_state, False) + for column, title in (("name", _("Name")), ("type", _("Type")), ("size", _("Size"))): + tree.heading( + column, + text=title, + command=lambda col=column, panel=prefix: self._on_sort_heading(panel, col), + ) + + def _on_sort_heading(self, panel: str, column: str) -> None: + """Toggle the selected column direction, or start a new ascending sort.""" + state_name = "remote_sort_column" if panel == "remote" else "local_sort_column" + reverse_state = "remote_sort_reverse" if panel == "remote" else "local_sort_reverse" + previous_column = getattr(self, state_name) + previous_reverse = getattr(self, reverse_state) + reverse = not previous_reverse if previous_column == column else False + self._sort_panel_by_column(panel, column, reverse) + + def _sort_panel_by_column(self, panel: str, column: str, reverse: bool) -> None: + """Sort one panel by name, type, or numeric size.""" + tree = self.remote_tree if panel == "remote" else self.local_tree + entries = self.remote_entries if panel == "remote" else self.local_entries + state_name = "remote_sort_column" if panel == "remote" else "local_sort_column" + reverse_state = "remote_sort_reverse" if panel == "remote" else "local_sort_reverse" + previous = getattr(self, state_name) + if previous and previous != column: + self._set_heading_text(tree, previous, None) + self._set_heading_text(tree, column, reverse) + setattr(self, state_name, column) + setattr(self, reverse_state, reverse) + rows = [(self._panel_sort_key(entries, item_id, column), item_id) for item_id in tree.get_children("")] + rows.sort(key=lambda row: row[0], reverse=reverse) + for position, (_key, item_id) in enumerate(rows): + tree.move(item_id, "", position) + tree.heading(column, command=lambda: self._on_sort_heading(panel, column)) + + @staticmethod + def _panel_sort_key( + entries: Sequence[FlightControllerLogFile | LocalFileEntry], + item_id: str, + column: str, + ) -> tuple[int, int | str, str]: + """Return a stable sort key from an entry list.""" + item_text = str(item_id) + if not item_text.isdigit() or int(item_text) >= len(entries): + return (0, 0, "") + entry = entries[int(item_text)] + name = entry.name.casefold() + if entry.name == "..": # type: ignore[attr-defined] + return (0, 0, "") + if column == "size": + return (1, entry.size_bytes, name) + if column == "type": + return (1, 0 if entry.is_directory else 1, name) + return (1, name, entry.name) + + @staticmethod + def _set_heading_text(tree: ttk.Treeview, column: str, reverse: bool | None) -> None: + """Set a heading's label and optional direction marker.""" + title = {"name": _("Name"), "type": _("Type"), "size": _("Size")}[column] + if reverse is not None: + title += " ▼" if reverse else " ▲" + tree.heading(column, text=title) + + def _on_remote_directory_return(self, _event: tk.Event | None = None) -> str: + """Refresh the remote listing when Enter is pressed in its entry.""" + if hasattr(self, "remote_directory_var"): + self.refresh_remote_panel() + else: + self.refresh_remote_files() + return "break" + + def _on_local_directory_return(self, _event: tk.Event | None = None) -> str: + """Refresh the local listing when Enter is pressed in its entry.""" + self.refresh_local_panel() + return "break" + + def _remember_panel(self, panel: str) -> None: + """Remember which panel was most recently focused or selected.""" + if panel in {"remote", "local"}: + self.last_selected_panel = panel + + def _remember_panel_from_click(self, panel: str, event: tk.Event) -> None: + """Remember the panel and row most recently clicked by the user.""" + self._remember_panel(panel) + tree = self.remote_tree if panel == "remote" else self.local_tree + item_id = tree.identify_row(event.y) + if item_id: + self.last_selected_items[panel] = item_id + + def _remember_panel_from_selection(self, panel: str) -> None: + """Remember the panel and focused row after a Treeview selection change.""" + self._remember_panel(panel) + tree = self.remote_tree if panel == "remote" else self.local_tree + item_id = tree.focus() + if item_id: + self.last_selected_items[panel] = item_id + + def _on_backspace(self, event: tk.Event | None = None) -> str: + """Navigate to the parent directory of the last selected panel.""" + widget = getattr(event, "widget", None) + if widget is not None and widget is getattr(self, "remote_tree", None): + self._remember_panel("remote") + elif widget is not None and widget is getattr(self, "local_tree", None): + self._remember_panel("local") + + if self.last_selected_panel == "remote": + self.navigate_remote_parent() + else: + self.navigate_local_parent() + return "break" + + def _on_delete_key(self, event: tk.Event | None = None) -> str: + """Delete all selected entries in the last selected panel.""" + widget = getattr(event, "widget", None) + if widget is self.remote_tree: + self._remember_panel("remote") + elif widget is self.local_tree: + self._remember_panel("local") + if self.last_selected_panel == "remote": + self.delete_selected_remote_entries() + else: + self.delete_selected_local_entries() + return "break" + + def _on_rename_key(self, event: tk.Event | None = None) -> str: + """Start in-place rename for exactly one currently selected entry.""" + widget = getattr(event, "widget", None) + if widget is getattr(self, "remote_tree", None) and widget is not None: + self._remember_panel("remote") + elif widget is getattr(self, "local_tree", None) and widget is not None: + self._remember_panel("local") + + panel = self.last_selected_panel + tree = self.remote_tree if panel == "remote" else self.local_tree + selected_items = tuple(tree.selection()) + if len(selected_items) != 1: + return "break" + item_id = selected_items[0] + self.last_selected_items[panel] = item_id + entries = self.remote_entries if panel == "remote" else self.local_entries + entry = self._entry_from_tree(entries, item_id or "") + if entry is None or entry.name == "..": + return "break" + self._start_inline_rename(panel, item_id or "", entry) + return "break" + + def _start_inline_rename( + self, + panel: str, + item_id: str, + entry: FlightControllerLogFile | LocalFileEntry, + ) -> None: + """Create an inline name editor, falling back to a dialog if needed.""" + tree = self.remote_tree if panel == "remote" else self.local_tree + try: + bounds = tree.bbox(item_id, "name") + if len(bounds) != 4: + msg = "The selected row is not visible" + raise tk.TclError(msg) + x, y, width, height = (int(value) for value in bounds) + if width <= 0 or height <= 0: + msg = "The selected row is not visible" + raise tk.TclError(msg) + editor = ttk.Entry(tree) + editor.insert(0, entry.name) + editor.select_range(0, tk.END) + editor.place(x=x, y=y, width=width, height=height) + editor.focus_set() + editor.bind( + "", + lambda _event: self._finish_inline_rename(panel, entry, editor), + ) + editor.bind("", lambda _event: self._cancel_inline_rename(editor)) + except (AttributeError, tk.TclError, TypeError, ValueError): + self._rename_entry_with_dialog(panel, entry) + + def _finish_inline_rename( + self, + panel: str, + entry: FlightControllerLogFile | LocalFileEntry, + editor: ttk.Entry, + ) -> str: + """Commit an inline rename and refresh the affected panel.""" + new_name = editor.get() + editor.destroy() + if not self._safe_name(new_name): + self.ui.show_error(_("Rename error"), _("The new name must be one file or directory name.")) + return "break" + self._rename_entry(panel, entry, new_name) + return "break" + + @staticmethod + def _cancel_inline_rename(editor: ttk.Entry) -> str: + """Cancel an inline rename editor.""" + editor.destroy() + return "break" + + def _rename_entry_with_dialog(self, panel: str, entry: FlightControllerLogFile | LocalFileEntry) -> None: + """Prompt for a new name when inline editing cannot be created.""" + new_name = self.ui.askstring( + _("Rename remote entry") if panel == "remote" else _("Rename local entry"), + _("New name:"), + initialvalue=entry.name, + parent=self.root, + ) + if new_name is not None and self._safe_name(new_name): + self._rename_entry(panel, entry, new_name) + elif new_name is not None: + self.ui.show_error(_("Rename error"), _("The new name must be one file or directory name.")) + + def _rename_entry(self, panel: str, entry: FlightControllerLogFile | LocalFileEntry, new_name: str) -> bool: + """Rename one remote or local entry and refresh its panel.""" + if panel == "remote" and isinstance(entry, FlightControllerLogFile): + return self._rename_remote_entry(entry, new_name) + if panel == "local" and isinstance(entry, LocalFileEntry): + return self._rename_local_entry(entry, new_name) + return False + + def _rename_remote_entry(self, entry: FlightControllerLogFile, new_name: str) -> bool: + """Rename one remote entry.""" + new_path = posixpath.join(posixpath.dirname(entry.remote_path.rstrip("/")), new_name) + if not self._confirm_remote_mutation_scope(_("rename"), (entry.remote_path, new_path)): + return False + if self.parameter_editor.rename_remote_path(entry.remote_path, new_path): + self.ui.show_info( + _("Rename summary"), + _("Renamed %(old)s to %(new)s.") % {"old": entry.name, "new": new_name}, + ) + self.refresh_remote_panel() + return True + self.ui.show_error(_("Rename error"), _("Could not rename the remote entry.")) + return False + + def _rename_local_entry(self, entry: LocalFileEntry, new_name: str) -> bool: + """Rename one local entry.""" + target = entry.path.with_name(new_name) + if target.exists(): + self.ui.show_error(_("Rename error"), _("The target name already exists.")) + return False + try: + entry.path.rename(target) + except OSError as error: + self.ui.show_error(_("Rename error"), str(error)) + return False + self.ui.show_info( + _("Rename summary"), + _("Renamed %(old)s to %(new)s.") % {"old": entry.name, "new": new_name}, + ) + self.refresh_local_panel() + return True + + def _on_select_all_key(self, _event: tk.Event | None = None) -> str: + """Select all entries in the focused panel, except parent navigation.""" + if not hasattr(self, "remote_tree"): + self.select_all_files() + return "break" + tree = getattr(_event, "widget", None) + if tree not in {self.remote_tree, self.local_tree}: + tree = self.remote_tree + self._select_all_tree_entries(tree) + return "break" + + @staticmethod + def _select_all_tree_entries(tree: ttk.Treeview) -> None: + """Select all visible entries except the parent-navigation row.""" + tree.selection_set([item_id for item_id in tree.get_children() if tree.item(item_id, "values")[0] != ".."]) + + def select_all_remote_entries(self) -> None: + """Select all remote entries except the parent-navigation row.""" + if hasattr(self, "remote_tree"): + self._select_all_tree_entries(self.remote_tree) + return + self.select_all_files() + + def select_all_local_entries(self) -> None: + """Select all local entries except the parent-navigation row.""" + if hasattr(self, "local_tree"): + self._select_all_tree_entries(self.local_tree) + + def select_all_files(self) -> None: + """Select all remote files for compatibility with the original window API.""" + tree = self.tree + entries = self.remote_files + item_ids = [str(index) for index, entry in enumerate(entries) if entry.name != ".."] + tree.selection_set(item_ids) + self._on_tree_selection_change() + + def _on_remote_double_click(self, event: tk.Event) -> None: + """Open a remote directory on double click.""" + item_id = self.remote_tree.identify_row(event.y) + if not item_id: + return + entry = self._entry_from_tree(self.remote_entries, item_id) + if isinstance(entry, FlightControllerLogFile) and entry.is_directory: + self.remote_directory_var.set(entry.remote_path) + self.refresh_remote_panel() + + def _on_local_double_click(self, event: tk.Event) -> None: + """Open a local directory on double click.""" + item_id = self.local_tree.identify_row(event.y) + if not item_id: + return + entry = self._entry_from_tree(self.local_entries, item_id) + if isinstance(entry, LocalFileEntry) and entry.is_directory: + self.local_directory_var.set(str(entry.path)) + self.refresh_local_panel() + + @staticmethod + def _entry_from_tree( + entries: Sequence[FlightControllerLogFile | LocalFileEntry], + item_id: str, + ) -> FlightControllerLogFile | LocalFileEntry | None: + """Resolve a stable Treeview item id to its application entry.""" + if not str(item_id).isdigit(): + return None + index = int(item_id) + return entries[index] if 0 <= index < len(entries) else None + + def _selected_remote_entries(self) -> list[FlightControllerLogFile]: + """Return selected remote entries, including directories.""" + selected: list[FlightControllerLogFile] = [] + for item_id in self.remote_tree.selection(): + entry = self._entry_from_tree(self.remote_entries, item_id) + if isinstance(entry, FlightControllerLogFile): + selected.append(entry) + return selected + + def _selected_local_entries(self) -> list[LocalFileEntry]: + """Return selected local entries, including directories.""" + selected: list[LocalFileEntry] = [] + for item_id in self.local_tree.selection(): + entry = self._entry_from_tree(self.local_entries, item_id) + if isinstance(entry, LocalFileEntry): + selected.append(entry) + return selected + + @staticmethod + def _safe_name(name: str) -> bool: + """Return whether a user-provided rename is one safe path component.""" + return bool(name) and name not in {".", ".."} and "/" not in name and "\\" not in name + + def _progress_window(self, title: str, message: str) -> ProgressWindow: + """Create a standard application progress window.""" + return self.ui.create_progress_window(self.root, title, message, False) # noqa: FBT003 + + def _on_cancel_or_close(self) -> None: + """Cancel an active transfer, or close the browser when idle.""" + cancel_event = getattr(self, "_operation_cancel_event", None) + if cancel_event is not None: + cancel_event.set() + self.cancel_button.configure(text=_("Cancelling…"), state="disabled") + return + self.root.destroy() + + def _start_background_operation( + self, + title: str, + message: str, + worker: Callable[[Callable[[int, int], None], Event], tuple[list[str], list[str], bool]], + completion: Callable[[list[str], list[str], bool], None], + ) -> None: + """Run one blocking MAVFTP operation away from Tk's event loop.""" + if getattr(self, "_operation_active", False): + return + + cancel_event = Event() + + def report_progress(current: int, total: int) -> None: + if hasattr(self, "root"): + self._operation_queue.put(("progress", (current, total))) + elif self._operation_progress is not None: + self._operation_progress.update_progress_bar(current, total) + + def run() -> None: + try: + result = worker(report_progress, cancel_event) + except Exception as error: # pylint: disable=broad-exception-caught + result = ([], [str(error)], cancel_event.is_set()) + self._operation_queue.put(("done", result)) + + # Unit-level tests construct this class with __new__ and no Tk root. + # Keep that seam synchronous while real windows always use a worker. + if not hasattr(self, "root"): + self._operation_progress = self._progress_window(title, message) + result = worker(report_progress, cancel_event) + self._operation_progress.destroy() + completion(*result) + return + + self._operation_active = True + self._operation_cancel_event = cancel_event + self._operation_completion = completion + self._operation_progress = self._progress_window(title, message) + self.cancel_button.configure(text=_("Cancel operation"), state="normal") + self._operation_thread = Thread(target=run, name="mavftp-transfer", daemon=True) + self._operation_thread.start() + self.root.after(50, self._poll_background_operation) + + def _poll_background_operation(self) -> None: + """Apply worker progress and completion messages on the Tk thread.""" + try: + while True: + kind, payload = self._operation_queue.get_nowait() + if kind == "progress" and self._operation_progress is not None: + current, total = cast("tuple[int, int]", payload) + self._operation_progress.update_progress_bar(current, total) + elif kind == "done": + succeeded, failed, cancelled = cast("tuple[list[str], list[str], bool]", payload) + if self._operation_progress is not None: + self._operation_progress.destroy() + self._operation_progress = None + self._operation_thread = None + self._operation_cancel_event = None + self._operation_active = False + self.cancel_button.configure(text=_("Cancel"), state="normal") + completion = getattr(self, "_operation_completion", None) + self._operation_completion = None + if completion is not None: + completion(succeeded, failed, cancelled) + return + except queue.Empty: + pass + except tk.TclError: + return + self.root.after(50, self._poll_background_operation) + + def _show_summary(self, title: str, succeeded: list[str], failed: list[str], cancelled: bool = False) -> None: + """Show a compact per-entry operation summary.""" + lines = [(_("Succeeded: %s") % name) for name in succeeded] + lines.extend(_("Failed: %s") % name for name in failed) + if cancelled: + lines.append(_("Cancelled by user.")) + (self.ui.show_error if failed else self.ui.show_info)(title, "\n".join(lines) or _("No entries processed.")) + + @staticmethod + def _remote_path_is_in_log_scope(remote_path: str) -> bool: + """Return whether a remote path is inside the default log directory.""" + normalized = posixpath.normpath(remote_path.replace("\\", "/")) + return normalized == "/APM/LOGS" or normalized.startswith("/APM/LOGS/") + + def _confirm_remote_mutation_scope(self, operation: str, remote_paths: Sequence[str]) -> bool: + """Warn before a mutation leaves the normal `/APM/LOGS/` scope.""" + outside_scope = tuple(path for path in remote_paths if not self._remote_path_is_in_log_scope(path)) + if not outside_scope: + return True + paths = "\n".join(outside_scope[:8]) + if len(outside_scope) > 8: + paths += "\n…" + self.ui.show_warning( + _("Warning: outside log directory"), + _( + "The following remote paths are outside /APM/LOGS/:\n\n" + "%(paths)s\n\n" + "The %(operation)s operation can modify flight-controller files outside the log directory." + ) + % {"paths": paths, "operation": operation}, + ) + return self.ui.ask_yesno( + _("Confirm remote file operation"), + _("Proceed with %(operation)s outside /APM/LOGS/?") % {"operation": operation}, + ) + + @staticmethod + def _cancellable_progress( + report_progress: Callable[[int, int], None], + cancel_event: Event, + ) -> Callable[[int, int], None]: + """Adapt backend progress callbacks to the cancellable worker.""" + + def callback(current: int, total: int) -> None: + if cancel_event.is_set(): + raise _TransferCancelledError + report_progress(current, total) + + return callback + + def _remote_download_plan( + self, + entry: FlightControllerLogFile, + local_target: Path, + failures: list[str] | None = None, + ) -> tuple[list[Path], list[tuple[FlightControllerLogFile, Path]]]: + """Recursively expand one remote entry into local directories and files.""" + if not entry.is_directory: + if not self._safe_remote_entry_name(entry.name): + if failures is not None: + failures.append(entry.remote_path) + return [], [] + return [], [(entry, local_target)] + directories = [local_target] + files: list[tuple[FlightControllerLogFile, Path]] = [] + try: + children = self.parameter_editor.get_remote_files(entry.remote_path) + except Exception: # pylint: disable=broad-exception-caught + if failures is not None: + failures.append(entry.remote_path) + return directories, files + for child in children: + if child.name == "..": + continue + if not self._safe_remote_entry_name(child.name): + if failures is not None: + failures.append(child.remote_path) + continue + child_dirs, child_files = self._remote_download_plan(child, local_target / child.name, failures) + directories.extend(child_dirs) + files.extend(child_files) + return directories, files + + def download_selected_remote_entries(self) -> None: # noqa: PLR0915 + """Recursively download selected remote entries into the local panel directory.""" + if getattr(self, "_operation_active", False): + return + selected = [entry for entry in self._selected_remote_entries() if entry.name != ".."] + if not selected: + return + local_directory = Path(self.local_directory_var.get()).expanduser() + if not local_directory.is_dir(): + self.ui.show_error(_("Download error"), _("The selected local directory does not exist.")) + return + directories: list[Path] = [] + files: list[tuple[FlightControllerLogFile, Path]] = [] + failed: list[str] = [] + for entry in selected: + if not self._safe_remote_entry_name(entry.name): + failed.append(entry.remote_path) + continue + child_dirs, child_files = self._remote_download_plan(entry, local_directory / entry.name, failed) + directories.extend(child_dirs) + files.extend(child_files) + conflicts = [ + path + for path in [*(path for _entry, path in files), *directories] + if path.exists() and (path.is_file() or (path in directories and not path.is_dir())) + ] + if conflicts and not self.ui.ask_yesno( + _("Overwrite existing local entries?"), + _("Some local entries already exist. Overwrite them?"), + ): + return + total = max(sum(max(entry.size_bytes, 1) for entry, _path in files), 1) + + def worker( + report_progress: Callable[[int, int], None], + cancel_event: Event, + ) -> tuple[list[str], list[str], bool]: + completed = 0 + succeeded: list[str] = [] + worker_failed = list(failed) + for directory in directories: + if cancel_event.is_set(): + return succeeded, worker_failed, True + try: + if directory.exists() and not directory.is_dir(): + worker_failed.append(str(directory)) + continue + directory.mkdir(parents=True, exist_ok=True) + except OSError: + worker_failed.append(str(directory)) + for entry, target in files: + if cancel_event.is_set(): + return succeeded, worker_failed, True + units = max(entry.size_bytes, 1) + if target.parent.exists() and not target.parent.is_dir(): + worker_failed.append(entry.remote_path) + completed += units + report_progress(completed, total) + continue + callback = self._cancellable_progress( + lambda current, maximum, offset=completed, size=units: report_progress( + min(total, int(offset + size * (current / maximum if maximum else 0.0))), + total, + ), + cancel_event, + ) + try: + success = self.parameter_editor.download_remote_file(entry.remote_path, str(target), callback) + except _TransferCancelledError: + return succeeded, worker_failed, True + except Exception: # pylint: disable=broad-exception-caught + success = False + if cancel_event.is_set(): + return succeeded, worker_failed, True + if success: + succeeded.append(entry.remote_path) + else: + worker_failed.append(entry.remote_path) + completed += units + report_progress(completed, total) + return succeeded, worker_failed, False + + def completion(succeeded: list[str], worker_failed: list[str], cancelled: bool) -> None: + self._show_summary(_("Download summary"), succeeded, worker_failed, cancelled) + self.refresh_local_panel() + + self._start_background_operation( + _("Downloading selected entries"), + _("Downloaded {} of {} bytes"), + worker, + completion, + ) + + def _local_upload_plan(self, entry: LocalFileEntry, remote_target: str) -> tuple[list[str], list[tuple[Path, str, int]]]: + """Recursively expand one local entry into remote directories and files.""" + if not entry.is_directory: + return [], [(entry.path, remote_target, entry.size_bytes)] + directories = [remote_target] + files: list[tuple[Path, str, int]] = [] + try: + children = sorted(entry.path.iterdir(), key=lambda path: path.name.casefold()) + except OSError: + return directories, files + for child in children: + if child.is_symlink(): + continue + try: + is_directory = child.is_dir() + size_bytes = 0 if is_directory else child.stat().st_size + except OSError: + continue + child_entry = LocalFileEntry(child.name, child, size_bytes, is_directory) + child_dirs, child_files = self._local_upload_plan( + child_entry, + posixpath.join(remote_target.rstrip("/"), child.name), + ) + directories.extend(child_dirs) + files.extend(child_files) + return directories, files + + def upload_selected_local_entries(self) -> None: + """Recursively upload selected local entries to the remote panel directory.""" + if getattr(self, "_operation_active", False): + return + selected = [entry for entry in self._selected_local_entries() if entry.name != ".."] + if not selected: + return + remote_directory = self.remote_directory_var.get().rstrip("/") or "/" + if not self._safe_remote_directory(remote_directory): + self.ui.show_error(_("Upload error"), _("The remote destination must be an absolute directory path.")) + return + if not self._confirm_remote_mutation_scope(_("upload"), (remote_directory,)): + return + if not self.ui.ask_yesno( + _("Upload selected entries?"), + _("Uploading may overwrite remote files. Continue?"), + ): + return + directories: list[str] = [] + files: list[tuple[Path, str, int]] = [] + for entry in selected: + target = posixpath.join(remote_directory, entry.name) + child_dirs, child_files = self._local_upload_plan(entry, target) + directories.extend(child_dirs) + files.extend(child_files) + total = max(sum(max(size, 1) for _path, _remote, size in files), 1) + + def worker( + report_progress: Callable[[int, int], None], + cancel_event: Event, + ) -> tuple[list[str], list[str], bool]: + completed = 0 + succeeded: list[str] = [] + failed: list[str] = [] + for directory in directories: + if cancel_event.is_set(): + return succeeded, failed, True + if not self._call_remote_bool(self.parameter_editor.make_remote_directory, directory): + failed.append(directory) + for local_path, remote_path, size in files: + if cancel_event.is_set(): + return succeeded, failed, True + units = max(size, 1) + callback = self._cancellable_progress( + lambda current, maximum, offset=completed, file_size=units: report_progress( + min(total, int(offset + file_size * (current / maximum if maximum else 0.0))), + total, + ), + cancel_event, + ) + try: + success = self.parameter_editor.upload_file_to_fc(str(local_path), remote_path, callback) + except _TransferCancelledError: + return succeeded, failed, True + except Exception: # pylint: disable=broad-exception-caught + success = False + if cancel_event.is_set(): + return succeeded, failed, True + if success: + succeeded.append(remote_path) + else: + failed.append(remote_path) + completed += units + report_progress(completed, total) + return succeeded, failed, False + + def completion(succeeded: list[str], failed: list[str], cancelled: bool) -> None: + self._show_summary(_("Upload summary"), succeeded, failed, cancelled) + self.refresh_remote_panel() + + self._start_background_operation( + _("Uploading selected entries"), + _("Uploaded {} of {} bytes"), + worker, + completion, + ) + + def _delete_remote_entry(self, entry: FlightControllerLogFile, succeeded: list[str], failed: list[str]) -> None: + """Delete one remote file or an empty remote directory.""" + if entry.is_directory: + try: + children = [child for child in self.parameter_editor.get_remote_files(entry.remote_path) if child.name != ".."] + except Exception: # pylint: disable=broad-exception-caught + failed.append(entry.remote_path) + return + if children: + failed.append(entry.remote_path) + return + if self._call_remote_bool(self.parameter_editor.delete_remote_path, entry.remote_path, entry.is_directory): + succeeded.append(entry.remote_path) + else: + failed.append(entry.remote_path) + + def delete_selected_remote_entries(self) -> None: + """Delete selected remote files and empty directories after confirmation.""" + selected = [entry for entry in self._selected_remote_entries() if entry.name != ".."] + if selected and not self._confirm_remote_mutation_scope(_("delete"), tuple(entry.remote_path for entry in selected)): + return + if not selected or not self.ui.ask_yesno( + _("Delete remote entries?"), + _("Delete selected files and empty directories?"), + ): + return + succeeded: list[str] = [] + failed: list[str] = [] + for entry in selected: + self._delete_remote_entry(entry, succeeded, failed) + self._show_summary(_("Remote delete summary"), succeeded, failed) + self.refresh_remote_panel() + + def delete_selected_local_entries(self) -> None: + """Delete selected local files and empty directories after confirmation.""" + selected = [entry for entry in self._selected_local_entries() if entry.name != ".."] + if not selected or not self.ui.ask_yesno( + _("Delete local entries?"), + _("Delete selected files and empty directories?"), + ): + return + succeeded: list[str] = [] + failed: list[str] = [] + for entry in selected: + if self._delete_local_entry(entry): + succeeded.append(str(entry.path)) + else: + failed.append(str(entry.path)) + self._show_summary(_("Local delete summary"), succeeded, failed) + self.refresh_local_panel() + + def rename_selected_remote_entry(self) -> None: + """Rename one selected remote entry.""" + selected = [entry for entry in self._selected_remote_entries() if entry.name != ".."] + if len(selected) != 1: + self.ui.show_error(_("Rename error"), _("Select exactly one remote entry to rename.")) + return + entry = selected[0] + new_name = self.ui.askstring(_("Rename remote entry"), _("New name:"), initialvalue=entry.name, parent=self.root) + if new_name is None: + return + if not self._safe_name(new_name): + self.ui.show_error(_("Rename error"), _("The new name must be one file or directory name.")) + return + new_path = posixpath.join(posixpath.dirname(entry.remote_path.rstrip("/")), new_name) + if not self._confirm_remote_mutation_scope(_("rename"), (entry.remote_path, new_path)): + return + if self.parameter_editor.rename_remote_path(entry.remote_path, new_path): + self.ui.show_info(_("Rename summary"), _("Renamed %(old)s to %(new)s.") % {"old": entry.name, "new": new_name}) + self.refresh_remote_panel() + else: + self.ui.show_error(_("Rename error"), _("Could not rename the remote entry.")) + + def rename_selected_local_entry(self) -> None: + """Rename one selected local entry.""" + selected = [entry for entry in self._selected_local_entries() if entry.name != ".."] + if len(selected) != 1: + self.ui.show_error(_("Rename error"), _("Select exactly one local entry to rename.")) + return + entry = selected[0] + new_name = self.ui.askstring(_("Rename local entry"), _("New name:"), initialvalue=entry.name, parent=self.root) + if new_name is None: + return + if not self._safe_name(new_name): + self.ui.show_error(_("Rename error"), _("The new name must be one file or directory name.")) + return + target = entry.path.with_name(new_name) + if target.exists(): + self.ui.show_error(_("Rename error"), _("The target name already exists.")) + return + try: + entry.path.rename(target) + except OSError as error: + self.ui.show_error(_("Rename error"), str(error)) + return + self.ui.show_info(_("Rename summary"), _("Renamed %(old)s to %(new)s.") % {"old": entry.name, "new": new_name}) + self.refresh_local_panel() + + # Compatibility workflow retained for existing callers of the original single/multi download UI. + def download_selected_files(self) -> None: + """Download selected regular files using the original destination dialogs.""" + selected_files = self._selected_files() + if not selected_files: + return + if len(selected_files) == 1: + destination = self.ui.asksaveasfilename( + title=_("Save flight-controller file as"), + initialfile=selected_files[0].name, + filetypes=[(_("All files"), "*.*"), (_("Binary log files"), "*.bin")], + ) + destination_is_directory = False + else: + destination = self.ui.askdirectory(title=_("Select local destination directory")) + destination_is_directory = True + if not destination: + return + progress_window = self._progress_window(_("Downloading flight-controller file(s)"), _("Downloaded {} of {} bytes")) + try: + self.parameter_editor.download_selected_bin_logs_workflow( + selected_files=selected_files, + destination=destination, + destination_is_directory=destination_is_directory, + ask_overwrite=self.ui.ask_yesno, + show_error=self.ui.show_error, + show_info=self.ui.show_info, + progress_callback=progress_window.update_progress_bar, + ) + finally: + progress_window.destroy() + + def _selected_files(self) -> list[FlightControllerLogFile]: + """Return selected regular files for the compatibility workflow.""" + tree = self.tree + selected: list[FlightControllerLogFile] = [] + for item_id in tree.selection(): + entry = self._entry_from_tree(self.remote_files, item_id) + if isinstance(entry, FlightControllerLogFile) and not entry.is_directory: + selected.append(entry) + return selected + + @staticmethod + def _delete_local_entry(entry: LocalFileEntry) -> bool: + """Delete one local entry and return whether it succeeded.""" + try: + if entry.is_directory: + entry.path.rmdir() + else: + entry.path.unlink() + except OSError: + return False + return True + + def _on_tree_selection_change(self, _event: tk.Event | None = None) -> None: + """Retained for compatibility with the original remote-only window.""" + download_button = getattr(self, "download_button", None) + if download_button is not None: + download_button.configure(state="normal" if self.tree.selection() else "disabled") + + def _sort_by_column(self, column: str, reverse: bool) -> None: + """Retained compatibility wrapper for the original remote-only sorter.""" + if hasattr(self, "remote_tree") and hasattr(self, "remote_entries"): + self._sort_panel_by_column("remote", column, reverse) + return + rows = [(self._panel_sort_key(self.remote_files, item_id, column), item_id) for item_id in self.tree.get_children("")] + rows.sort(key=lambda row: row[0], reverse=reverse) + for position, (_key, item_id) in enumerate(rows): + self.tree.move(item_id, "", position) + + @staticmethod + def _safe_remote_entry_name(name: str) -> bool: + """Return whether a remote listing name is safe as one path component.""" + return bool(name) and name not in {".", ".."} and "/" not in name and "\\" not in name + + @staticmethod + def _safe_remote_directory(directory: str) -> bool: + """Return whether a remote destination is an absolute directory path.""" + return bool(directory) and directory.startswith("/") and ".." not in directory.split("/") + + @staticmethod + def _call_remote_bool(callback: Callable[..., bool], *args: object) -> bool: + """Call a remote-operation callback without aborting a batch on one exception.""" + try: + return bool(callback(*args)) + except Exception: # pylint: disable=broad-exception-caught + return False + + def download_last_flight_log(self) -> None: + """Download the last flight log without blocking the Tk event loop.""" + if getattr(self, "_operation_active", False): + return + if not self.parameter_editor.is_fc_connected: + self.ui.show_error(_("Error"), _("No flight controller connected")) + return + if not self.parameter_editor.is_mavftp_supported: + self.ui.show_error(_("Error"), _("MAVFTP is not supported by the flight controller")) + return + filename = self.ui.asksaveasfilename( + title=_("Save flight log as"), + defaultextension=".bin", + filetypes=[(_("Binary log files"), "*.bin"), (_("All files"), "*.*")], + ) + if not filename: + return + + def worker( + report_progress: Callable[[int, int], None], + cancel_event: Event, + ) -> tuple[list[str], list[str], bool]: + callback = self._cancellable_progress(report_progress, cancel_event) + try: + success = self.parameter_editor.download_last_flight_log(filename, callback) + except _TransferCancelledError: + return [], [], True + except Exception: # pylint: disable=broad-exception-caught + success = False + if cancel_event.is_set(): + return [], [], True + return ([filename], []) if success else ([], [filename], False) + + def completion(succeeded: list[str], failed: list[str], cancelled: bool) -> None: + self._show_summary(_("Download summary"), succeeded, failed, cancelled) + self.refresh_local_panel() + + self._start_background_operation( + _("Downloading Flight Log"), + _("Downloaded {}% from {}%"), + worker, + completion, + ) diff --git a/ardupilot_methodic_configurator/frontend_tkinter_parameter_editor.py b/ardupilot_methodic_configurator/frontend_tkinter_parameter_editor.py index bb98df04a..144827b5d 100755 --- a/ardupilot_methodic_configurator/frontend_tkinter_parameter_editor.py +++ b/ardupilot_methodic_configurator/frontend_tkinter_parameter_editor.py @@ -26,7 +26,7 @@ from logging import warning as logging_warning from sys import exit as sys_exit from sys import platform as sys_platform -from tkinter import filedialog, ttk +from tkinter import filedialog, simpledialog, ttk from typing import TYPE_CHECKING, Optional, Protocol, Union, cast # from logging import critical as logging_critical @@ -55,6 +55,7 @@ ) from ardupilot_methodic_configurator.frontend_tkinter_component_editor import ComponentEditorWindow from ardupilot_methodic_configurator.frontend_tkinter_directory_selection import VehicleDirectorySelectionWidgets +from ardupilot_methodic_configurator.frontend_tkinter_download_bin_logs import DownloadBinLogsWindow from ardupilot_methodic_configurator.frontend_tkinter_fc_banner_window import FlightControllerBannerWindow from ardupilot_methodic_configurator.frontend_tkinter_font import get_safe_font_config from ardupilot_methodic_configurator.frontend_tkinter_log_availability import LogAvailabilityReportWindow @@ -109,6 +110,8 @@ def __init__( # noqa: PLR0913, PLR0917 # pylint: disable=too-many-arguments, to extract_log_data: Callable[[str, Callable[[int, int], None] | None], LogData], analyze_log_data_callback: Callable[..., LogSummary], load_apm_doc: Callable[[str, str, str], APMDoc | None], + askdirectory: Callable[..., str] | None = None, + askstring: Callable[..., str | None] | None = None, ) -> None: self.create_progress_window = create_progress_window self.ask_yesno = ask_yesno @@ -118,6 +121,8 @@ def __init__( # noqa: PLR0913, PLR0917 # pylint: disable=too-many-arguments, to self.show_info = show_info self.asksaveasfilename = asksaveasfilename self.askopenfilename = askopenfilename + self.askdirectory = askdirectory or filedialog.askdirectory + self.askstring = askstring or simpledialog.askstring self.sys_exit = exit_callback self.extract_log_data = extract_log_data self.analyze_log_data = analyze_log_data_callback @@ -153,6 +158,8 @@ def _load_apm_doc(vehicle_dir: str, vehicle_type: str, firmware_version: str) -> extract_log_data=extract_log, analyze_log_data_callback=analyze_log_data, load_apm_doc=_load_apm_doc, + askdirectory=filedialog.askdirectory, + askstring=simpledialog.askstring, ) def upload_params_with_progress( @@ -557,11 +564,11 @@ def _create_parameter_area_widgets(self) -> None: else _("No flight controller connected, upload not available"), ) - # Create download last flight log button + # Create download .bin log files button download_log_button = ttk.Button( buttons_frame, - text=_("Download last flight log"), - command=self.on_download_last_flight_log_click, + text=_("Download .bin log file(s)"), + command=self.on_download_bin_logs_click, ) download_log_button.configure( state=( @@ -573,10 +580,7 @@ def _create_parameter_area_widgets(self) -> None: download_log_button.pack(side=tk.LEFT, padx=(8, 8)) # Add padding on both sides of the download log button show_tooltip( download_log_button, - _( - "Download the last flight log from the flight controller\n" - "This will save the previous flight log to a file on your computer for analysis" - ) + _("Browse files in the flight controller log directory and download one or more files") if (self.parameter_editor.is_fc_connected and self.parameter_editor.is_mavftp_supported) else _("No flight controller connected or MAVFTP not supported"), ) @@ -1541,6 +1545,10 @@ def _upload_params(self, selected_params: dict, upload_callback: Callable[..., b logging_exception("Parameter upload failed") return False + def on_download_bin_logs_click(self) -> None: + """Open the modal window for browsing and downloading FC log files.""" + DownloadBinLogsWindow(self.root, self.parameter_editor, self.ui) + def on_download_last_flight_log_click(self) -> None: """Handle the download last flight log button click.""" # Create a progress window for the download diff --git a/tests/test_backend_flightcontroller_factory_mavftp.py b/tests/test_backend_flightcontroller_factory_mavftp.py index ebde4bd6e..2fac35dcb 100755 --- a/tests/test_backend_flightcontroller_factory_mavftp.py +++ b/tests/test_backend_flightcontroller_factory_mavftp.py @@ -20,6 +20,31 @@ create_mavftp, create_mavftp_safe, ) +from ardupilot_methodic_configurator.backend_mavftp import FTP_OP, OP_Ack, OP_ResetSessions + + +def _mock_master() -> MagicMock: + """Create a mock connection that can complete MAVFTP session reset.""" + master = MagicMock() + master.source_system = 1 + master.source_component = 1 + reply = FTP_OP( + seq=1, + session=0, + opcode=OP_Ack, + size=0, + req_opcode=OP_ResetSessions, + burst_complete=0, + offset=0, + payload=None, + ) + packet = MagicMock() + packet.get_type.return_value = "FILE_TRANSFER_PROTOCOL" + packet.target_system = 1 + packet.target_component = 1 + packet.payload = reply.pack() + master.recv_match.return_value = packet + return master class TestCreateMavftpFactory: @@ -35,7 +60,7 @@ def test_user_can_create_mavftp_with_valid_connection(self) -> None: AND: MAVFTP should be initialized with correct target parameters """ # Given: Valid MAVLink connection - mock_master = MagicMock() + mock_master = _mock_master() mock_master.target_system = 1 mock_master.target_component = 1 @@ -67,7 +92,7 @@ def test_create_mavftp_passes_target_system_to_mavftp(self) -> None: THEN: MAVFTP should be initialized with correct target_system """ # Given: Connection with specific target system - mock_master = MagicMock() + mock_master = _mock_master() mock_master.target_system = 42 mock_master.target_component = 1 @@ -86,7 +111,7 @@ def test_create_mavftp_passes_target_component_to_mavftp(self) -> None: THEN: MAVFTP should be initialized with correct target_component """ # Given: Connection with specific target component - mock_master = MagicMock() + mock_master = _mock_master() mock_master.target_system = 1 mock_master.target_component = 191 # MAV_COMP_ID_AUTOPILOT @@ -110,7 +135,7 @@ def test_user_can_create_mavftp_safe_with_valid_connection(self) -> None: AND: Return value should not be None """ # Given: Valid MAVLink connection - mock_master = MagicMock() + mock_master = _mock_master() mock_master.target_system = 1 mock_master.target_component = 1 @@ -147,7 +172,7 @@ def test_create_mavftp_safe_returns_none_when_mavftp_unavailable(self) -> None: # Note: This test validates the safety check for MAVFTP availability # In normal operation, MAVFTP will be imported, but the function has # defensive checks for when it might not be available - mock_master = MagicMock() + mock_master = _mock_master() mock_master.target_system = 1 mock_master.target_component = 1 @@ -166,7 +191,7 @@ def test_create_mavftp_safe_passes_target_system_to_mavftp(self) -> None: THEN: MAVFTP should be initialized with correct target_system """ # Given: Connection with specific target system - mock_master = MagicMock() + mock_master = _mock_master() mock_master.target_system = 99 mock_master.target_component = 1 @@ -185,7 +210,7 @@ def test_create_mavftp_safe_passes_target_component_to_mavftp(self) -> None: THEN: MAVFTP should be initialized with correct target_component """ # Given: Connection with specific target component - mock_master = MagicMock() + mock_master = _mock_master() mock_master.target_system = 1 mock_master.target_component = 50 @@ -244,7 +269,7 @@ def test_create_mavftp_safe_returns_none_when_initialization_fails(self) -> None THEN: The exception should not escape to the UI workflow AND: None should be returned so callers can fail gracefully """ - mock_master = MagicMock() + mock_master = _mock_master() mock_master.target_system = 1 mock_master.target_component = 1 @@ -269,7 +294,7 @@ def test_create_mavftp_with_zero_target_system(self) -> None: THEN: MAVFTP should be created (0 is valid, though unusual) """ # Given: Connection with target_system = 0 - mock_master = MagicMock() + mock_master = _mock_master() mock_master.target_system = 0 mock_master.target_component = 1 @@ -288,7 +313,7 @@ def test_create_mavftp_with_max_target_ids(self) -> None: THEN: MAVFTP should be created successfully """ # Given: Connection with maximum IDs - mock_master = MagicMock() + mock_master = _mock_master() mock_master.target_system = 255 # Max system ID mock_master.target_component = 255 # Max component ID @@ -325,7 +350,7 @@ def test_create_mavftp_with_connection_attributes(self) -> None: AND: No AttributeError should occur """ # Given: Connection with required attributes (MagicMock includes mav attribute) - mock_master = MagicMock() + mock_master = _mock_master() mock_master.target_system = 10 mock_master.target_component = 20 diff --git a/tests/test_backend_flightcontroller_files.py b/tests/test_backend_flightcontroller_files.py index 12e936f88..bd7e29c7d 100755 --- a/tests/test_backend_flightcontroller_files.py +++ b/tests/test_backend_flightcontroller_files.py @@ -20,6 +20,7 @@ import pytest from ardupilot_methodic_configurator.backend_flightcontroller_files import FlightControllerFiles +from ardupilot_methodic_configurator.backend_mavftp import DirectoryEntry from ardupilot_methodic_configurator.data_model_flightcontroller_info import FlightControllerInfo @@ -601,12 +602,12 @@ class ListingResult: """Directory listing result.""" def __init__(self) -> None: - self.directory_listing: dict[str, dict[str, object]] = { - "00000005.BIN": {}, - "README.TXT": {}, - "00000012.BIN": {}, - "junk": {}, - } + self.directory_listing: list[DirectoryEntry] = [ + DirectoryEntry("00000005.BIN", is_dir=False, size_b=0), + DirectoryEntry("README.TXT", is_dir=False, size_b=0), + DirectoryEntry("00000012.BIN", is_dir=False, size_b=0), + DirectoryEntry("junk", is_dir=False, size_b=0), + ] mock_mavftp.cmd_list.return_value = ListingResult() @@ -747,10 +748,10 @@ class ListingResult: """List the FTP directory contents.""" def __init__(self) -> None: - self.directory_listing: dict[str, dict[str, object]] = { - FakeName("12BADVAL.BIN"): {}, - "00000099.BIN": {}, - } + self.directory_listing: list[DirectoryEntry] = [ + DirectoryEntry(FakeName("12BADVAL.BIN"), is_dir=False, size_b=0), + DirectoryEntry("00000099.BIN", is_dir=False, size_b=0), + ] mock_mavftp.cmd_list.return_value = ListingResult() @@ -767,7 +768,10 @@ class ListingResult: """List the FTP directory contents.""" def __init__(self) -> None: - self.directory_listing: dict[str, dict[str, object]] = {"README.TXT": {}, "notes.log": {}} + self.directory_listing: list[DirectoryEntry] = [ + DirectoryEntry("README.TXT", is_dir=False, size_b=0), + DirectoryEntry("notes.log", is_dir=False, size_b=0), + ] mock_mavftp.cmd_list.return_value = ListingResult() diff --git a/tests/test_backend_flightcontroller_sitl.py b/tests/test_backend_flightcontroller_sitl.py index 178b9c022..be6a7297a 100755 --- a/tests/test_backend_flightcontroller_sitl.py +++ b/tests/test_backend_flightcontroller_sitl.py @@ -54,7 +54,7 @@ ) from ardupilot_methodic_configurator.backend_flightcontroller_factory_mavlink import SystemMavlinkConnectionFactory from ardupilot_methodic_configurator.backend_flightcontroller_files import FlightControllerFiles -from ardupilot_methodic_configurator.backend_mavftp import ERR_FileExists +from ardupilot_methodic_configurator.backend_mavftp import FtpError from ardupilot_methodic_configurator.data_model_flightcontroller_info import FlightControllerInfo if TYPE_CHECKING: @@ -87,7 +87,7 @@ def _ensure_remote_logs_directory(mavftp) -> None: if result is None: continue error_code = getattr(result, "error_code", 0) - if error_code not in (0, ERR_FileExists): + if error_code not in (FtpError.Success, FtpError.FileExists): pytest.skip(f"Unable to create required directory {directory}: MAVFTP error {error_code}") diff --git a/tests/test_backend_mavftp.py b/tests/test_backend_mavftp.py index 909987dce..1f949ee3e 100755 --- a/tests/test_backend_mavftp.py +++ b/tests/test_backend_mavftp.py @@ -22,35 +22,17 @@ from pymavlink import mavutil -# from ardupilot_methodic_configurator.backend_mavftp import ERR_NoErrorCodeInPayload -# from ardupilot_methodic_configurator.backend_mavftp import ERR_NoErrorCodeInNack -# from ardupilot_methodic_configurator.backend_mavftp import ERR_NoFilesystemErrorInPayload -# from ardupilot_methodic_configurator.backend_mavftp import ERR_PayloadTooLarge -# from ardupilot_methodic_configurator.backend_mavftp import ERR_InvalidOpcode +# from ardupilot_methodic_configurator.backend_mavftp import FtpError from ardupilot_methodic_configurator.backend_mavftp import ( FTP_OP, MAVFTP, - ERR_EndOfFile, - ERR_Fail, - ERR_FailErrno, - ERR_FailToOpenLocalFile, - ERR_FileExists, - ERR_FileNotFound, - ERR_FileProtected, - ERR_InvalidArguments, - ERR_InvalidDataSize, - ERR_InvalidErrorCode, - ERR_InvalidSession, - ERR_None, - ERR_NoSessionsAvailable, - ERR_PutAlreadyInProgress, - ERR_RemoteReplyTimeout, - ERR_UnknownCommand, + FtpError, MAVFTPReturn, OP_Ack, OP_ListDirectory, OP_Nack, OP_ReadFile, + OP_ResetSessions, ) PARAM_HEADER_STRUCT = struct.Struct(" result = self.mav_ftp.cmd_getparams(["values.param", "defaults.param"]) - assert result.error_code == ERR_Fail + assert result.error_code == FtpError.Fail + + def test_successful_callback_does_not_write_virtual_remote_path(self) -> None: + """A callback consumes the download without creating a local remote-path file.""" + self.mav_ftp.fh = BytesIO(b"data") + self.mav_ftp.filename = "param.pck?withdefaults=1" + self.mav_ftp.op_start = 1.0 + self.mav_ftp.read_gaps = [] + self.mav_ftp.reached_eof = True + self.mav_ftp.read_total = 4 + self.mav_ftp.requested_offset = 0 + self.mav_ftp.requested_size = 4 + self.mav_ftp.callback = lambda _file: MAVFTPReturn("GetParams", FtpError.Success) + + with patch("builtins.open", side_effect=AssertionError("callback data must not be published as a file")): + assert self.mav_ftp._MAVFTP__check_read_finished() # pylint: disable=protected-access def test_process_ftp_reply_propagates_getparams_callback_failure(self) -> None: """A parameter-decoding callback failure makes the transfer fail, enabling fallback.""" - callback_failure = MAVFTPReturn("GetParams", ERR_Fail) - packet_result = MAVFTPReturn("BurstReadFile", ERR_None) + callback_failure = MAVFTPReturn("GetParams", FtpError.Fail) + packet_result = MAVFTPReturn("BurstReadFile", FtpError.Success) self.mav_ftp.master = Mock() - self.mav_ftp.master.recv_match.return_value = Mock() + reply = FTP_OP( + seq=1, + session=0, + opcode=OP_Ack, + size=0, + req_opcode=OP_ResetSessions, + burst_complete=0, + offset=0, + payload=None, + ) + packet = Mock() + packet.get_type.return_value = "FILE_TRANSFER_PROTOCOL" + packet.target_system = 1 + packet.target_component = 1 + packet.payload = reply.pack() + self.mav_ftp.master.source_system = 1 + self.mav_ftp.master.source_component = 1 + self.mav_ftp.master.recv_match.return_value = packet def simulate_finished_transfer(_message: object) -> MAVFTPReturn: self.mav_ftp.callback_failure = callback_failure @@ -205,7 +219,7 @@ def test_getparams_read_error_returns_ftp_error_instead_of_exiting(self) -> None result = self.mav_ftp.cmd_getparams(["values.param", "defaults.param"]) # Assert (Then): The caller receives a recoverable FTP failure - assert result.error_code == ERR_Fail + assert result.error_code == FtpError.Fail @staticmethod def ftp_operation(seq: int, opcode: int, req_opcode: int, payload: bytearray) -> FTP_OP: @@ -223,65 +237,69 @@ def test_decode_ftp_ack_and_nack(self) -> None: }, { "name": "Generic Failure", - "op": self.ftp_operation(seq=2, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([ERR_Fail])), + "op": self.ftp_operation(seq=2, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([FtpError.Fail])), "expected_message": "ListDirectory failed, generic error", }, { "name": "System Error", "op": self.ftp_operation( - seq=3, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([ERR_FailErrno, 1]) + seq=3, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([FtpError.FailErrno, 1]) ), # System error 1 "expected_message": "ListDirectory failed, system error 1", }, { "name": "Invalid Data Size", "op": self.ftp_operation( - seq=4, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([ERR_InvalidDataSize]) + seq=4, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([FtpError.InvalidDataSize]) ), "expected_message": "ListDirectory failed, invalid data size", }, { "name": "Invalid Session", "op": self.ftp_operation( - seq=5, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([ERR_InvalidSession]) + seq=5, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([FtpError.InvalidSession]) ), "expected_message": "ListDirectory failed, session is not currently open", }, { "name": "No Sessions Available", "op": self.ftp_operation( - seq=6, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([ERR_NoSessionsAvailable]) + seq=6, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([FtpError.NoSessionsAvailable]) ), "expected_message": "ListDirectory failed, no sessions available", }, { "name": "End of File", - "op": self.ftp_operation(seq=7, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([ERR_EndOfFile])), + "op": self.ftp_operation( + seq=7, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([FtpError.EndOfFile]) + ), "expected_message": "ListDirectory failed, offset past end of file", }, { "name": "Unknown Command", "op": self.ftp_operation( - seq=8, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([ERR_UnknownCommand]) + seq=8, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([FtpError.UnknownCommand]) ), "expected_message": "ListDirectory failed, unknown command", }, { "name": "File Exists", - "op": self.ftp_operation(seq=9, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([ERR_FileExists])), + "op": self.ftp_operation( + seq=9, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([FtpError.FileExists]) + ), "expected_message": "ListDirectory failed, file/directory already exists", }, { "name": "File Protected", "op": self.ftp_operation( - seq=10, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([ERR_FileProtected]) + seq=10, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([FtpError.FileProtected]) ), "expected_message": "ListDirectory failed, file/directory is protected", }, { "name": "File Not Found", "op": self.ftp_operation( - seq=11, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([ERR_FileNotFound]) + seq=11, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([FtpError.FileNotFound]) ), "expected_message": "ListDirectory failed, file/directory not found", }, @@ -292,18 +310,22 @@ def test_decode_ftp_ack_and_nack(self) -> None: }, { "name": "No Error Code in Nack", - "op": self.ftp_operation(seq=13, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([ERR_None])), + "op": self.ftp_operation( + seq=13, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([FtpError.Success]) + ), "expected_message": "ListDirectory failed, no error code", }, { "name": "No Filesystem Error in Payload", - "op": self.ftp_operation(seq=14, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([ERR_FailErrno])), + "op": self.ftp_operation( + seq=14, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([FtpError.FailErrno]) + ), "expected_message": "ListDirectory failed, file-system error missing in payload", }, { "name": "Invalid Error Code", "op": self.ftp_operation( - seq=15, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([ERR_InvalidErrorCode]) + seq=15, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([FtpError.InvalidErrorCode]) ), "expected_message": "ListDirectory failed, invalid error code", }, @@ -320,14 +342,14 @@ def test_decode_ftp_ack_and_nack(self) -> None: { "name": "Unknown Opcode in Request", "op": self.ftp_operation( - seq=19, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([ERR_UnknownCommand]) + seq=19, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([FtpError.UnknownCommand]) ), # Assuming 100 is an unknown opcode "expected_message": "ListDirectory failed, unknown command", }, { "name": "Payload with System Error", "op": self.ftp_operation( - seq=20, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([ERR_FailErrno, 2]) + seq=20, opcode=OP_Nack, req_opcode=OP_ListDirectory, payload=bytes([FtpError.FailErrno, 2]) ), # System error 2 "expected_message": "ListDirectory failed, system error 2", }, @@ -360,7 +382,7 @@ def test_decode_ftp_ack_and_nack(self) -> None: self.log_stream.truncate(0) # Invalid Arguments - ret = MAVFTPReturn("Command arguments", ERR_InvalidArguments) + ret = MAVFTPReturn("Command arguments", FtpError.InvalidArguments) ret.display_message() log_output = self.log_stream.getvalue().strip() assert "Command arguments failed, invalid arguments" in log_output, "Expected invalid arguments message" @@ -380,7 +402,7 @@ def test_decode_ftp_ack_and_nack(self) -> None: self.log_stream.truncate(0) # Put already in progress - ret = MAVFTPReturn("Put", ERR_PutAlreadyInProgress) + ret = MAVFTPReturn("Put", FtpError.PutAlreadyInProgress) ret.display_message() log_output = self.log_stream.getvalue().strip() assert "Put failed, put already in progress" in log_output, "Expected put already in progress message" @@ -388,7 +410,7 @@ def test_decode_ftp_ack_and_nack(self) -> None: self.log_stream.truncate(0) # Fail to open local file - ret = MAVFTPReturn("Put", ERR_FailToOpenLocalFile) + ret = MAVFTPReturn("Put", FtpError.FailToOpenLocalFile) ret.display_message() log_output = self.log_stream.getvalue().strip() assert "Put failed, failed to open local file" in log_output, "Expected fail to open local file message" @@ -396,7 +418,7 @@ def test_decode_ftp_ack_and_nack(self) -> None: self.log_stream.truncate(0) # Remote Reply Timeout - ret = MAVFTPReturn("Put", ERR_RemoteReplyTimeout) + ret = MAVFTPReturn("Put", FtpError.RemoteReplyTimeout) ret.display_message() log_output = self.log_stream.getvalue().strip() assert "Put failed, remote reply timeout" in log_output, "Expected remote reply timeout message" diff --git a/tests/test_backend_mavftp_aux.py b/tests/test_backend_mavftp_aux.py index b922e16d3..1f71f34c8 100755 --- a/tests/test_backend_mavftp_aux.py +++ b/tests/test_backend_mavftp_aux.py @@ -16,27 +16,7 @@ from ardupilot_methodic_configurator.backend_mavftp import ( FTP_OP, - ERR_EndOfFile, - ERR_Fail, - ERR_FailErrno, - ERR_FailToOpenLocalFile, - ERR_FileExists, - ERR_FileNotFound, - ERR_FileProtected, - ERR_InvalidArguments, - ERR_InvalidDataSize, - ERR_InvalidErrorCode, - ERR_InvalidOpcode, - ERR_InvalidSession, - ERR_NoErrorCodeInNack, - ERR_NoErrorCodeInPayload, - ERR_NoFilesystemErrorInPayload, - ERR_None, - ERR_NoSessionsAvailable, - ERR_PayloadTooLarge, - ERR_PutAlreadyInProgress, - ERR_RemoteReplyTimeout, - ERR_UnknownCommand, + FtpError, MAVFTPReturn, MAVFTPSetting, MAVFTPSettings, @@ -274,18 +254,18 @@ class TestMAVFTPReturn(unittest.TestCase): def test_init(self) -> None: """Test initialization with different parameters.""" - ret = MAVFTPReturn("TestOp", ERR_None) + ret = MAVFTPReturn("TestOp", FtpError.Success) assert ret.operation_name == "TestOp" - assert ret.error_code == ERR_None + assert ret.error_code == FtpError.Success assert ret.system_error == 0 assert ret.invalid_error_code == 0 assert ret.invalid_opcode == 0 assert ret.invalid_payload_size == 0 # Test with all parameters - ret = MAVFTPReturn("TestOp", ERR_Fail, 1, 2, 3, 4) + ret = MAVFTPReturn("TestOp", FtpError.Fail, 1, 2, 3, 4) assert ret.operation_name == "TestOp" - assert ret.error_code == ERR_Fail + assert ret.error_code == FtpError.Fail assert ret.system_error == 1 assert ret.invalid_error_code == 2 assert ret.invalid_opcode == 3 @@ -293,15 +273,15 @@ def test_init(self) -> None: def test_return_code(self) -> None: """Test return_code property.""" - ret = MAVFTPReturn("TestOp", ERR_None) - assert ret.return_code == ERR_None + ret = MAVFTPReturn("TestOp", FtpError.Success) + assert ret.return_code == FtpError.Success - ret = MAVFTPReturn("TestOp", ERR_Fail) - assert ret.return_code == ERR_Fail + ret = MAVFTPReturn("TestOp", FtpError.Fail) + assert ret.return_code == FtpError.Fail def test_display_message_success(self) -> None: """Test display_message for successful operations.""" - ret = MAVFTPReturn("TestOp", ERR_None) + ret = MAVFTPReturn("TestOp", FtpError.Success) with self.assertLogs(level="INFO") as cm: ret.display_message() assert "TestOp succeeded" in cm.output[0] @@ -310,27 +290,27 @@ def test_display_message_errors(self) -> None: """Test display_message for various error conditions.""" error_test_cases = [ # ERROR level messages - (ERR_Fail, "TestOp failed, generic error", "ERROR"), - (ERR_FailErrno, "TestOp failed, system error 42", "ERROR"), - (ERR_InvalidDataSize, "TestOp failed, invalid data size", "ERROR"), - (ERR_InvalidSession, "TestOp failed, session is not currently open", "ERROR"), - (ERR_NoSessionsAvailable, "TestOp failed, no sessions available", "ERROR"), - (ERR_EndOfFile, "TestOp failed, offset past end of file", "ERROR"), - (ERR_UnknownCommand, "TestOp failed, unknown command", "ERROR"), - (ERR_NoErrorCodeInPayload, "TestOp failed, payload contains no error code", "ERROR"), - (ERR_NoErrorCodeInNack, "TestOp failed, no error code", "ERROR"), - (ERR_NoFilesystemErrorInPayload, "TestOp failed, file-system error missing in payload", "ERROR"), - (ERR_InvalidErrorCode, "TestOp failed, invalid error code 42", "ERROR"), - (ERR_PayloadTooLarge, "TestOp failed, payload is too long 42", "ERROR"), - (ERR_InvalidOpcode, "TestOp failed, invalid opcode 42", "ERROR"), - (ERR_InvalidArguments, "TestOp failed, invalid arguments", "ERROR"), - (ERR_PutAlreadyInProgress, "TestOp failed, put already in progress", "ERROR"), - (ERR_FailToOpenLocalFile, "TestOp failed, failed to open local file", "ERROR"), - (ERR_RemoteReplyTimeout, "TestOp failed, remote reply timeout", "ERROR"), + (FtpError.Fail, "TestOp failed, generic error", "ERROR"), + (FtpError.FailErrno, "TestOp failed, system error 42", "ERROR"), + (FtpError.InvalidDataSize, "TestOp failed, invalid data size", "ERROR"), + (FtpError.InvalidSession, "TestOp failed, session is not currently open", "ERROR"), + (FtpError.NoSessionsAvailable, "TestOp failed, no sessions available", "ERROR"), + (FtpError.EndOfFile, "TestOp failed, offset past end of file", "ERROR"), + (FtpError.UnknownCommand, "TestOp failed, unknown command", "ERROR"), + (FtpError.NoErrorCodeInPayload, "TestOp failed, payload contains no error code", "ERROR"), + (FtpError.NoErrorCodeInNack, "TestOp failed, no error code", "ERROR"), + (FtpError.NoFilesystemErrorInPayload, "TestOp failed, file-system error missing in payload", "ERROR"), + (FtpError.InvalidErrorCode, "TestOp failed, invalid error code 42", "ERROR"), + (FtpError.PayloadTooLarge, "TestOp failed, payload is too long 42", "ERROR"), + (FtpError.InvalidOpcode, "TestOp failed, invalid opcode 42", "ERROR"), + (FtpError.InvalidArguments, "TestOp failed, invalid arguments", "ERROR"), + (FtpError.PutAlreadyInProgress, "TestOp failed, put already in progress", "ERROR"), + (FtpError.FailToOpenLocalFile, "TestOp failed, failed to open local file", "ERROR"), + (FtpError.RemoteReplyTimeout, "TestOp failed, remote reply timeout", "ERROR"), # WARNING level messages - (ERR_FileExists, "TestOp failed, file/directory already exists", "WARNING"), - (ERR_FileProtected, "TestOp failed, file/directory is protected", "WARNING"), - (ERR_FileNotFound, "TestOp failed, file/directory not found", "WARNING"), + (FtpError.FileExists, "TestOp failed, file/directory already exists", "WARNING"), + (FtpError.FileProtected, "TestOp failed, file/directory is protected", "WARNING"), + (FtpError.FileNotFound, "TestOp failed, file/directory not found", "WARNING"), ] for error_code, expected_message, level in error_test_cases: diff --git a/tests/test_download_bin_logs.py b/tests/test_download_bin_logs.py new file mode 100755 index 000000000..43a7f011c --- /dev/null +++ b/tests/test_download_bin_logs.py @@ -0,0 +1,954 @@ +#!/usr/bin/env python3 + +""" +BDD tests for flight-controller log-file listing and download workflows. + +This file is part of ArduPilot Methodic Configurator. + +SPDX-FileCopyrightText: 2026 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +from pathlib import Path +from types import SimpleNamespace +from typing import Any, cast +from unittest.mock import ANY, MagicMock, call, patch + +from ardupilot_methodic_configurator.backend_flightcontroller_files import ( + FlightControllerFiles, + FlightControllerLogFile, +) +from ardupilot_methodic_configurator.backend_mavftp import DirectoryEntry +from ardupilot_methodic_configurator.data_model_flightcontroller_info import FlightControllerInfo +from ardupilot_methodic_configurator.data_model_parameter_editor import ParameterEditor +from ardupilot_methodic_configurator.frontend_tkinter_download_bin_logs import ( + DownloadBinLogsWindow, + LocalFileEntry, +) +from ardupilot_methodic_configurator.frontend_tkinter_parameter_editor import ParameterEditorWindow + + +def _files_manager() -> FlightControllerFiles: + """Build a connected MAVFTP-capable file manager for unit tests.""" + connection_manager = MagicMock() + connection_manager.master = MagicMock() + connection_manager.info = FlightControllerInfo() + connection_manager.info.is_mavftp_supported = True + return FlightControllerFiles(connection_manager=connection_manager) + + +def _parameter_editor_model() -> ParameterEditor: + """Build the minimum ParameterEditor state needed by log workflows.""" + model = ParameterEditor.__new__(ParameterEditor) + model._flight_controller = MagicMock() # pylint: disable=protected-access + model._flight_controller.master = MagicMock() # pylint: disable=protected-access + model._flight_controller.info.is_mavftp_supported = True # pylint: disable=protected-access + return model + + +class TestFlightControllerLogListing: + """Verify the user can browse all files in the remote log directory.""" + + def test_user_sees_all_regular_files_in_default_log_directory(self) -> None: + """ + The remote panel lists every regular file, not only numbered BIN logs. + + GIVEN: The FC log directory contains BIN, TXT, and other regular files + WHEN: The application lists the default log directory + THEN: All regular files are returned with their remote paths and sizes + AND: Remote directories are not selectable files + """ + files_manager = _files_manager() + mavftp = MagicMock() + mavftp.cmd_list.return_value = SimpleNamespace( + directory_listing=[ + DirectoryEntry("00000012.BIN", is_dir=False, size_b=120), + DirectoryEntry("LASTLOG.TXT", is_dir=False, size_b=8), + DirectoryEntry("flight log 01.BIN", is_dir=False, size_b=64), + DirectoryEntry("notes.dat", is_dir=False, size_b=42), + DirectoryEntry("subdirectory", is_dir=True, size_b=0), + ] + ) + + with patch( + "ardupilot_methodic_configurator.backend_flightcontroller_files.create_mavftp_safe", + return_value=mavftp, + ): + files = files_manager.list_bin_log_files() + + assert files == [ + FlightControllerLogFile(name="00000012.BIN", remote_path="/APM/LOGS/00000012.BIN", size_bytes=120), + FlightControllerLogFile(name="LASTLOG.TXT", remote_path="/APM/LOGS/LASTLOG.TXT", size_bytes=8), + FlightControllerLogFile( + name="flight log 01.BIN", + remote_path="/APM/LOGS/flight log 01.BIN", + size_bytes=64, + ), + FlightControllerLogFile(name="notes.dat", remote_path="/APM/LOGS/notes.dat", size_bytes=42), + ] + mavftp.cmd_list.assert_called_once_with(["/APM/LOGS/"]) + + def test_remote_path_normalization_preserves_filename_whitespace(self) -> None: + """Whitespace in a remote filename remains part of the MAVFTP path.""" + assert FlightControllerFiles._normalize_remote_path("/APM/LOGS/ flight log .BIN ") == ("/APM/LOGS/ flight log .BIN ") + + def test_explicit_download_preserves_filename_whitespace(self) -> None: + """Downloading a remote filename with whitespace passes the exact path to MAVFTP.""" + files_manager = _files_manager() + mavftp = MagicMock() + mavftp.process_ftp_reply.return_value = SimpleNamespace(error_code=0) + + with patch( + "ardupilot_methodic_configurator.backend_flightcontroller_files.create_mavftp_safe", + return_value=mavftp, + ): + assert files_manager.download_remote_file( + "/APM/LOGS/ flight log .BIN ", + "local flight log.bin", + ) + + mavftp.cmd_get.assert_called_once_with( + ["/APM/LOGS/ flight log .BIN ", "local flight log.bin"], + progress_callback=ANY, + ) + + def test_user_can_browse_a_remote_directory_selected_in_the_remote_panel(self) -> None: + """ + The remote destination selector controls which directory is listed. + + GIVEN: The user enters another absolute MAVFTP directory + WHEN: The remote panel is refreshed + THEN: The selected directory is passed to the flight-controller backend + """ + files_manager = _files_manager() + mavftp = MagicMock() + mavftp.cmd_list.return_value = SimpleNamespace(directory_listing=[]) + + with patch( + "ardupilot_methodic_configurator.backend_flightcontroller_files.create_mavftp_safe", + return_value=mavftp, + ): + files_manager.list_bin_log_files("/APM/LOGS/temperature") + + mavftp.cmd_list.assert_called_once_with(["/APM/LOGS/temperature/"]) + + def test_remote_browser_listing_includes_directories(self) -> None: + """ + The remote browser displays both regular files and directories. + + GIVEN: MAVFTP returns a file and a subdirectory + WHEN: The browser requests a generic remote listing + THEN: Both entries are returned with directory metadata + """ + files_manager = _files_manager() + mavftp = MagicMock() + mavftp.cmd_list.return_value = SimpleNamespace( + directory_listing=[ + DirectoryEntry("nested", is_dir=True, size_b=0), + DirectoryEntry("log.bin", is_dir=False, size_b=42), + ] + ) + + with patch( + "ardupilot_methodic_configurator.backend_flightcontroller_files.create_mavftp_safe", + return_value=mavftp, + ): + entries = files_manager.list_remote_files("/APM/LOGS/") + + assert entries == [ + FlightControllerLogFile("nested", "/APM/LOGS/nested", 0, is_directory=True), + FlightControllerLogFile("log.bin", "/APM/LOGS/log.bin", 42), + ] + + def test_remote_file_manager_supports_delete_rename_and_directory_creation(self) -> None: + """ + Remote management operations delegate to MAVFTP safely. + + GIVEN: A connected MAVFTP-capable flight controller + WHEN: Remote create, delete, and rename operations are requested + THEN: The corresponding MAVFTP commands receive normalized paths + """ + files_manager = _files_manager() + mavftp = MagicMock() + success = SimpleNamespace(error_code=0) + mavftp.cmd_mkdir.return_value = success + mavftp.cmd_rm.return_value = success + mavftp.cmd_rmdir.return_value = success + mavftp.cmd_rename.return_value = success + + with patch( + "ardupilot_methodic_configurator.backend_flightcontroller_files.create_mavftp_safe", + return_value=mavftp, + ): + assert files_manager.make_remote_directory("/APM/LOGS/nested/") + assert files_manager.delete_remote_path("/APM/LOGS/log.bin") + assert files_manager.delete_remote_path("/APM/LOGS/nested", is_directory=True) + assert files_manager.rename_remote_path("/APM/LOGS/old.bin", "/APM/LOGS/new.bin") + + mavftp.cmd_mkdir.assert_called_once_with(["/APM/LOGS/nested"]) + mavftp.cmd_rm.assert_called_once_with(["/APM/LOGS/log.bin"]) + mavftp.cmd_rmdir.assert_called_once_with(["/APM/LOGS/nested"]) + mavftp.cmd_rename.assert_called_once_with(["/APM/LOGS/old.bin", "/APM/LOGS/new.bin"]) + + +class TestParameterEditorLogDownloadWorkflow: + """Verify selected-file download behavior.""" + + def test_user_can_download_multiple_selected_files_after_one_overwrite_confirmation(self) -> None: + """ + A batch download asks once before replacing existing local files. + + GIVEN: Two remote files are selected and one local target already exists + WHEN: The user confirms the overwrite prompt + THEN: Both files are downloaded using their remote basenames + AND: The overwrite callback is called exactly once + """ + model = _parameter_editor_model() + flight_controller = cast("Any", model._flight_controller) # pylint: disable=protected-access + local_download_directory = Path("C:/downloads") + existing_target = local_download_directory / "LASTLOG.TXT" + selected = [ + FlightControllerLogFile(name="LASTLOG.TXT", remote_path="/APM/LOGS/LASTLOG.TXT", size_bytes=10), + FlightControllerLogFile(name="00000012.BIN", remote_path="/APM/LOGS/00000012.BIN", size_bytes=100), + ] + ask_overwrite = MagicMock(return_value=True) + + with ( + patch.object(Path, "is_dir", return_value=True), + patch.object(Path, "exists", side_effect=[True, False]), + ): + result = model.download_selected_bin_logs_workflow( + selected_files=selected, + destination=str(local_download_directory), + destination_is_directory=True, + ask_overwrite=ask_overwrite, + show_error=MagicMock(), + show_info=MagicMock(), + ) + + assert result.successful == ("LASTLOG.TXT", "00000012.BIN") + assert result.failed == () + ask_overwrite.assert_called_once() + assert flight_controller.download_bin_log_file.call_count == 2 + flight_controller.download_bin_log_file.assert_any_call("/APM/LOGS/LASTLOG.TXT", str(existing_target), None) + flight_controller.download_bin_log_file.assert_any_call( + "/APM/LOGS/00000012.BIN", str(local_download_directory / "00000012.BIN"), None + ) + + def test_user_can_cancel_a_batch_before_any_existing_file_is_overwritten(self) -> None: + """ + Declining the overwrite prompt cancels the whole batch. + + GIVEN: A selected batch contains an existing local target + WHEN: The user declines the single overwrite prompt + THEN: No remote file is downloaded + """ + model = _parameter_editor_model() + flight_controller = cast("Any", model._flight_controller) # pylint: disable=protected-access + local_download_directory = Path("C:/downloads") + ask_overwrite = MagicMock(return_value=False) + + with ( + patch.object(Path, "is_dir", return_value=True), + patch.object(Path, "exists", return_value=True), + ): + result = model.download_selected_bin_logs_workflow( + selected_files=[ + FlightControllerLogFile(name="LASTLOG.TXT", remote_path="/APM/LOGS/LASTLOG.TXT", size_bytes=10) + ], + destination=str(local_download_directory), + destination_is_directory=True, + ask_overwrite=ask_overwrite, + show_error=MagicMock(), + show_info=MagicMock(), + ) + + assert result.cancelled is True + flight_controller.download_bin_log_file.assert_not_called() + + def test_batch_continues_after_a_failed_transfer_and_reports_each_file(self) -> None: + """ + A failed remote file does not stop the remaining batch. + + GIVEN: The first selected remote file fails and the second succeeds + WHEN: The batch download runs + THEN: Both transfer attempts are made + AND: The result summary identifies each file's outcome + """ + model = _parameter_editor_model() + flight_controller = cast("Any", model._flight_controller) # pylint: disable=protected-access + flight_controller.download_bin_log_file.side_effect = [False, True] + show_error = MagicMock() + selected = [ + FlightControllerLogFile("missing.BIN", "/APM/LOGS/missing.BIN", 10), + FlightControllerLogFile("available.BIN", "/APM/LOGS/available.BIN", 20), + ] + + with ( + patch.object(Path, "is_dir", return_value=True), + patch.object(Path, "exists", return_value=False), + ): + result = model.download_selected_bin_logs_workflow( + selected_files=selected, + destination="C:/downloads", + destination_is_directory=True, + ask_overwrite=MagicMock(return_value=True), + show_error=show_error, + show_info=MagicMock(), + ) + + assert result.successful == ("available.BIN",) + assert result.failed == ("missing.BIN",) + assert flight_controller.download_bin_log_file.call_count == 2 + summary = show_error.call_args.args[1] + assert "Failed: missing.BIN" in summary + assert "Downloaded: available.BIN" in summary + + +class TestDownloadBinLogsWindow: + """Verify the modal's remote destination selector behavior.""" + + def test_local_panel_defaults_to_current_vehicle_directory(self) -> None: + """The local browser starts in the vehicle directory used by the editor.""" + parameter_editor = MagicMock() + parameter_editor.get_vehicle_directory.return_value = str(Path.cwd()) + + assert DownloadBinLogsWindow._default_local_directory(parameter_editor) == str(Path.cwd()) + + def test_remote_refresh_uses_the_selected_destination_directory(self) -> None: + """ + Refreshing the remote panel uses the path shown in its selector. + + GIVEN: The modal's remote destination selector contains a path + WHEN: The user presses Open/Refresh + THEN: The parameter editor lists files from that exact path + """ + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + window.remote_directory_var = MagicMock() + window.remote_directory_var.get.return_value = "/APM/LOGS/temperature" + window.parameter_editor = MagicMock() + window.parameter_editor.get_bin_log_files.return_value = [] + window.tree = MagicMock() + window.remote_directory_label = MagicMock() + window.empty_state_label = MagicMock() + window.download_button = MagicMock() + window._populate_tree = MagicMock() # pylint: disable=protected-access + + window.refresh_remote_files() + + window.parameter_editor.get_bin_log_files.assert_called_once_with("/APM/LOGS/temperature") + + def test_enter_in_remote_destination_refreshes_the_listing(self) -> None: + """ + Enter in the remote destination entry performs Open/Refresh. + + GIVEN: The remote destination entry has keyboard focus + WHEN: The user presses Enter + THEN: The remote listing is refreshed + AND: Tk stops processing the key event + """ + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + window.refresh_remote_files = MagicMock() + + result = window._on_remote_directory_return() # pylint: disable=protected-access + + window.refresh_remote_files.assert_called_once_with() + assert result == "break" + + def test_backspace_navigates_to_parent_of_last_selected_remote_panel(self) -> None: + """Backspace opens the remote parent directory when the remote panel was last selected.""" + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + window.last_selected_panel = "remote" + window.remote_directory_var = MagicMock() + window.remote_directory_var.get.return_value = "/APM/LOGS/nested/" + window.refresh_remote_panel = MagicMock() + + result = window._on_backspace() # pylint: disable=protected-access + + window.remote_directory_var.set.assert_called_once_with("/APM/LOGS") + window.refresh_remote_panel.assert_called_once_with() + assert result == "break" + + def test_backspace_navigates_to_parent_of_last_selected_local_panel(self) -> None: + """Backspace opens the local parent directory when the local panel was last selected.""" + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + window.last_selected_panel = "local" + window.local_directory_var = MagicMock() + window.local_directory_var.get.return_value = "C:/logs/nested" + window.refresh_local_panel = MagicMock() + + result = window._on_backspace() # pylint: disable=protected-access + + window.local_directory_var.set.assert_called_once_with("C:\\logs") + window.refresh_local_panel.assert_called_once_with() + assert result == "break" + + def test_remote_parent_button_navigates_to_parent_directory(self) -> None: + """The remote parent button navigates without creating a `..` tree row.""" + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + window.remote_directory_var = MagicMock() + window.remote_directory_var.get.return_value = "/APM/LOGS/nested/" + window.refresh_remote_panel = MagicMock() + + window.navigate_remote_parent() + + window.remote_directory_var.set.assert_called_once_with("/APM/LOGS") + window.refresh_remote_panel.assert_called_once_with() + + def test_local_parent_button_navigates_to_parent_directory(self) -> None: + """The local parent button navigates to the filesystem parent.""" + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + window.local_directory_var = MagicMock() + window.local_directory_var.get.return_value = "C:/logs/nested" + window.refresh_local_panel = MagicMock() + + window.navigate_local_parent() + + window.local_directory_var.set.assert_called_once_with("C:\\logs") + window.refresh_local_panel.assert_called_once_with() + + def test_select_all_ignores_parent_directory_entry(self) -> None: + """ + Select all selects files without selecting the parent directory entry. + + GIVEN: The remote listing contains a parent-directory entry + WHEN: The user presses Select all + THEN: Every regular file row is selected + AND: The `..` row is excluded + """ + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + window.remote_files = [ + FlightControllerLogFile("..", "/APM/LOGS/..", 0), + FlightControllerLogFile("one.BIN", "/APM/LOGS/one.BIN", 10), + FlightControllerLogFile("two.BIN", "/APM/LOGS/two.BIN", 20), + ] + window.tree = MagicMock() + window.download_button = MagicMock() + window.tree.selection.return_value = ("1", "2") + + window.select_all_files() + + window.tree.selection_set.assert_called_once_with(["1", "2"]) + window.download_button.configure.assert_called_once_with(state="normal") + + def test_remote_download_plan_expands_directories_recursively(self) -> None: + """ + A selected remote directory expands into nested local directories/files. + + GIVEN: A remote directory contains a nested directory and a file + WHEN: A download plan is built + THEN: Every remote file is mapped beneath the selected local directory + """ + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + window.parameter_editor = MagicMock() + root = FlightControllerLogFile("folder", "/APM/LOGS/folder", 0, is_directory=True) + nested = FlightControllerLogFile("nested", "/APM/LOGS/folder/nested", 0, is_directory=True) + leaf = FlightControllerLogFile("leaf.bin", "/APM/LOGS/folder/nested/leaf.bin", 12) + window.parameter_editor.get_remote_files.side_effect = [[nested], [leaf]] + + directories, files = window._remote_download_plan(root, Path("C:/downloads/folder")) # pylint: disable=protected-access + + assert directories == [Path("C:/downloads/folder"), Path("C:/downloads/folder/nested")] + assert files == [(leaf, Path("C:/downloads/folder/nested/leaf.bin"))] + + def test_local_upload_plan_expands_directories_recursively(self) -> None: + """ + A selected local directory expands into remote directories/files. + + GIVEN: A local directory entry contains one nested file + WHEN: An upload plan is built + THEN: The remote directory and file paths preserve the hierarchy + """ + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + root_path = MagicMock() + child_path = MagicMock() + root_path.iterdir.return_value = [child_path] + root_path.name = "folder" + child_path.name = "leaf.bin" + root_path.is_symlink.return_value = False + child_path.is_symlink.return_value = False + root_path.is_dir.return_value = True + child_path.is_dir.return_value = False + child_path.stat.return_value.st_size = 12 + entry = LocalFileEntry("folder", root_path, 0, is_directory=True) + + directories, files = window._local_upload_plan(entry, "/APM/LOGS/folder") # pylint: disable=protected-access + + assert directories == ["/APM/LOGS/folder"] + assert files == [(child_path, "/APM/LOGS/folder/leaf.bin", 12)] + + def test_ctrl_a_selects_all_files(self) -> None: + """ + Ctrl+A selects all files in the remote panel. + + GIVEN: The remote file Treeview has selectable files + WHEN: The user presses Ctrl+A + THEN: The same Select all behavior is invoked + AND: Tk stops processing the shortcut + """ + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + window.select_all_files = MagicMock() + + result = window._on_select_all_key() # pylint: disable=protected-access + + window.select_all_files.assert_called_once_with() + assert result == "break" + + def test_select_all_local_entries_ignores_parent_directory(self) -> None: + """The local Select all action does not select the parent-navigation row.""" + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + window.local_tree = MagicMock() + window.local_tree.get_children.return_value = ("0", "1") + window.local_tree.item.side_effect = [ + ("..", "Directory", ""), + ("file.bin", "File", "12 B"), + ] + + window.select_all_local_entries() + + window.local_tree.selection_set.assert_called_once_with(["1"]) + + def test_remote_download_execution_continues_after_one_file_fails(self) -> None: + """A failed remote transfer does not stop the remaining selected files.""" + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + first = FlightControllerLogFile("first.bin", "/APM/LOGS/first.bin", 10) + second = FlightControllerLogFile("second.bin", "/APM/LOGS/second.bin", 20) + window.remote_entries = [first, second] + window.remote_tree = MagicMock() + window.remote_tree.selection.return_value = ("0", "1") + window.local_directory_var = MagicMock() + local_directory = Path.cwd() + window.local_directory_var.get.return_value = str(local_directory) + window.parameter_editor = MagicMock() + window.parameter_editor.download_remote_file.side_effect = [False, True] + window.ui = MagicMock() + window.ui.ask_yesno.return_value = True + window._progress_window = MagicMock(return_value=MagicMock()) # pylint: disable=protected-access + window.refresh_local_panel = MagicMock() + + window.download_selected_remote_entries() + + assert window.parameter_editor.download_remote_file.call_count == 2 + assert window.parameter_editor.download_remote_file.call_args_list[0].args[:2] == ( + "/APM/LOGS/first.bin", + str(local_directory / "first.bin"), + ) + assert window.parameter_editor.download_remote_file.call_args_list[1].args[:2] == ( + "/APM/LOGS/second.bin", + str(local_directory / "second.bin"), + ) + window.ui.show_error.assert_called_once() + window.refresh_local_panel.assert_called_once_with() + + def test_local_upload_execution_creates_directories_and_uploads_files_recursively(self) -> None: + """Uploading a local directory preserves its hierarchy on the FC.""" + local_root = MagicMock() + nested = MagicMock() + leaf = MagicMock() + local_root.iterdir.return_value = [nested] + local_root.is_dir.return_value = True + local_root.is_symlink.return_value = False + local_root.name = "folder" + nested.iterdir.return_value = [leaf] + nested.is_dir.return_value = True + nested.is_symlink.return_value = False + nested.name = "nested" + leaf.is_dir.return_value = False + leaf.is_symlink.return_value = False + leaf.name = "leaf log.bin" + leaf.stat.return_value.st_size = 7 + + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + window.local_entries = [LocalFileEntry("folder", local_root, 0, is_directory=True)] + window.local_tree = MagicMock() + window.local_tree.selection.return_value = ("0",) + window.remote_directory_var = MagicMock() + window.remote_directory_var.get.return_value = "/APM/LOGS/" + window.parameter_editor = MagicMock() + window.parameter_editor.make_remote_directory.return_value = True + window.parameter_editor.upload_file_to_fc.return_value = True + window.ui = MagicMock() + window.ui.ask_yesno.return_value = True + window._progress_window = MagicMock(return_value=MagicMock()) # pylint: disable=protected-access + window.refresh_remote_panel = MagicMock() + + window.upload_selected_local_entries() + + assert window.parameter_editor.make_remote_directory.call_args_list == [ + call("/APM/LOGS/folder"), + call("/APM/LOGS/folder/nested"), + ] + window.parameter_editor.upload_file_to_fc.assert_called_once() + assert window.parameter_editor.upload_file_to_fc.call_args.args[:2] == ( + str(leaf), + "/APM/LOGS/folder/nested/leaf log.bin", + ) + window.refresh_remote_panel.assert_called_once_with() + + def test_remote_delete_only_removes_files_and_empty_directories(self) -> None: + """Remote delete skips non-empty directories while continuing the batch.""" + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + root = FlightControllerLogFile("folder", "/APM/LOGS/folder", 0, is_directory=True) + empty = FlightControllerLogFile("empty", "/APM/LOGS/empty", 0, is_directory=True) + child = FlightControllerLogFile("leaf.bin", "/APM/LOGS/folder/leaf.bin", 10) + file_entry = FlightControllerLogFile("file.bin", "/APM/LOGS/file.bin", 10) + window.remote_entries = [root, empty, file_entry] + window.remote_tree = MagicMock() + window.remote_tree.selection.return_value = ("0", "1", "2") + window.parameter_editor = MagicMock() + window.parameter_editor.get_remote_files.side_effect = [[child], []] + window.parameter_editor.delete_remote_path.return_value = True + window.ui = MagicMock() + window.ui.ask_yesno.return_value = True + window.refresh_remote_panel = MagicMock() + + window.delete_selected_remote_entries() + + assert window.parameter_editor.delete_remote_path.call_args_list == [ + call("/APM/LOGS/empty", True), # noqa: FBT003 + call("/APM/LOGS/file.bin", False), # noqa: FBT003 + ] + window.ui.show_error.assert_called_once() + window.refresh_remote_panel.assert_called_once_with() + + def test_local_delete_removes_files_and_empty_directories(self) -> None: + """Local delete uses unlink/rmdir and continues across multiple selections.""" + local_root = MagicMock() + local_file = MagicMock() + + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + window.local_entries = [ + LocalFileEntry("folder", local_root, 0, is_directory=True), + LocalFileEntry("file.bin", local_file, 10), + ] + window.local_tree = MagicMock() + window.local_tree.selection.return_value = ("0", "1") + window.ui = MagicMock() + window.ui.ask_yesno.return_value = True + window.refresh_local_panel = MagicMock() + + window.delete_selected_local_entries() + + local_root.rmdir.assert_called_once_with() + local_file.unlink.assert_called_once_with() + window.refresh_local_panel.assert_called_once_with() + + def test_f2_falls_back_to_name_dialog_when_inline_editor_is_unavailable(self) -> None: + """F2 uses the dialog fallback when the selected row cannot be edited inline.""" + entry = FlightControllerLogFile("old.bin", "/APM/LOGS/old.bin", 10) + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + window.last_selected_panel = "remote" + window.last_selected_items = {"remote": "0", "local": None} + window.remote_entries = [entry] + window.remote_tree = MagicMock() + window.remote_tree.bbox.return_value = () + window.remote_tree.selection.return_value = ("0",) + window.ui = MagicMock() + window.ui.askstring.return_value = "new.bin" + window.root = MagicMock() + window.parameter_editor = MagicMock() + window.parameter_editor.rename_remote_path.return_value = True + window.refresh_remote_panel = MagicMock() + + result = window._on_rename_key() # pylint: disable=protected-access + + assert result == "break" + window.ui.askstring.assert_called_once() + window.parameter_editor.rename_remote_path.assert_called_once_with( + "/APM/LOGS/old.bin", + "/APM/LOGS/new.bin", + ) + + def test_f2_commits_an_inline_remote_rename(self) -> None: + """F2 commits the edited name without opening a dialog when inline editing works.""" + entry = FlightControllerLogFile("old.bin", "/APM/LOGS/old.bin", 10) + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + window.remote_tree = MagicMock() + window.remote_tree.bbox.return_value = (1, 2, 100, 20) + window.remote_tree.selection.return_value = ("0",) + window.remote_entries = [entry] + window.last_selected_panel = "remote" + window.last_selected_items = {"remote": "0", "local": None} + window.ui = MagicMock() + window.parameter_editor = MagicMock() + window.parameter_editor.rename_remote_path.return_value = True + window.refresh_remote_panel = MagicMock() + editor = MagicMock() + editor.get.return_value = "new log.bin" + + with patch( + "ardupilot_methodic_configurator.frontend_tkinter_download_bin_logs.ttk.Entry", + return_value=editor, + ): + result = window._on_rename_key() # pylint: disable=protected-access + + assert result == "break" + editor.place.assert_called_once_with(x=1, y=2, width=100, height=20) + editor.focus_set.assert_called_once_with() + editor.bind.assert_any_call("", ANY) + window.parameter_editor.rename_remote_path.assert_not_called() + + finish_result = window._finish_inline_rename("remote", entry, editor) # pylint: disable=protected-access + + assert finish_result == "break" + window.parameter_editor.rename_remote_path.assert_called_once_with( + "/APM/LOGS/old.bin", + "/APM/LOGS/new log.bin", + ) + + def test_f2_does_not_rename_a_stale_row_without_current_selection(self) -> None: + """F2 refuses to act on a cached row after the current selection is gone.""" + entry = FlightControllerLogFile("new.bin", "/APM/LOGS/new.bin", 10) + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + window.remote_tree = MagicMock() + window.remote_tree.selection.return_value = () + window.remote_entries = [entry] + window.last_selected_panel = "remote" + window.last_selected_items = {"remote": "0", "local": None} + window.ui = MagicMock() + window.parameter_editor = MagicMock() + + assert window._on_rename_key() == "break" # pylint: disable=protected-access + + window.ui.askstring.assert_not_called() + window.parameter_editor.rename_remote_path.assert_not_called() + + def test_remote_mutation_outside_log_directory_requires_warning_and_confirmation(self) -> None: + """Remote deletion outside the log directory requires an explicit second confirmation.""" + entry = FlightControllerLogFile("params.bin", "/APM/params.bin", 10) + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + window.remote_entries = [entry] + window.remote_tree = MagicMock() + window.remote_tree.selection.return_value = ("0",) + window.parameter_editor = MagicMock() + window.parameter_editor.delete_remote_path.return_value = True + window.ui = MagicMock() + window.ui.ask_yesno.side_effect = [True, True] + window.refresh_remote_panel = MagicMock() + + window.delete_selected_remote_entries() + + window.ui.show_warning.assert_called_once() + assert window.ui.ask_yesno.call_count == 2 + window.parameter_editor.delete_remote_path.assert_called_once_with("/APM/params.bin", False) # noqa: FBT003 + + def test_remote_rename_uses_a_single_safe_new_name(self) -> None: + """Remote rename uses the selected entry's parent directory.""" + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + entry = FlightControllerLogFile("old.bin", "/APM/LOGS/old.bin", 10) + window.remote_entries = [entry] + window.remote_tree = MagicMock() + window.remote_tree.selection.return_value = ("0",) + window.ui = MagicMock() + window.ui.askstring.return_value = "new.bin" + window.root = MagicMock() + window.parameter_editor = MagicMock() + window.parameter_editor.rename_remote_path.return_value = True + window.refresh_remote_panel = MagicMock() + + window.rename_selected_remote_entry() + + window.parameter_editor.rename_remote_path.assert_called_once_with( + "/APM/LOGS/old.bin", + "/APM/LOGS/new.bin", + ) + window.refresh_remote_panel.assert_called_once_with() + + def test_local_rename_uses_a_single_safe_new_name(self) -> None: + """Local rename changes only the selected entry name.""" + old_path = MagicMock() + target_path = MagicMock() + old_path.with_name.return_value = target_path + target_path.exists.return_value = False + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + entry = LocalFileEntry("old.bin", old_path, 7) + window.local_entries = [entry] + window.local_tree = MagicMock() + window.local_tree.selection.return_value = ("0",) + window.ui = MagicMock() + window.ui.askstring.return_value = "new.bin" + window.root = MagicMock() + window.refresh_local_panel = MagicMock() + + window.rename_selected_local_entry() + + old_path.rename.assert_called_once_with(target_path) + window.refresh_local_panel.assert_called_once_with() + + def test_user_can_sort_remote_files_by_filename(self) -> None: + """ + Clicking the filename heading sorts rows alphabetically. + + GIVEN: The remote panel contains files in an unsorted order + WHEN: The filename heading is activated + THEN: Rows are moved into case-insensitive filename order + """ + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + window.sort_column = "" + window.remote_files = [ + FlightControllerLogFile("zeta.BIN", "/APM/LOGS/zeta.BIN", 10), + FlightControllerLogFile("Alpha.BIN", "/APM/LOGS/Alpha.BIN", 20), + FlightControllerLogFile("middle.BIN", "/APM/LOGS/middle.BIN", 30), + ] + window.tree = MagicMock() + window.tree.get_children.return_value = ("0", "1", "2") + + window._sort_by_column("name", reverse=False) # pylint: disable=protected-access + + assert window.tree.move.call_args_list == [ + call("1", "", 0), + call("2", "", 1), + call("0", "", 2), + ] + + def test_user_can_toggle_remote_filename_sort_direction(self) -> None: + """Clicking the filename heading repeatedly alternates ascending and descending.""" + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + window.remote_sort_column = "" + window.remote_sort_reverse = False + window.remote_entries = [ + FlightControllerLogFile("alpha.BIN", "/APM/LOGS/alpha.BIN", 10), + FlightControllerLogFile("zeta.BIN", "/APM/LOGS/zeta.BIN", 20), + ] + window.remote_tree = MagicMock() + window.remote_tree.get_children.return_value = ("0", "1") + + window._on_sort_heading("remote", "name") # pylint: disable=protected-access + window.remote_tree.move.reset_mock() + window._on_sort_heading("remote", "name") # pylint: disable=protected-access + + assert window.remote_sort_column == "name" + assert window.remote_sort_reverse is True + assert window.remote_tree.move.call_args_list == [ + call("1", "", 0), + call("0", "", 1), + ] + + def test_sorting_with_parent_entry_does_not_compare_strings_and_integers(self) -> None: + """Sorting a navigable panel remains valid when the synthetic `..` row is present.""" + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + window.remote_sort_column = "" + window.remote_sort_reverse = False + window.remote_entries = [ + FlightControllerLogFile("..", "/APM/LOGS", 0, is_directory=True), + FlightControllerLogFile("log.bin", "/APM/LOGS/log.bin", 10), + ] + window.remote_tree = MagicMock() + window.remote_tree.get_children.return_value = ("0", "1") + + window._on_sort_heading("remote", "name") # pylint: disable=protected-access + + assert window.remote_tree.move.call_args_list == [ + call("0", "", 0), + call("1", "", 1), + ] + + def test_user_can_sort_remote_files_by_numeric_size(self) -> None: + """ + Clicking the size heading sorts by bytes rather than formatted text. + + GIVEN: The remote panel contains files with sizes 100, 2, and 12 bytes + WHEN: The size heading is activated + THEN: Rows are moved in numeric size order + """ + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + window.sort_column = "" + window.remote_files = [ + FlightControllerLogFile("hundred.BIN", "/APM/LOGS/hundred.BIN", 100), + FlightControllerLogFile("two.BIN", "/APM/LOGS/two.BIN", 2), + FlightControllerLogFile("twelve.BIN", "/APM/LOGS/twelve.BIN", 12), + ] + window.tree = MagicMock() + window.tree.get_children.return_value = ("0", "1", "2") + + window._sort_by_column("size", reverse=False) # pylint: disable=protected-access + + assert window.tree.move.call_args_list == [ + call("1", "", 0), + call("2", "", 1), + call("0", "", 2), + ] + + def test_single_selection_uses_a_save_file_selector_and_progress_callback(self) -> None: + """ + Downloading one selected file asks for a complete local filename. + + GIVEN: The remote panel has one selected file + WHEN: The user presses Download and chooses a local filename + THEN: The save-file selector receives the remote basename + AND: The workflow receives the progress-window callback + """ + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + selected_file = FlightControllerLogFile( + name="LASTLOG.TXT", + remote_path="/APM/LOGS/LASTLOG.TXT", + size_bytes=8, + ) + window._selected_files = MagicMock(return_value=[selected_file]) # pylint: disable=protected-access + window.ui = MagicMock() + window.ui.asksaveasfilename.return_value = "C:/downloads/custom-name.txt" + window.parameter_editor = MagicMock() + window.root = MagicMock() + progress_window = MagicMock() + window.ui.create_progress_window.return_value = progress_window + + window.download_selected_files() + + window.ui.asksaveasfilename.assert_called_once() + assert window.ui.asksaveasfilename.call_args.kwargs["initialfile"] == "LASTLOG.TXT" + workflow_kwargs = window.parameter_editor.download_selected_bin_logs_workflow.call_args.kwargs + assert workflow_kwargs["destination"] == "C:/downloads/custom-name.txt" + assert workflow_kwargs["destination_is_directory"] is False + assert workflow_kwargs["progress_callback"] is progress_window.update_progress_bar + progress_window.destroy.assert_called_once() + + def test_multiple_selection_uses_a_directory_selector(self) -> None: + """ + Downloading several selected files asks for one local directory. + + GIVEN: The remote panel has multiple selected files + WHEN: The user presses Download and chooses a destination directory + THEN: The directory selector is used instead of the save-file selector + AND: The workflow receives the directory destination + """ + window = DownloadBinLogsWindow.__new__(DownloadBinLogsWindow) + window._selected_files = MagicMock( # pylint: disable=protected-access + return_value=[ + FlightControllerLogFile("one.BIN", "/APM/LOGS/one.BIN", 10), + FlightControllerLogFile("two.BIN", "/APM/LOGS/two.BIN", 20), + ] + ) + window.ui = MagicMock() + window.ui.askdirectory.return_value = "C:/downloads" + window.parameter_editor = MagicMock() + window.root = MagicMock() + progress_window = MagicMock() + window.ui.create_progress_window.return_value = progress_window + + window.download_selected_files() + + window.ui.askdirectory.assert_called_once() + window.ui.asksaveasfilename.assert_not_called() + workflow_kwargs = window.parameter_editor.download_selected_bin_logs_workflow.call_args.kwargs + assert workflow_kwargs["destination"] == "C:/downloads" + assert workflow_kwargs["destination_is_directory"] is True + progress_window.destroy.assert_called_once() + + def test_parameter_editor_button_opens_the_log_download_modal(self) -> None: + """ + The renamed Parameter Editor button opens the new modal. + + GIVEN: A configured Parameter Editor window + WHEN: The user clicks Download .bin log file(s) + THEN: A modal is opened with the editor model and UI services + """ + editor = ParameterEditorWindow.__new__(ParameterEditorWindow) + editor.root = MagicMock() + editor.parameter_editor = MagicMock() + editor.ui = MagicMock() + + with patch("ardupilot_methodic_configurator.frontend_tkinter_parameter_editor.DownloadBinLogsWindow") as modal: + editor.on_download_bin_logs_click() + + modal.assert_called_once_with(editor.root, editor.parameter_editor, editor.ui)