Make MAVProxy a pymavlink MAVFTP wrapper

[amilcarlucas]

Replace MAVProxy's forked FTP protocol implementation with an FTPWorker adapter around pymavlink.mavftp.MAVFTP.

Keep MAVProxy-specific responsibilities in FTPModule: concurrent session allocation and quarantine, queued operation scheduling, target snapshotting and reply routing, shared MAVLink batching and link simulation, plus console output and callback compatibility. Delegate FTP packet construction, transfer state machines, retries, directory parsing, CRC handling, and file operations to pymavlink.

Use pymavlink's managed transport API so each worker has an explicit session, does not reset vehicle sessions, sends raw payloads through MAVProxy's shared transport, and completes asynchronously without blocking MAVProxy's event loop. Preserve module-level protocol/error aliases for existing MAVProxy callers.

Add coverage for event-loop-driven CRC comparisons and correct synthetic download replies to use the protocol-valid request sequence number.

requires http://github.com/ArduPilot/pymavlink/pull/1288

[amilcarlucas]

This will require releasing an pymavlink 2.4.50 and updating the dependencies to depend on it.

[AP-Review]

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Full report: https://uav.tridgell.net/DevCallReviews/2026_09_23/devcall_pr_reviews.html#prMAVProxy-1762

Reviewed at head d37f0b10d5. REQUEST CHANGES. Two independent passes reached the same verdict and the same two headline findings, each reproducing them separately — so I have unusually high confidence here. The architecture is sound and the session/scheduling layer is preserved almost verbatim; the problems are a merge-order blocker and a near-total loss of user-visible output.

1. The module cannot import against any released pymavlink — and CI says so

MAVProxy/modules/mavproxy_ftp.py:11. setup.py:29 declares pymavlink>=2.4.38 and requirements.txt:1 says >=2.4.14; neither is changed by this PR (the diff touches only mavproxy_ftp.py and its test). Latest on PyPI is 2.4.49, and against an installed 2.4.49 four of the 27 names this module imports do not exist: MAX_FTP_NAME, MAX_NETWORK_BATCH, MAVLinkBatchWriter, OP_ListDirectoryWithTime. MAVFTP also lacks cmd_crccmp, cmd_crclocal, transfer_status, idle_task, mavlink_packet and every managed-transport kwarg.

This is not niche: ftp is in MAVProxy's default module list (mavproxy.py:1413), so the import raises at startup and takes FTP parameter download, FTP mission download and MAVFTP camera definitions with it. Your own CI reproduces it — tests fails with:

