From 5f896c74f42289d5d0dd2b10fae7dc039dec2cba Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Thu, 27 Aug 2026 17:18:01 +0200 Subject: [PATCH 01/32] fix(mavftp): return failed download callback results Capture a MAVFTPReturn failure from a download callback and return it from\nthe reply loop.\n\nThis lets callers of cmd_getparams detect malformed packed parameter data\ninstead of reporting a successful transfer after the callback has rejected\nthe payload. The callback result is cleared for every new download so a\nprior failure cannot affect a later operation. finish termination before returning callback errors return packed-parameter decode failures consume reported callback failures fix(mavftp): skip rejected download output --- mavftp.py | 38 +++++- tests/test_mavftp.py | 269 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 295 insertions(+), 12 deletions(-) diff --git a/mavftp.py b/mavftp.py index dcfe44b68..83e536142 100644 --- a/mavftp.py +++ b/mavftp.py @@ -336,6 +336,7 @@ def __init__( # noqa: PLR0915 pylint: disable=too-many-statements self.fh: Union[None, SIO, BufferedReader, BufferedWriter, BufferedRandom] = None self.filename: Union[None, str] = None self.callback = None + self.callback_failure: Optional[MAVFTPReturn] = None self.callback_progress = None self.put_callback = None self.put_callback_progress = None @@ -356,6 +357,10 @@ def __init__( # noqa: PLR0915 pylint: disable=too-many-statements self.rtt = 0.5 self.reached_eof = False self.read_complete = False + # Set by operations with an explicit successful terminal reply. + # Unlike read_complete, these operations do not need a session-close + # handshake to prove their result to process_ftp_reply(). + self.operation_complete = False # 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 @@ -504,6 +509,7 @@ def __terminate_session(self) -> None: def cmd_list(self, args: List[str]) -> MAVFTPReturn: """List files.""" + self.operation_complete = False self.list_result = [] self.list_temp_result = [] if len(args) == 0: @@ -570,6 +576,7 @@ def __handle_list_reply(self, op: FTP_OP, _m) -> MAVFTPReturn: and op.payload[0] == FtpError.EndOfFile ): self.list_result = self.list_temp_result + self.operation_complete = True return MAVFTPReturn( "ListDirectory", FtpError.Success, directory_listing=self.list_result ) @@ -661,6 +668,7 @@ def cmd_get( if len(args) == 0 or len(args) > 2: logging.error("Usage: get [FILENAME ]") return MAVFTPReturn("OpenFileRO", FtpError.InvalidArguments) + self.operation_complete = False fname = args[0] if len(args) > 1: self.filename = args[1] @@ -670,6 +678,7 @@ def cmd_get( logging.info("Getting %s to %s", fname, self.filename) self.op_start = time.time() self.callback = callback + self.callback_failure = None self.callback_progress = progress_callback self.read_retries = 0 self.duplicates = 0 @@ -758,9 +767,16 @@ def __check_read_finished(self) -> bool: ofs = self.fh.tell() dt = time.time() - self.op_start rate = (ofs / dt) / 1024.0 + publish_result = True if self.callback is not None: self.fh.seek(0) - self.callback(self.fh) + callback_result = self.callback(self.fh) + 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) @@ -795,7 +811,7 @@ def __check_read_finished(self) -> bool: logging.info("read %u bytes", len(self.get_result)) self.fh.flush() try: - if self.filename and self.filename != "-": + 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: @@ -1001,6 +1017,7 @@ def cmd_put( if self.write_list is not None: logging.error("FTP: put already in progress") return MAVFTPReturn("CreateFile", FtpError.PutAlreadyInProgress) + self.operation_complete = False fname = args[0] self.fh = fh if self.fh is None: @@ -1086,6 +1103,7 @@ def __send_more_writes(self) -> None: # all done self.__put_finished(self.write_file_size) self.__terminate_session() + self.operation_complete = True return now = time.time() @@ -1562,8 +1580,16 @@ def process_ftp_reply( ret = MAVFTPReturn(operation_name, FtpError.Success) else: ret = self.__mavlink_packet(m) + if ( + self.callback_failure is not None + and operation_name != "TerminateSession" + ): + callback_failure = self.callback_failure + self.callback_failure = None + return callback_failure if self.pending_terminate_seq is None and ( self.read_complete + or self.operation_complete or operation_name == "TerminateSession" or ( operation_name == "ResetSessions" @@ -1812,10 +1838,11 @@ def decode_and_save_params(fh) -> MAVFTPReturn: data = fh.read() except OSError as exp: logging.error("FTP: Failed to read file param.pck: %s", exp) - sys.exit(1) + return MAVFTPReturn("GetParams", FtpError.Fail) pdata = MAVFTP.ftp_param_decode(data) if pdata is None: - sys.exit(1) + logging.error("FTP: Failed to decode parameter file param.pck") + return MAVFTPReturn("GetParams", FtpError.Fail) param_values = MAVFTP.extract_params(pdata.params, sort_type) param_defaults = MAVFTP.extract_params(pdata.defaults, sort_type) @@ -2244,6 +2271,7 @@ 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 @@ -2251,6 +2279,7 @@ def main() -> None: elif isinstance(ret, MAVFTPReturn): if ret.error_code or args.command in {"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" @@ -2261,6 +2290,7 @@ def main() -> None: ) master.close() + sys.exit(exit_code) if __name__ == "__main__": diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index 4380d3f4d..1a3999194 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -8,17 +8,270 @@ SPDX-License-Identifier: GPL-3.0-or-later ''' +import logging +import os +import struct +import tempfile import unittest +from io import BytesIO, StringIO + #from unittest.mock import patch -from io import StringIO -import logging from pymavlink import mavutil -from pymavlink.mavftp import FTP_OP, MAVFTP, MAVFTPReturn -from pymavlink.mavftp import FtpError -from pymavlink.mavftp import OP_ListDirectory -from pymavlink.mavftp import OP_ReadFile -from pymavlink.mavftp import OP_Ack -from pymavlink.mavftp import OP_Nack +from pymavlink.mavftp import ( + FTP_OP, + MAVFTP, + FtpError, + MAVFTPReturn, + OP_Ack, + OP_BurstReadFile, + OP_CreateFile, + OP_ListDirectory, + OP_Nack, + OP_OpenFileRO, + OP_ReadFile, + OP_ResetSessions, + OP_TerminateSession, + OP_WriteFile, +) + + +class FakeFTPMessage: + """Minimal FILE_TRANSFER_PROTOCOL message for reply-loop tests.""" + + def __init__(self, op): + self.payload = op.pack() + self.target_system = 1 + self.target_component = 1 + + @staticmethod + def get_type(): + return "FILE_TRANSFER_PROTOCOL" + + +class FakeMAV: + """Record FTP sends without requiring a MAVLink transport.""" + + def __init__(self): + self.sent = [] + + def file_transfer_protocol_send(self, *args): + self.sent.append(args) + + +class FakeMaster: + """Serve a predetermined sequence of FTP replies.""" + + source_system = 1 + source_component = 1 + + def __init__(self, replies): + self.mav = FakeMAV() + self.replies = replies + + def recv_match(self, **_kwargs): + if self.replies: + return self.replies.pop(0) + return None + + +def ftp_reply(seq, opcode, req_opcode, payload=None, offset=0, burst_complete=0): + """Create a parsed FTP response represented as a minimal MAVLink message.""" + data = bytearray(payload) if payload is not None else bytearray() + return FakeFTPMessage( + FTP_OP( + seq=seq, + session=0, + opcode=opcode, + size=len(data), + req_opcode=req_opcode, + burst_complete=burst_complete, + offset=offset, + payload=data, + ) + ) + + +class TestMAVFTPReplyCompletion(unittest.TestCase): + """Regression tests for command completion and idle fallback.""" + + @staticmethod + def make_ftp(replies): + master = FakeMaster( + [ftp_reply(1, OP_Ack, OP_ResetSessions)] + replies + ) + ftp = MAVFTP(master, target_system=1, target_component=1) + ftp.ftp_settings.idle_detection_time = 0.02 + ftp.ftp_settings.read_retry_time = 0.01 + ftp.ftp_settings.retry_time = 0.2 + return ftp, master + + def test_put_returns_after_completion_before_late_write_reply(self): + ftp, master = self.make_ftp( + [ + ftp_reply(2, OP_Ack, OP_CreateFile), + ftp_reply(3, OP_Ack, OP_WriteFile, offset=0), + ftp_reply(5, OP_Ack, OP_TerminateSession), + ftp_reply(3, OP_Ack, OP_WriteFile, offset=0), + ] + ) + + ftp.cmd_put(["local", "remote"], fh=BytesIO(b"x")) + result = ftp.process_ftp_reply("put", timeout=1) + + self.assertEqual(result.error_code, FtpError.Success) + self.assertEqual(len(master.replies), 1) + + def test_list_returns_after_eof_before_late_error(self): + ftp, master = self.make_ftp( + [ + ftp_reply(2, OP_Nack, OP_ListDirectory, payload=[FtpError.EndOfFile]), + ftp_reply(3, OP_Nack, OP_ListDirectory, payload=[FtpError.Fail]), + ] + ) + + result = ftp.cmd_list([]) + + self.assertEqual(result.error_code, FtpError.Success) + self.assertEqual(len(master.replies), 1) + + def test_stale_list_ack_does_not_resend_remove(self): + ftp, master = self.make_ftp( + [ftp_reply(2, OP_Nack, OP_ListDirectory, payload=[FtpError.EndOfFile])] + ) + self.assertEqual(ftp.cmd_list([]).error_code, FtpError.Success) + + # A delayed list ACK arrives while waiting for RemoveFile. It must not + # be dispatched to __handle_list_reply(), which would resend last_op + # (the RemoveFile request) with a new sequence number. + master.replies.extend( + [ + ftp_reply(2, OP_Ack, OP_ListDirectory), + ftp_reply(3, OP_Ack, OP_RemoveFile), + ] + ) + result = ftp.cmd_rm(["remote"]) + + self.assertEqual(result.error_code, FtpError.Success) + self.assertEqual(master.replies, []) + self.assertEqual(len(master.mav.sent), 3) + + def test_out_of_order_burst_reply_is_dispatched(self): + ftp, _master = self.make_ftp( + [ + ftp_reply(2, OP_Ack, OP_OpenFileRO, payload=[81, 0, 0, 0]), + ftp_reply( + 3, + OP_Ack, + OP_BurstReadFile, + payload=b"x" * 80, + burst_complete=1, + ), + # This is the remaining part of the first burst. The next + # burst request has already been sent, so its sequence is + # older than last_op but still belongs to this download. + ftp_reply(3, OP_Ack, OP_BurstReadFile, payload=b"y", offset=80, burst_complete=1), + ftp_reply(5, OP_Ack, OP_TerminateSession), + ] + ) + + ftp.cmd_get( + ["remote", "-"], + callback=lambda _fh: MAVFTPReturn("Get", FtpError.Success), + ) + result = ftp.process_ftp_reply("get", timeout=1) + + self.assertEqual(result.error_code, FtpError.Success) + + def test_completed_put_skips_late_reply_after_termination_timeout(self): + ftp, master = self.make_ftp( + [ + ftp_reply(2, OP_Ack, OP_WriteFile), + ftp_reply(3, OP_Ack, OP_WriteFile), + ] + ) + ftp.pending_terminate_seq = 7 + + def complete_operation(_message): + ftp.completed_reply = (OP_WriteFile, 6) + return MAVFTPReturn("WriteFile", FtpError.Success) + + setattr(ftp, "_MAVFTP__mavlink_packet", complete_operation) + result = ftp.process_ftp_reply("put", timeout=1) + + self.assertEqual(result.error_code, FtpError.Success) + self.assertEqual(len(master.replies), 1) + + def test_incomplete_burst_read_reports_timeout_on_idle(self): + ftp, _master = self.make_ftp( + [ + ftp_reply(2, OP_Ack, OP_OpenFileRO, payload=[160, 0, 0, 0]), + ftp_reply(3, OP_Ack, OP_BurstReadFile, payload=b"x" * 80), + ] + ) + + ftp.cmd_get( + ["remote"], + callback=lambda _fh: MAVFTPReturn("Get", FtpError.Success), + ) + result = ftp.process_ftp_reply("get", timeout=1) + + self.assertEqual(result.error_code, FtpError.RemoteReplyTimeout) + self.assertIsNone(ftp.get_result) + + def test_callback_failure_does_not_publish_download(self): + with tempfile.TemporaryDirectory() as tempdir: + destination = f"{tempdir}/param.pck" + ftp, _master = self.make_ftp( + [ + ftp_reply(2, OP_Ack, OP_OpenFileRO, payload=[3, 0, 0, 0]), + ftp_reply( + 3, + OP_Ack, + OP_BurstReadFile, + payload=b"bad", + burst_complete=1, + ), + ftp_reply(4, OP_Ack, OP_TerminateSession), + ] + ) + + ftp.cmd_get( + ["@PARAM/param.pck", destination], + callback=lambda _fh: MAVFTPReturn("GetParams", FtpError.Fail), + ) + result = ftp.process_ftp_reply("getparams", timeout=1) + + self.assertEqual(result.error_code, FtpError.Fail) + self.assertFalse(os.path.exists(destination)) + + +class TestMAVFTPParamDecode(unittest.TestCase): + """Validate packed parameter name constraints.""" + + @staticmethod + def packed_param(name): + # A float parameter with one name component and no defaults. + header = struct.pack(" Date: Thu, 27 Aug 2026 17:17:26 +0200 Subject: [PATCH 02/32] fix(mavftp): accept partial burst-read acknowledgements A successful BurstReadFile reply can contain only part of a download.\n\nReturn success after processing every ACK, including replies that request\nthe next burst. This prevents a valid partial reply from being reported as\na transfer failure on slow telemetry links, while preserving EOF and gap\nrecovery handling. fix(mavftp): safely decode malformed burst nacks --- mavftp.py | 20 ++++++++++---------- tests/test_mavftp.py | 42 ++++++++++++++++++++++++++++++++++++------ 2 files changed, 46 insertions(+), 16 deletions(-) diff --git a/mavftp.py b/mavftp.py index 83e536142..27dd1ef8c 100644 --- a/mavftp.py +++ b/mavftp.py @@ -927,13 +927,12 @@ def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, "FTP: burst continue at %u %u", more.offset, self.fh.tell() ) self.__send(more) - elif op.opcode == OP_Nack: - ecode = ( - FtpError(op.payload[0]) - if op.payload is not None - else FtpError.NoErrorCodeInNack - ) - if ecode in (FtpError.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: @@ -956,12 +955,11 @@ def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, if self.__check_read_finished(): return MAVFTPReturn("BurstReadFile", FtpError.Success) self.__check_read_send() - elif self.ftp_settings.debug > 0: - logging.info("FTP: burst Nack (ecode:%u): %s", ecode, op) return MAVFTPReturn("BurstReadFile", FtpError.Fail) if self.ftp_settings.debug > 0: logging.error("FTP: burst nack: %s", op) - return MAVFTPReturn("BurstReadFile", FtpError.Fail) + self.__terminate_session() + return nack_result else: logging.warning("FTP: burst error: %s", op) return MAVFTPReturn("BurstReadFile", FtpError.Fail) @@ -1598,6 +1596,8 @@ def process_ftp_reply( ): 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( diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index 1a3999194..42eab126b 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -35,7 +35,7 @@ ) -class FakeFTPMessage: +class FakeFTPMessage: # pylint: disable=too-few-public-methods """Minimal FILE_TRANSFER_PROTOCOL message for reply-loop tests.""" def __init__(self, op): @@ -48,7 +48,7 @@ def get_type(): return "FILE_TRANSFER_PROTOCOL" -class FakeMAV: +class FakeMAV: # pylint: disable=too-few-public-methods """Record FTP sends without requiring a MAVLink transport.""" def __init__(self): @@ -58,7 +58,7 @@ def file_transfer_protocol_send(self, *args): self.sent.append(args) -class FakeMaster: +class FakeMaster: # pylint: disable=too-few-public-methods """Serve a predetermined sequence of FTP replies.""" source_system = 1 @@ -74,7 +74,7 @@ def recv_match(self, **_kwargs): return None -def ftp_reply(seq, opcode, req_opcode, payload=None, offset=0, burst_complete=0): +def ftp_reply(seq, opcode, req_opcode, payload=None, offset=0): """Create a parsed FTP response represented as a minimal MAVLink message.""" data = bytearray(payload) if payload is not None else bytearray() return FakeFTPMessage( @@ -84,7 +84,7 @@ def ftp_reply(seq, opcode, req_opcode, payload=None, offset=0, burst_complete=0) opcode=opcode, size=len(data), req_opcode=req_opcode, - burst_complete=burst_complete, + burst_complete=0, offset=offset, payload=data, ) @@ -169,7 +169,14 @@ def test_out_of_order_burst_reply_is_dispatched(self): # This is the remaining part of the first burst. The next # burst request has already been sent, so its sequence is # older than last_op but still belongs to this download. - ftp_reply(3, OP_Ack, OP_BurstReadFile, payload=b"y", offset=80, burst_complete=1), + ftp_reply( + 3, + OP_Ack, + OP_BurstReadFile, + payload=b"y", + offset=80, + burst_complete=1, + ), ftp_reply(5, OP_Ack, OP_TerminateSession), ] ) @@ -244,6 +251,28 @@ def test_callback_failure_does_not_publish_download(self): self.assertEqual(result.error_code, FtpError.Fail) self.assertFalse(os.path.exists(destination)) + def test_malformed_burst_nacks_are_decoded(self): + for payload, expected_error in ( + (b"", FtpError.NoErrorCodeInPayload), + (b"\xff", FtpError.InvalidErrorCode), + ): + with self.subTest(payload=payload): + ftp, master = self.make_ftp( + [ + ftp_reply(2, OP_Ack, OP_OpenFileRO, payload=[1, 0, 0, 0]), + ftp_reply(3, OP_Nack, OP_BurstReadFile, payload=payload), + ftp_reply(4, OP_Ack, OP_TerminateSession), + ] + ) + ftp.cmd_get( + ["remote", "-"], + callback=lambda _fh: MAVFTPReturn("Get", FtpError.Success), + ) + result = ftp.process_ftp_reply("get", timeout=1) + + self.assertEqual(result.error_code, expected_error) + self.assertEqual(master.replies, []) + class TestMAVFTPParamDecode(unittest.TestCase): """Validate packed parameter name constraints.""" @@ -273,6 +302,7 @@ def test_rejects_name_longer_than_16_bytes(self): self.assertIsNone(MAVFTP.ftp_param_decode(header + first + second)) self.assertIn("parameter name is too long", logs.output[0]) + class TestMAVFTPPayloadDecoding(unittest.TestCase): """Test MAVFTP payload decoding""" From 08fc340be6f0909a39d3c03b2e327b3655316963 Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Thu, 27 Aug 2026 17:18:25 +0200 Subject: [PATCH 03/32] fix(mavftp): reject truncated packed parameter records Validate each packed-parameter record before slicing or unpacking it.\n\nMalformed @PARAM responses can end in a partial header or value, or claim a\nshared name prefix longer than the prior parameter name. Report these cases\nas decode failures rather than raising struct errors or producing corrupted\nparameter names. --- mavftp.py | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/mavftp.py b/mavftp.py index 27dd1ef8c..5c2beecec 100644 --- a/mavftp.py +++ b/mavftp.py @@ -1683,7 +1683,7 @@ def __decode_ftp_ack_and_nack( ) @staticmethod - def ftp_param_decode(data: bytes) -> Union[None, ParamData]: # pylint: disable=too-many-locals + def ftp_param_decode(data: bytes) -> Union[None, ParamData]: # pylint: disable=too-many-locals,too-many-statements,too-many-branches,too-many-return-statements """Decode parameter data, returning ParamData.""" pdata = ParamData() @@ -1718,6 +1718,9 @@ def ftp_param_decode(data: bytes) -> Union[None, ParamData]: # pylint: disable= if len(data) == 0: break + if len(data) < 2: + logging.error("paramftp: truncated parameter header") + return None ptype, plen = struct.unpack("> 4) & 0x0F @@ -1733,10 +1736,31 @@ def ftp_param_decode(data: bytes) -> Union[None, ParamData]: # pylint: disable= 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] - vdata = data[2 + name_len : 2 + name_len + type_len + default_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[2 + name_len + type_len + default_len :] + data = data[record_len:] if with_defaults: if has_default: ( From 0454c89fa53010ef30a74b0f8b2ab09aafa91518 Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Thu, 27 Aug 2026 17:18:48 +0200 Subject: [PATCH 04/32] fix(mavftp): validate parameter response record count Validate decoded packed-parameter records against the transmitted num_params\nheader field.\n\ntotal_params describes the controller-wide parameter count and can be larger\nthan a valid subset response. Using num_params accepts those subset downloads\nwhile still rejecting incomplete or overlong payloads. --- mavftp.py | 13 ++++++++++--- tests/test_mavftp.py | 10 ++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/mavftp.py b/mavftp.py index 5c2beecec..3b1205106 100644 --- a/mavftp.py +++ b/mavftp.py @@ -1694,10 +1694,17 @@ def ftp_param_decode(data: bytes) -> Union[None, ParamData]: # pylint: disable= "paramftp: Not enough data do decode, only %u bytes", len(data) ) return None - 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:] @@ -1778,8 +1785,8 @@ def ftp_param_decode(data: bytes) -> Union[None, ParamData]: # pylint: disable= pdata.add_param(name, v, ptype) count += 1 - if count != total_params: - logging.error("paramftp: bad count %u should be %u", count, total_params) + if count != num_params: + logging.error("paramftp: bad count %u should be %u", count, num_params) return None return pdata diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index 42eab126b..754948409 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -293,6 +293,16 @@ def test_rejects_non_utf8_name(self): self.assertIsNone(MAVFTP.ftp_param_decode(self.packed_param(b"bad\xff"))) self.assertIn("parameter name is not valid UTF-8", logs.output[0]) + def test_rejects_count_larger_than_total(self): + first = self.packed_param(b"PARAM_A")[6:] + second = self.packed_param(b"PARAM_B")[6:] + data = struct.pack(" Date: Thu, 27 Aug 2026 17:19:22 +0200 Subject: [PATCH 05/32] fix(mavftp): close uploads opened by the client Mark file handles opened internally by cmd_put as MAVFTP-owned.\n\nThe existing staging-resource cleanup then closes those handles when the FTP\nsession ends, preventing descriptor leaks and file-lock problems on repeated\nuploads. Handles supplied through cmd_put's fh argument remain caller-owned\nand are left open. --- mavftp.py | 20 +++++++++++--------- tests/test_mavftp.py | 22 ++++++++++++++++++++-- 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/mavftp.py b/mavftp.py index 3b1205106..8fb1083e2 100644 --- a/mavftp.py +++ b/mavftp.py @@ -1018,9 +1018,11 @@ def cmd_put( self.operation_complete = False 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", FtpError.FailToOpenLocalFile) @@ -1558,6 +1560,7 @@ def process_ftp_reply( # terminate reply and for operations with no positive # completion signal. self.read_complete = False + self.operation_complete = False while True: # an FTP operation can have multiple responses m = self.master.recv_match( type=["FILE_TRANSFER_PROTOCOL"], timeout=recv_timeout @@ -1585,15 +1588,14 @@ def process_ftp_reply( callback_failure = self.callback_failure self.callback_failure = None return callback_failure - if self.pending_terminate_seq is None and ( - self.read_complete - or self.operation_complete - or operation_name == "TerminateSession" - or ( - operation_name == "ResetSessions" - and self.pending_reset_seq is None - ) - ): + reply_complete = self.operation_complete + 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: diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index 754948409..ab9577e94 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -29,6 +29,7 @@ OP_Nack, OP_OpenFileRO, OP_ReadFile, + OP_RemoveFile, OP_ResetSessions, OP_TerminateSession, OP_WriteFile, @@ -67,14 +68,18 @@ class FakeMaster: # pylint: disable=too-few-public-methods def __init__(self, replies): self.mav = FakeMAV() self.replies = replies + self.empty_polls = 0 def recv_match(self, **_kwargs): + if self.empty_polls: + self.empty_polls -= 1 + return None if self.replies: return self.replies.pop(0) return None -def ftp_reply(seq, opcode, req_opcode, payload=None, offset=0): +def ftp_reply(seq, opcode, req_opcode, payload=None, offset=0, burst_complete=0): """Create a parsed FTP response represented as a minimal MAVLink message.""" data = bytearray(payload) if payload is not None else bytearray() return FakeFTPMessage( @@ -84,7 +89,7 @@ def ftp_reply(seq, opcode, req_opcode, payload=None, offset=0): opcode=opcode, size=len(data), req_opcode=req_opcode, - burst_complete=0, + burst_complete=burst_complete, offset=offset, payload=data, ) @@ -155,6 +160,19 @@ def test_stale_list_ack_does_not_resend_remove(self): self.assertEqual(master.replies, []) self.assertEqual(len(master.mav.sent), 3) + def test_follow_up_command_waits_after_completed_list(self): + ftp, master = self.make_ftp( + [ftp_reply(2, OP_Nack, OP_ListDirectory, payload=[FtpError.EndOfFile])] + ) + self.assertEqual(ftp.cmd_list([]).error_code, FtpError.Success) + + master.empty_polls = 1 + master.replies.append(ftp_reply(3, OP_Ack, OP_RemoveFile)) + result = ftp.cmd_rm(["remote"]) + + self.assertEqual(result.error_code, FtpError.Success) + self.assertEqual(master.replies, []) + def test_out_of_order_burst_reply_is_dispatched(self): ftp, _master = self.make_ftp( [ From 94cf5265fd891f54a92fe68fc091bd5ad8f83686 Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Wed, 2 Sep 2026 09:41:43 +0200 Subject: [PATCH 06/32] fix(mavftp): correlate operation completion replies Replace the instance-wide operation_complete flag with a completion record containing the request opcode and reply sequence. This prevents delayed or duplicated replies from a previous operation from completing the command currently being awaited. Add a regression test covering a delayed ListDirectory EOF arriving before a RemoveFile acknowledgement. --- mavftp.py | 172 ++++++++++++++++++++++++++++++++++++------- tests/test_mavftp.py | 132 ++++++++++++++++++++++++++++----- 2 files changed, 258 insertions(+), 46 deletions(-) diff --git a/mavftp.py b/mavftp.py index 8fb1083e2..e589b9269 100644 --- a/mavftp.py +++ b/mavftp.py @@ -343,6 +343,10 @@ def __init__( # noqa: PLR0915 pylint: disable=too-many-statements 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.last_gap_send = 0.0 self.read_retries = 0 self.read_total = 0 @@ -350,6 +354,10 @@ def __init__( # noqa: PLR0915 pylint: disable=too-many-statements self.duplicates = 0 self.last_read = None self.last_burst_read: Union[None, float] = 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: Optional[int] = None self.op_start: Union[None, float] = None self.dir_offset = 0 self.last_op_time = time.time() @@ -357,10 +365,10 @@ def __init__( # noqa: PLR0915 pylint: disable=too-many-statements self.rtt = 0.5 self.reached_eof = False self.read_complete = False - # Set by operations with an explicit successful terminal reply. - # Unlike read_complete, these operations do not need a session-close - # handshake to prove their result to process_ftp_reply(). - self.operation_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: Optional[Tuple[int, int]] = 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 @@ -377,6 +385,9 @@ def __init__( # noqa: PLR0915 pylint: disable=too-many-statements 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.write_last_send: Union[None, float] = None self.open_retries = 0 self.list_result: List[DirectoryEntry] = [] @@ -446,7 +457,14 @@ def __send(self, op: FTP_OP) -> None: 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 + elif op.opcode == OP_ReadFile: + self.pending_read_replies[expected_reply_seq] = (op.offset, op.size) + elif op.opcode == OP_WriteFile: + self.pending_write_replies[expected_reply_seq] = op.offset + self.seq = (self.seq + 1) % 65536 self.last_op = op now = time.time() if self.ftp_settings.debug > 1: @@ -497,11 +515,14 @@ def __terminate_session(self) -> None: self.read_gaps = [] self.read_total = 0 self.read_gap_times = {} + self.pending_read_replies = {} self.last_read = None self.last_burst_read = None + self.pending_burst_offset = None self.reached_eof = False self.backlog = 0 self.duplicates = 0 + self.pending_write_replies = {} if self.ftp_settings.debug > 0: logging.info("FTP: Terminated session") self.process_ftp_reply("TerminateSession") @@ -509,7 +530,6 @@ def __terminate_session(self) -> None: def cmd_list(self, args: List[str]) -> MAVFTPReturn: """List files.""" - self.operation_complete = False self.list_result = [] self.list_temp_result = [] if len(args) == 0: @@ -576,7 +596,7 @@ def __handle_list_reply(self, op: FTP_OP, _m) -> MAVFTPReturn: and op.payload[0] == FtpError.EndOfFile ): self.list_result = self.list_temp_result - self.operation_complete = True + self.completed_reply = (op.req_opcode, op.seq) return MAVFTPReturn( "ListDirectory", FtpError.Success, directory_listing=self.list_result ) @@ -668,7 +688,6 @@ def cmd_get( if len(args) == 0 or len(args) > 2: logging.error("Usage: get [FILENAME ]") return MAVFTPReturn("OpenFileRO", FtpError.InvalidArguments) - self.operation_complete = False fname = args[0] if len(args) > 1: self.filename = args[1] @@ -916,6 +935,7 @@ def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, time.time() - self.op_start, ) self.reached_eof = True + self.pending_burst_offset = None if self.__check_read_finished(): return MAVFTPReturn("BurstReadFile", FtpError.Success) self.__check_read_send() @@ -952,6 +972,7 @@ def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, time.time() - self.op_start, ) self.reached_eof = True + self.pending_burst_offset = None if self.__check_read_finished(): return MAVFTPReturn("BurstReadFile", FtpError.Success) self.__check_read_send() @@ -960,12 +981,12 @@ def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, logging.error("FTP: burst nack: %s", op) self.__terminate_session() return nack_result - else: - logging.warning("FTP: burst error: %s", op) + logging.warning("FTP: burst error: %s", op) return MAVFTPReturn("BurstReadFile", FtpError.Fail) def __handle_reply_read(self, op: FTP_OP, _m) -> MAVFTPReturn: """Handle OP_ReadFile reply.""" + self.pending_read_replies.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") @@ -978,6 +999,11 @@ def __handle_reply_read(self, op: FTP_OP, _m) -> 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 + } ofs = self.fh.tell() self.__write_payload(op) self.fh.seek(ofs) @@ -1015,7 +1041,6 @@ def cmd_put( if self.write_list is not None: logging.error("FTP: put already in progress") return MAVFTPReturn("CreateFile", FtpError.PutAlreadyInProgress) - self.operation_complete = False fname = args[0] self.fh = fh self.fh_owned = False @@ -1090,20 +1115,24 @@ def __handle_create_file_reply(self, op: FTP_OP, _m) -> MAVFTPReturn: self.__terminate_session() return MAVFTPReturn("CreateFile", FtpError.FileNotFound) if op.opcode == OP_Ack: - self.__send_more_writes() + self.__send_more_writes(op) else: ret = self.__decode_ftp_ack_and_nack(op) self.__terminate_session() return ret return MAVFTPReturn("CreateFile", FtpError.Success) - def __send_more_writes(self) -> None: + def __send_more_writes(self, completed_reply: Optional[FTP_OP] = 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() - self.operation_complete = True + if completed_reply is not None: + self.completed_reply = ( + completed_reply.req_opcode, + completed_reply.seq, + ) return now = time.time() @@ -1141,6 +1170,13 @@ def __send_more_writes(self) -> None: def __handle_write_reply(self, op: FTP_OP, _m) -> MAVFTPReturn: """Handle OP_WriteFile reply.""" + expected_offset = self.pending_write_replies.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 + } if self.fh is None: self.__terminate_session() return MAVFTPReturn("WriteFile", FtpError.FileNotFound) @@ -1161,7 +1197,7 @@ def __handle_write_reply(self, op: FTP_OP, _m) -> 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() + self.__send_more_writes(op) return MAVFTPReturn("WriteFile", FtpError.Success) def cmd_rm(self, args: List[str]) -> MAVFTPReturn: @@ -1308,6 +1344,27 @@ def __op_parse(self, m) -> FTP_OP: seq, session, opcode, size, req_opcode, burst_complete, offset, payload ) + 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 + + if ( + self.last_op is not None + and op.req_opcode == self.last_op.opcode + and op.seq == (self.last_op.seq + 1) % 65536 + ): + return True + + return False + def __mavlink_packet(self, m) -> MAVFTPReturn: # noqa: PLR0911, PGH004, pylint: disable=too-many-branches, too-many-return-statements """Handle a mavlink packet.""" operation_name = "mavlink_packet" @@ -1349,9 +1406,15 @@ def __mavlink_packet(self, m) -> MAVFTPReturn: # noqa: PLR0911, PGH004, pylint: logging.warning("FTP: dropping packet RX") 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 + 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) @@ -1367,7 +1430,7 @@ def __mavlink_packet(self, m) -> MAVFTPReturn: # noqa: PLR0911, PGH004, pylint: if ( op.req_opcode == OP_TerminateSession and self.pending_terminate_seq is not None - and op.seq == (self.pending_terminate_seq + 1) % 256 + and op.seq == (self.pending_terminate_seq + 1) % 65536 ): # Ack or Nack (InvalidSession means it was already # closed): the handshake has been answered @@ -1527,14 +1590,14 @@ def __handle_reset_sessions_reply(self, op: FTP_OP, _m) -> MAVFTPReturn: """Handle reset sessions reply.""" if ( self.pending_reset_seq is not None - and op.seq == (self.pending_reset_seq + 1) % 256 + 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( + def process_ftp_reply( # 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.""" @@ -1560,7 +1623,7 @@ def process_ftp_reply( # terminate reply and for operations with no positive # completion signal. self.read_complete = False - self.operation_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 @@ -1573,14 +1636,52 @@ def process_ftp_reply( # __terminate_session from this very wait op = self.__op_parse(m) if ( - op.req_opcode == OP_TerminateSession + 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) % 256 + 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) + # 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" @@ -1588,7 +1689,26 @@ def process_ftp_reply( callback_failure = self.callback_failure self.callback_failure = None return callback_failure - reply_complete = self.operation_complete + # 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: @@ -1853,7 +1973,7 @@ def save_params( f.write("\n") logging.info("Outputted %u parameters to %s", len(pdict), filename) - def cmd_getparams( # pylint: disable=too-many-arguments + def cmd_getparams( self, args: List[str], progress_callback=None, diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index ab9577e94..617ace23f 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -79,13 +79,15 @@ def recv_match(self, **_kwargs): return None -def ftp_reply(seq, opcode, req_opcode, payload=None, offset=0, burst_complete=0): +def ftp_reply( # pylint: disable=too-many-arguments + seq, opcode, req_opcode, payload=None, offset=0, burst_complete=0, session=0 +): """Create a parsed FTP response represented as a minimal MAVLink message.""" data = bytearray(payload) if payload is not None else bytearray() return FakeFTPMessage( FTP_OP( seq=seq, - session=0, + session=session, opcode=opcode, size=len(data), req_opcode=req_opcode, @@ -110,6 +112,26 @@ def make_ftp(replies): ftp.ftp_settings.retry_time = 0.2 return ftp, master + def test_terminate_ignores_reply_for_wrong_target_or_session(self): + """TerminateSession accepts replies only from its target and session.""" + for target_system, session in ((99, 0), (1, 1)): + with self.subTest(target_system=target_system, session=session): + ftp, master = self.make_ftp([]) + ftp.pending_terminate_seq = ftp.seq + reply = ftp_reply( + ftp.seq + 1, + OP_Ack, + OP_TerminateSession, + session=session, + ) + reply.target_system = target_system + master.replies.append(reply) + + result = ftp.process_ftp_reply("TerminateSession") + + self.assertEqual(result.error_code, FtpError.Fail) + self.assertEqual(ftp.pending_terminate_seq, ftp.seq) + def test_put_returns_after_completion_before_late_write_reply(self): ftp, master = self.make_ftp( [ @@ -160,19 +182,6 @@ def test_stale_list_ack_does_not_resend_remove(self): self.assertEqual(master.replies, []) self.assertEqual(len(master.mav.sent), 3) - def test_follow_up_command_waits_after_completed_list(self): - ftp, master = self.make_ftp( - [ftp_reply(2, OP_Nack, OP_ListDirectory, payload=[FtpError.EndOfFile])] - ) - self.assertEqual(ftp.cmd_list([]).error_code, FtpError.Success) - - master.empty_polls = 1 - master.replies.append(ftp_reply(3, OP_Ack, OP_RemoveFile)) - result = ftp.cmd_rm(["remote"]) - - self.assertEqual(result.error_code, FtpError.Success) - self.assertEqual(master.replies, []) - def test_out_of_order_burst_reply_is_dispatched(self): ftp, _master = self.make_ftp( [ @@ -184,17 +193,16 @@ def test_out_of_order_burst_reply_is_dispatched(self): payload=b"x" * 80, burst_complete=1, ), - # This is the remaining part of the first burst. The next - # burst request has already been sent, so its sequence is - # older than last_op but still belongs to this download. + # The next burst starts at offset 80. This delayed duplicate + # from the completed burst must not reach its handler. ftp_reply( 3, OP_Ack, OP_BurstReadFile, - payload=b"y", - offset=80, + payload=b"x" * 80, burst_complete=1, ), + ftp_reply(4, OP_Ack, OP_BurstReadFile, payload=b"y", offset=80, burst_complete=1), ftp_reply(5, OP_Ack, OP_TerminateSession), ] ) @@ -206,6 +214,90 @@ def test_out_of_order_burst_reply_is_dispatched(self): result = ftp.process_ftp_reply("get", timeout=1) self.assertEqual(result.error_code, FtpError.Success) + self.assertEqual(ftp.duplicates, 0) + + def test_out_of_order_gap_reply_is_dispatched(self): + ftp, _master = self.make_ftp([]) + ftp.fh = BytesIO() + ftp.filename = "-" + ftp.read_gaps = [(0, 2), (2, 2)] + ftp.read_gap_times = {(0, 2): 0, (2, 2): 0} + + ftp._MAVFTP__send_gap_read((0, 2)) # pylint: disable=protected-access + ftp._MAVFTP__send_gap_read((2, 2)) # pylint: disable=protected-access + + result = ftp._MAVFTP__mavlink_packet( # pylint: disable=protected-access + ftp_reply(3, OP_Ack, OP_ReadFile, payload=b"cd", offset=2) + ) + + self.assertEqual(result.error_code, FtpError.Success) + self.assertEqual(ftp.read_gaps, [(0, 2)]) + self.assertEqual(ftp.fh.getvalue(), b"\x00\x00cd") + + stale_result = ftp._MAVFTP__mavlink_packet( # pylint: disable=protected-access + ftp_reply(99, OP_Ack, OP_ReadFile, payload=b"zz", offset=0) + ) + + self.assertEqual(stale_result.error_code, FtpError.Fail) + self.assertEqual(ftp.read_gaps, [(0, 2)]) + self.assertEqual(ftp.fh.getvalue(), b"\x00\x00cd") + + def test_stale_write_reply_is_discarded(self): + ftp, master = self.make_ftp( + [ + ftp_reply(2, OP_Ack, OP_CreateFile), + ftp_reply(99, OP_Ack, OP_WriteFile, offset=0), + ] + ) + + ftp.cmd_put(["local", "remote"], fh=BytesIO(b"x")) + ftp._MAVFTP__mavlink_packet( # pylint: disable=protected-access + master.replies.pop(0) + ) + result = ftp._MAVFTP__mavlink_packet( # pylint: disable=protected-access + master.replies.pop(0) + ) + + self.assertEqual(result.error_code, FtpError.Fail) + self.assertIsNotNone(ftp.write_list) + self.assertEqual(ftp.write_acks, 0) + + def test_noncurrent_write_nack_fails_upload(self): + ftp, _master = self.make_ftp( + [ + ftp_reply(2, OP_Ack, OP_CreateFile), + ftp_reply( + 3, + OP_Nack, + OP_WriteFile, + payload=[FtpError.FileProtected], + offset=0, + ), + ftp_reply(5, OP_Ack, OP_TerminateSession), + ] + ) + + ftp.cmd_put(["local", "remote"], fh=BytesIO(b"x" * 160)) + result = ftp.process_ftp_reply("put", timeout=1) + + self.assertEqual(result.error_code, FtpError.FileProtected) + + def test_remove_accepts_16_bit_sequence_wrap(self): + ftp, master = self.make_ftp([]) + ftp.seq = 255 + master.replies.append(ftp_reply(256, OP_Ack, OP_RemoveFile)) + + result = ftp.cmd_rm(["remote"]) + + self.assertEqual(result.error_code, FtpError.Success) + + def test_wrong_session_reply_is_not_retained(self): + ftp, master = self.make_ftp([]) + master.replies.append(ftp_reply(2, OP_Ack, OP_RemoveFile, session=1)) + + result = ftp.cmd_rm(["remote"]) + + self.assertEqual(result.error_code, FtpError.Fail) def test_completed_put_skips_late_reply_after_termination_timeout(self): ftp, master = self.make_ftp( From 8ab6106014a1b54edeccbb28e3d3ed4e86a13ec4 Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Wed, 2 Sep 2026 18:57:57 +0200 Subject: [PATCH 07/32] fix(mavftp): honor FTP sessions and retransmission rules Adopt the session IDs returned by OpenFileRO and CreateFile before issuing follow-up requests. Accept those allocation ACKs even though their session differs from the request session. Retain in-flight ReadFile, BurstReadFile, and WriteFile requests so timeout retries resend the original request with its original sequence number. Return decoded NACK errors for gap reads and writes instead of reporting success or FileProtected. Terminate active remote file sessions when either download loop times out. Add regression coverage for session allocation, NACK propagation, retransmission sequence reuse, and both timeout cleanup paths. --- mavftp.py | 149 ++++++++++++++++++++++++++---------- tests/test_mavftp.py | 177 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 282 insertions(+), 44 deletions(-) diff --git a/mavftp.py b/mavftp.py index e589b9269..ad605dc5f 100644 --- a/mavftp.py +++ b/mavftp.py @@ -347,6 +347,7 @@ def __init__( # noqa: PLR0915 pylint: disable=too-many-statements # 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 @@ -358,6 +359,7 @@ def __init__( # noqa: PLR0915 pylint: disable=too-many-statements # 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: Optional[int] = None + self.pending_burst_request: Optional[FTP_OP] = None self.op_start: Union[None, float] = None self.dir_offset = 0 self.last_op_time = time.time() @@ -388,6 +390,7 @@ def __init__( # noqa: PLR0915 pylint: disable=too-many-statements # 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: Union[None, float] = None self.open_retries = 0 self.list_result: List[DirectoryEntry] = [] @@ -447,9 +450,10 @@ def cmd_ftp(self, args: List[str]) -> MAVFTPReturn: # noqa: PLR0911 pylint: dis logging.error(usage) 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: @@ -460,11 +464,15 @@ def __send(self, op: FTP_OP) -> None: 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.seq = (self.seq + 1) % 65536 + 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: @@ -516,18 +524,38 @@ def __terminate_session(self) -> None: 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 = [] @@ -653,6 +681,8 @@ def read(self, path: str, size: int, offset: int = 0) -> Optional[bytes]: 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) @@ -719,6 +749,7 @@ def __handle_open_ro_reply(self, op: FTP_OP, _m) -> MAVFTPReturn: if op.opcode == OP_Ack: if self.filename is None: return MAVFTPReturn("OpenFileRO", FtpError.FileNotFound) + self.session = op.session try: if self.callback is not None or self.filename == "-": self.fh = SIO() @@ -936,11 +967,14 @@ def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, ) self.reached_eof = True self.pending_burst_offset = None + self.pending_burst_request = None if self.__check_read_finished(): return MAVFTPReturn("BurstReadFile", FtpError.Success) self.__check_read_send() return MAVFTPReturn("BurstReadFile", FtpError.Success) - more = self.last_op + 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( @@ -973,6 +1007,7 @@ def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, ) self.reached_eof = True self.pending_burst_offset = None + self.pending_burst_request = None if self.__check_read_finished(): return MAVFTPReturn("BurstReadFile", FtpError.Success) self.__check_read_send() @@ -987,6 +1022,7 @@ def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, def __handle_reply_read(self, op: FTP_OP, _m) -> 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") @@ -1004,6 +1040,11 @@ def __handle_reply_read(self, op: FTP_OP, _m) -> MAVFTPReturn: 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) @@ -1027,7 +1068,9 @@ def __handle_reply_read(self, op: FTP_OP, _m) -> MAVFTPReturn: 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", FtpError.Success) @@ -1115,6 +1158,7 @@ def __handle_create_file_reply(self, op: FTP_OP, _m) -> MAVFTPReturn: self.__terminate_session() return MAVFTPReturn("CreateFile", FtpError.FileNotFound) if op.opcode == OP_Ack: + self.session = op.session self.__send_more_writes(op) else: ret = self.__decode_ftp_ack_and_nack(op) @@ -1151,19 +1195,30 @@ def __send_more_writes(self, completed_reply: Optional[FTP_OP] = None) -> 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), + write = next( + ( + pending_write + for pending_write in self.pending_write_requests.values() + if pending_write.offset == ofs + ), + None, ) - self.__send(write) + 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 @@ -1171,19 +1226,26 @@ def __send_more_writes(self, completed_reply: Optional[FTP_OP] = None) -> None: def __handle_write_reply(self, op: FTP_OP, _m) -> 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", 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", FtpError.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 @@ -1389,7 +1451,11 @@ def __mavlink_packet(self, m) -> MAVFTPReturn: # noqa: PLR0911, PGH004, pylint: 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", @@ -1465,8 +1531,21 @@ def __send_gap_read(self, g) -> None: len(self.read_gaps), self.backlog, ) - read = FTP_OP(self.seq, self.session, OP_ReadFile, length, 0, 0, offset, None) - self.__send(read) + 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() @@ -1524,13 +1603,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 @@ -1557,18 +1630,8 @@ def __idle_task(self) -> bool: self.rtt, dt, ) - self.__send( - FTP_OP( - self.seq, - self.session, - OP_BurstReadFile, - self.burst_size, - 0, - 0, - self.fh.tell(), - None, - ) - ) + 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 @@ -1727,6 +1790,12 @@ def process_ftp_reply( # pylint: disable=too-many-branches, too-many-locals ) 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( diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index 617ace23f..33a5d8f5f 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -15,7 +15,7 @@ import unittest from io import BytesIO, StringIO -#from unittest.mock import patch +from unittest.mock import patch from pymavlink import mavutil from pymavlink.mavftp import ( FTP_OP, @@ -35,6 +35,8 @@ OP_WriteFile, ) +# pylint: disable=protected-access + class FakeFTPMessage: # pylint: disable=too-few-public-methods """Minimal FILE_TRANSFER_PROTOCOL message for reply-loop tests.""" @@ -98,8 +100,8 @@ def ftp_reply( # pylint: disable=too-many-arguments ) -class TestMAVFTPReplyCompletion(unittest.TestCase): - """Regression tests for command completion and idle fallback.""" +class TestMAVFTPReplyCompletion(unittest.TestCase): # pylint: disable=too-many-public-methods + """Regression tests for FTP replies, retries, and session cleanup.""" @staticmethod def make_ftp(replies): @@ -132,12 +134,179 @@ def test_terminate_ignores_reply_for_wrong_target_or_session(self): self.assertEqual(result.error_code, FtpError.Fail) self.assertEqual(ftp.pending_terminate_seq, ftp.seq) + @staticmethod + def sent_request_sequences(master, opcode): + """Return FTP request sequence numbers sent for an opcode.""" + return [ + struct.unpack_from(" Date: Wed, 2 Sep 2026 21:41:12 +0200 Subject: [PATCH 08/32] fix(mavftp): use runtime containers for parameter sorting The parameter decoder imported typing.Tuple and typing.Dict for annotations but called them as constructors at runtime. This broke getparams on supported Python versions after a successful FTP transfer. Use the built-in tuple and dict constructors for sorting and rebuilding decoded parameter mappings. --- mavftp.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mavftp.py b/mavftp.py index ad605dc5f..d8364885d 100644 --- a/mavftp.py +++ b/mavftp.py @@ -1985,7 +1985,7 @@ def ftp_param_decode(data: bytes) -> Union[None, ParamData]: # pylint: disable= @staticmethod def missionplanner_sort(item: str) -> Tuple[str, ...]: """Sorts a parameter name according to the rules defined in the Mission Planner software.""" - return Tuple(item.split("_")) + return tuple(item.split("_")) @staticmethod def extract_params( @@ -1998,13 +1998,13 @@ def extract_params( pdict[name.decode("utf-8")] = (value, ptype) if sort_type == "missionplanner": - pdict = Dict( + pdict = dict( sorted( pdict.items(), key=lambda x: MAVFTP.missionplanner_sort(x[0]) ) ) # sort alphabetically elif sort_type == "mavproxy": - pdict = Dict(sorted(pdict.items())) # sort in ASCIIbetical order + pdict = dict(sorted(pdict.items())) # sort in ASCIIbetical order elif sort_type == "none": pass return pdict From a89b3a29ba8185fd0601e98952f52a89579a895a Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Thu, 3 Sep 2026 01:13:44 +0200 Subject: [PATCH 09/32] fix(mavftp): avoid writing callback downloads to disk Treat callback-owned downloads as fully consumed by the callback and skip the generic publish step. This prevents virtual MAVFTP paths such as @PARAM/param.pck?withdefaults=1 from being treated as local files. Add regression coverage for successful callbacks. --- mavftp.py | 6 +++++- tests/test_mavftp.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/mavftp.py b/mavftp.py index d8364885d..322e218c1 100644 --- a/mavftp.py +++ b/mavftp.py @@ -819,6 +819,10 @@ def __check_read_finished(self) -> bool: 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 ( @@ -2042,7 +2046,7 @@ def save_params( f.write("\n") logging.info("Outputted %u parameters to %s", len(pdict), filename) - def cmd_getparams( + def cmd_getparams( # pylint: disable=too-many-arguments self, args: List[str], progress_callback=None, diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index 33a5d8f5f..0e45fd5bb 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -530,6 +530,38 @@ def test_callback_failure_does_not_publish_download(self): self.assertEqual(result.error_code, FtpError.Fail) self.assertFalse(os.path.exists(destination)) + def test_callback_success_does_not_publish_download(self): + with tempfile.TemporaryDirectory() as tempdir: + destination = f"{tempdir}/param.pck" + callback_data = [] + ftp, _master = self.make_ftp( + [ + ftp_reply(2, OP_Ack, OP_OpenFileRO, payload=[3, 0, 0, 0]), + ftp_reply( + 3, + OP_Ack, + OP_BurstReadFile, + payload=b"data", + burst_complete=1, + ), + ftp_reply(4, OP_Ack, OP_TerminateSession), + ] + ) + + def callback(fh): + callback_data.append(fh.read()) + return MAVFTPReturn("GetParams", FtpError.Success) + + ftp.cmd_get( + ["@PARAM/param.pck?withdefaults=1", destination], + callback=callback, + ) + result = ftp.process_ftp_reply("getparams", timeout=1) + + self.assertEqual(result.error_code, FtpError.Success) + self.assertEqual(callback_data, [b"data"]) + self.assertFalse(os.path.exists(destination)) + def test_malformed_burst_nacks_are_decoded(self): for payload, expected_error in ( (b"", FtpError.NoErrorCodeInPayload), From 60fbd719a9239f2e239e3ef229788973a1c7fb65 Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Thu, 3 Sep 2026 12:01:12 +0200 Subject: [PATCH 10/32] fix(mavftp): retain successful active read replies Keep a reply result when it matches an in-flight request, including successful replies. A final out-of-order gap ReadFile ACK can complete a download after a later BurstReadFile becomes last_op; previously the data was correct but process_ftp_reply() returned Fail. Add regression coverage for this completion path, stabilize the mocked read-timeout clock, and log the number of remaining gaps correctly. --- mavftp.py | 19 +++++++---------- tests/test_mavftp.py | 51 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 58 insertions(+), 12 deletions(-) diff --git a/mavftp.py b/mavftp.py index 322e218c1..9d87f130b 100644 --- a/mavftp.py +++ b/mavftp.py @@ -680,12 +680,12 @@ def read(self, path: str, size: int, offset: int = 0) -> Optional[bytes]: logging.error(e) self.__idle_task() time.sleep(0.0001) - logging.info("loop closed, gaps:%u, done: %u", self.read_gaps, self.done) + logging.info("loop closed, gaps:%u, done: %u", len(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) + logging.error("closed read with %u gaps", len(self.read_gaps)) return None def cmd_set(self, args: List[str]) -> MAVFTPReturn: @@ -1713,11 +1713,11 @@ def process_ftp_reply( # pylint: disable=too-many-branches, too-many-locals self.pending_terminate_seq = None ret = MAVFTPReturn(operation_name, FtpError.Success) else: - # 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. + # Keep a result from the latest request or an active + # in-flight request. Packet handlers must still see + # stale replies so they can maintain their own state, + # but retaining a stale 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 @@ -1743,10 +1743,7 @@ def process_ftp_reply( # pylint: disable=too-many-branches, too-many-locals if ( reply_matches_last_op or completed_upload - or ( - reply_matches_active_request - and packet_ret.error_code != FtpError.Success - ) + or reply_matches_active_request ): ret = packet_ret if ( diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index 0e45fd5bb..01e1dd1bc 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -296,7 +296,13 @@ def test_read_timeout_terminates_active_session(self): lambda: terminated.append(True), ) - with patch("pymavlink.mavftp.time.time", side_effect=[0, 0, 0, 0, 6]): + clock_calls = [0] + + def fake_time(): + clock_calls[0] += 1 + return 0 if clock_calls[0] <= 20 else 6 + + with patch("pymavlink.mavftp.time.time", side_effect=fake_time): self.assertIsNone(ftp.read("remote", 1)) self.assertEqual(terminated, [True]) @@ -411,6 +417,49 @@ def test_out_of_order_gap_reply_is_dispatched(self): self.assertEqual(ftp.read_gaps, [(0, 2)]) self.assertEqual(ftp.fh.getvalue(), b"\x00\x00cd") + def test_out_of_order_final_gap_reply_reports_success(self): + """A successful final gap repair completes a read when it is not last_op.""" + ftp, master = self.make_ftp([]) + ftp.fh = BytesIO() + ftp.filename = "-" + ftp.op_start = 1 + ftp.requested_size = 240 + ftp.burst_size = 239 + ftp.reached_eof = True + ftp.read_gaps = [(0, 120), (120, 120)] + ftp.read_gap_times = {(0, 120): 0, (120, 120): 0} + + ftp._MAVFTP__send_gap_read((0, 120)) + ftp._MAVFTP__send_gap_read((120, 120)) + # A burst request was sent after the gap requests, so neither gap + # reply matches last_op even though both remain active requests. + ftp._MAVFTP__send( + FTP_OP( + ftp.seq, + ftp.session, + OP_BurstReadFile, + 239, + 0, + 0, + 240, + None, + ) + ) + master.replies.extend( + [ + ftp_reply(3, OP_Ack, OP_ReadFile, payload=b"b" * 120, offset=120), + ftp_reply(2, OP_Ack, OP_ReadFile, payload=b"a" * 120, offset=0), + ftp_reply(5, OP_Ack, OP_TerminateSession), + ] + ) + + result = ftp.process_ftp_reply("get", timeout=1) + + self.assertEqual(result.error_code, FtpError.Success) + self.assertTrue(ftp.read_complete) + self.assertEqual(ftp.read_gaps, []) + self.assertEqual(ftp.get_result, b"a" * 120 + b"b" * 120) + def test_stale_write_reply_is_discarded(self): ftp, master = self.make_ftp( [ From ab0c39e2435a5179e81fb8442723d48f01057920 Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Thu, 3 Sep 2026 12:01:52 +0200 Subject: [PATCH 11/32] fix(mavftp): fix reporting of target component --- mavftp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mavftp.py b/mavftp.py index 9d87f130b..bd088e070 100644 --- a/mavftp.py +++ b/mavftp.py @@ -2440,7 +2440,7 @@ def wait_heartbeat(m) -> None: 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_system + "Heartbeat from system %u, component %u", m.target_system, m.target_component ) From 40b54f39cb238e5c2706503d3cecf54b52f3e346 Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Thu, 3 Sep 2026 12:46:18 +0200 Subject: [PATCH 12/32] refactor(mavftp): simplify callback download state handling The callback path already clears publish_result before invoking the callback, making the failure-branch assignment redundant. Remove the dead assignment while preserving callback failure propagation and the guarantee that callback-owned downloads are not published as files. Document the callback success and failure regression coverage, and make the successful callback fixture advertise the exact four-byte payload it consumes. --- mavftp.py | 1 - tests/test_mavftp.py | 4 +++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/mavftp.py b/mavftp.py index bd088e070..adacc29bd 100644 --- a/mavftp.py +++ b/mavftp.py @@ -830,7 +830,6 @@ def __check_read_finished(self) -> bool: 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) diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index 01e1dd1bc..ac5c51e40 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -554,6 +554,7 @@ def test_incomplete_burst_read_reports_timeout_on_idle(self): self.assertIsNone(ftp.get_result) def test_callback_failure_does_not_publish_download(self): + """Regression: a failing callback must not publish its temporary download.""" with tempfile.TemporaryDirectory() as tempdir: destination = f"{tempdir}/param.pck" ftp, _master = self.make_ftp( @@ -580,12 +581,13 @@ def test_callback_failure_does_not_publish_download(self): self.assertFalse(os.path.exists(destination)) def test_callback_success_does_not_publish_download(self): + """Regression: callbacks consume all four advertised bytes without publishing.""" with tempfile.TemporaryDirectory() as tempdir: destination = f"{tempdir}/param.pck" callback_data = [] ftp, _master = self.make_ftp( [ - ftp_reply(2, OP_Ack, OP_OpenFileRO, payload=[3, 0, 0, 0]), + ftp_reply(2, OP_Ack, OP_OpenFileRO, payload=[4, 0, 0, 0]), ftp_reply( 3, OP_Ack, From 8dde2aa23f274280465c871e6802215fb7a2b5b3 Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Thu, 3 Sep 2026 13:07:18 +0200 Subject: [PATCH 13/32] fix(mavftp): preserve synchronous read ranges Keep read_sector() downloads in memory, retain the caller's requested size, and start BurstReadFile at the requested offset. This prevents FUSE reads from downloading the whole remote file, returning an oversized range, or publishing a local file named after the remote path. Add regression coverage for offset reads, returned range length, and absence of local output. --- mavftp.py | 38 +++++++++++++++++++++++++------------- tests/test_mavftp.py | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 13 deletions(-) diff --git a/mavftp.py b/mavftp.py index adacc29bd..cb567e94e 100644 --- a/mavftp.py +++ b/mavftp.py @@ -397,6 +397,9 @@ def __init__( # noqa: PLR0915 pylint: disable=too-many-statements self.list_temp_result: List[DirectoryEntry] = [] self.requested_size: int = 0 self.requested_offset: int = 0 + # The synchronous read/read_sector API returns data to its caller and + # must never publish a file named after the remote path. + self.read_to_memory = False # 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 @@ -505,6 +508,7 @@ def __terminate_session(self) -> None: self.__release_staging() self.fh = None self.filename = None + self.read_to_memory = False self.write_list = None if self.callback is not None: # tell caller that the transfer failed @@ -642,6 +646,10 @@ def read(self, path: str, size: int, offset: int = 0) -> Optional[bytes]: self.requested_offset = offset self.requested_size = size self.filename = path + self.read_to_memory = True + self.callback = None + self.callback_failure = None + self.callback_progress = None self.done = False logging.info( @@ -726,6 +734,7 @@ def cmd_get( if callback is None or self.ftp_settings.debug > 1: logging.info("Getting %s to %s", fname, self.filename) self.op_start = time.time() + self.read_to_memory = False self.callback = callback self.callback_failure = None self.callback_progress = progress_callback @@ -751,8 +760,10 @@ def __handle_open_ro_reply(self, op: FTP_OP, _m) -> MAVFTPReturn: return MAVFTPReturn("OpenFileRO", FtpError.FileNotFound) self.session = op.session try: - if self.callback is not None or self.filename == "-": + if self.callback is not None or self.filename == "-" or self.read_to_memory: self.fh = SIO() + if self.read_to_memory: + self.fh.seek(self.requested_offset) else: self.__release_staging() (temp_fd, self.temp_filename) = tempfile.mkstemp(prefix="mavftp_") @@ -764,16 +775,6 @@ def __handle_open_ro_reply(self, op: FTP_OP, _m) -> MAVFTPReturn: 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 @@ -789,11 +790,19 @@ def __handle_open_ro_reply(self, op: FTP_OP, _m) -> MAVFTPReturn: ) if self.ftp_settings.debug > 0: logging.info("Remote file size: %u", self.remote_file_size) - self.requested_size = self.remote_file_size + if not self.read_to_memory: + self.requested_size = self.remote_file_size else: self.remote_file_size = 0 read = FTP_OP( - self.seq, self.session, OP_BurstReadFile, self.burst_size, 0, 0, 0, None + self.seq, + self.session, + OP_BurstReadFile, + self.burst_size, + 0, + 0, + self.requested_offset if self.read_to_memory else 0, + None, ) self.last_burst_read = time.time() self.__send(read) @@ -831,6 +840,9 @@ def __check_read_finished(self) -> bool: ): self.callback_failure = callback_result self.callback = None + elif self.read_to_memory: + publish_result = False + self.done = True elif self.filename == "-": self.fh.seek(0) else: diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index ac5c51e40..14d1f6a87 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -307,6 +307,45 @@ def fake_time(): self.assertEqual(terminated, [True]) + def test_read_sector_returns_only_requested_range_without_local_output(self): + """A sector read starts at its offset and must not publish the remote path.""" + with tempfile.TemporaryDirectory() as tempdir: + previous_cwd = os.getcwd() + os.chdir(tempdir) + try: + ftp, master = self.make_ftp( + [ + ftp_reply( + 2, + OP_Ack, + OP_OpenFileRO, + payload=[8, 0, 0, 0], + session=7, + ), + ftp_reply( + 3, + OP_Ack, + OP_BurstReadFile, + payload=b"defgh", + offset=3, + burst_complete=1, + session=7, + ), + ftp_reply(4, OP_Ack, OP_TerminateSession, session=7), + ] + ) + + self.assertEqual(ftp.read_sector("remote", 3, 2), b"de") + self.assertFalse(os.path.exists("remote")) + burst_request = next( + sent[-1] + for sent in master.mav.sent + if sent[-1][3] == OP_BurstReadFile + ) + self.assertEqual(struct.unpack_from(" Date: Thu, 3 Sep 2026 13:08:17 +0200 Subject: [PATCH 14/32] fix(mavftp): write parameter datatype comments Use integer parameter type IDs in save_params(), matching the values returned by ftp_param_decode() and extract_params(). This makes the getparams datatype-comment option usable for valid parameter files. Add regression coverage for the emitted float datatype comment. --- mavftp.py | 10 +++++----- tests/test_mavftp.py | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/mavftp.py b/mavftp.py index cb567e94e..47726aea0 100644 --- a/mavftp.py +++ b/mavftp.py @@ -2023,7 +2023,7 @@ def extract_params( @staticmethod def save_params( - pdict: Dict[str, Tuple[float, str]], + pdict: Dict[str, Tuple[float, int]], filename: str, sort_type: str, add_datatype_comments: bool, @@ -2034,10 +2034,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( diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index 14d1f6a87..2a302bcc9 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -713,6 +713,22 @@ def test_rejects_name_longer_than_16_bytes(self): self.assertIsNone(MAVFTP.ftp_param_decode(header + first + second)) self.assertIn("parameter name is too long", logs.output[0]) + def test_save_params_writes_integer_datatype_comments(self): + """Decoded integer type IDs produce the documented datatype comment.""" + with tempfile.TemporaryDirectory() as tempdir: + output = f"{tempdir}/params.txt" + + MAVFTP.save_params( + {"TEST_PARAM": (1.0, 4)}, + output, + "missionplanner", + add_datatype_comments=True, + add_timestamp_comment=False, + ) + + with open(output, encoding="utf-8") as param_file: + self.assertEqual(param_file.read(), "TEST_PARAM,1 # 32-bit float\n") + class TestMAVFTPPayloadDecoding(unittest.TestCase): """Test MAVFTP payload decoding""" From c609a2021142df0c474cc1f336dfb683a1176e44 Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Thu, 3 Sep 2026 13:13:32 +0200 Subject: [PATCH 15/32] fix(mavftp): fail unexpected short gap replies Return a ReadFile failure after terminating a session for a short acknowledgement that does not satisfy an outstanding gap. This prevents the reply loop from reporting a completed download after a file-size race or malformed reply. Add regression coverage for the unexpected short gap-ACK path. --- mavftp.py | 1 + tests/test_mavftp.py | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/mavftp.py b/mavftp.py index 47726aea0..daeca3eff 100644 --- a/mavftp.py +++ b/mavftp.py @@ -1075,6 +1075,7 @@ def __handle_reply_read(self, op: FTP_OP, _m) -> MAVFTPReturn: elif op.size < self.burst_size: logging.info("FTP: file size changed to %u", op.offset + op.size) self.__terminate_session() + return MAVFTPReturn("ReadFile", FtpError.Fail) else: self.duplicates += 1 if self.ftp_settings.debug > 0: diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index 2a302bcc9..1d045063a 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -196,6 +196,28 @@ def test_gap_read_nack_preserves_server_error(self): self.assertEqual(result.error_code, FtpError.FileNotFound) self.assertEqual(terminated, [True]) + def test_unexpected_short_gap_ack_reports_failure(self): + """A short reply for an unknown gap must not report a completed read.""" + ftp, _master = self.make_ftp([]) + ftp.fh = BytesIO() + ftp.filename = "remote" + ftp.read_gaps = [(4, 2)] + ftp.read_gap_times = {(4, 2): 1} + terminated = [] + setattr( + ftp, + "_MAVFTP__terminate_session", + lambda: terminated.append(True), + ) + + result = ftp._MAVFTP__handle_reply_read( + FTP_OP(1, 0, OP_Ack, 1, OP_ReadFile, 0, 0, bytearray(b"x")), + None, + ) + + self.assertEqual(result.error_code, FtpError.Fail) + self.assertEqual(terminated, [True]) + def test_write_nack_preserves_server_error(self): """WriteFile NACKs retain their precise protocol error code.""" ftp, _master = self.make_ftp([]) From 63f1997c2ee80d546178c302ac4c6b3a5c19a4de Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Thu, 3 Sep 2026 13:15:48 +0200 Subject: [PATCH 16/32] fix(mavftp): clean up failed download callbacks Catch download callback exceptions, retain an FTP failure result, and continue through the normal session cleanup path. This also makes getparams output failures fail the command rather than leaking the active FTP session. Add regression coverage for callback exceptions during download completion. --- mavftp.py | 19 ++++++++++++------- tests/test_mavftp.py | 22 ++++++++++++++++++++++ 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/mavftp.py b/mavftp.py index daeca3eff..7c67ec037 100644 --- a/mavftp.py +++ b/mavftp.py @@ -833,13 +833,18 @@ def __check_read_finished(self) -> bool: # 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 != FtpError.Success - ): - self.callback_failure = callback_result - self.callback = None + try: + callback_result = self.callback(self.fh) + if ( + isinstance(callback_result, MAVFTPReturn) + and callback_result.error_code != FtpError.Success + ): + self.callback_failure = callback_result + except Exception as exc: # pylint: disable=broad-exception-caught + logging.error("FTP: download callback failed: %s", exc) + self.callback_failure = MAVFTPReturn("Get", FtpError.Fail) + finally: + self.callback = None elif self.read_to_memory: publish_result = False self.done = True diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index 1d045063a..ce337c88d 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -641,6 +641,28 @@ def test_callback_failure_does_not_publish_download(self): self.assertEqual(result.error_code, FtpError.Fail) self.assertFalse(os.path.exists(destination)) + def test_callback_exception_terminates_download(self): + """Callback exceptions are reported as FTP failures after session cleanup.""" + ftp, _master = self.make_ftp([]) + ftp.fh = BytesIO(b"data") + ftp.filename = "-" + ftp.op_start = 1 + ftp.requested_size = 4 + ftp.read_total = 4 + ftp.reached_eof = True + terminated = [] + setattr(ftp, "_MAVFTP__terminate_session", lambda: terminated.append(True)) + + def failing_callback(_fh): + raise RuntimeError("decode failed") + + ftp.callback = failing_callback + + self.assertTrue(ftp._MAVFTP__check_read_finished()) + self.assertEqual(terminated, [True]) + self.assertIsNotNone(ftp.callback_failure) + self.assertEqual(ftp.callback_failure.error_code, FtpError.Fail) + def test_callback_success_does_not_publish_download(self): """Regression: callbacks consume all four advertised bytes without publishing.""" with tempfile.TemporaryDirectory() as tempdir: From 5212adc758e28f1b1877cdc63db060fa72772e9e Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Thu, 3 Sep 2026 13:17:58 +0200 Subject: [PATCH 17/32] fix(mavftp): validate transfer settings Reject unsafe command settings before they can violate retry invariants, stall transfer queues, divide by zero, or exceed the MAVFTP payload limit. Defensively validate upload write sizing for callers that set settings directly. Add regressions for invalid command settings and direct invalid upload block sizes. --- mavftp.py | 58 +++++++++++++++++++++++++++++++++++++++++++- tests/test_mavftp.py | 26 ++++++++++++++++++++ 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/mavftp.py b/mavftp.py index 7c67ec037..816f5e851 100644 --- a/mavftp.py +++ b/mavftp.py @@ -14,6 +14,7 @@ # FLAKE_CLEAN import logging +import math import os import tempfile import random @@ -715,6 +716,55 @@ def cmd_set(self, args: List[str]) -> MAVFTPReturn: logging.error("Invalid parameter value: %s", args[1]) return MAVFTPReturn("Set", FtpError.InvalidArguments) + setting = self.ftp_settings._vars[setting_name] # pylint: disable=protected-access + if not math.isfinite(setting_value): + logging.error("Invalid parameter value: %s", args[1]) + return MAVFTPReturn("Set", FtpError.InvalidArguments) + if setting.type is int: + if not setting_value.is_integer(): + logging.error("Invalid integer parameter value: %s", args[1]) + return MAVFTPReturn("Set", FtpError.InvalidArguments) + setting_value = int(setting_value) + + bounded_settings = { + "debug": (0, 2), + "pkt_loss_tx": (0, 100), + "pkt_loss_rx": (0, 100), + "max_backlog": (1, None), + "burst_read_size": (1, MAX_Payload), + "write_size": (1, MAX_Payload), + "write_qsize": (1, None), + "read_retry_time": (0, None), + "retry_time": (0.1, None), + } + minimum, maximum = bounded_settings.get(setting_name, (None, None)) + if ( + (minimum is not None and setting_value <= minimum and setting_name == "retry_time") + or (minimum is not None and setting_value < minimum) + or (maximum is not None and setting_value > maximum) + ): + logging.error("Invalid value for %s: %s", setting_name, setting_value) + return MAVFTPReturn("Set", FtpError.InvalidArguments) + + idle_detection_time = ( + setting_value + if setting_name == "idle_detection_time" + else self.ftp_settings.idle_detection_time + ) + read_retry_time = ( + setting_value + if setting_name == "read_retry_time" + else self.ftp_settings.read_retry_time + ) + if setting_name == "idle_detection_time" and setting_value <= 0: + logging.error("Invalid value for %s: %s", setting_name, setting_value) + return MAVFTPReturn("Set", FtpError.InvalidArguments) + if idle_detection_time <= read_retry_time: + logging.error( + "idle_detection_time must be greater than read_retry_time" + ) + 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) @@ -1105,6 +1155,13 @@ def cmd_put( if self.write_list is not None: logging.error("FTP: put already in progress") return MAVFTPReturn("CreateFile", FtpError.PutAlreadyInProgress) + self.write_block_size = int(self.ftp_settings.write_size) + if not 1 <= self.write_block_size <= MAX_Payload: + logging.error("FTP: write_size must be between 1 and %u", MAX_Payload) + return MAVFTPReturn("CreateFile", FtpError.InvalidArguments) + if self.ftp_settings.write_qsize < 1: + logging.error("FTP: write_qsize must be at least 1") + return MAVFTPReturn("CreateFile", FtpError.InvalidArguments) fname = args[0] self.fh = fh self.fh_owned = False @@ -1128,7 +1185,6 @@ def cmd_put( self.fh.seek(0) # setup write list - self.write_block_size = int(self.ftp_settings.write_size) self.write_file_size = file_size write_blockcount = file_size // self.write_block_size diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index ce337c88d..fc30ea0a1 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -167,6 +167,32 @@ def test_create_file_ack_uses_allocated_session(self): self.assertEqual(ftp.session, 37) self.assertEqual(master.mav.sent[-1][-1][2], 37) + def test_cmd_set_rejects_unsafe_transfer_settings(self): + """Transfer settings must remain valid for the FTP state machine.""" + ftp, _master = self.make_ftp([]) + + for setting, value in ( + ("write_size", "0"), + ("write_size", "240"), + ("write_qsize", "0"), + ("max_backlog", "0"), + ("retry_time", "0.1"), + ("idle_detection_time", "0.01"), + ("read_retry_time", "3.7"), + ): + with self.subTest(setting=setting, value=value): + result = ftp.cmd_set([setting, value]) + self.assertEqual(result.error_code, FtpError.InvalidArguments) + + def test_put_rejects_invalid_write_size(self): + """An API-set invalid write size must not reach division or packet packing.""" + ftp, _master = self.make_ftp([]) + ftp.ftp_settings.write_size = 0 + + result = ftp.cmd_put(["local", "remote"], fh=BytesIO(b"x")) + + self.assertEqual(result.error_code, FtpError.InvalidArguments) + def test_gap_read_nack_preserves_server_error(self): """A failed gap repair must not turn a ReadFile NACK into success.""" ftp, _master = self.make_ftp([]) From 5c82c6529da963cf821811f6cdff02b98f53642a Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Thu, 3 Sep 2026 13:18:45 +0200 Subject: [PATCH 18/32] fix(mavftp): reject malformed directory entries Validate file listing entries before splitting their name and size fields. Malformed server data now returns InvalidDataSize instead of raising ValueError from the reply-processing loop. Add regression coverage for a file entry without a size separator. --- mavftp.py | 8 ++++++-- tests/test_mavftp.py | 20 ++++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/mavftp.py b/mavftp.py index 816f5e851..c8d494fb7 100644 --- a/mavftp.py +++ b/mavftp.py @@ -6,7 +6,7 @@ Original from MAVProxy/MAVProxy/modules/mavproxy_ftp.py. -SPDX-FileCopyrightText: 2011-2024 Andrew Tridgell, 2024-2025 Amilcar Lucas +SPDX-FileCopyrightText: 2011-2024 Andrew Tridgell, 2024-2026 Amilcar Lucas SPDX-License-Identifier: GPL-3.0-or-later """ @@ -607,7 +607,11 @@ def __handle_list_reply(self, op: FTP_OP, _m) -> MAVFTPReturn: 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: + (name, size_str) = dir_entry[1:].rsplit("\t", 1) + except ValueError: + logging.error("Invalid file entry: %s", dir_entry) + return MAVFTPReturn("ListDirectory", FtpError.InvalidDataSize) try: size = int(size_str) except (ValueError, TypeError, OverflowError): diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index fc30ea0a1..5f75c3733 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -244,6 +244,26 @@ def test_unexpected_short_gap_ack_reports_failure(self): self.assertEqual(result.error_code, FtpError.Fail) self.assertEqual(terminated, [True]) + def test_malformed_directory_entry_reports_invalid_data(self): + """A malformed file listing entry must not crash reply processing.""" + ftp, _master = self.make_ftp([]) + + result = ftp._MAVFTP__handle_list_reply( + FTP_OP( + 1, + 0, + OP_Ack, + len(b"Fmissing-size"), + OP_ListDirectory, + 0, + 0, + bytearray(b"Fmissing-size"), + ), + None, + ) + + self.assertEqual(result.error_code, FtpError.InvalidDataSize) + def test_write_nack_preserves_server_error(self): """WriteFile NACKs retain their precise protocol error code.""" ftp, _master = self.make_ftp([]) From 2b34bcfbfac7a54da4c8876fb668a8d69f7b2562 Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Thu, 3 Sep 2026 18:54:50 +0200 Subject: [PATCH 19/32] test(mavftp): add end-to-end hardware replay utility Add an executable MAVFTP integration script for replaying operations against a connected flight controller. The test verifies heartbeat communication and exercises status, configuration, cancellation, listing, upload, CRC, download, rename, removal, directory creation/removal, and parameter retrieval. Temporary remote paths are unique per run and cleaned up on completion or failure. --- tools/test_mavftp_hardware.py | 141 ++++++++++++++++++++++++++++++++++ 1 file changed, 141 insertions(+) create mode 100755 tools/test_mavftp_hardware.py diff --git a/tools/test_mavftp_hardware.py b/tools/test_mavftp_hardware.py new file mode 100755 index 000000000..f4b06071f --- /dev/null +++ b/tools/test_mavftp_hardware.py @@ -0,0 +1,141 @@ +#!/usr/bin/env python3 + +""" +Exercise MAVFTP end-to-end against a connected flight controller. + +This destructive integration test uses a uniquely named temporary path on the +vehicle. It covers the public MAVFTP commands, verifies an upload by download +and CRC, and removes all temporary files and directories when complete (or on +best-effort failure cleanup). Run it only against hardware whose filesystem +may be modified. The default Pixhawk USB port is ``/dev/ttyACM0``; pass a +different device, baud rate, or component ID on the command line as needed. + +SPDX-FileCopyrightText: 2026 Amilcar Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +# FLAKE_CLEAN + +import argparse +import hashlib +import io +import sys +import tempfile +import time +import zlib + +from pymavlink import mavutil +from pymavlink.mavftp import FtpError, MAVFTP + + +def run(device: str, baud: int, component: int) -> None: # pylint: disable=too-many-branches,too-many-locals,too-many-statements + payload = (b"pymavlink-mavftp-hardware-test\x00" * 8) + bytes(range(64)) + remote = f"/APM/mavftp_hwtest_{int(time.time())}.bin" + renamed = remote + ".renamed" + master = mavutil.mavlink_connection( + device, baud=baud, source_system=250, autoreconnect=False + ) + ftp = None + try: + if master.wait_heartbeat(timeout=10) is None: + raise RuntimeError("no MAVLink heartbeat received") + print( + f"heartbeat system={master.target_system} component={component}", + flush=True, + ) + ftp = MAVFTP(master, target_system=master.target_system, target_component=component) + result = ftp.cmd_status() + print(f"status: {result.error_code.name}", flush=True) + result = ftp.cmd_set(["debug", "0"]) + print(f"set debug: {result.error_code.name}", flush=True) + result = ftp.cmd_cancel() + print(f"cancel: {result.error_code.name}", flush=True) + listing = ftp.cmd_list(["/APM"]) + print(f"list: {listing.error_code.name}", flush=True) + if listing.error_code != FtpError.Success: + raise RuntimeError(f"directory listing failed: {listing.error_code.name}") + + # Remove leftovers from an interrupted prior run; FileNotFound is fine. + for path in (remote, renamed): + ftp.cmd_rm([path]) + + result = ftp.cmd_put(["-", remote], fh=io.BytesIO(payload)) + result = ftp.process_ftp_reply("CreateFile", timeout=60) + print(f"upload: {result.error_code.name}", flush=True) + if result.error_code != FtpError.Success: + raise RuntimeError(f"upload failed: {result.error_code.name}") + + result = ftp.cmd_crc([remote]) + expected_crc = zlib.crc32(payload) & 0xFFFFFFFF + print(f"crc: {result.error_code.name} (expected 0x{expected_crc:08x})", flush=True) + if result.error_code != FtpError.Success: + raise RuntimeError(f"crc failed: {result.error_code.name}") + + result = ftp.cmd_get([remote, "-"]) + result = ftp.process_ftp_reply("Get", timeout=60) + print(f"get: {result.error_code.name}", flush=True) + if result.error_code != FtpError.Success: + raise RuntimeError(f"get failed: {result.error_code.name}") + + result = ftp.cmd_list(["/APM"]) + names = {entry.name for entry in (result.directory_listing or [])} + print(f"list uploaded file: {result.error_code.name} ({remote.rsplit('/', 1)[-1] in names})", flush=True) + if result.error_code != FtpError.Success or remote.rsplit("/", 1)[-1] not in names: + raise RuntimeError("uploaded file was not listed") + + data = ftp.read_sector(remote, 0, len(payload)) + digest = hashlib.sha256(data).hexdigest() if data is not None else "n/a" + print(f"download: {len(data) if data is not None else 'none'} bytes sha256={digest}", flush=True) + if data != payload: + raise RuntimeError("downloaded data does not match uploaded data") + + result = ftp.cmd_rename([remote, renamed]) + print(f"rename: {result.error_code.name}", flush=True) + if result.error_code != FtpError.Success: + raise RuntimeError(f"rename failed: {result.error_code.name}") + result = ftp.cmd_rm([renamed]) + print(f"delete: {result.error_code.name}", flush=True) + if result.error_code != FtpError.Success: + raise RuntimeError(f"delete failed: {result.error_code.name}") + + directory = f"/APM/mavftp_hwtest_{int(time.time())}" + result = ftp.cmd_mkdir([directory]) + print(f"mkdir: {result.error_code.name}", flush=True) + if result.error_code != FtpError.Success: + raise RuntimeError(f"mkdir failed: {result.error_code.name}") + result = ftp.cmd_rmdir([directory]) + print(f"rmdir: {result.error_code.name}", flush=True) + if result.error_code != FtpError.Success: + raise RuntimeError(f"rmdir failed: {result.error_code.name}") + + with tempfile.TemporaryDirectory(prefix="mavftp_params_") as temp_dir: + values = f"{temp_dir}/values.txt" + result = ftp.cmd_getparams([values]) + result = ftp.process_ftp_reply("GetParams", timeout=60) + print(f"getparams: {result.error_code.name}", flush=True) + if result.error_code != FtpError.Success: + raise RuntimeError(f"getparams failed: {result.error_code.name}") + print("MAVFTP HARDWARE TEST PASSED", flush=True) + finally: + if ftp is not None: + for path in (remote, renamed): + try: + ftp.cmd_rm([path]) + except Exception: # pylint: disable=broad-exception-caught + # best-effort cleanup only + pass + master.close() + + +if __name__ == "__main__": + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("device", nargs="?", default="/dev/ttyACM0") + parser.add_argument("--baud", type=int, default=115200) + parser.add_argument("--component", type=int, default=1) + args = parser.parse_args() + try: + run(args.device, args.baud, args.component) + except Exception as error: # pylint: disable=broad-exception-caught + print(f"MAVFTP HARDWARE TEST FAILED: {error}", file=sys.stderr, flush=True) + sys.exit(1) From 53efedb241942cb288ea35c7db27d9741ef30b8b Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Thu, 3 Sep 2026 19:01:24 +0200 Subject: [PATCH 20/32] fix(mavftp): stop completed range reads Complete synchronous range reads as soon as the requested bytes and any gaps are satisfied, even when the reply is a full-sized burst.\n\nAdd a regression covering a small request fulfilled by a full burst so the client terminates instead of requesting data through EOF. --- mavftp.py | 2 ++ tests/test_mavftp.py | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/mavftp.py b/mavftp.py index c8d494fb7..ccade4b3e 100644 --- a/mavftp.py +++ b/mavftp.py @@ -1024,6 +1024,8 @@ def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, self.__write_payload(op) else: self.__write_payload(op) + if self.__check_read_finished(): + return MAVFTPReturn("BurstReadFile", FtpError.Success) if op.burst_complete: if op.size > 0 and op.size < self.burst_size: # a burst complete with non-zero size and less than burst packet size diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index 5f75c3733..d1c245178 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -414,6 +414,50 @@ def test_read_sector_returns_only_requested_range_without_local_output(self): finally: os.chdir(previous_cwd) + def test_read_sector_stops_after_requested_range_in_full_burst(self): + """A full burst must not continue after a small range is satisfied.""" + ftp, master = self.make_ftp([]) + ftp.fh = BytesIO() + ftp.filename = "remote" + ftp.read_to_memory = True + ftp.requested_offset = 0 + ftp.requested_size = 2 + ftp.op_start = 1 + ftp.burst_size = 80 + ftp.session = 7 + ftp.pending_burst_request = FTP_OP( + seq=1, + session=7, + opcode=OP_BurstReadFile, + size=80, + req_opcode=0, + burst_complete=0, + offset=0, + payload=None, + ) + + result = ftp._MAVFTP__handle_burst_read( # pylint: disable=protected-access + FTP_OP( + seq=2, + session=7, + opcode=OP_Ack, + size=80, + req_opcode=OP_BurstReadFile, + burst_complete=1, + offset=0, + payload=bytearray(b"x" * 80), + ), + None, + ) + + self.assertEqual(result.error_code, FtpError.Success) + self.assertTrue(ftp.done) + self.assertEqual(ftp.get_result, b"xx") + self.assertEqual(master.mav.sent[-1][-1][3], OP_TerminateSession) + self.assertNotIn( + OP_BurstReadFile, [sent[-1][3] for sent in master.mav.sent[1:]] + ) + def test_put_returns_after_completion_before_late_write_reply(self): ftp, master = self.make_ftp( [ From b56f4bfb2f4569215869e6377ce4c8b9054c3825 Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Thu, 3 Sep 2026 19:02:39 +0200 Subject: [PATCH 21/32] fix(mavftp): bound synchronous read memory Store synchronous range-read payloads relative to the requested offset so a small read does not allocate a buffer proportional to the remote file offset.\n\nReturn the in-memory range directly from the compact buffer and add a regression covering a one-megabyte offset with a two-byte read. --- mavftp.py | 14 ++++++++++---- tests/test_mavftp.py | 23 +++++++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/mavftp.py b/mavftp.py index ccade4b3e..e1a51360b 100644 --- a/mavftp.py +++ b/mavftp.py @@ -924,9 +924,12 @@ def __check_read_finished(self) -> bool: 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 - ] + if self.read_to_memory: + self.get_result = result[: self.requested_size] + else: + 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( @@ -950,7 +953,10 @@ def __check_read_finished(self) -> bool: def __write_payload(self, op: FTP_OP) -> None: """Write payload from a read op.""" - self.fh.seek(op.offset) + write_offset = op.offset + if self.read_to_memory: + write_offset -= self.requested_offset + self.fh.seek(write_offset) self.fh.write(op.payload) self.read_total += len(op.payload) if self.callback_progress is not None and self.remote_file_size: diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index d1c245178..729329748 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -458,6 +458,29 @@ def test_read_sector_stops_after_requested_range_in_full_burst(self): OP_BurstReadFile, [sent[-1][3] for sent in master.mav.sent[1:]] ) + def test_read_sector_memory_uses_range_relative_offset(self): + """A range read buffer must scale with the range, not remote offset.""" + ftp, _master = self.make_ftp([]) + ftp.fh = BytesIO() + ftp.filename = "remote" + ftp.read_to_memory = True + ftp.requested_offset = 1024 * 1024 + + ftp._MAVFTP__write_payload( # pylint: disable=protected-access + FTP_OP( + seq=1, + session=0, + opcode=OP_Ack, + size=2, + req_opcode=OP_BurstReadFile, + burst_complete=0, + offset=ftp.requested_offset, + payload=bytearray(b"xy"), + ) + ) + + self.assertEqual(ftp.fh.getvalue(), b"xy") + def test_put_returns_after_completion_before_late_write_reply(self): ftp, master = self.make_ftp( [ From bee9c28e98fe5edf2d1a9feab8662af6d6114826 Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Thu, 3 Sep 2026 19:04:42 +0200 Subject: [PATCH 22/32] fix(mavftp): bound synchronous read buffers Store synchronous range-read payloads relative to the requested offset so small reads do not allocate memory proportional to the remote offset. Keep remote and buffer positions distinct while handling burst gaps, retries, and completion, and add a regression for a two-byte read at a one-megabyte offset. --- mavftp.py | 39 ++++++++++++++++++++------------ tests/test_mavftp.py | 54 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 14 deletions(-) diff --git a/mavftp.py b/mavftp.py index e1a51360b..95b021698 100644 --- a/mavftp.py +++ b/mavftp.py @@ -816,8 +816,6 @@ def __handle_open_ro_reply(self, op: FTP_OP, _m) -> MAVFTPReturn: try: if self.callback is not None or self.filename == "-" or self.read_to_memory: self.fh = SIO() - if self.read_to_memory: - self.fh.seek(self.requested_offset) else: self.__release_staging() (temp_fd, self.temp_filename) = tempfile.mkstemp(prefix="mavftp_") @@ -877,7 +875,7 @@ def __check_read_finished(self) -> bool: if len(self.read_gaps) == 0 and ( self.reached_eof or self.read_total >= self.requested_size ): - ofs = self.fh.tell() + ofs = self.__read_position() dt = time.time() - self.op_start rate = (ofs / dt) / 1024.0 publish_result = True @@ -962,6 +960,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 __read_position(self) -> int: + """Return the current remote offset represented by the read buffer.""" + position = self.fh.tell() + if self.read_to_memory: + position += self.requested_offset + return position + + def __seek_read_position(self, offset: int) -> None: + """Seek the read buffer to a remote offset.""" + if self.read_to_memory: + offset -= self.requested_offset + self.fh.seek(offset) + def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, PLR0915 pylint: disable=too-many-statements,too-many-branches,too-many-return-statements """Handle OP_BurstReadFile reply.""" if ( @@ -986,7 +997,7 @@ def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, if self.ftp_settings.debug > 0: logging.info("FTP: Setting burst size to %u", self.burst_size) if op.opcode == OP_Ack and self.fh is not None: - ofs = self.fh.tell() + ofs = self.__read_position() if op.offset < ofs: # writing an earlier portion, possibly remove a gap gap = (op.offset, len(op.payload)) @@ -1006,12 +1017,12 @@ def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, "FTP: dup read reply at %u of len %u ofs=%u", op.offset, op.size, - self.fh.tell(), + self.__read_position(), ) self.duplicates += 1 return MAVFTPReturn("BurstReadFile", FtpError.Fail) self.__write_payload(op) - self.fh.seek(ofs) + self.__seek_read_position(ofs) if self.__check_read_finished(): return MAVFTPReturn("BurstReadFile", FtpError.Success) elif op.offset > ofs: @@ -1043,7 +1054,7 @@ def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, ): logging.info( "FTP: EOF at %u with %u gaps t=%.2f", - self.fh.tell(), + self.__read_position(), len(self.read_gaps), time.time() - self.op_start, ) @@ -1060,7 +1071,7 @@ def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, 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() + "FTP: burst continue at %u %u", more.offset, self.__read_position() ) self.__send(more) # A valid burst reply may be only one part of the transfer. @@ -1069,11 +1080,11 @@ def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, 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(): + if not self.reached_eof and op.offset > self.__read_position(): # 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 + "FTP: burst lost EOF %u %u", self.__read_position(), op.offset ) return MAVFTPReturn("BurstReadFile", FtpError.Fail) if ( @@ -1083,7 +1094,7 @@ def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, ): logging.info( "FTP: EOF at %u with %u gaps t=%.2f", - self.fh.tell(), + self.__read_position(), len(self.read_gaps), time.time() - self.op_start, ) @@ -1127,9 +1138,9 @@ def __handle_reply_read(self, op: FTP_OP, _m) -> MAVFTPReturn: for seq, pending_read in self.pending_read_requests.items() if (pending_read.offset, pending_read.size) != gap } - ofs = self.fh.tell() + ofs = self.__read_position() self.__write_payload(op) - self.fh.seek(ofs) + self.__seek_read_position(ofs) if self.ftp_settings.debug > 0: logging.info( "FTP: removed gap %u, %u, %u", @@ -1715,7 +1726,7 @@ def __idle_task(self) -> bool: if self.ftp_settings.debug > 0: logging.info( "FTP: Retry read at %u rtt=%.2f dt=%.2f", - self.fh.tell(), + self.__read_position(), self.rtt, dt, ) diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index 729329748..5fe3073be 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -481,6 +481,60 @@ def test_read_sector_memory_uses_range_relative_offset(self): self.assertEqual(ftp.fh.getvalue(), b"xy") + def test_read_sector_relative_buffer_preserves_remote_gap_offsets(self): + """Compact buffers still track absolute remote offsets for gaps.""" + ftp, _master = self.make_ftp([]) + ftp.fh = BytesIO() + ftp.filename = "remote" + ftp.read_to_memory = True + ftp.requested_offset = 100 + ftp.requested_size = 4 + ftp.burst_size = 2 + ftp.op_start = 1 + ftp.session = 7 + ftp.pending_burst_request = FTP_OP( + seq=1, + session=7, + opcode=OP_BurstReadFile, + size=2, + req_opcode=0, + burst_complete=0, + offset=100, + payload=None, + ) + + first = ftp._MAVFTP__handle_burst_read( # pylint: disable=protected-access + FTP_OP( + seq=2, + session=7, + opcode=OP_Ack, + size=2, + req_opcode=OP_BurstReadFile, + burst_complete=0, + offset=102, + payload=bytearray(b"cd"), + ), + None, + ) + self.assertEqual(first.error_code, FtpError.Success) + self.assertEqual(ftp.read_gaps, [(100, 2)]) + + second = ftp._MAVFTP__handle_burst_read( # pylint: disable=protected-access + FTP_OP( + seq=3, + session=7, + opcode=OP_Ack, + size=2, + req_opcode=OP_BurstReadFile, + burst_complete=1, + offset=100, + payload=bytearray(b"ab"), + ), + None, + ) + self.assertEqual(second.error_code, FtpError.Success) + self.assertEqual(ftp.get_result, b"abcd") + def test_put_returns_after_completion_before_late_write_reply(self): ftp, master = self.make_ftp( [ From 48efecda368a940ddd8c6bc1a127057ed07268b8 Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Thu, 3 Sep 2026 20:15:53 +0200 Subject: [PATCH 23/32] test(mavftp): expand hardware replay coverage Exercise directory creation/removal and synchronous range reads using the crafted uploaded file, including a small full-burst request and a high-offset request. Reinitialize the MAVFTP connection before rename/delete to avoid late range-read termination replies interfering with subsequent mutations. Document the controller firmware limitation when that handshake remains incomplete. --- tools/test_mavftp_hardware.py | 52 ++++++++++++++++++++++++++++------- 1 file changed, 42 insertions(+), 10 deletions(-) diff --git a/tools/test_mavftp_hardware.py b/tools/test_mavftp_hardware.py index f4b06071f..d6c9eb1ef 100755 --- a/tools/test_mavftp_hardware.py +++ b/tools/test_mavftp_hardware.py @@ -10,6 +10,12 @@ may be modified. The default Pixhawk USB port is ``/dev/ttyACM0``; pass a different device, baud rate, or component ID on the command line as needed. +Known hardware limitation: some flight-controller firmware leaves the FTP +session handshake incomplete after synchronous range reads. In that case the +range checks pass, but the subsequent MAVFTP reinitialization can block before +rename/delete; reboot the controller and remove the uniquely named test file +if cleanup did not complete. + SPDX-FileCopyrightText: 2026 Amilcar Lucas SPDX-License-Identifier: GPL-3.0-or-later @@ -55,6 +61,15 @@ def run(device: str, baud: int, component: int) -> None: # pylint: disable=too- print(f"list: {listing.error_code.name}", flush=True) if listing.error_code != FtpError.Success: raise RuntimeError(f"directory listing failed: {listing.error_code.name}") + directory = f"/APM/mavftp_hwtest_{int(time.time())}" + result = ftp.cmd_mkdir([directory]) + print(f"mkdir: {result.error_code.name}", flush=True) + if result.error_code != FtpError.Success: + raise RuntimeError(f"mkdir failed: {result.error_code.name}") + result = ftp.cmd_rmdir([directory]) + print(f"rmdir: {result.error_code.name}", flush=True) + if result.error_code != FtpError.Success: + raise RuntimeError(f"rmdir failed: {result.error_code.name}") # Remove leftovers from an interrupted prior run; FileNotFound is fine. for path in (remote, renamed): @@ -90,6 +105,32 @@ def run(device: str, baud: int, component: int) -> None: # pylint: disable=too- if data != payload: raise RuntimeError("downloaded data does not match uploaded data") + small_range = ftp.read_sector(remote, 0, 2) + print(f"small full-burst range: {len(small_range) if small_range is not None else 'none'} bytes", flush=True) + if small_range != payload[:2]: + raise RuntimeError("small full-burst range did not match uploaded data") + high_offset = len(payload) - 2 + tail_range = ftp.read_sector(remote, high_offset, 2) + print( + f"high-offset range: {len(tail_range) if tail_range is not None else 'none'} " + f"bytes at {high_offset}", + flush=True, + ) + if tail_range != payload[high_offset:]: + raise RuntimeError("high-offset range did not match uploaded data") + + # Reopen after synchronous reads so late termination replies cannot + # interfere with the following rename and delete operations. Some FC + # firmware does not complete this handshake; see the module note. + master.close() + time.sleep(0.5) + master = mavutil.mavlink_connection( + device, baud=baud, source_system=250, autoreconnect=False + ) + if master.wait_heartbeat(timeout=10) is None: + raise RuntimeError("no MAVLink heartbeat after range reads") + ftp = MAVFTP(master, target_system=master.target_system, target_component=component) + result = ftp.cmd_rename([remote, renamed]) print(f"rename: {result.error_code.name}", flush=True) if result.error_code != FtpError.Success: @@ -99,16 +140,6 @@ def run(device: str, baud: int, component: int) -> None: # pylint: disable=too- if result.error_code != FtpError.Success: raise RuntimeError(f"delete failed: {result.error_code.name}") - directory = f"/APM/mavftp_hwtest_{int(time.time())}" - result = ftp.cmd_mkdir([directory]) - print(f"mkdir: {result.error_code.name}", flush=True) - if result.error_code != FtpError.Success: - raise RuntimeError(f"mkdir failed: {result.error_code.name}") - result = ftp.cmd_rmdir([directory]) - print(f"rmdir: {result.error_code.name}", flush=True) - if result.error_code != FtpError.Success: - raise RuntimeError(f"rmdir failed: {result.error_code.name}") - with tempfile.TemporaryDirectory(prefix="mavftp_params_") as temp_dir: values = f"{temp_dir}/values.txt" result = ftp.cmd_getparams([values]) @@ -116,6 +147,7 @@ def run(device: str, baud: int, component: int) -> None: # pylint: disable=too- print(f"getparams: {result.error_code.name}", flush=True) if result.error_code != FtpError.Success: raise RuntimeError(f"getparams failed: {result.error_code.name}") + print("MAVFTP HARDWARE TEST PASSED", flush=True) finally: if ftp is not None: From e589f711abfd2bfc0dbab986f162dfb9117bcd33 Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Fri, 4 Sep 2026 12:43:39 +0200 Subject: [PATCH 24/32] chore(mavftp): Add pylint disable statements to some hard-to-fix issues --- mavftp.py | 6 ++++-- tests/test_mavftp.py | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/mavftp.py b/mavftp.py index 95b021698..ce8115028 100644 --- a/mavftp.py +++ b/mavftp.py @@ -701,7 +701,9 @@ def read(self, path: str, size: int, offset: int = 0) -> Optional[bytes]: logging.error("closed read with %u gaps", len(self.read_gaps)) return None - def cmd_set(self, args: List[str]) -> MAVFTPReturn: + def cmd_set( # pylint: disable=too-many-return-statements,too-many-boolean-expressions + self, args: List[str] + ) -> MAVFTPReturn: """Set a MAVFTP configuration parameter.""" if len(args) != 2: logging.error("Usage: set PARAMETERNAME PARAMETERVALUE") @@ -866,7 +868,7 @@ def __handle_open_ro_reply(self, op: FTP_OP, _m) -> MAVFTPReturn: self.__terminate_session() return ret - def __check_read_finished(self) -> bool: + def __check_read_finished(self) -> bool: # pylint: disable=too-many-branches """Check if download has completed.""" if self.fh is None: return True diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index 5fe3073be..68a996df7 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -35,7 +35,7 @@ OP_WriteFile, ) -# pylint: disable=protected-access +# pylint: disable=protected-access,too-many-lines class FakeFTPMessage: # pylint: disable=too-few-public-methods From 5cc87a9f861e2cb96e8686ed42e4f118569d992b Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Thu, 3 Sep 2026 23:25:59 +0200 Subject: [PATCH 25/32] fix(mavftp): correct rename destination argument Keep the rename destination stored as arg2 so main() forwards both paths correctly, while displaying new_remote_path in help output. --- mavftp.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mavftp.py b/mavftp.py index ce8115028..514e04869 100644 --- a/mavftp.py +++ b/mavftp.py @@ -2447,9 +2447,9 @@ def create_argument_parser() -> ArgumentParser: help="Current path of the file/directory.", ) parser_rename.add_argument( - "new_remote_path", + "arg2", type=str, - metavar="arg2", + metavar="new_remote_path", help="New path for the file/directory.", ) From c4c66e53cfa3c5ed3c5ae06f2d5c46a3247e23d5 Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Fri, 4 Sep 2026 01:08:26 +0200 Subject: [PATCH 26/32] feat(mavftp): add ProfiCNC and Matek port detection --- mavftp.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/mavftp.py b/mavftp.py index 514e04869..c78997202 100644 --- a/mavftp.py +++ b/mavftp.py @@ -2478,6 +2478,7 @@ def auto_detect_serial() -> List[mavutil.SerialPort]: "*Ardu*", "*PX4*", "*Hex_*", + "*ProfiCNC*", "*Holybro_*", "*mRo*", "*FMU*", @@ -2485,6 +2486,7 @@ def auto_detect_serial() -> List[mavutil.SerialPort]: "*Serial*", "*CubePilot*", "*Qiotek*", + "*Matek*", ] serial_list: List[mavutil.SerialPort] = mavutil.auto_detect_serial( preferred_list=preferred_ports From c4a222c2d9494bb4ba0aae1472f927c2bf876db1 Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Fri, 4 Sep 2026 14:54:07 +0200 Subject: [PATCH 27/32] fix(mavftp): reset range-read state for downloads --- mavftp.py | 2 ++ tests/test_mavftp.py | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/mavftp.py b/mavftp.py index c78997202..ab9f201af 100644 --- a/mavftp.py +++ b/mavftp.py @@ -791,6 +791,8 @@ def cmd_get( logging.info("Getting %s to %s", fname, self.filename) self.op_start = time.time() self.read_to_memory = False + self.requested_offset = 0 + self.requested_size = 0 self.callback = callback self.callback_failure = None self.callback_progress = progress_callback diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index 68a996df7..6ab975883 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -155,6 +155,25 @@ def test_open_file_ack_uses_allocated_session(self): self.assertEqual(ftp.session, 42) self.assertEqual(master.mav.sent[-1][-1][2], 42) + def test_cmd_get_clears_range_read_state(self): + """A normal download must not inherit a prior range-read offset.""" + ftp, master = self.make_ftp([]) + ftp.requested_offset = 123 + ftp.requested_size = 2 + + try: + ftp.cmd_get(["remote", "download"]) + ftp._MAVFTP__mavlink_packet( # pylint: disable=protected-access + ftp_reply(2, OP_Ack, OP_OpenFileRO, payload=[4, 0, 0, 0], session=7) + ) + + self.assertEqual(ftp.requested_offset, 0) + self.assertEqual(ftp.requested_size, 4) + self.assertEqual(ftp.fh.tell(), 0) + self.assertEqual(struct.unpack_from(" Date: Fri, 4 Sep 2026 16:20:54 +0200 Subject: [PATCH 28/32] fix(mavftp): accept string parameter datatype IDs --- mavftp.py | 7 +++++-- tests/test_mavftp.py | 31 +++++++++++++++++-------------- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/mavftp.py b/mavftp.py index ab9f201af..80bc1b2b5 100644 --- a/mavftp.py +++ b/mavftp.py @@ -70,6 +70,8 @@ def __init__(self, *args, **kwargs): OP_WriteFile, ) +ParameterDataType = Union[str, int] + # pylint: disable=invalid-name class FtpError(IntEnum): @@ -2112,7 +2114,7 @@ def extract_params( @staticmethod def save_params( - pdict: Dict[str, Tuple[float, int]], + pdict: Dict[str, Tuple[float, ParameterDataType]], filename: str, sort_type: str, add_datatype_comments: bool, @@ -2139,7 +2141,8 @@ def save_params( f.write(f"{name:<16} {value:<8.6f}") if add_datatype_comments: - f.write(f" # {parameter_data_types[datatype]}") + datatype_id = int(datatype) + f.write(f" # {parameter_data_types[datatype_id]}") f.write("\n") logging.info("Outputted %u parameters to %s", len(pdict), filename) diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index 6ab975883..a35d1ae50 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -943,21 +943,24 @@ def test_rejects_name_longer_than_16_bytes(self): self.assertIsNone(MAVFTP.ftp_param_decode(header + first + second)) self.assertIn("parameter name is too long", logs.output[0]) - def test_save_params_writes_integer_datatype_comments(self): - """Decoded integer type IDs produce the documented datatype comment.""" + def test_save_params_accepts_integer_and_string_datatype_ids(self): + """Both public save_params datatype representations produce valid comments.""" with tempfile.TemporaryDirectory() as tempdir: - output = f"{tempdir}/params.txt" - - MAVFTP.save_params( - {"TEST_PARAM": (1.0, 4)}, - output, - "missionplanner", - add_datatype_comments=True, - add_timestamp_comment=False, - ) - - with open(output, encoding="utf-8") as param_file: - self.assertEqual(param_file.read(), "TEST_PARAM,1 # 32-bit float\n") + for datatype in (4, "4"): + with self.subTest(datatype=datatype): + output = f"{tempdir}/params-{datatype}.txt" + MAVFTP.save_params( + {"TEST_PARAM": (1.0, datatype)}, + output, + "missionplanner", + add_datatype_comments=True, + add_timestamp_comment=False, + ) + + with open(output, encoding="utf-8") as param_file: + self.assertEqual( + param_file.read(), "TEST_PARAM,1 # 32-bit float\n" + ) class TestMAVFTPPayloadDecoding(unittest.TestCase): From 4d9e8976e7c1af2c631796fb0feaa3d17aad9847 Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Fri, 4 Sep 2026 16:23:39 +0200 Subject: [PATCH 29/32] fix(mavftp): correlate burst replies by sequence --- mavftp.py | 18 ++++++-- tests/test_mavftp.py | 101 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 3 deletions(-) diff --git a/mavftp.py b/mavftp.py index 80bc1b2b5..26ba22e2e 100644 --- a/mavftp.py +++ b/mavftp.py @@ -358,10 +358,11 @@ def __init__( # noqa: PLR0915 pylint: disable=too-many-statements self.duplicates = 0 self.last_read = None self.last_burst_read: Union[None, float] = 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. + # The start offset and first expected reply sequence of the active burst. + # Burst packets are streamed with advancing sequence numbers, so both + # identify whether a reply belongs to the current burst. self.pending_burst_offset: Optional[int] = None + self.pending_burst_seq: Optional[int] = None self.pending_burst_request: Optional[FTP_OP] = None self.op_start: Union[None, float] = None self.dir_offset = 0 @@ -470,6 +471,7 @@ def __send(self, op: FTP_OP, retry: bool = False) -> None: expected_reply_seq = (op.seq + 1) % 65536 if op.opcode == OP_BurstReadFile: self.pending_burst_offset = op.offset + self.pending_burst_seq = expected_reply_seq self.pending_burst_request = op elif op.opcode == OP_ReadFile: self.pending_read_replies[expected_reply_seq] = (op.offset, op.size) @@ -535,6 +537,7 @@ def __terminate_session(self) -> None: self.last_read = None self.last_burst_read = None self.pending_burst_offset = None + self.pending_burst_seq = None self.pending_burst_request = None self.reached_eof = False self.backlog = 0 @@ -1066,6 +1069,7 @@ def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, ) self.reached_eof = True self.pending_burst_offset = None + self.pending_burst_seq = None self.pending_burst_request = None if self.__check_read_finished(): return MAVFTPReturn("BurstReadFile", FtpError.Success) @@ -1106,6 +1110,7 @@ def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, ) self.reached_eof = True self.pending_burst_offset = None + self.pending_burst_seq = None self.pending_burst_request = None if self.__check_read_finished(): return MAVFTPReturn("BurstReadFile", FtpError.Success) @@ -1517,6 +1522,8 @@ def __reply_matches_active_request(self, op: FTP_OP) -> bool: if op.req_opcode == OP_BurstReadFile: return ( self.pending_burst_offset is not None + and self.pending_burst_seq is not None + and self.__seq_is_at_or_after(op.seq, self.pending_burst_seq) and op.offset >= self.pending_burst_offset ) if op.req_opcode == OP_ReadFile: @@ -1533,6 +1540,11 @@ def __reply_matches_active_request(self, op: FTP_OP) -> bool: return False + @staticmethod + def __seq_is_at_or_after(seq: int, expected: int) -> bool: + """Return whether a uint16 sequence is equal to or newer than expected.""" + return ((seq - expected) & 0xFFFF) < 0x8000 + def __mavlink_packet(self, m) -> MAVFTPReturn: # noqa: PLR0911, PGH004, pylint: disable=too-many-branches, too-many-return-statements """Handle a mavlink packet.""" operation_name = "mavlink_packet" diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index a35d1ae50..15bb12420 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -638,6 +638,107 @@ def test_out_of_order_burst_reply_is_dispatched(self): self.assertEqual(result.error_code, FtpError.Success) self.assertEqual(ftp.duplicates, 0) + def test_stale_burst_reply_sequence_is_discarded_for_reused_session(self): + """A delayed burst packet must not match a new request in session 0.""" + ftp, _master = self.make_ftp([]) + ftp.fh = BytesIO() + ftp.filename = "-" + ftp.pending_burst_offset = 0 + ftp.pending_burst_seq = 11 + ftp.pending_burst_request = FTP_OP( + seq=10, + session=0, + opcode=OP_BurstReadFile, + size=80, + req_opcode=0, + burst_complete=0, + offset=0, + payload=None, + ) + + result = ftp._MAVFTP__mavlink_packet( # pylint: disable=protected-access + ftp_reply( + 10, + OP_Ack, + OP_BurstReadFile, + payload=b"stale", + offset=0, + session=0, + ) + ) + + self.assertEqual(result.error_code, FtpError.Fail) + self.assertEqual(ftp.fh.getvalue(), b"") + + def test_out_of_order_replies_in_one_burst_fill_the_gap(self): + """Burst reply sequencing is a floor, not a per-reply ratchet.""" + ftp, _master = self.make_ftp([]) + ftp.fh = BytesIO() + ftp.filename = "-" + ftp.read_to_memory = True + ftp.requested_size = 240 + ftp.burst_size = 80 + ftp.op_start = 1 + ftp.pending_burst_offset = 0 + ftp.pending_burst_seq = 2 + ftp.pending_burst_request = FTP_OP( + seq=1, + session=0, + opcode=OP_BurstReadFile, + size=80, + req_opcode=0, + burst_complete=0, + offset=0, + payload=None, + ) + + for seq, offset, payload in ( + (2, 0, b"a" * 80), + (4, 160, b"c" * 80), + (3, 80, b"b" * 80), + ): + result = ftp._MAVFTP__mavlink_packet( # pylint: disable=protected-access + ftp_reply(seq, OP_Ack, OP_BurstReadFile, payload=payload, offset=offset) + ) + self.assertEqual(result.error_code, FtpError.Success) + + self.assertEqual(ftp.read_gaps, []) + self.assertEqual(ftp.get_result, b"a" * 80 + b"b" * 80 + b"c" * 80) + + def test_retry_straggler_does_not_block_restarted_burst(self): + """A high-sequence straggler cannot advance the restarted burst floor.""" + ftp, _master = self.make_ftp([]) + ftp.fh = BytesIO() + ftp.filename = "-" + ftp.read_to_memory = True + ftp.requested_size = 80 + ftp.burst_size = 40 + ftp.op_start = 1 + ftp.pending_burst_offset = 0 + ftp.pending_burst_seq = 2 + ftp.pending_burst_request = FTP_OP( + seq=1, + session=0, + opcode=OP_BurstReadFile, + size=40, + req_opcode=0, + burst_complete=0, + offset=0, + payload=None, + ) + + straggler = ftp._MAVFTP__mavlink_packet( # pylint: disable=protected-access + ftp_reply(42, OP_Ack, OP_BurstReadFile, payload=b"b" * 40, offset=40) + ) + restarted = ftp._MAVFTP__mavlink_packet( # pylint: disable=protected-access + ftp_reply(2, OP_Ack, OP_BurstReadFile, payload=b"a" * 40, offset=0) + ) + + self.assertEqual(straggler.error_code, FtpError.Success) + self.assertEqual(restarted.error_code, FtpError.Success) + self.assertEqual(ftp.read_gaps, []) + self.assertEqual(ftp.get_result, b"a" * 40 + b"b" * 40) + def test_out_of_order_gap_reply_is_dispatched(self): ftp, _master = self.make_ftp([]) ftp.fh = BytesIO() From f8cf52ae2f63486285edc608c2dca9854b7b9b2d Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Sat, 5 Sep 2026 11:33:11 +0200 Subject: [PATCH 30/32] fix(mavftp): handle overflow in setting value conversion Catch OverflowError when converting API-provided setting values to float, returning InvalidArguments instead of leaking a traceback. Add regression coverage for arbitrarily large integers. --- mavftp.py | 2 +- tests/test_mavftp.py | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/mavftp.py b/mavftp.py index 26ba22e2e..945fd792f 100644 --- a/mavftp.py +++ b/mavftp.py @@ -723,7 +723,7 @@ def cmd_set( # pylint: disable=too-many-return-statements,too-many-boolean-expr try: setting_value = float(args[1]) - except (ValueError, TypeError): + except (ValueError, TypeError, OverflowError): logging.error("Invalid parameter value: %s", args[1]) return MAVFTPReturn("Set", FtpError.InvalidArguments) diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index 15bb12420..723d33b2e 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -203,6 +203,14 @@ def test_cmd_set_rejects_unsafe_transfer_settings(self): result = ftp.cmd_set([setting, value]) self.assertEqual(result.error_code, FtpError.InvalidArguments) + def test_cmd_set_rejects_an_integer_too_large_for_float(self): + """An overflow while normalising an API-provided integer is invalid input.""" + ftp, _master = self.make_ftp([]) + + result = ftp.cmd_set(["debug", 10**1000]) + + self.assertEqual(result.error_code, FtpError.InvalidArguments) + def test_put_rejects_invalid_write_size(self): """An API-set invalid write size must not reach division or packet packing.""" ftp, _master = self.make_ftp([]) From 38a468bda7b54a4113cf14e73af893f29aaf9f88 Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Sat, 5 Sep 2026 11:48:45 +0200 Subject: [PATCH 31/32] refactor(mavftp): add Python 3.8-compatible annotations and Ruff cleanup Add explicit annotations for MAVFTP state, operations, parameter decoding, and command interfaces while retaining compatibility with the project's supported Python versions. Apply the accompanying Ruff-driven cleanup to simplify formatting, resource cleanup, and type-safe optional values without changing the FTP protocol contract. Keep generated sources untouched so later behavioral fixes can be reviewed independently. --- mavftp.py | 582 ++++++++++++++++++------------------------------------ 1 file changed, 189 insertions(+), 393 deletions(-) diff --git a/mavftp.py b/mavftp.py index 945fd792f..96b89ba33 100644 --- a/mavftp.py +++ b/mavftp.py @@ -13,13 +13,14 @@ # FLAKE_CLEAN +import contextlib import logging import math import os -import tempfile import random import struct import sys +import tempfile import time from argparse import ArgumentParser from dataclasses import dataclass @@ -27,7 +28,7 @@ from enum import IntEnum from io import BufferedRandom, BufferedReader, BufferedWriter from io import BytesIO as SIO # noqa: N814 -from typing import Dict, List, Optional, Set, Tuple, Union +from typing import Any, Callable, Dict, List, Optional, Set, Tuple, Union, cast try: import argcomplete @@ -38,8 +39,10 @@ _ARGCOMPLETE_AVAILABLE = False # Dummy class to avoid errors when argcomplete is not available - class FilesCompleter: # pylint: disable=too-few-public-methods,missing-class-docstring - def __init__(self, *args, **kwargs): + class FilesCompleter: # type: ignore[no-redef] # pylint: disable=too-few-public-methods + """Fallback completer used when argcomplete is unavailable.""" + + def __init__(self, *args, **kwargs) -> None: pass @@ -47,7 +50,6 @@ def __init__(self, *args, **kwargs): # pylint: disable=too-many-lines # mypy: disable-error-code="union-attr,arg-type" - from pymavlink.mavftp_op import ( FTP_OP, OP_Ack, @@ -71,6 +73,9 @@ def __init__(self, *args, **kwargs): ) ParameterDataType = Union[str, int] +MavlinkObject = Any +Callback = Callable[..., Any] +FileHandle = Union[SIO, BufferedReader, BufferedWriter, BufferedRandom] # pylint: disable=invalid-name @@ -131,12 +136,8 @@ class ParamData: """A class to manage parameter values and defaults for ArduPilot configuration.""" def __init__(self) -> None: - self.params: List[ - Tuple[bytes, float, type] - ] = [] # params as (name, value, ptype) - self.defaults: Union[None, List[Tuple[bytes, float, type]]] = ( - None # defaults as (name, value, ptype) - ) + self.params: List[Tuple[bytes, float, type]] = [] # params as (name, value, ptype) + self.defaults: Optional[List[Tuple[bytes, float, type]]] = None # defaults as (name, value, ptype) def add_param(self, name: bytes, value: float, ptype: type) -> None: self.params.append((name, value, ptype)) @@ -150,7 +151,7 @@ def add_default(self, name: bytes, value: float, ptype: type) -> None: class MAVFTPSetting: # pylint: disable=too-few-public-methods """A single MAVFTP setting with a name, type, value and default value.""" - def __init__(self, name: str, s_type: type, default: Union[int, float]) -> None: + def __init__(self, name: str, s_type: type, default: float) -> None: self.name: str = name self.type = s_type self.default: Union[int, float] = default @@ -160,12 +161,12 @@ def __init__(self, name: str, s_type: type, default: Union[int, float]) -> None: class MAVFTPSettings: """A collection of MAVFTP settings.""" - def __init__(self, s_vars) -> None: + def __init__(self, s_vars: List[Union[MAVFTPSetting, Tuple[str, type, float]]]) -> None: self._vars: Dict[str, MAVFTPSetting] = {} for v in s_vars: self.append(v) - def append(self, v) -> None: + def append(self, v: Union[MAVFTPSetting, Tuple[str, type, float]]) -> None: if isinstance(v, MAVFTPSetting): setting = v else: @@ -173,14 +174,22 @@ def append(self, v) -> 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 get_setting(self, name: str) -> MAVFTPSetting: + """Return a named setting.""" + return self._vars[name] + def __getattr__(self, name: str) -> Union[int, float]: """Get attribute.""" try: - return self._vars[name].type(self._vars[name].value) + return cast(Union[int, float], self._vars[name].type(self._vars[name].value)) except Exception as exc: raise AttributeError from exc - def __setattr__(self, name: str, value: Union[int, float]) -> None: + def __setattr__(self, name: str, value: float) -> None: """Set attribute.""" if name[0] == "_": self.__dict__[name] = value @@ -218,15 +227,11 @@ def display_message(self) -> None: # pylint: disable=too-many-branches, too-man elif self.error_code == FtpError.Fail: logging.error("%s failed, generic error", self.operation_name) elif self.error_code == FtpError.FailErrno: - logging.error( - "%s failed, system error %u", self.operation_name, self.system_error - ) + logging.error("%s failed, system error %u", self.operation_name, self.system_error) elif self.error_code == FtpError.InvalidDataSize: logging.error("%s failed, invalid data size", self.operation_name) elif self.error_code == FtpError.InvalidSession: - logging.error( - "%s failed, session is not currently open", self.operation_name - ) + logging.error("%s failed, session is not currently open", self.operation_name) elif self.error_code == FtpError.NoSessionsAvailable: logging.error("%s failed, no sessions available", self.operation_name) elif self.error_code == FtpError.EndOfFile: @@ -234,26 +239,18 @@ def display_message(self) -> None: # pylint: disable=too-many-branches, too-man elif self.error_code == FtpError.UnknownCommand: logging.error("%s failed, unknown command", self.operation_name) elif self.error_code == FtpError.FileExists: - logging.warning( - "%s failed, file/directory already exists", self.operation_name - ) + logging.warning("%s failed, file/directory already exists", self.operation_name) elif self.error_code == FtpError.FileProtected: - logging.warning( - "%s failed, file/directory is protected", self.operation_name - ) + logging.warning("%s failed, file/directory is protected", self.operation_name) elif self.error_code == FtpError.FileNotFound: logging.warning("%s failed, file/directory not found", self.operation_name) elif self.error_code == FtpError.NoErrorCodeInPayload: - logging.error( - "%s failed, payload contains no error code", self.operation_name - ) + logging.error("%s failed, payload contains no error code", self.operation_name) elif self.error_code == FtpError.NoErrorCodeInNack: logging.error("%s failed, no error code", self.operation_name) elif self.error_code == FtpError.NoFilesystemErrorInPayload: - logging.error( - "%s failed, file-system error missing in payload", self.operation_name - ) + logging.error("%s failed, file-system error missing in payload", self.operation_name) elif self.error_code == FtpError.InvalidErrorCode: logging.error( "%s failed, invalid error code %u", @@ -267,9 +264,7 @@ def display_message(self) -> None: # pylint: disable=too-many-branches, too-man self.invalid_payload_size, ) elif self.error_code == FtpError.InvalidOpcode: - logging.error( - "%s failed, invalid opcode %u", self.operation_name, self.invalid_opcode - ) + logging.error("%s failed, invalid opcode %u", self.operation_name, self.invalid_opcode) elif self.error_code == FtpError.InvalidArguments: logging.error("%s failed, invalid arguments", self.operation_name) elif self.error_code == FtpError.PutAlreadyInProgress: @@ -311,7 +306,7 @@ class MAVFTP: # pylint: disable=too-many-instance-attributes def __init__( # noqa: PLR0915 pylint: disable=too-many-statements self, - master, + master: MavlinkObject, target_system: int, target_component: int, settings: Optional[MAVFTPSettings] = None, @@ -335,14 +330,14 @@ def __init__( # noqa: PLR0915 pylint: disable=too-many-statements self.seq = 0 self.session = 0 self.network = 0 - self.last_op: Union[None, FTP_OP] = None - self.fh: Union[None, SIO, BufferedReader, BufferedWriter, BufferedRandom] = None - self.filename: Union[None, str] = None - self.callback = None + self.last_op: Optional[FTP_OP] = None + self.fh: Optional[Union[SIO, BufferedReader, BufferedWriter, BufferedRandom]] = None + self.filename: Optional[str] = None + self.callback: Optional[Callback] = None self.callback_failure: Optional[MAVFTPReturn] = None - self.callback_progress = None - self.put_callback = None - self.put_callback_progress = None + self.callback_progress: Optional[Callback] = None + self.put_callback: Optional[Callback] = None + self.put_callback_progress: Optional[Callback] = None self.total_size = 0 self.read_gaps: List[Tuple[int, int]] = [] self.read_gap_times: Dict[Tuple[int, int], float] = {} @@ -357,14 +352,14 @@ def __init__( # noqa: PLR0915 pylint: disable=too-many-statements self.remote_file_size: int = 0 self.duplicates = 0 self.last_read = None - self.last_burst_read: Union[None, float] = None + self.last_burst_read: Optional[float] = None # The start offset and first expected reply sequence of the active burst. # Burst packets are streamed with advancing sequence numbers, so both # identify whether a reply belongs to the current burst. self.pending_burst_offset: Optional[int] = None self.pending_burst_seq: Optional[int] = None self.pending_burst_request: Optional[FTP_OP] = None - self.op_start: Union[None, float] = None + self.op_start: Optional[float] = None self.dir_offset = 0 self.last_op_time = time.time() self.last_send_time = time.time() @@ -379,11 +374,11 @@ def __init__( # noqa: PLR0915 pylint: disable=too-many-statements # 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 = None - self.pending_reset_seq = None + self.pending_terminate_seq: Optional[int] = None + self.pending_reset_seq: Optional[int] = None self.backlog = 0 self.burst_size: int = int(self.ftp_settings.burst_read_size) - self.write_list: Union[None, Set[int]] = None + self.write_list: Optional[Set[int]] = None self.write_block_size: int = 0 self.write_acks = 0 self.write_total = 0 @@ -395,7 +390,7 @@ def __init__( # noqa: PLR0915 pylint: disable=too-many-statements # 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: Union[None, float] = None + self.write_last_send: Optional[float] = None self.open_retries = 0 self.list_result: List[DirectoryEntry] = [] self.list_temp_result: List[DirectoryEntry] = [] @@ -408,7 +403,7 @@ def __init__( # noqa: PLR0915 pylint: disable=too-many-statements # 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 = None + self.temp_filename: Optional[str] = None # only close file handles this instance opened itself; cmd_put # stores a caller-owned handle in self.fh self.fh_owned = False @@ -416,7 +411,7 @@ def __init__( # noqa: PLR0915 pylint: disable=too-many-statements self.master = master self.target_system = target_system self.target_component = target_component - self.get_result: Union[None, bytes] = None + self.get_result: Optional[bytes] = None self.done = False # Reset the flight controller FTP state-machine @@ -465,9 +460,7 @@ def __send(self, op: FTP_OP, retry: bool = False) -> None: 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.master.mav.file_transfer_protocol_send(self.network, self.target_system, self.target_component, payload) expected_reply_seq = (op.seq + 1) % 65536 if op.opcode == OP_BurstReadFile: self.pending_burst_offset = op.offset @@ -489,27 +482,24 @@ def __send(self, op: FTP_OP, retry: bool = False) -> None: 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.""" + """ + 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: - try: + with contextlib.suppress(OSError): self.fh.close() - except OSError: - pass self.fh_owned = False if self.temp_filename is not None: - try: + with contextlib.suppress(OSError): os.unlink(self.temp_filename) - except OSError: - pass 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) - ) + self.__send(FTP_OP(self.seq, self.session, OP_TerminateSession, 0, 0, 0, 0, None)) self.__release_staging() self.fh = None self.filename = None @@ -594,7 +584,7 @@ def cmd_list(self, args: List[str]) -> MAVFTPReturn: self.__send(op) return self.process_ftp_reply("ListDirectory") - def __handle_list_reply(self, op: FTP_OP, _m) -> 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")) @@ -608,9 +598,7 @@ def __handle_list_reply(self, op: FTP_OP, _m) -> MAVFTPReturn: logging.debug(error) continue if dir_entry[0] == "D": - self.list_temp_result.append( - DirectoryEntry(name=dir_entry[1:], is_dir=True, size_b=0) - ) + self.list_temp_result.append(DirectoryEntry(name=dir_entry[1:], is_dir=True, size_b=0)) elif dir_entry[0] == "F": try: (name, size_str) = dir_entry[1:].rsplit("\t", 1) @@ -622,26 +610,17 @@ def __handle_list_reply(self, op: FTP_OP, _m) -> MAVFTPReturn: except (ValueError, TypeError, OverflowError): logging.error("Invalid file size: %s", size_str) size = 0 - self.list_temp_result.append( - DirectoryEntry(name=name, is_dir=False, size_b=size) - ) + self.list_temp_result.append(DirectoryEntry(name=name, is_dir=False, size_b=size)) else: 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] == FtpError.EndOfFile - ): + 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 - ) + return MAVFTPReturn("ListDirectory", FtpError.Success, directory_listing=self.list_result) else: return self.__decode_ftp_ack_and_nack(op) return MAVFTPReturn("ListDirectory", FtpError.Success) @@ -677,9 +656,7 @@ def read(self, path: str, size: int, offset: int = 0) -> Optional[bytes]: 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 - ) + 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: @@ -717,7 +694,7 @@ def cmd_set( # pylint: disable=too-many-return-statements,too-many-boolean-expr setting_name = args[0] # Check if parameter exists in settings - if setting_name not in self.ftp_settings._vars: # pylint: disable=protected-access + if not self.ftp_settings.has_setting(setting_name): logging.error("Invalid parameter name: %s", setting_name) return MAVFTPReturn("Set", FtpError.InvalidArguments) @@ -727,7 +704,7 @@ def cmd_set( # pylint: disable=too-many-return-statements,too-many-boolean-expr logging.error("Invalid parameter value: %s", args[1]) return MAVFTPReturn("Set", FtpError.InvalidArguments) - setting = self.ftp_settings._vars[setting_name] # pylint: disable=protected-access + setting = self.ftp_settings.get_setting(setting_name) if not math.isfinite(setting_value): logging.error("Invalid parameter value: %s", args[1]) return MAVFTPReturn("Set", FtpError.InvalidArguments) @@ -749,7 +726,7 @@ def cmd_set( # pylint: disable=too-many-return-statements,too-many-boolean-expr "retry_time": (0.1, None), } minimum, maximum = bounded_settings.get(setting_name, (None, None)) - if ( + if ( # pylint: disable=too-many-boolean-expressions (minimum is not None and setting_value <= minimum and setting_name == "retry_time") or (minimum is not None and setting_value < minimum) or (maximum is not None and setting_value > maximum) @@ -757,23 +734,13 @@ def cmd_set( # pylint: disable=too-many-return-statements,too-many-boolean-expr logging.error("Invalid value for %s: %s", setting_name, setting_value) return MAVFTPReturn("Set", FtpError.InvalidArguments) - idle_detection_time = ( - setting_value - if setting_name == "idle_detection_time" - else self.ftp_settings.idle_detection_time - ) - read_retry_time = ( - setting_value - if setting_name == "read_retry_time" - else self.ftp_settings.read_retry_time - ) + idle_detection_time = setting_value if setting_name == "idle_detection_time" else self.ftp_settings.idle_detection_time + read_retry_time = setting_value if setting_name == "read_retry_time" else self.ftp_settings.read_retry_time if setting_name == "idle_detection_time" and setting_value <= 0: logging.error("Invalid value for %s: %s", setting_name, setting_value) return MAVFTPReturn("Set", FtpError.InvalidArguments) if idle_detection_time <= read_retry_time: - logging.error( - "idle_detection_time must be greater than read_retry_time" - ) + logging.error("idle_detection_time must be greater than read_retry_time") return MAVFTPReturn("Set", FtpError.InvalidArguments) setattr(self.ftp_settings, setting_name, setting_value) @@ -781,7 +748,10 @@ def cmd_set( # pylint: disable=too-many-return-statements,too-many-boolean-expr return MAVFTPReturn("Set", FtpError.Success) def cmd_get( - self, args: List[str], callback=None, progress_callback=None + self, + args: List[str], + callback: Optional[Callback] = None, + progress_callback: Optional[Callback] = None, ) -> MAVFTPReturn: """Get file.""" if len(args) == 0 or len(args) > 2: @@ -810,13 +780,11 @@ def cmd_get( 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 - ) + op = FTP_OP(self.seq, self.session, OP_OpenFileRO, len(enc_fname), 0, 0, 0, enc_fname) self.__send(op) return MAVFTPReturn("OpenFileRO", FtpError.Success) - def __handle_open_ro_reply(self, op: FTP_OP, _m) -> 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: @@ -837,18 +805,11 @@ def __handle_open_ro_reply(self, op: FTP_OP, _m) -> MAVFTPReturn: self.fh.truncate(0) self.fh.seek(self.requested_offset) except Exception as ex: # pylint: disable=broad-except - logging.error( - "FTP: Failed to open local file %s: %s", self.filename, ex - ) + logging.error("FTP: Failed to open local file %s: %s", self.filename, ex) self.__terminate_session() 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) - ) + 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) if not self.read_to_memory: @@ -881,9 +842,7 @@ def __check_read_finished(self) -> bool: # pylint: disable=too-many-branches 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 - ): + if len(self.read_gaps) == 0 and (self.reached_eof or self.read_total >= self.requested_size): ofs = self.__read_position() dt = time.time() - self.op_start rate = (ofs / dt) / 1024.0 @@ -896,10 +855,7 @@ def __check_read_finished(self) -> bool: # pylint: disable=too-many-branches self.fh.seek(0) try: callback_result = self.callback(self.fh) - if ( - isinstance(callback_result, MAVFTPReturn) - and callback_result.error_code != FtpError.Success - ): + if isinstance(callback_result, MAVFTPReturn) and callback_result.error_code != FtpError.Success: self.callback_failure = callback_result except Exception as exc: # pylint: disable=broad-exception-caught logging.error("FTP: download callback failed: %s", exc) @@ -928,20 +884,16 @@ def __check_read_finished(self) -> bool: # pylint: disable=too-many-branches ) self.done = True - assert self.fh is not None # noqa: S101 + assert self.fh is not None self.fh.seek(0) result = self.fh.read() if self.read_to_memory: self.get_result = result[: self.requested_size] else: - self.get_result = result[ - self.requested_offset : self.requested_offset + self.requested_size - ] - assert self.get_result is not None # noqa: S101 + self.get_result = result[self.requested_offset : self.requested_offset + self.requested_size] + assert self.get_result is not None if len(self.get_result) < self.requested_size: - logging.warning( - "expected %u, got %u", self.requested_size, len(self.get_result) - ) + 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: @@ -982,12 +934,11 @@ def __seek_read_position(self, offset: int) -> None: offset -= self.requested_offset self.fh.seek(offset) - def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, PLR0915 pylint: disable=too-many-statements,too-many-branches,too-many-return-statements + def __handle_burst_read( # pylint: disable=too-many-statements,too-many-branches,too-many-return-statements # noqa: C901, PLR0911, PLR0912, PLR0915 + self, op: FTP_OP, _m: MavlinkObject + ) -> MAVFTPReturn: """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.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", FtpError.Fail) @@ -1056,11 +1007,7 @@ def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, 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.op_start - and self.ftp_settings.debug > 0 - ): + 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.__read_position(), @@ -1080,9 +1027,7 @@ def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, 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.__read_position() - ) + logging.info("FTP: burst continue at %u %u", more.offset, self.__read_position()) self.__send(more) # A valid burst reply may be only one part of the transfer. # It is successful even when it does not complete the read. @@ -1093,15 +1038,9 @@ def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, if not self.reached_eof and op.offset > self.__read_position(): # we lost the last part of the burst if self.ftp_settings.debug > 0: - logging.error( - "FTP: burst lost EOF %u %u", self.__read_position(), op.offset - ) + logging.error("FTP: burst lost EOF %u %u", self.__read_position(), op.offset) return MAVFTPReturn("BurstReadFile", FtpError.Fail) - if ( - not self.reached_eof - and self.op_start - and self.ftp_settings.debug > 0 - ): + 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.__read_position(), @@ -1123,7 +1062,7 @@ def __handle_burst_read(self, op: FTP_OP, _m) -> MAVFTPReturn: # noqa: PLR0911, logging.warning("FTP: burst error: %s", op) return MAVFTPReturn("BurstReadFile", FtpError.Fail) - def __handle_reply_read(self, op: FTP_OP, _m) -> 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) @@ -1140,9 +1079,7 @@ def __handle_reply_read(self, op: FTP_OP, _m) -> MAVFTPReturn: 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 + seq: pending_gap for seq, pending_gap in self.pending_read_replies.items() if pending_gap != gap } self.pending_read_requests = { seq: pending_read @@ -1170,9 +1107,7 @@ def __handle_reply_read(self, op: FTP_OP, _m) -> MAVFTPReturn: if self.ftp_settings.debug > 0: 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) - ) + 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 @@ -1180,7 +1115,11 @@ def __handle_reply_read(self, op: FTP_OP, _m) -> MAVFTPReturn: return MAVFTPReturn("ReadFile", FtpError.Success) def cmd_put( - self, args: List[str], fh=None, callback=None, progress_callback=None + self, + args: List[str], + fh: FileHandle = None, + callback: Optional[Callback] = None, + progress_callback: Optional[Callback] = None, ) -> MAVFTPReturn: """Put file.""" if len(args) == 0 or len(args) > 2: @@ -1238,9 +1177,7 @@ def cmd_put( self.read_retries = 0 self.op_start = time.time() enc_fname = bytearray(self.filename, "ascii") - op = FTP_OP( - self.seq, self.session, OP_CreateFile, len(enc_fname), 0, 0, 0, enc_fname - ) + op = FTP_OP(self.seq, self.session, OP_CreateFile, len(enc_fname), 0, 0, 0, enc_fname) self.__send(op) return MAVFTPReturn("CreateFile", FtpError.Success) @@ -1263,7 +1200,7 @@ def __put_finished(self, flen: int) -> None: rate, ) - def __handle_create_file_reply(self, op: FTP_OP, _m) -> 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() @@ -1291,15 +1228,11 @@ def __send_more_writes(self, completed_reply: Optional[FTP_OP] = None) -> None: return now = time.time() - if self.write_last_send is not None and now - self.write_last_send > max( - min(10 * self.rtt, 1), 0.2 - ): + if self.write_last_send is not None and now - self.write_last_send > max(min(10 * self.rtt, 1), 0.2): # we seem to have lost a block of replies self.write_pending = max(0, self.write_pending - 1) - n = min( - self.ftp_settings.write_qsize - self.write_pending, len(self.write_list) - ) + 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 idx = self.write_idx @@ -1307,11 +1240,7 @@ def __send_more_writes(self, completed_reply: Optional[FTP_OP] = None) -> None: idx = (idx + 1) % self.write_total ofs = idx * self.write_block_size write = next( - ( - pending_write - for pending_write in self.pending_write_requests.values() - if pending_write.offset == ofs - ), + (pending_write for pending_write in self.pending_write_requests.values() if pending_write.offset == ofs), None, ) if write is None: @@ -1334,15 +1263,13 @@ def __send_more_writes(self, completed_reply: Optional[FTP_OP] = None) -> None: self.write_pending += 1 self.write_last_send = now - def __handle_write_reply(self, op: FTP_OP, _m) -> 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 + seq: offset for seq, offset in self.pending_write_replies.items() if offset != expected_offset } self.pending_write_requests = { seq: pending_write @@ -1381,9 +1308,7 @@ def cmd_rm(self, args: List[str]) -> MAVFTPReturn: fname = args[0] logging.info("Removing file %s", fname) enc_fname = bytearray(fname, "ascii") - op = FTP_OP( - self.seq, self.session, OP_RemoveFile, len(enc_fname), 0, 0, 0, enc_fname - ) + op = FTP_OP(self.seq, self.session, OP_RemoveFile, len(enc_fname), 0, 0, 0, enc_fname) self.__send(op) return self.process_ftp_reply("RemoveFile") @@ -1408,7 +1333,7 @@ def cmd_rmdir(self, args: List[str]) -> MAVFTPReturn: self.__send(op) return self.process_ftp_reply("RemoveDirectory") - def __handle_remove_reply(self, op: FTP_OP, _m) -> MAVFTPReturn: + def __handle_remove_reply(self, op: FTP_OP, _m: MavlinkObject) -> MAVFTPReturn: """Handle remove reply.""" return self.__decode_ftp_ack_and_nack(op) @@ -1427,7 +1352,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) -> MAVFTPReturn: + def __handle_rename_reply(self, op: FTP_OP, _m: MavlinkObject) -> MAVFTPReturn: """Handle rename reply.""" return self.__decode_ftp_ack_and_nack(op) @@ -1439,13 +1364,11 @@ def cmd_mkdir(self, args: List[str]) -> MAVFTPReturn: name = args[0] logging.info("Creating directory %s", name) enc_name = bytearray(name, "ascii") - op = FTP_OP( - self.seq, self.session, OP_CreateDirectory, len(enc_name), 0, 0, 0, enc_name - ) + op = FTP_OP(self.seq, self.session, OP_CreateDirectory, len(enc_name), 0, 0, 0, enc_name) self.__send(op) return self.process_ftp_reply("CreateDirectory") - def __handle_mkdir_reply(self, op: FTP_OP, _m) -> MAVFTPReturn: + def __handle_mkdir_reply(self, op: FTP_OP, _m: MavlinkObject) -> MAVFTPReturn: """Handle mkdir reply.""" return self.__decode_ftp_ack_and_nack(op) @@ -1472,15 +1395,13 @@ def cmd_crc(self, args: List[str]) -> MAVFTPReturn: self.__send(op) return self.process_ftp_reply("CalcFileCRC32") - def __handle_crc_reply(self, op: FTP_OP, _m) -> 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: @@ -1506,16 +1427,12 @@ def cmd_status(self) -> MAVFTPReturn: ) return MAVFTPReturn("Status", FtpError.Success) - def __op_parse(self, m) -> FTP_OP: + 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(" bool: """Return whether a reply can safely be dispatched to the active operation.""" @@ -1531,21 +1448,18 @@ def __reply_matches_active_request(self, op: FTP_OP) -> bool: if op.req_opcode == OP_WriteFile: return op.seq in self.pending_write_replies - if ( - self.last_op is not None - and op.req_opcode == self.last_op.opcode - and op.seq == (self.last_op.seq + 1) % 65536 - ): - return True - - return False + 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 + ) @staticmethod def __seq_is_at_or_after(seq: int, expected: int) -> bool: """Return whether a uint16 sequence is equal to or newer than expected.""" return ((seq - expected) & 0xFFFF) < 0x8000 - def __mavlink_packet(self, m) -> MAVFTPReturn: # noqa: PLR0911, PGH004, pylint: disable=too-many-branches, too-many-return-statements + def __mavlink_packet( # pylint: disable=too-many-branches, too-many-return-statements # noqa: C901, PLR0911 + self, m: MavlinkObject + ) -> MAVFTPReturn: """Handle a mavlink packet.""" operation_name = "mavlink_packet" mtype = m.get_type() @@ -1553,10 +1467,7 @@ def __mavlink_packet(self, m) -> MAVFTPReturn: # noqa: PLR0911, PGH004, pylint: logging.error("FTP: Unexpected MAVLink message type %s", mtype) return MAVFTPReturn(operation_name, FtpError.Fail) - if ( - m.target_system != self.master.source_system - or m.target_component != self.master.source_component - ): + 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, @@ -1569,10 +1480,7 @@ def __mavlink_packet(self, m) -> MAVFTPReturn: # noqa: PLR0911, PGH004, pylint: dt = now - self.last_op_time if self.ftp_settings.debug > 1: logging.info("FTP: < %s dt=%.2f", op, dt) - allocated_session_reply = ( - op.opcode == OP_Ack - and op.req_opcode in {OP_OpenFileRO, OP_CreateFile} - ) + 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( @@ -1582,10 +1490,7 @@ def __mavlink_packet(self, m) -> MAVFTPReturn: # noqa: PLR0911, PGH004, pylint: ) 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.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, FtpError.Fail) @@ -1595,11 +1500,7 @@ def __mavlink_packet(self, m) -> MAVFTPReturn: # noqa: PLR0911, PGH004, pylint: logging.warning("FTP: stale reply. Will discard message: %s", op) return MAVFTPReturn(operation_name, FtpError.Fail) - if ( - self.last_op is not None - and op.req_opcode == self.last_op.opcode - and op.seq == (self.last_op.seq + 1) % 65536 - ): + 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: @@ -1638,7 +1539,7 @@ def __mavlink_packet(self, m) -> MAVFTPReturn: # noqa: PLR0911, PGH004, pylint: logging.info("FTP Unknown %s", str(op)) return MAVFTPReturn(operation_name, FtpError.InvalidOpcode) - def __send_gap_read(self, g) -> None: + 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: @@ -1658,9 +1559,7 @@ def __send_gap_read(self, g) -> None: None, ) if read is None: - read = FTP_OP( - self.seq, self.session, OP_ReadFile, length, 0, 0, offset, None - ) + read = FTP_OP(self.seq, self.session, OP_ReadFile, length, 0, 0, offset, None) self.__send(read) else: self.__send(read, retry=True) @@ -1702,9 +1601,9 @@ def __check_read_send(self) -> None: def __idle_task(self) -> bool: """Check for file gaps and lost requests.""" now = time.time() - assert ( # noqa: S101 - self.ftp_settings.idle_detection_time > self.ftp_settings.read_retry_time - ), "settings.idle_detection_time must be > settings.read_retry_time" + assert self.ftp_settings.idle_detection_time > self.ftp_settings.read_retry_time, ( # noqa: S101 + "settings.idle_detection_time must be > settings.read_retry_time" + ) # see if we lost an open reply if ( @@ -1723,11 +1622,7 @@ def __idle_task(self) -> bool: logging.info("FTP: retry open") 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 - ): + 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) if self.fh is None: @@ -1760,37 +1655,28 @@ def __idle_task(self) -> bool: return self.__last_send_time_was_more_than_idle_detection_time_ago(now) - 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 __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) -> 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 - ): + 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( # pylint: disable=too-many-branches, too-many-locals + def process_ftp_reply( # pylint: disable=too-many-branches, too-many-locals # noqa: PLR0915 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, 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 + assert timeout == 0 or timeout > float(self.ftp_settings.idle_detection_time), ( # noqa: S101 + "timeout must be > settings.idle_detection_time" + ) + assert recv_timeout < self.ftp_settings.retry_time, "recv_timeout must be < settings.retry_time" # noqa: S101 # A read operation reports its completion positively: EOF seen # with no gaps outstanding (__check_read_finished). Return as @@ -1806,9 +1692,7 @@ def process_ftp_reply( # pylint: disable=too-many-branches, too-many-locals 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 - ) + m = self.master.recv_match(type=["FILE_TRANSFER_PROTOCOL"], timeout=recv_timeout) if m is not None: if operation_name == "TerminateSession": # consume only the terminate reply itself: stale @@ -1839,10 +1723,7 @@ def process_ftp_reply( # pylint: disable=too-many-branches, too-many-locals 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) - ) + 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 @@ -1851,19 +1732,11 @@ def process_ftp_reply( # pylint: disable=too-many-branches, too-many-locals completed_upload = ( operation_name.lower() == "put" and self.completed_reply is not None - and self.completed_reply[0] - in {OP_CreateFile, OP_WriteFile} + and self.completed_reply[0] in {OP_CreateFile, OP_WriteFile} ) - if ( - reply_matches_last_op - or completed_upload - or reply_matches_active_request - ): + if reply_matches_last_op or completed_upload or reply_matches_active_request: ret = packet_ret - if ( - self.callback_failure is not None - and operation_name != "TerminateSession" - ): + if self.callback_failure is not None and operation_name != "TerminateSession": callback_failure = self.callback_failure self.callback_failure = None return callback_failure @@ -1873,10 +1746,7 @@ def process_ftp_reply( # pylint: disable=too-many-branches, too-many-locals 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 - ) + 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 @@ -1900,9 +1770,7 @@ def process_ftp_reply( # pylint: disable=too-many-branches, too-many-locals 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 - ) + logging.error("FTP: timed out after %f seconds", time.time() - start_time) ret = MAVFTPReturn(operation_name, FtpError.RemoteReplyTimeout) break if ( @@ -1913,9 +1781,7 @@ def process_ftp_reply( # pylint: disable=too-many-branches, too-many-locals self.__terminate_session() return ret - def __decode_ftp_ack_and_nack( - self, op: FTP_OP, operation_name: str = "" - ) -> MAVFTPReturn: + def __decode_ftp_ack_and_nack(self, op: FTP_OP, operation_name: str = "") -> MAVFTPReturn: """Decode FTP Acknowledge reply.""" system_error = 0 invalid_error_code = 0 @@ -1937,9 +1803,7 @@ def __decode_ftp_ack_and_nack( OP_CalcFileCRC32: "CalcFileCRC32", OP_BurstReadFile: "BurstReadFile", } - op_ret_name = operation_name or operation_name_dict.get( - op.req_opcode, "Unknown" - ) + op_ret_name = operation_name or operation_name_dict.get(op.req_opcode, "Unknown") len_payload = len(op.payload) if op.payload is not None else 0 if op.opcode == OP_Ack: error_code = FtpError.Success @@ -1968,11 +1832,7 @@ def __decode_ftp_ack_and_nack( ]: invalid_error_code = error_code error_code = FtpError.InvalidErrorCode - elif ( - op.payload is not None - and op.payload[0] == FtpError.FailErrno - and len_payload == 2 - ): + elif op.payload is not None and op.payload[0] == FtpError.FailErrno and len_payload == 2: system_error = op.payload[1] error_code = FtpError.FailErrno else: @@ -1989,16 +1849,16 @@ def __decode_ftp_ack_and_nack( ) @staticmethod - def ftp_param_decode(data: bytes) -> Union[None, ParamData]: # pylint: disable=too-many-locals,too-many-statements,too-many-branches,too-many-return-statements + def ftp_param_decode( # pylint: disable=too-many-locals,too-many-statements,too-many-branches,too-many-return-statements # noqa: PLR0911, PLR0915 + data: bytes, + ) -> Optional[ParamData]: """Decode parameter data, returning ParamData.""" pdata = ParamData() magic = 0x671B magic_defaults = 0x671C if len(data) < 6: - logging.error( - "paramftp: Not enough data do decode, only %u bytes", len(data) - ) + logging.error("paramftp: Not enough data do decode, only %u bytes", len(data)) return None magic2, num_params, total_params = struct.unpack(" Union[None, ParamData]: # pylint: disable= 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) - ) + logging.error("paramftp: parameter name is too long (%u bytes)", len(name)) return None try: name.decode("utf-8") @@ -2103,9 +1961,7 @@ def missionplanner_sort(item: str) -> Tuple[str, ...]: return tuple(item.split("_")) @staticmethod - def extract_params( - pdata: List[Tuple[bytes, float, type]], sort_type: str - ) -> Dict[str, Tuple[float, type]]: + 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: @@ -2113,11 +1969,7 @@ def extract_params( pdict[name.decode("utf-8")] = (value, ptype) if sort_type == "missionplanner": - pdict = dict( - sorted( - pdict.items(), key=lambda x: MAVFTP.missionplanner_sort(x[0]) - ) - ) # sort alphabetically + pdict = dict(sorted(pdict.items(), key=lambda x: MAVFTP.missionplanner_sort(x[0]))) # sort alphabetically elif sort_type == "mavproxy": pdict = dict(sorted(pdict.items())) # sort in ASCIIbetical order elif sort_type == "none": @@ -2143,9 +1995,7 @@ def save_params( 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" - ) + f.write(f"# Parameters saved at {datetime.now(tz=None).strftime('%Y-%m-%d %H:%M:%S')}\n") for name, (value, datatype) in pdict.items(): if sort_type == "missionplanner": f.write(f"{name},{format(value, '.6f').rstrip('0').rstrip('.')}") @@ -2161,14 +2011,14 @@ def save_params( def cmd_getparams( # pylint: disable=too-many-arguments self, args: List[str], - progress_callback=None, + progress_callback: Optional[Callback] = 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) -> MAVFTPReturn: + def decode_and_save_params(fh: FileHandle) -> MAVFTPReturn: if fh is None: logging.error("FTP: no parameter file handler") return MAVFTPReturn("GetParams", FtpError.Fail) @@ -2217,11 +2067,7 @@ def decode_and_save_params(fh) -> MAVFTPReturn: return MAVFTPReturn("GetParams", FtpError.Success) return self.cmd_get( - [ - "@PARAM/param.pck?withdefaults=1" - if len(args) > 1 - else "@PARAM/param.pck" - ], + ["@PARAM/param.pck?withdefaults=1" if len(args) > 1 else "@PARAM/param.pck"], callback=decode_and_save_params, progress_callback=progress_callback, ) @@ -2262,9 +2108,7 @@ def create_argument_parser() -> ArgumentParser: 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" - ) + parser.add_argument("--loglevel", default="INFO", help="log level. Default is %(default)s") # MAVFTP settings parser.add_argument( @@ -2286,18 +2130,14 @@ def create_argument_parser() -> ArgumentParser: 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("--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( - "--write_size", type=int, default=80, help="Write 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, @@ -2326,9 +2166,7 @@ def create_argument_parser() -> ArgumentParser: 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 = subparsers.add_parser("set", help="Set a MAVFTP internal configuration parameter.") parser_set.add_argument( "arg1", type=str, @@ -2343,9 +2181,7 @@ def create_argument_parser() -> ArgumentParser: ) # Get command - parser_get = subparsers.add_parser( - "get", help="Get a file from the remote flight controller." - ) + parser_get = subparsers.add_parser("get", help="Get a file from the remote flight controller.") parser_get.add_argument( "arg1", type=str, @@ -2361,9 +2197,7 @@ def create_argument_parser() -> ArgumentParser: ).completer = FilesCompleter() # type: ignore[no-untyped-call] # Getparams command - parser_getparams = subparsers.add_parser( - "getparams", help="Get and decode parameters from the remote flight controller." - ) + 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, @@ -2400,9 +2234,7 @@ def create_argument_parser() -> ArgumentParser: ) # Put command - parser_put = subparsers.add_parser( - "put", help="Put a file to the remote flight controller." - ) + 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, @@ -2418,9 +2250,7 @@ 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 = subparsers.add_parser("list", help="List files in a directory on the remote flight controller.") parser_list.add_argument( "arg1", nargs="?", @@ -2430,33 +2260,19 @@ def create_argument_parser() -> ArgumentParser: ) # Mkdir command - parser_mkdir = subparsers.add_parser( - "mkdir", help="Create a directory on the remote flight controller." - ) - parser_mkdir.add_argument( - "arg1", type=str, metavar="remote_path", help="Path to the directory to create." - ) + parser_mkdir = subparsers.add_parser("mkdir", help="Create a directory on the remote flight controller.") + parser_mkdir.add_argument("arg1", type=str, metavar="remote_path", help="Path to the directory to create.") # Rmdir command - parser_rmdir = subparsers.add_parser( - "rmdir", help="Remove a directory on the remote flight controller." - ) - parser_rmdir.add_argument( - "arg1", type=str, metavar="remote_path", help="Path to the directory to remove." - ) + parser_rmdir = subparsers.add_parser("rmdir", help="Remove a directory on the remote flight controller.") + parser_rmdir.add_argument("arg1", type=str, metavar="remote_path", help="Path to the directory to remove.") # Rm command - parser_rm = subparsers.add_parser( - "rm", help="Remove a file on the remote flight controller." - ) - parser_rm.add_argument( - "arg1", type=str, metavar="remote_path", help="Path to the file to remove." - ) + parser_rm = subparsers.add_parser("rm", help="Remove a file on the remote flight controller.") + parser_rm.add_argument("arg1", type=str, metavar="remote_path", help="Path to the file to remove.") # Rename command - parser_rename = subparsers.add_parser( - "rename", help="Rename a file or directory on the remote flight controller." - ) + parser_rename = subparsers.add_parser("rename", help="Rename a file or directory on the remote flight controller.") parser_rename.add_argument( "arg1", type=str, @@ -2471,9 +2287,7 @@ def create_argument_parser() -> ArgumentParser: ) # CRC command - parser_crc = subparsers.add_parser( - "crc", help="Calculate the CRC of a file on the remote flight controller." - ) + 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, @@ -2505,9 +2319,7 @@ def auto_detect_serial() -> List[mavutil.SerialPort]: "*Qiotek*", "*Matek*", ] - serial_list: List[mavutil.SerialPort] = mavutil.auto_detect_serial( - preferred_list=preferred_ports - ) + serial_list: List[mavutil.SerialPort] = mavutil.auto_detect_serial(preferred_list=preferred_ports) serial_list.sort(key=lambda x: x.device) # remove OTG2 ports for dual CDC @@ -2521,7 +2333,7 @@ def auto_detect_serial() -> List[mavutil.SerialPort]: return serial_list -def auto_connect(device) -> mavutil.SerialPort: +def auto_connect(device: Optional[str]) -> mavutil.SerialPort: comport = None if device: comport = mavutil.SerialPort(device=device, description=device) @@ -2536,44 +2348,34 @@ def auto_connect(device) -> mavutil.SerialPort: # Get the directory part of the soft link softlink_dir = os.path.dirname(dev) # Resolve the soft link and join it with the directory part - resolved_path = os.path.abspath( - os.path.join(softlink_dir, os.readlink(dev)) - ) + resolved_path = os.path.abspath(os.path.join(softlink_dir, os.readlink(dev))) autodetect_serial[0].device = resolved_path logging.debug("Resolved soft link %s to %s", dev, resolved_path) except OSError: pass # Not a soft link, proceed with the original device path comport = autodetect_serial[0] else: - logging.error( - "No serial ports found. Please connect a flight controller and try again." - ) + logging.error("No serial ports found. Please connect a flight controller and try again.") sys.exit(1) return comport -def wait_heartbeat(m) -> 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_component) def main() -> None: """For testing/example purposes only.""" args = create_argument_parser().parse_args() - logging.basicConfig( - level=logging.getLevelName(args.loglevel), format="%(levelname)s - %(message)s" - ) + logging.basicConfig(level=logging.getLevelName(args.loglevel), format="%(levelname)s - %(message)s") # create a mavlink serial instance comport = auto_connect(args.device) - master = mavutil.mavlink_connection( - comport.device, baud=args.baudrate, source_system=args.source_system - ) + master = mavutil.mavlink_connection(comport.device, baud=args.baudrate, source_system=args.source_system) # wait for the heartbeat msg to find the system ID wait_heartbeat(master) @@ -2613,21 +2415,15 @@ def main() -> None: exit_code = 1 if isinstance(ret, str): - logging.error( - "Command returned: %s, but it should return a MAVFTPReturn instead", ret - ) + logging.error("Command returned: %s, but it should return a MAVFTPReturn instead", ret) elif isinstance(ret, MAVFTPReturn): if ret.error_code or args.command in {"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" - ) + 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" - ) + logging.error("Command returned: something strange, but it should return a MAVFTPReturn instead") master.close() sys.exit(exit_code) From 78e9df58a62210007caa10cee2386ff1798c495f Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Fri, 4 Sep 2026 13:35:08 +0200 Subject: [PATCH 32/32] fix(mavftp): validate transfer settings consistently Move MAVFTP transfer-setting constraints into MAVFTPSettings so constructor inputs, direct library assignments, CLI options, and cmd_set() share one validation path. Reject non-finite and out-of-range values, enforce the idle-detection/read-retry relationship, and restore the prior value when an assignment fails validation. CLI validation now happens before opening a MAVLink transport. Add regression coverage for command, constructor, and direct-library entry points to prevent invalid settings from reaching the transfer state machine. --- mavftp.py | 172 ++++++++++++++++++++++++++----------------- tests/test_mavftp.py | 50 ++++++++++++- 2 files changed, 154 insertions(+), 68 deletions(-) diff --git a/mavftp.py b/mavftp.py index 96b89ba33..f37056bfb 100644 --- a/mavftp.py +++ b/mavftp.py @@ -76,6 +76,8 @@ def __init__(self, *args, **kwargs) -> None: MavlinkObject = Any Callback = Callable[..., Any] FileHandle = Union[SIO, BufferedReader, BufferedWriter, BufferedRandom] +SettingValue = Union[int, float] +ParamEntry = Tuple[bytes, SettingValue, int] # pylint: disable=invalid-name @@ -136,13 +138,13 @@ class ParamData: """A class to manage parameter values and defaults for ArduPilot configuration.""" def __init__(self) -> None: - self.params: List[Tuple[bytes, float, type]] = [] # params as (name, value, ptype) - self.defaults: Optional[List[Tuple[bytes, float, type]]] = None # defaults as (name, value, ptype) + self.params: List[ParamEntry] = [] # params as (name, value, ptype) + self.defaults: Optional[List[ParamEntry]] = None # defaults as (name, value, ptype) - def add_param(self, name: bytes, value: float, ptype: type) -> None: + def add_param(self, name: bytes, value: SettingValue, ptype: int) -> None: self.params.append((name, value, ptype)) - def add_default(self, name: bytes, value: float, ptype: type) -> None: + def add_default(self, name: bytes, value: SettingValue, ptype: int) -> None: if self.defaults is None: self.defaults = [] self.defaults.append((name, value, ptype)) @@ -151,36 +153,89 @@ def add_default(self, name: bytes, value: float, ptype: type) -> None: class MAVFTPSetting: # pylint: disable=too-few-public-methods """A single MAVFTP setting with a name, type, value and default value.""" - def __init__(self, name: str, s_type: type, default: float) -> None: + def __init__(self, name: str, s_type: type, default: SettingValue) -> None: self.name: str = name self.type = s_type - self.default: Union[int, float] = default - self.value: Union[int, float] = default + self.default: SettingValue = default + self.value: SettingValue = default class MAVFTPSettings: """A collection of MAVFTP settings.""" - def __init__(self, s_vars: List[Union[MAVFTPSetting, Tuple[str, type, float]]]) -> None: + _BOUNDS = { + "debug": (0, 2, False), + "pkt_loss_tx": (0, 100, False), + "pkt_loss_rx": (0, 100, False), + "max_backlog": (1, None, False), + "burst_read_size": (1, MAX_Payload, False), + "write_size": (1, MAX_Payload, False), + "write_qsize": (1, None, False), + "read_retry_time": (0, None, False), + "retry_time": (0.1, None, True), + "idle_detection_time": (0, None, True), + } + + def __init__(self, s_vars: List[Union[MAVFTPSetting, Tuple[str, type, SettingValue]]]) -> None: self._vars: Dict[str, MAVFTPSetting] = {} for v in s_vars: self.append(v) + self.validate() - def append(self, v: Union[MAVFTPSetting, Tuple[str, type, float]]) -> None: + def append(self, v: Union[MAVFTPSetting, Tuple[str, type, SettingValue]]) -> None: + """Add or replace a setting after validating the resulting collection.""" if isinstance(v, MAVFTPSetting): - setting = v + setting = self.__copy_setting(v) else: (name, s_type, default) = v setting = MAVFTPSetting(name, s_type, default) - self._vars[setting.name] = setting + candidate_vars = self._vars.copy() + candidate_vars[setting.name] = setting + self.__validate_vars(candidate_vars) + self._vars = candidate_vars + + def validate(self) -> None: + """Validate settings required by the MAVFTP state machine.""" + self.__validate_vars(self._vars) + + @classmethod + def __validate_vars(cls, settings: Dict[str, MAVFTPSetting]) -> None: + """Validate a candidate settings collection without mutating the active one.""" + for name, setting in settings.items(): + value = setting.value + # Python integers are mathematically finite, but math.isfinite() + # converts them to float and can overflow for very large values. + if not isinstance(value, int) and not math.isfinite(value): + raise ValueError(f"{name} must be finite") + bounds = cls._BOUNDS.get(name) + if bounds is None: + continue + minimum, maximum, exclusive_minimum = bounds + if ( + (minimum is not None and (value <= minimum if exclusive_minimum else value < minimum)) + or (maximum is not None and value > maximum) + ): + raise ValueError(f"invalid value for {name}: {value}") + idle_detection = settings.get("idle_detection_time") + read_retry = settings.get("read_retry_time") + if idle_detection is not None and read_retry is not None: + if idle_detection.type(idle_detection.value) <= read_retry.type(read_retry.value): + raise ValueError("idle_detection_time must be greater than read_retry_time") + + @staticmethod + def __copy_setting(setting: MAVFTPSetting) -> MAVFTPSetting: + """Return a detached setting snapshot.""" + setting_copy = MAVFTPSetting(setting.name, setting.type, setting.default) + setting_copy.value = setting.value + return setting_copy def has_setting(self, name: str) -> bool: """Return whether a setting exists.""" return name in self._vars def get_setting(self, name: str) -> MAVFTPSetting: - """Return a named setting.""" - return self._vars[name] + """Return a detached snapshot of a named setting.""" + return self.__copy_setting(self._vars[name]) def __getattr__(self, name: str) -> Union[int, float]: """Get attribute.""" @@ -189,13 +244,18 @@ def __getattr__(self, name: str) -> Union[int, float]: except Exception as exc: raise AttributeError from exc - def __setattr__(self, name: str, value: float) -> None: + def __setattr__(self, name: str, value: Any) -> None: """Set attribute.""" if name[0] == "_": self.__dict__[name] = value return if name in self._vars: - self._vars[name].value = self._vars[name].type(value) + setting = self.__copy_setting(self._vars[name]) + setting.value = setting.type(value) + candidate_vars = self._vars.copy() + candidate_vars[name] = setting + self.__validate_vars(candidate_vars) + self._vars = candidate_vars return raise AttributeError @@ -683,7 +743,7 @@ def read(self, path: str, size: int, offset: int = 0) -> Optional[bytes]: logging.error("closed read with %u gaps", len(self.read_gaps)) return None - def cmd_set( # pylint: disable=too-many-return-statements,too-many-boolean-expressions + def cmd_set( # pylint: disable=too-many-return-statements self, args: List[str] ) -> MAVFTPReturn: """Set a MAVFTP configuration parameter.""" @@ -705,45 +765,21 @@ def cmd_set( # pylint: disable=too-many-return-statements,too-many-boolean-expr return MAVFTPReturn("Set", FtpError.InvalidArguments) setting = self.ftp_settings.get_setting(setting_name) - if not math.isfinite(setting_value): - logging.error("Invalid parameter value: %s", args[1]) - return MAVFTPReturn("Set", FtpError.InvalidArguments) if setting.type is int: + if not math.isfinite(setting_value): + logging.error("Invalid parameter value: %s", args[1]) + return MAVFTPReturn("Set", FtpError.InvalidArguments) if not setting_value.is_integer(): logging.error("Invalid integer parameter value: %s", args[1]) return MAVFTPReturn("Set", FtpError.InvalidArguments) setting_value = int(setting_value) - bounded_settings = { - "debug": (0, 2), - "pkt_loss_tx": (0, 100), - "pkt_loss_rx": (0, 100), - "max_backlog": (1, None), - "burst_read_size": (1, MAX_Payload), - "write_size": (1, MAX_Payload), - "write_qsize": (1, None), - "read_retry_time": (0, None), - "retry_time": (0.1, None), - } - minimum, maximum = bounded_settings.get(setting_name, (None, None)) - if ( # pylint: disable=too-many-boolean-expressions - (minimum is not None and setting_value <= minimum and setting_name == "retry_time") - or (minimum is not None and setting_value < minimum) - or (maximum is not None and setting_value > maximum) - ): - logging.error("Invalid value for %s: %s", setting_name, setting_value) - return MAVFTPReturn("Set", FtpError.InvalidArguments) - - idle_detection_time = setting_value if setting_name == "idle_detection_time" else self.ftp_settings.idle_detection_time - read_retry_time = setting_value if setting_name == "read_retry_time" else self.ftp_settings.read_retry_time - if setting_name == "idle_detection_time" and setting_value <= 0: - logging.error("Invalid value for %s: %s", setting_name, setting_value) - return MAVFTPReturn("Set", FtpError.InvalidArguments) - if idle_detection_time <= read_retry_time: - logging.error("idle_detection_time must be greater than read_retry_time") + try: + setattr(self.ftp_settings, setting_name, setting_value) + except (TypeError, ValueError) as exc: + logging.error("Invalid value for %s: %s", setting_name, exc) 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) @@ -1117,7 +1153,7 @@ def __handle_reply_read(self, op: FTP_OP, _m: MavlinkObject) -> MAVFTPReturn: def cmd_put( self, args: List[str], - fh: FileHandle = None, + fh: Optional[FileHandle] = None, callback: Optional[Callback] = None, progress_callback: Optional[Callback] = None, ) -> MAVFTPReturn: @@ -1961,7 +1997,7 @@ def missionplanner_sort(item: str) -> Tuple[str, ...]: return tuple(item.split("_")) @staticmethod - def extract_params(pdata: List[Tuple[bytes, float, type]], sort_type: str) -> Dict[str, Tuple[float, type]]: + def extract_params(pdata: Optional[List[ParamEntry]], sort_type: str) -> Dict[str, Tuple[SettingValue, int]]: """Extract parameter values to an optionally sorted dictionary of name->(value, type).""" pdict = {} if pdata: @@ -2018,7 +2054,7 @@ def cmd_getparams( # pylint: disable=too-many-arguments ) -> MAVFTPReturn: """Decode the parameter file and save the values and defaults to disk.""" - def decode_and_save_params(fh: FileHandle) -> MAVFTPReturn: + def decode_and_save_params(fh: Optional[FileHandle]) -> MAVFTPReturn: if fh is None: logging.error("FTP: no parameter file handler") return MAVFTPReturn("GetParams", FtpError.Fail) @@ -2369,10 +2405,29 @@ def wait_heartbeat(m: MavlinkObject) -> None: def main() -> None: """For testing/example purposes only.""" - args = create_argument_parser().parse_args() + parser = create_argument_parser() + args = parser.parse_args() logging.basicConfig(level=logging.getLevelName(args.loglevel), format="%(levelname)s - %(message)s") + try: + ftp_settings = MAVFTPSettings( + [ + ("debug", int, args.debug), + ("pkt_loss_tx", int, args.pkt_loss_tx), + ("pkt_loss_rx", int, args.pkt_loss_rx), + ("max_backlog", int, args.max_backlog), + ("burst_read_size", int, args.burst_read_size), + ("write_size", int, args.write_size), + ("write_qsize", int, args.write_qsize), + ("idle_detection_time", float, args.idle_detection_time), + ("read_retry_time", float, args.read_retry_time), + ("retry_time", float, args.retry_time), + ] + ) + except ValueError as exc: + parser.error(str(exc)) + # create a mavlink serial instance comport = auto_connect(args.device) master = mavutil.mavlink_connection(comport.device, baud=args.baudrate, source_system=args.source_system) @@ -2380,21 +2435,6 @@ def main() -> None: # wait for the heartbeat msg to find the system ID wait_heartbeat(master) - ftp_settings = MAVFTPSettings( - [ - ("debug", int, args.debug), - ("pkt_loss_tx", int, args.pkt_loss_tx), - ("pkt_loss_rx", int, args.pkt_loss_rx), - ("max_backlog", int, args.max_backlog), - ("burst_read_size", int, args.burst_read_size), - ("write_size", int, args.write_size), - ("write_qsize", int, args.write_qsize), - ("idle_detection_time", float, args.idle_detection_time), - ("read_retry_time", float, args.read_retry_time), - ("retry_time", float, args.retry_time), - ] - ) - mav_ftp = MAVFTP( master, target_system=master.target_system, diff --git a/tests/test_mavftp.py b/tests/test_mavftp.py index 723d33b2e..08ad504e9 100644 --- a/tests/test_mavftp.py +++ b/tests/test_mavftp.py @@ -20,6 +20,8 @@ from pymavlink.mavftp import ( FTP_OP, MAVFTP, + MAVFTPSetting, + MAVFTPSettings, FtpError, MAVFTPReturn, OP_Ack, @@ -109,8 +111,8 @@ def make_ftp(replies): [ftp_reply(1, OP_Ack, OP_ResetSessions)] + replies ) ftp = MAVFTP(master, target_system=1, target_component=1) - ftp.ftp_settings.idle_detection_time = 0.02 ftp.ftp_settings.read_retry_time = 0.01 + ftp.ftp_settings.idle_detection_time = 0.02 ftp.ftp_settings.retry_time = 0.2 return ftp, master @@ -213,13 +215,57 @@ def test_cmd_set_rejects_an_integer_too_large_for_float(self): def test_put_rejects_invalid_write_size(self): """An API-set invalid write size must not reach division or packet packing.""" + ftp, _master = self.make_ftp([]) - ftp.ftp_settings.write_size = 0 + ftp.ftp_settings._vars["write_size"].value = 0 # pylint: disable=protected-access result = ftp.cmd_put(["local", "remote"], fh=BytesIO(b"x")) self.assertEqual(result.error_code, FtpError.InvalidArguments) + def test_settings_reject_invalid_values_from_library_callers(self): + """Library callers cannot install unsafe settings after construction.""" + ftp, _master = self.make_ftp([]) + + with self.assertRaises(ValueError): + ftp.ftp_settings.write_size = 0 + with self.assertRaises(ValueError): + ftp.ftp_settings.retry_time = 0.1 + + def test_settings_append_and_accessor_cannot_bypass_validation(self): + """Settings collection mutation must remain validated and encapsulated.""" + settings = MAVFTPSettings([("retry_time", float, 0.5)]) + + with self.assertRaises(ValueError): + settings.append(("retry_time", float, 0.1)) + self.assertEqual(settings.retry_time, 0.5) + + setting = settings.get_setting("retry_time") + setting.value = 0.1 + self.assertEqual(settings.retry_time, 0.5) + + supplied_setting = MAVFTPSetting("retry_time", float, 0.5) + supplied_settings = MAVFTPSettings([supplied_setting]) + supplied_setting.value = 0.1 + self.assertEqual(supplied_settings.retry_time, 0.5) + + def test_settings_constructor_rejects_invalid_values(self): + """CLI-provided settings are validated during construction.""" + with self.assertRaises(ValueError): + MAVFTPSettings( + [ + ("idle_detection_time", float, 1.0), + ("read_retry_time", float, 1.0), + ] + ) + + def test_settings_accepts_arbitrarily_large_integer(self): + """Large integer settings must not overflow during finiteness validation.""" + settings = MAVFTPSettings([("max_backlog", int, 10**1000)]) + + self.assertEqual(settings.max_backlog, 10**1000) + + def test_gap_read_nack_preserves_server_error(self): """A failed gap repair must not turn a ReadFile NACK into success.""" ftp, _master = self.make_ftp([])