[master] core: ardupilot_manager: Fix Pixhawk restart reconnect

[joaoantoniocardoso]

This is a port of #4114 into master.

[github-actions]

Automated PR Review

0. Summary

• Verdict: MINOR SUGGESTIONS ✏️

Serialises restart_ardupilot() with an asyncio.Lock, and for PlatformType.Serial boards issues the MAVLink reboot, waits (best-effort) for the board to drop from available_boards() and then to re-enumerate, then stops the router and calls start_serial() on the freshly-detected FlightController so the router no longer holds the pre-reboot USB path. Frontend /restart axios timeout is raised from 10 s → 60 s to cover the ~40 s backend window. Fix is targeted and reads well; a few small concerns below.

1. Correctness & Implementation Bugs

• 1.1 [minor] core/services/ardupilot_manager/autopilot_manager.py:701 — this changes the fallback behaviour for PlatformType.Manual and PlatformType.Unknown. Previously (self.current_board.type in [PlatformType.SITL, PlatformType.Linux]) those platforms fell through to vehicle_manager.reboot_vehicle() and got a MAVLink MAV_CMD_PREFLIGHT_REBOOT_SHUTDOWN. Under board.type != PlatformType.Serial, they now take the kill_ardupilot() → start_ardupilot() branch, which for Manual boards (start_manual_board() — no owned subprocess) reduces to disarm + mavlink_manager.stop()/start() without ever asking the vehicle to reboot. The PR title and body only mention Pixhawk/Serial; if this Manual-endpoint change is intentional, please call it out (and consider whether users pressing "Restart Autopilot" on a Manual endpoint still get the FC reboot they expect).
• 1.2 [minor] core/services/ardupilot_manager/autopilot_manager.py:717 — the disconnect loop uses the while … else: construct, which only runs the else when the loop exits normally (deadline reached without a break). It's valid Python but easy to misread; a plain disconnected = False flag set on break, checked after the loop, would be more obvious for the next reader.

4. Performance

• 4.1 [minor] core/services/ardupilot_manager/autopilot_manager.py:713-728 — _detect_serial_board() calls self.available_boards() which calls BoardDetector.detect(True) on every 0.5 s tick, i.e. up to ~80 full USB re-enumerations per restart in the worst case. Not a hot path, but bumping the sleep to 1 s (or backing off after the first few polls) would halve syscall/log volume with negligible UX cost — reconnect latency is dominated by the board coming back, not by poll granularity.

6. Code Quality & Style

• 6.1 [minor] core/services/ardupilot_manager/autopilot_manager.py:717,725 — the 10.0 disconnect budget, 30.0 reconnect budget, and 0.5 poll interval are inline magic numbers. Lifting them to module-level constants (e.g. SERIAL_REBOOT_DISCONNECT_TIMEOUT_S = 10.0, SERIAL_REBOOT_RECONNECT_TIMEOUT_S = 30.0, SERIAL_REBOOT_POLL_INTERVAL_S = 0.5) would make the frontend/backend timeout budget (currently 40 s backend vs. 60 s frontend axios) auditable in one place.
• 6.2 [nit] core/services/ardupilot_manager/autopilot_manager.py:735-741 — the except Exception: cleanup then raises bare after resetting state; that's fine, but the inner try/except Exception as error: logger.warning(...) around mavlink_manager.stop() silently swallows the cleanup error. Consider logger.exception(...) so the traceback is visible when diagnosing a failed restart.

7. Tests

• 7.1 [nit] No unit tests added for the new reboot-and-reconnect flow. The ardupilot_manager module has no existing tests for restart_ardupilot(), so this isn't a regression against ./.hooks/pre-push, but the state machine here (lock, watchdog stand-down, two timeouts, cleanup path) is exactly the kind of logic that benefits from a fake available_boards() / mavlink_manager in a follow-up.

8. Documentation

• 8.1 [nit] core/services/ardupilot_manager/autopilot_manager.py:707 — the comment "everything else (SITL, Linux, Manual, unknown) is a process/router we can just bounce" implicitly documents the behaviour change from finding 1.1; if the Manual path is really meant to lose its MAVLink reboot, note that explicitly so the intent survives future refactors.

Generated by PR Review Bot. This is advisory, a human reviewer must still approve.