Skip to content

Fix MAVFTP file transfers on slow telemetry links #1334

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
amilcarlucas merged 1 commit into from
Feb 27, 2026
Merged

Fix MAVFTP file transfers on slow telemetry links #1334

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions ardupilot_methodic_configurator/backend_mavftp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,7 +691,8 @@ def __handle_burst_read(self, op, _m) -> MAVFTPReturn: # noqa PRL0911, PGH004,
if self.ftp_settings.debug > 0:
logging.info("FTP: burst continue at %u %u", more.offset, self.fh.tell())
self.__send(more)
elif op.opcode == OP_Nack:
return MAVFTPReturn("BurstReadFile", ERR_None)
if op.opcode == OP_Nack:
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing from elif to if creates a logic error. The code now returns success (line 694) when handling OP_Ack, but then immediately continues to check if op.opcode == OP_Nack (line 695). Since the function has already returned, the Nack check becomes unreachable code. The original elif structure was correct - after handling OP_Ack, the function should not check for OP_Nack on the same operation.

Suggested change
if op.opcode == OP_Nack:
elif op.opcode == OP_Nack:

Copilot uses AI. Check for mistakes.
ecode = op.payload[0]
if ecode in {ERR_EndOfFile, 0}:
if not self.reached_eof and op.offset > self.fh.tell():
Expand All @@ -707,12 +708,8 @@ def __handle_burst_read(self, op, _m) -> MAVFTPReturn: # noqa PRL0911, PGH004,
if self.__check_read_finished():
return MAVFTPReturn("BurstReadFile", ERR_None)
self.__check_read_send()
elif self.ftp_settings.debug > 0:
logging.info("FTP: burst Nack (ecode:%u): %s", ecode, op)
return MAVFTPReturn("BurstReadFile", ERR_Fail)
if self.ftp_settings.debug > 0:
logging.error("FTP: burst nack: %s", op)
return MAVFTPReturn("BurstReadFile", ERR_Fail)
logging.info("FTP: burst Nack (ecode:%u): %s", ecode, op)
Comment on lines 711 to +712
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After removing the return statement that was inside the elif block on the original line 713, the function now continues execution after logging this Nack message, eventually reaching line 715 which returns MAVFTPReturn("BurstReadFile", ERR_Fail). However, for EOF and ecode==0 cases (lines 697-710), the code has specific handling logic that may or may not return. This unconditional log at line 712 will now execute for all Nack cases that don't return early, which may not be the intended behavior. The removed duplicate error handling (original lines 711-714) suggests there was supposed to be different handling for different error codes.

Copilot uses AI. Check for mistakes.
else:
logging.warning("FTP: burst error: %s", op)
return MAVFTPReturn("BurstReadFile", ERR_Fail)
Expand Down