MAVProxy/modules/mavproxy_ftp.py:11: in <module>
    from pymavlink.mavftp import (
E   ImportError: cannot import name 'MAX_FTP_NAME' from 'pymavlink.mavftp'

and .github/workflows/python-cleanliness.yml:39 pins pymavlink==2.4.49 deliberately ("Exercise the released dialect"), so it will keep failing until 2.4.50 ships. With #1288's mavftp.py/mavftp_op.py/mavftpfs.py dropped in, all 40 tests pass.

One correction to the dependency picture, since it changes the ordering: pymavlink #1274 and #1288 are not siblings — e27038f3ac (#1274) is an ancestor of 70477cb1bf (#1288), which adds exactly one commit, 70477cb1 "mavftp: support externally managed FTP sessions". That commit is what introduces session=, reset_sessions=, send_payloads=, operation_callback=, source_system=/source_component= (mavftp.py:469-533) — i.e. the whole API this PR depends on. Also worth knowing: OP_ListDirectoryWithTime lives in pymavlink/mavftp_op.py, which neither pymavlink PR touches — it came from an already-merged-but-unreleased commit, which independently confirms a new release is mandatory, not just the two PRs.

So the order is: merge #1274 → merge #1288 → release pymavlink 2.4.50 → in this PR, bump setup.py:29, requirements.txt:1 and the CI pin at python-cleanliness.yml:39 to 2.4.50 → merge. You already flagged the release in your comment; the pieces above are the concrete checklist.

2. Essentially all FTP console output is silently lost

pymavlink's mavftp reports through logging on the root logger — there is no logging.getLogger anywhere in the file (73 logging.info call sites), and logging.basicConfig appears only inside its main(). MAVProxy configures logging nowhere (no basicConfig/dictConfig/setLevel/addHandler anywhere in MAVProxy/). With no root handler, Python's lastResort handler is level WARNING, so every logging.info is discarded and logging.error goes to stderr with an ERROR:root: prefix.

Worse, remote NACKs never even reach logging.error: MAVFTPReturn.display_message() is called only from mavftp.py:1872 and pymavlink's own CLI, and the wrapper never calls it — so ftp rm /nonexistent produces nothing on stdout or stderr.

Measured side by side on the same scripted session, old module vs new:

OLD                                    NEW
Listing /                                 subdir/	-
 D subdir	2023-11-15 09:13:20           file.txt	123	1700000000
   file.txt	123	2023-11-15 09:13:20
Total size 0.12 kByte

Getting CRC for /foo.txt               <NO OUTPUT>
crc: /foo.txt 0xdeadbeef in 0.0s

Removing /nope                         <NO OUTPUT on stdout AND stderr>
Remove failed OP seq:1 … plen=1 [10]

ftp crc and ftp crclocal now produce nothing at all, and their printed value is the command's entire purpose. ftp rm/rmdir/rename/mkdir are silent on success and failure. ftp get/put lose both their start and completion lines. crccmp loses every MATCH/DIFFER/MISSING line and the summary.

The clean fix belongs in #1288 — give pymavlink/mavftp.py a module logger (logger = logging.getLogger(__name__) and logger.info(...)), then have FTPModule.__init__ attach a small handler that print()s. Configuring the root logger from a MAVProxy module is not an acceptable substitute — it would change logging for the whole process. In the meantime the module can report results itself in FTPWorker._operation_finished and print self.last_crc in cmd_crc/cmd_crclocal, the way the listing entries are already handled at :188-200.

3. ftp list mtime regressions

MAVProxy/modules/mavproxy_ftp.py:190-198. Two distinct defects, both reproduced:

• Directories: suffix = '/' if entry.mtime is None else '/\t-' — any non-None mtime prints -, so a server that does report directory mtimes now shows nothing. pymavlink's own formatter (mavftp.py:427-451) correctly distinguishes mtime == 0 from a real value.
• Files: %u prints the raw epoch (1700000123) where the old module rendered 2023-11-15 09:15:23 via list_mtime_str (5ff0c4c332:…:376).

It is a downgrade against both the old MAVProxy code and pymavlink. Also lost: the Listing <dir> header, the Total size %.2f kByte footer, and the D name → name/ prefix change (a silent format change if anything parses ftp list). Suggested, matching the deleted helper including its guards:

def _mtime_str(mtime):
    if mtime == 0:
        return '-'
    try:
        return time.strftime("%Y-%m-%d %H:%M:%S", time.localtime(mtime))
    except (ValueError, OSError, OverflowError):
        return str(mtime)

applied to both branches, with entry.size_b accumulated for the footer.

4. ftp cancel tells the user nothing, and outcome is dead

:173-176, :582, :504-525. The override is terminate_session(self, outcome='failed') but outcome is never used — it calls super().terminate_session() (pymavlink signature (success=False, result=None)). The old code set Downloading <file> cancelled / Uploading <file> failed, and finished_status() set Finished downloading … (3 bytes …). Now worker_done → _update_console_status(force=True) finds no active workers and blanks the row:

OLD  final console: ('Finished downloading …/dl2.bin (3 bytes 0.0 seconds, 19.2 kbyte/sec)', 4)
NEW  final console: ('', 4)

Note the PR's own new test encodes the regression (self.assertEqual(self.mpstate.console.status['FTP'], ('', 4))), so it will not be caught by CI. Set the terminal console line from result before clearing, using outcome for the abort case — or drop the dead parameter if the removal is deliberate.

Separately, the override is bypassed on every normal completion: pymavlink finishes via the name-mangled __terminate_session/__finish_session, so manager.discard_delayed(self) only runs on explicit cancel. Benign today because worker_done extends the session quarantine past any queued lag-simulator packet (:489-499), but worth a comment or moving discard_delayed into worker_done.

5. ftp set no longer validates

:207-244 passes an MPSettings where pymavlink expects MAVFTPSettings. Confirmed at runtime that MPSettings has no validate(), so none of pymavlink's bounds are enforced — ftp set idle_detection_time 0.1 is accepted even though read_retry_time is 1.0, a combination MAVFTPSettings rejects with ValueError (mavftp.py:286-292).

Latent hazard alongside it: mavftp.py:3879 calls self.ftp_settings.validate() inside process_ftp_reply, which would raise AttributeError on MPSettings — not caught by the surrounding except (TypeError, ValueError). Unreachable today only because every wrapper call passes wait=False (all nine call sites traced), so no process_ftp_reply site is reached. That is a fragile coupling: one missed wait=False in a future edit becomes an unhandled exception in the main loop.

Notes

• 20 unused imports at :11-37 (flake8 F401). CI misses them because scripts/run_flake8.py only checks files carrying the AP_FLAKE8_CLEAN marker and this file has none. If they are intentional re-exports for tests and third parties — which is plausible — say so with # noqa: F401 so a future reader does not delete them.
• A dropped test assertion: test_server_session_exhaustion_waits_and_retries loses the trailing self.assertEqual(callbacks, []) after idle_task(), which was the check that a NoSessionsAvailable retry does not spuriously fire the completion callback. Restore it or say why it no longer holds.

Feature parity — the good news

Method and argument plumbing is preserved for every command, and the manager half of the module is essentially byte-identical to the old one. encode_filename() (used by mavproxy_camera/parameters.py:103) survives with a new test; the ERR_* aliases are numerically identical to FtpError 0–10; cmd_get keeps max_size=, callback=, target_system=/target_component= and the (fh, byte_count) progress signature, so mavproxy_param.py, mission_item_protocol.py, mavproxy_oldwp.py and mavproxy_camera all still fit; ftp status still prints; the in-transfer progress line still updates. Two settings are added, none removed. The losses are all user-visible output, not function.

Also checked and clean: no blocking — managed mode is fully event-driven and terminate_session returns immediately (mavftp.py:1294-1308), no thread is created so there is nothing to join on unload; routing is unchanged — FTPModule.mavlink_packet is byte-identical to the old one, filters on source_system/source_component, then session→worker, then cross-checks srcSystem/srcComponent, and because process_ftp_reply is never entered pymavlink never calls recv_match and cannot steal other modules' packets; multi-sysid targeting preserved; session-id lifetime safe in managed mode; cmd_crclocal's unregistered FTPWorker(self, 0) is harmless (reset_sessions=False sends no packet and worker_done's identity guard makes the stray completion a no-op); no bare except:; no Python-version issue.

Not covered: everything here is fake-link and unit-level — no run against a real or SITL vehicle, and the loss/lag simulator and multi-vehicle routing were not exercised end to end.

[tridge]

no thanks, I deliberately want to keep them separate