Sync backend with pymavlink implementation - #2050
amilcarlucas wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The download completion path currently reads entire staged downloads into memory before writing to disk, which is unsafe for large flight logs and should be fixed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the project’s MAVFTP integration to align with pymavlink’s mavftp_op API, replacing the local FTP operation implementation and adapting application code and tests to the new FtpError and directory-entry structures. It also hardens download handling so callback-driven “virtual” remote paths aren’t accidentally treated as local filenames.
Changes:
- Replace local MAVFTP opcodes/error constants with pymavlink
FTP_OPops and a newFtpErrorenum, and update consumers accordingly. - Switch directory listings to a structured
DirectoryEntrylist API and update flight-log listing logic + fixtures. - Update factories and tests to support session reset semantics during MAVFTP initialization.
File summaries
| File | Description |
|---|---|
ty.toml |
Excludes backend_mavftp.py from ty checks. |
pyproject.toml |
Excludes backend_mavftp.py from ruff and ignores mypy errors for the module. |
.pylintrc |
Excludes backend_mavftp.py from pylint. |
ardupilot_methodic_configurator/backend_mavftp.py |
Reworks MAVFTP to use pymavlink’s FTP ops, introduces FtpError/DirectoryEntry, and adjusts transfer/session handling. |
ardupilot_methodic_configurator/backend_flightcontroller_params.py |
Updates parameter download flow to use FtpError.Success semantics. |
ardupilot_methodic_configurator/backend_flightcontroller_files.py |
Updates file operations and directory listing consumption for the new APIs. |
tests/test_backend_mavftp.py |
Updates tests to use FtpError and adds coverage for callback downloads not writing virtual remote paths. |
tests/test_backend_mavftp_aux.py |
Updates MAVFTPReturn tests for FtpError values and messages. |
tests/test_backend_flightcontroller_sitl.py |
Updates log-directory creation checks to use FtpError. |
tests/test_backend_flightcontroller_files.py |
Updates mocks/fixtures to use DirectoryEntry lists. |
tests/test_backend_flightcontroller_factory_mavftp.py |
Adds a reset-session-aware mocked master for factory tests. |
Review details
- Files reviewed: 10/11 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 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 | ||
| assert self.get_result is not None # noqa: S101 | ||
| if len(self.get_result) < self.requested_size: | ||
| logging.warning( | ||
| "expected %u, got %u", self.requested_size, len(self.get_result) | ||
| ) | ||
| logging.info("read %u bytes", len(self.get_result)) | ||
| self.fh.flush() | ||
| try: | ||
| if publish_result and self.filename and self.filename != "-": | ||
| # Move the result to the final location | ||
| logging.info("Moving %s to %s", self.temp_filename, self.filename) | ||
| with open(self.filename, "wb") as final_file: | ||
| final_file.write(self.get_result) | ||
| finally: | ||
| # terminate the remote session and release the staging | ||
| # file even when the destination cannot be written | ||
| self.__terminate_session() | ||
| self.read_complete = True | ||
| return True |
| while True: | ||
| while len(data) > 0 and data[0] == pad_byte: | ||
| data = data[1:] # skip pad bytes | ||
|
|
☂️ Code Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
Test Results 4 files 4 suites 34m 13s ⏱️ Results for commit 7342b85. ♻️ This comment has been updated with latest results. |
amilcarlucas
added
the
AIReview
Previous review (2026-09-10)Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Reviewed at head Verdict: COMMENT — no blockers. Nothing here will corrupt user data, and the question most likely to have been broken — whether the pinned pymavlink release actually provides what the new code imports — checks out against the real PyPI wheel. Three things are worth changing before merge. The flight-log download path now buffers the whole file in RAM, and keeps it
This is a regression against the code being replaced, not a pre-existing wart. The base implementation streamed straight to the destination — Measured rather than asserted — driving
This confirms and strengthens Copilot's inline comment at An unwritable destination now fails after the whole transfer, as an uncaught
|
| file:line | change | messages actually silenced |
|---|---|---|
.pylintrc:7 |
ignore=decode_devid.py,backend_mavftp.py |
2 |
pyproject.toml:193 |
ruff exclude |
138 |
pyproject.toml:270 |
mypy ignore_errors = true |
10 |
ty.toml:12 |
src.exclude |
0 |
- pylint 4.0.5: exactly two messages, both
R0917 too-many-positional-arguments(lines 199 and 2215), score 9.99/10 — both fixable with the# pylint: disable=idiom already used elsewhere in the same file. Excluding the file also disables this repo's ownenable=useless-suppression/fail-on=useless-suppressionfor it, and ruff independently reports 4RUF100 unused-noqain the new code, so stale suppressions are already accumulating in there. - mypy 2.3.1: 10 errors, 6 of them
Unused "type: ignore" comment. The two substantive ones (:513,:834) are both fixed by annotatingself.temp_filename: Optional[str] = Noneatbackend_mavftp.py:414. A blanketignore_errors = truefor a 2691-line MAVLink I/O module is a lot of collateral for that. - ty 0.0.74:
All checks passed!— the exclusion silences nothing. I checked that ty really was analysing the file rather than skipping it another way, by appending a deliberately wrong return type and confirming it reportederror[invalid-return-type]. - ruff: 138 errors versus 0 on the base version of the same file. The bulk is the legacy typing style adopted wholesale from upstream —
39×UP006,16×UP007,10×UP045,4×UP035— on a project withrequires-python = ">=3.10"andtarget-version = "py310", plus26×ANN001. I take it the real motivation isruff-format: the file is now in upstream's 88-column style andruff format --diffwould rewrite 435 lines against this project'sline-length = 127. That's a fair reason to exclude formatting; it doesn't require giving up the 138 lint rules.
The justification is also wrong on the facts. .pylintrc:5-6 reads "Synchronized verbatim from ArduPilot by update_flightcontroller_ids.yml; do not lint the local copy", and backend_mavftp.py was added to that line — but grep -rn backend_mavftp .github/workflows/ returns nothing (no workflow syncs it, unlike decode_devid.py, which update_flightcontroller_ids.yml:59 copies verbatim), and it is not verbatim: against pymavlink/mavftp.py extracted from the released 2.4.49 wheel, 915 of 2691 lines match — 34% identical, 1773 added and 753 removed. It's a heavily diverged fork holding the app's flight-log and parameter I/O, and the green ruff/pylint/mypy checks at this head are green because of these four entries. Please either fix the ~12 real diagnostics and keep the checks, or at minimum correct the comment so the next reader knows the file is unlinted by choice rather than because it's a mirror.
ftp_param_decode's repeated slicing is a regression — this is the bit Copilot couldn't have known
backend_mavftp.py:2043. I agree with Copilot's comment at :2082, and the important context is that the base implementation was already offset-based and O(n) (__decode_param_record(data, offset, …), which is in this PR's removed set). Syncing to upstream threw away this project's own optimisation.
That said, I measured it rather than leaving it as a scare: 500 params 0.51 ms, 1000 1.09 ms, 2000 2.44 ms, 4000 5.99 ms. Clearly superlinear (8× params → ~12× time) but ~1 ms at a realistic ArduPilot parameter count, against a MAVFTP transfer measured in seconds. Low priority — worth restoring because the better version already existed here, not because users will notice.
On tolerating a bad parameter-count header (backend_mavftp.py:2150)
Your motivation checks out — ArduPilot itself concedes the header count can be wrong (AP_Filesystem_Param.cpp:195-198 invalidates its cached count_parameters() for exactly this reason), so rejecting the whole download over it is wrong.
My concern is that count != num_params was the only end-to-end record-count integrity check on the packed file, and the result isn't used benignly: backend_flightcontroller_params.py:258-261 writes the decoded set to complete_param_filename as the FC's "complete" snapshot, which the editor then diffs against the intended values. A genuinely short transfer would yield a silently partial snapshot in which missing parameters look like parameters the FC doesn't have.
The new test test_param_decode_keeps_records_when_header_count_differs (tests/test_backend_mavftp.py:161) packs count=1 with two records — i.e. only count > num_params, the harmless direction. I couldn't construct a case where a short transfer actually reaches this code (the reached_eof/read_gaps machinery and the len(self.get_result) < self.requested_size warning at :943 should catch it first), so I'm marking the risk unconfirmed. Suggestion: tolerate count > num_params and keep rejecting count < num_params, which is what truncation looks like. Separately — this commit diverges from pymavlink upstream in the very function being synced, so it should go upstream too or the next sync silently reverts it.
The new "completed upload" fast path names an operation no caller passes
backend_mavftp.py:1938 scopes it to operation_name.lower() == "put", but both upload call sites pass "CreateFile" — backend_flightcontroller_files.py:115 and the untouched mavftp_example.py:188.
An independent Codex pass reported this as a bug that makes an acknowledged upload read as timed out. I checked and that part is wrong, so please don't chase it: tests/test_backend_flightcontroller_sitl.py:1145 and :1194 drive files_mgr.upload_file() against a live ArduCopter SITL MAVFTP server, .github/workflows/pytest.yml:178 runs them with -m "sitl or not sitl" on Linux, and pytest (ubuntu-latest, 3.10) and (ubuntu-latest, 3.14) are both green at this head with 5159 tests and 0 failures — so the ordinary completed_opcode == last_op.opcode path does complete the upload. The fast path is simply never taken, and its comment claims to cover a case it can't reach. Either drop it or match on the opcode rather than the caller-supplied display name. Worth noting that every upload test in test_backend_flightcontroller_files.py mocks process_ftp_reply outright, so the unit suite wouldn't distinguish the two.
Minor
backend_mavftp.py:952logs "Moving %s to %s" but performs a read-and-rewrite. Becomes accurate if the first finding is fixed withos.replace.backend_mavftp.py:653-710:read()andread_sector()are new and unused anywhere in this package (they exist upstream formavftpfs) — ~60 lines of dead code with a hardcoded 5 s timeout and atime.sleep(0.0001)busy loop.backend_mavftp.py:14: a stray# FLAKE_CLEANmarker, the only occurrence in the repo.- The file header drops
This file is part of ArduPilot Methodic Configurator…, which 81 of the 91 modules inardupilot_methodic_configurator/carry. - CLI default
--idle_detection_timechanges 1.2 → 3.7, a real behaviour change for the shippedmavftpconsole script (it now matches the library default, so arguably a fix — just worth being deliberate). data_model_parameter_editor.py:1192-1199: the new "Uploaded and verified N parameters in M ms" timer starts at line 1131, beforeupload_parameters_that_require_reset_workflow, which can block on a confirmation dialog and an FC reboot+reconnect — so the number will sometimes be dominated by human think-time. The test can't catch this because it mocks that workflow out entirely; it also patchestimewith exactly two values, so any futuretime()call on that path breaks it withStopIteration.
What was checked and is clean
The pymavlink dependency is fine. pyproject.toml:57 pins pymavlink==2.4.49 both before and after this PR, and backend_mavftp.py:51 imports FTP_OP and the OP_* constants from pymavlink.mavftp_op. I downloaded the actual PyPI wheel pymavlink-2.4.49-cp312-…-manylinux_2_28_x86_64.whl and AST-parsed it: it ships pymavlink/mavftp_op.py exporting exactly OP_None … OP_Nack and FTP_OP, whose signature, pack() and items() are behaviourally identical to the class this PR deletes. Every imported name is present in the released wheel — no module-scope-constant trap. FtpError and DirectoryEntry are defined locally (backend_mavftp.py:77, :111), not imported, so no version coupling there either.
No missed call sites — grep -rn "ERR_[A-Za-z]" over the whole package returns only unrelated hits in scripts/generate_pdef.xml_metadata.py; directory_listing is consumed only at backend_flightcontroller_files.py:282; and the one MAVFTP consumer not touched by this PR, mavftp_example.py, uses only FTP_OP and .items(), both still valid.
Path traversal is clean — the obvious worry given the "remote paths as local filenames" framing. The FC-supplied listing is only used to parse a log number (backend_flightcontroller_files.py:285-290), the local destination is built from an int, and the single-argument cmd_get path uses os.path.basename. No remote-controlled string reaches os.path.join or open().
Both new tests were proven by mutation, not by reading. Flipping backend_mavftp.py:899 from publish_result = False back to True makes test_successful_callback_does_not_write_virtual_remote_path fail, with the captured log showing the bug it guards: Moving None to param.pck?withdefaults=1. Restoring the old logging.error(...) + return None at :2150 makes the count-mismatch test fail at its assert result is not None. And the underlying bug is a genuinely good catch — cmd_getparams calls cmd_get(["@PARAM/param.pck?withdefaults=1"], callback=…) with one argument, so cmd_get sets self.filename = os.path.basename(fname) = param.pck?withdefaults=1, a name containing ?, illegal on Windows, which the old code would have tried to create.
A local run on this head in a dev venv gives 450 passed, 49 skipped across the seven affected test files. The one red check, Publish Tests Results, is the coverage report --fail-under=89 gate, which fails identically on master for this repo; all 5159 tests pass with 0 failures.
Not covered: the rewritten burst-read and gap-repair machinery (__handle_burst_read, __idle_task, __reply_matches_active_request, BURST_REPLY_SEQUENCE_WINDOW, MAX_READ_GAPS) is the largest behavioural surface in the diff and was only spot-checked — it plausibly deserves its own pass. Also not covered: cmd_put/__handle_create_file_reply beyond confirming the new fh_owned ownership logic, the GUI layer, Windows behaviour (reasoned about, not executed), and no hardware testing.
Reviewed by Claude plus an independent Codex pass that was shown only the PR number, never these findings.
374e9a8 to
38e208b
Compare
Previous review (2026-09-15)Automated review note — AI-generated (Claude), validated against the live diff (Claude + Codex cross-checked). Please sanity-check before acting. Re-reviewed at head Scope: this review covers the three commits up to
Verdict: COMMENT (unchanged).
Resolved
Partly resolved
Still open
New
Checked:
Not checked in depth: the burst, RTT and batch state machines, Windows, and hardware. CI at |
Previous review (2026-09-15)Automated review note — AI-generated (Claude), validated against the live diff (Claude + Codex cross-checked). Please sanity-check before acting. Re-reviewed at head This covers Verdict: REQUEST CHANGES (was COMMENT).
Should fix (new in
|
| scenario | before | after |
|---|---|---|
| TCP connection refused | 2.0 s | 8.0 s |
| TCP open, no heartbeat | 5.0 s | 17.0 s |
| auto-detect, no FC plugged in | 3.0 s | 10.0 s |
Suggest looping only on the reconnect path and retrying only port-open or no-heartbeat failures, with one layer owning the retry count.
The reboot countdown is gone, so the restart window freezes.
- The 1 s callback loop became a single
time_sleep(sleep_time)on the Tk thread (backend_flightcontroller.py:307). That is 8 s plus the extra delay. - Parameter editor: against ArduCopter SITL the total reconnect time is unchanged (14.3 s). But the window now gets no update for 8.0 s, where it used to get one per second.
- Motor test:
DelayedProgressCallback(plugins/frontend_tkinter_motor_test.py:83-91, wired at:521) swallows the 10% call. Nothing is shown for 10.3 s, where it used to appear after 1.3 s. - Keeping a per-second countdown through the reconnect callback would fix both.
Minor
-
Label is wrong and the bar goes backwards.
- The 90% "Waiting for MAVLink HEARTBEAT" update is sent after heartbeats arrive (
backend_flightcontroller_connection.py:908), while the code is actually waiting for the banner and AUTOPILOT_VERSION. - On a handshake retry the bar goes 30 → 90 → 50 → 90 → 100 (measured against a local MAVLink responder).
- The 90% "Waiting for MAVLink HEARTBEAT" update is sent after heartbeats arrive (
-
The stage-to-label mapping is copied three times.
- The copies are at
frontend_tkinter_parameter_editor.py:204-218and:1664-1678, andplugins/frontend_tkinter_motor_test.py:486-499. - Each hard-codes "of 3" and inverts the formula at
backend_flightcontroller_connection.py:888. - One helper, or letting the backend send the stage text, would stop them drifting.
- The copies are at
-
The restart window can be reused after it is destroyed.
get_connection_progress_callbackcaches the window (frontend_tkinter_parameter_editor.py:197), and reaching 100% destroys it (frontend_tkinter_progress_window.py:154-155).- If the user picks Retry after a validation failure (
data_model_parameter_editor.py:1171) and that pass resets again, no restart window is shown. - It doesn't crash, because
update_progress_barcheckswinfo_exists(). The previous head created a fresh window each time.
-
The tests miss most of the new behaviour. Of 19 mutations run against the 789 tests in the touched files, 11 survived. The survivors include:
- no retry on unsupported-autopilot or version/banner failure, which the commit message claims;
- no
disconnect()between attempts; - deleting the reboot
time_sleep(sleep_time)entirely; - all of the motor-test restart progress.
Only the port-open retry is tested directly (
tests/test_backend_flightcontroller_connection.py:1829), and that test mocks heartbeat and version retrieval to succeed. -
Reconnect reopens the same device name.
- For an auto-detected serial port,
comport.deviceis the resolved/dev/ttyACMx, so a board that comes back under a different name won't reconnect. This was checked with injected discovery, not on hardware. - The retry does help when the same name just comes back late.
- For an auto-detected serial port,
-
Dead code:
return _("Connection failed.")atbackend_flightcontroller_connection.py:948is unreachable. The two branches inFlightController.create_connection_with_retry(backend_flightcontroller.py:423-438) differ only in forwarding one kwarg.
Previous findings
- Resolved: header-count tolerance. The strict check is at
backend_mavftp.py:3310-3312. - Partly resolved: unwritable destination. It is caught (
backend_mavftp.py:1582-1588), but only after the whole download. - Still open, unchanged:
- whole-file RAM buffering on download (
backend_mavftp.py:1560,:1566,:1580-1581); - lint exclusions, with the wrong "synchronized by" comment (
.pylintrc:7,pyproject.toml:193,:270-272,ty.toml:12); - the
"put"upload fast path vs AMC's"CreateFile"(backend_mavftp.py:3020,backend_flightcontroller_files.py:115); ftp_param_decodeslicing (backend_mavftp.py:3246,:3292);- the copy of the still-open pymavlink#1274;
- no AMC tests for the sync;
- squashing
5e8526ab1cwith its import fix; - the opcode-16 probe on every listing;
- the minor mavftp items (ENOSPC staging leak,
get <remote> -, "Moving" log, unusedread(), file header).
- whole-file RAM buffering on download (
- Upload timer: this commit edited those lines, but the timer still starts before the reset workflow and still uses wall-clock
time()(data_model_parameter_editor.py:1117,:1124,:1184).
What was checked
- Tests and lint: 789 tests pass across the 8 touched test files (Python 3.14, pymavlink 2.4.49). ruff, pylint and pyright are clean on the changed files.
- Timing probes: refused, silent and auto-detect connects, plus a real ArduCopter SITL
reset_and_reconnect, all compared with38e208b6af. - Codex separately reproduced the retry multiplication (9 socket connects in 8.0 s) and the motor-test freeze.
- Earlier commits: the parent of
5053b0bb85is38e208b6af, so they are unchanged. - Not checked: USB re-enumeration on hardware, a Windows COM-port reopen, and SITL over UDP.
CI at 5053b0bb85: 30 passing, 1 skipping, and 1 failing. The failure is Publish Tests Results, as at the last two heads; the test-results bot reports 0 test failures.
24f9501 to
d886207
Compare
|
Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Re-reviewed at head Full report, including everything that was checked and found clean: https://uav.tridgell.net/DevCallReviews/2026_09_17_AIReview/devcall_pr_reviews.html#prMethodicConfigurator-2050 Verdict: REQUEST CHANGES A large, careful sync, and the previous round's two blockers are both fixed and measured: whole-file RAM buffering went from 67.3 MB peak / 67.1 MB retained at BUG — The batch write path bypasses WebSocket framing, putting raw MAVLink bytes on a WebSocket connection. BUG — Reconnect stops at the first heartbeat, so any second component on the link fails the reconnect outright. Three defects combine into one concrete failure. ISSUE — This file is a copy of an unmerged upstream PR, and four deliberate local fixes are absent from it. ISSUE — Four tool exclusions plus the coverage omission leave ~2300 added lines unmeasured and unlinted. ISSUE — Four public methods drop a positional parameter while the PR body ticks "No breaking changes". Lower-priority notes (1)
What was checked and found cleanTwelve previous findings RESOLVED, and the CI failure is not this PR's. Both blockers fixed: retry multiplication (1 factory call with |
Previous review (2026-09-16)Correction to my review above — AI-generated (Claude).
I said four MethodicConfigurator-local fixes were absent from #1274 and would be reverted by a future re-sync. Three of
What still stands:
Everything else in the review above is unaffected, including the two blockers. |
Log the number of uploaded parameters and elapsed verification time using descriptive, order-independent placeholders. Add regression coverage for the new message.
Replace the local MAVFTP backend with pymavlink's implementation and adapt consumers to the FtpError and DirectoryEntry APIs. Prevent callback-owned downloads from writing virtual remote paths as local files, and update the related tests and fixtures.
2bf74ab to
f1fb612
Compare
Replace separate reset and connection progress callbacks with one progress callback for the complete restart workflow. Emit deterministic restart progress stages: - 10% after the reset command is sent - 30%, 50%, and 70% for reconnect attempts - 90% after MAVLink heartbeat detection - 100% after the controller handshake completes Retry failed USB-port opens, heartbeat detection, and version/banner handshakes so a flight controller that re-enumerates after reboot can reconnect successfully. Use one shared restart/reconnect progress window in the parameter editor and motor-test plugin while keeping parameter-download progress unchanged. Add support for replacing a progress window message between stages and update acceptance and unit tests accordingly. BREAKING CHANGE: reset_progress_callback and connection_progress_callback are replaced by a single progress_callback.
f1fb612 to
7342b85
Compare
Previous review (2026-09-16)Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Re-reviewed at head Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_17_0922/devcall_pr_reviews.html#prMethodicConfigurator-2050 Both blockers from last round are fixed, and both are mutation-proven by two independent reviewers.
The vendored copy is now within three trivial hunks of upstream. Diffed against a freshly fetched I am also dropping a finding. I previously flagged the removal of the What blocks merge is mechanical: the PR breaks its own CI. Blocking
Non-blocking
Checked and clearedThe real delta is smaller than it looks — the PR was rebased and master absorbed the docs/locale/image churn, so against its own merge-base at each head the change is 143 lines added, 16 removed across 9 files, and every one maps to a previous finding. With exactly CI is still moving on this head — I counted 15 passing / 7 failing / 1 cancelled / 1 skipped at report time, having been 4 failing (all pylint) when first snapshotted, so treat the exact numbers as a moment in time. One structural note, unchanged: this file is a copy of an unmerged upstream PR, and |
|
Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Re-reviewed at head Both blockers from the previous round are fixed — but the fix for one of them broke a different linter, which is why CI is red. The independent cold pass also found two defects in the synced FTP code that neither of us had, and I reproduced both before raising them. Verdict: REQUEST CHANGES. Previous round
Blocking
Worth fixing
Minor
One CI failure is not yours
Checked and clearThe pymavlink sync claim holds, re-verified against #1274's current head |
Description
Replace the local MAVFTP backend with pymavlink's implementation and adapt consumers to the FtpError and DirectoryEntry APIs.
Prevent callback-owned downloads from writing virtual remote paths as local files, and update the related tests and fixtures.
Checklist
git commit --signoff)Testing
Describe how you tested these changes: