Fix MAVFTP file transfers on slow telemetry links#1334
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses file transfer issues on slow telemetry links by modifying the burst read handling logic in the MAVFTP implementation. The changes focus on improving the control flow when processing burst read acknowledgments and errors.
Changes:
- Modified the burst read continuation logic to return success immediately after sending more data
- Changed the conditional structure from
eliftoiffor Nack handling to allow both Ack and Nack paths to be evaluated - Simplified error logging by removing duplicate error handling paths
| self.__send(more) | ||
| elif op.opcode == OP_Nack: | ||
| return MAVFTPReturn("BurstReadFile", ERR_None) | ||
| if op.opcode == OP_Nack: |
There was a problem hiding this comment.
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.
| if op.opcode == OP_Nack: | |
| elif op.opcode == OP_Nack: |
| 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) |
There was a problem hiding this comment.
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.
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
No description provided.