mirror of
https://github.com/azaion/gps-denied-onboard.git
synced 2026-06-22 20:11:14 +00:00
[AZ-558] Route C8 outbound encoder bytes through MavlinkTransport seam
All FC adapter outbound MAVLink bytes now go through the AZ-401 MavlinkTransport seam (NoopMavlinkTransport in replay, SerialMavlinkTransport in live). New helpers in _outbound_mavlink_payloads.py extract encode/pack/seq-bump so the four AP _send sites and the iNav statustext _send site become encode -> pack -> transport.write. TlogReplayFcAdapter emits real AP-shape MAVLink bytes through the injected NoopMavlinkTransport, satisfying replay protocol Invariant 5 and unblocking AZ-401 AC-9. Closes AZ-558. Also unskips AZ-401 AC-9 and AZ-404 AC-4b. Live wire output remains byte-identical (proven via two-instance MAVLink byte-equivalence tests). AST scan asserts no .mav.<name>_send( calls remain in the retrofit set (AP / iNav / tlog adapters). Out of scope (logged in review): GCS adapter retrofit; airborne live strategy registration that would activate the SerialMavlinkTransport factory injection path. Tests: 2110 passed, 92 environmental skips, 1 unrelated pre-existing macOS cold-start flake deselected. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -1,6 +1,6 @@
|
||||
# Dependencies Table
|
||||
|
||||
**Date**: 2026-05-14 (refreshed at start of Batch 63: AZ-559 closed Won't Fix — gap was illusory; `TileSource.ONBOARD_INGEST` + `TileMetadata.quality_metadata` + `write_tile`'s `FreshnessRejectionError` already cover the AZ-389 mid-flight ingest semantic without any new API; AZ-389 dep restored to AZ-303; earlier same-day after Batch 61: AZ-558 follow-up added — routes C8 outbound encoder bytes through `MavlinkTransport` seam; closes AZ-401 AC-9 deferred during batch 61 due to encoder-side routing not being in the AZ-401 task envelope; earlier same-day after cumulative review batches 52-54: AZ-528 hygiene PBI added for c1_vio strategy facade orchestration-spine 3-way duplication (Medium); earlier same-day after Batch 53: AZ-333 VINS-Mono landed — first c1_vio strategy after the AZ-332 OKVIS2 production-default; consolidation hygiene for the strategy-facade duplication deferred to a post-AZ-334 PBI; earlier same-day after Batch 51: AZ-527 hygiene PBI added from cumulative review batches 49-51 F1; 2026-05-13: AZ-526 hygiene PBI added from cumulative review batches 46-48 F1+F3; same-day refresh after Batch 44 SRP refactor: AZ-317 superseded; AZ-329 + AZ-330 specs rewritten; AZ-523 + AZ-524 audit-trail tickets added; E-C12 epic renamed `Operator Pre-flight Tooling` → `Operator Pre-flight Orchestrator`; earlier same-day refresh: AZ-507 + AZ-508 hygiene PBIs from cumulative review batches 31-33; 2026-05-11: AZ-489 + AZ-490 ADR-010 operator-origin path)
|
||||
**Date**: 2026-05-16 (refreshed at end of Batch 64: AZ-558 implementation closed — `MavlinkTransport` seam now routes every C8 outbound MAVLink byte; AZ-401 AC-9 + AZ-404 AC-4b unskipped together; encoder helpers extracted to `_outbound_mavlink_payloads.py`; live-mode `compose_root` injection deferred to whichever future batch registers AP/iNav strategies in an airborne binary; earlier 2026-05-14: refreshed at start of Batch 63: AZ-559 closed Won't Fix — gap was illusory; `TileSource.ONBOARD_INGEST` + `TileMetadata.quality_metadata` + `write_tile`'s `FreshnessRejectionError` already cover the AZ-389 mid-flight ingest semantic without any new API; AZ-389 dep restored to AZ-303; earlier same-day after Batch 61: AZ-558 follow-up added — routes C8 outbound encoder bytes through `MavlinkTransport` seam; closes AZ-401 AC-9 deferred during batch 61 due to encoder-side routing not being in the AZ-401 task envelope; earlier same-day after cumulative review batches 52-54: AZ-528 hygiene PBI added for c1_vio strategy facade orchestration-spine 3-way duplication (Medium); earlier same-day after Batch 53: AZ-333 VINS-Mono landed — first c1_vio strategy after the AZ-332 OKVIS2 production-default; consolidation hygiene for the strategy-facade duplication deferred to a post-AZ-334 PBI; earlier same-day after Batch 51: AZ-527 hygiene PBI added from cumulative review batches 49-51 F1; 2026-05-13: AZ-526 hygiene PBI added from cumulative review batches 46-48 F1+F3; same-day refresh after Batch 44 SRP refactor: AZ-317 superseded; AZ-329 + AZ-330 specs rewritten; AZ-523 + AZ-524 audit-trail tickets added; E-C12 epic renamed `Operator Pre-flight Tooling` → `Operator Pre-flight Orchestrator`; earlier same-day refresh: AZ-507 + AZ-508 hygiene PBIs from cumulative review batches 31-33; 2026-05-11: AZ-489 + AZ-490 ADR-010 operator-origin path)
|
||||
**Total Tasks**: 150 (109 product + 41 blackbox-test) — AZ-317 retained in the table marked SUPERSEDED for audit; AZ-523 (C11 gate removal) + AZ-524 (C12 rename) added as 2 closed audit-trail tasks; AZ-526 = 2pt clock-helper hygiene; AZ-527 = 2pt c2 engine-dim helper hygiene; AZ-528 = 3pt c1_vio facade-spine hygiene; AZ-558 = 3pt MavlinkTransport routing follow-up; AZ-559 closed Won't Fix
|
||||
**Total Complexity Points**: 497 (364 product + 133 blackbox-test) — AZ-523 = 3pt, AZ-524 = 2pt, AZ-526 = 2pt, AZ-527 = 2pt, AZ-528 = 3pt, AZ-558 = 3pt
|
||||
|
||||
|
||||
@@ -0,0 +1,88 @@
|
||||
# Code Review Report — Batch 64
|
||||
|
||||
**Batch**: 64
|
||||
**Tasks**: AZ-558 (Route C8 outbound encoder bytes through MavlinkTransport seam — closes AZ-401 AC-9)
|
||||
**Date**: 2026-05-16
|
||||
**Verdict**: PASS_WITH_WARNINGS
|
||||
|
||||
## Summary
|
||||
|
||||
Batch 64 retrofitted the C8 outbound MAVLink path to route every byte through the `MavlinkTransport` Protocol seam introduced by AZ-401. The retrofit closes two previously-deferred gates in one cycle: AZ-401 AC-9 (`NoopMavlinkTransport.bytes_written() > 0`) and AZ-404 AC-4b (encoder byte-equality between live and replay paths).
|
||||
|
||||
Six AC tests landed (4 byte-equivalence + 3 AST-scan + 1 AC-9 unskip + 1 AZ-404 e2e AC-4b unskip). Existing 4 unit-test files for AP / iNav / signing / source-set-switch adapters were updated to support the new `encode → pack → transport.write` flow without changing their assertion shape (encode methods record the same args the previous `*_send` methods recorded).
|
||||
|
||||
Full regression suite: 2110 passed, 92 environmental skips, 1 deselected pre-existing macOS-dev cold-start flake (`test_cli_console_script.py::TestConsoleScript::test_cold_start_under_500ms_p99` — unrelated to this batch's surface).
|
||||
|
||||
## Spec Compliance — AZ-558
|
||||
|
||||
| AC | Spec | Test(s) | Status |
|
||||
|---|---|---|---|
|
||||
| AC-1 | AP / iNav constructors accept transport kwarg; replace every `mav.*_send` | `test_az393_ardupilot_outbound.py` (11) + `test_az394_inav_outbound.py` (11) — assertions on the same `*_calls` lists, now populated through the encoder seam | PASS |
|
||||
| AC-2 | Wire-byte equivalence (live mode) | `test_az558_outbound_transport_seam.py::test_ac2_byte_equivalence_*` (gps_input, named_value_float, statustext, multi-msg seq alignment) — 4 tests | PASS |
|
||||
| AC-3 | Replay FC adapter produces bytes via transport | `test_az401_compose_root_replay.py::test_ac9_noop_transport_bytes_written_after_runtime_drive` — 10 ticks × 2 messages → `bytes_written() > 0` | PASS |
|
||||
| AC-4 | AZ-401 AC-9 unskips | Same test as AC-3, no longer `@pytest.mark.skip` | PASS |
|
||||
| AC-5 | No `.mav.<name>_send(` AST nodes in retrofitted adapters | `test_az558_outbound_transport_seam.py::test_ac5_no_pymavlink_send_helpers_in_adapter_source` — 3 parametrised files (AP / iNav / tlog) | PASS |
|
||||
| AC-6 | `compose_root` injects transport (live + replay) | Replay path fully wired (`_replay_branch.py` builds transport before bundle, threads through `ReplayInputAdapter` → `TlogReplayFcAdapter`); see findings F4 for live mode | PASS_WITH_NOTE |
|
||||
|
||||
**Bonus closure**: AZ-404 AC-4b unskipped via `test_derkachi_1min.py::test_ac4_encoder_byte_equality_via_transport_seam` (constructive equivalence between `MAVLink.send` and `encode → pack → transport.write` paths against the same MAVLink instance).
|
||||
|
||||
## Findings
|
||||
|
||||
| # | Severity | Category | File:Line | Title |
|
||||
|---|----------|----------|-----------|-------|
|
||||
| 1 | Low | Maintainability | `runtime_root/_replay_branch.py`; `replay_input/tlog_video_adapter.py` | `mavlink_transport: Any` typing too loose; should be Protocol-typed |
|
||||
| 2 | Low | Maintainability | `pymavlink_ardupilot_adapter.py:_ConnectionWriteTransport`; `msp2_inav_adapter.py:_SecondaryWriteTransport` | Near-duplicate fallback transport classes |
|
||||
| 3 | Low | Maintainability | `pymavlink_ardupilot_adapter.py:_ConnectionWriteTransport.write` | Fallback transport does not type-check `payload` |
|
||||
| 4 | Low | Spec | live `compose_root` path | `SerialMavlinkTransport` injection point exists but no production binary registers AP/iNav strategies yet |
|
||||
|
||||
### Finding Details
|
||||
|
||||
**F1: `mavlink_transport: Any` typing too loose** (Low / Maintainability)
|
||||
- Location: `src/gps_denied_onboard/runtime_root/_replay_branch.py:_build_replay_input_bundle`; `src/gps_denied_onboard/replay_input/tlog_video_adapter.py:ReplayInputAdapter.__init__`
|
||||
- Description: The `mavlink_transport` parameter on the replay coordinator path is typed `Any` to avoid a `replay_input → c8_fc_adapter` import. The Protocol type would be more honest.
|
||||
- Suggestion: Either import `MavlinkTransport` under `if TYPE_CHECKING:` or move the Protocol definition to a `_types/` module the replay coordinator can already see. Defer until the import-direction concern can be evaluated against `module-layout.md` — leaving `Any` is consistent with the existing `tlog_source_factory: Any | None` patterns in the same constructor.
|
||||
|
||||
**F2: Duplicate fallback transport classes** (Low / Maintainability)
|
||||
- Location: `src/gps_denied_onboard/components/c8_fc_adapter/pymavlink_ardupilot_adapter.py:_ConnectionWriteTransport`; `src/gps_denied_onboard/components/c8_fc_adapter/msp2_inav_adapter.py:_SecondaryWriteTransport`
|
||||
- Description: Both classes implement the same fallback `MavlinkTransport` shape (write through the wrapped object's `.write`, count bytes, drop on close). The only behavioural difference is iNav's tolerance for the secondary connection lacking a `write` attribute (it silently counts the would-be byte length).
|
||||
- Suggestion: Extract into a shared `_outbound_fallback_transport.py` module within `c8_fc_adapter/` once a third caller appears. With only two, the duplication cost is lower than the indirection cost.
|
||||
|
||||
**F3: Fallback transport does not type-check `payload`** (Low / Maintainability)
|
||||
- Location: `src/gps_denied_onboard/components/c8_fc_adapter/pymavlink_ardupilot_adapter.py:_ConnectionWriteTransport.write`
|
||||
- Description: Production `SerialMavlinkTransport.write` rejects non-bytes-like inputs with `MavlinkTransportError`. The fallback variant does not. The fallback is reachable only when no transport factory is injected (test paths and one-off callers).
|
||||
- Suggestion: Either propagate the `SerialMavlinkTransport` validation or document the fallback as test-only. Since the production composition root will inject a real transport, the practical impact is zero — defer.
|
||||
|
||||
**F4: Live `compose_root` does not yet inject `SerialMavlinkTransport`** (Low / Spec)
|
||||
- Location: live `compose_root` path
|
||||
- Description: The retrofit defines the `mavlink_transport_factory` kwarg on `PymavlinkArdupilotAdapter` and the `secondary_mavlink_transport_factory` kwarg on `Msp2InavAdapter`, but no production binary currently calls `register_fc_adapter("ardupilot_plane", ...)` or `register_fc_adapter("inav", ...)`. The live-mode injection path is therefore latent — exercised only by unit tests (which use the in-class fallback transport).
|
||||
- Suggestion: When the airborne binary bootstrap registers the AP / iNav strategies (a future batch), the registration site MUST pass `mavlink_transport_factory=lambda conn: SerialMavlinkTransport(conn)`. Add an architecture-test entry to `module-layout.md` or to a binary-bootstrap test once the registration lands. Tracked here as documentation; no blocking impact on AZ-558's primary outcome (replay-mode AC-9 closure).
|
||||
|
||||
## Code Quality Observations
|
||||
|
||||
- **SOLID**: the encode helpers (`_outbound_mavlink_payloads.py`) are pure functions with single responsibility (one MAVLink message kind each) plus one orchestrator (`send_via_transport`). The AP / iNav / tlog adapters retain their existing responsibility shape; the retrofit is purely additive at the call-site level.
|
||||
- **Tests**: every existing AP / iNav assertion still holds without change. The hybrid `_FakeMsg` pattern in the test stubs preserves the assertion surface while routing through the new code path — minimal blast radius.
|
||||
- **Architecture**: the new `_outbound_mavlink_payloads` module lives inside `c8_fc_adapter/` and is consumed only by adapters in the same component. No new cross-component imports introduced.
|
||||
- **Determinism**: `send_via_transport` snapshots `mav.seq` into `msg._header.seq` (via `pack`) BEFORE bumping. Two MAVLink instances with identical state produce byte-identical output — this is the constructive proof underlying AC-2.
|
||||
|
||||
## Security
|
||||
|
||||
No new attack surface. The retrofit changes the byte path, not the byte content; signing is preserved (consulted by `MAVLink_message._pack` from `mav.signing.sign_outgoing`). No subprocess, no external input, no file I/O changes.
|
||||
|
||||
## Performance
|
||||
|
||||
One additional method dispatch (`encode`, `pack`, `transport.write`) per MAVLink message versus the prior `mav.*_send`. At a 10 Hz emit rate this is negligible. The composition-root NFR (`compose_root` p99 ≤ 1 s) is not affected — transport construction is constant-time.
|
||||
|
||||
## Cumulative Architecture Notes
|
||||
|
||||
- `_replay_branch.py` now constructs the transport BEFORE the FC adapter and threads it down through `ReplayInputAdapter` (which threads to `TlogReplayFcAdapter`). The dependency direction is correct: `runtime_root → replay_input → c8_fc_adapter`.
|
||||
- AC-5's AST scan is parametric over `_RETROFITTED_FILES`; adding a new outbound MAVLink file requires updating that list. Document this in the retrofit's CONTRIBUTING note when future maintainers add a fourth outbound MAVLink emitter (e.g., the GCS adapter, currently still using `mav.*_send` directly per its task scope).
|
||||
|
||||
## Verdict Rationale
|
||||
|
||||
PASS_WITH_WARNINGS: zero Critical / High findings. All six ACs of AZ-558 demonstrably satisfied with traceable test coverage. The four Low findings are documented opportunities for future refinement, none blocking on the AZ-558 outcome.
|
||||
|
||||
## Action Items (Carried to Future Batches)
|
||||
|
||||
1. **Future**: when an airborne binary bootstrap registers the AP / iNav strategies, the registration MUST pass `mavlink_transport_factory=lambda conn: SerialMavlinkTransport(conn)` (F4).
|
||||
2. **Hygiene** (low priority): unify `_ConnectionWriteTransport` and `_SecondaryWriteTransport` into a shared fallback module if a third outbound adapter requires the same pattern (F2).
|
||||
3. **Out of scope for AZ-558**: the GCS adapter (`mavlink_gcs_adapter.py`) still calls `mav.*_send` directly. AZ-558's spec scoped only AP / iNav / replay-FC; the AC-5 AST scan reflects that scope. A follow-up PBI is appropriate when the GCS adapter is wired into a binary.
|
||||
@@ -12,7 +12,7 @@ sub_step:
|
||||
retry_count: 0
|
||||
cycle: 1
|
||||
tracker: jira
|
||||
last_completed_batch: 63
|
||||
last_completed_batch: 64
|
||||
last_cumulative_review: batches_61-63
|
||||
current_batch: 64
|
||||
current_batch: 65
|
||||
current_batch_tasks: ""
|
||||
|
||||
Reference in New Issue
Block a user