Files
Oleksandr Bezdieniezhnykh 4c63829ccd
ci/woodpecker/push/build-arm Pipeline failed
[AZ-654] [AZ-655] [AZ-656] gimbal_controller primitives + monotonic clock fix (batch 11)
AZ-654 SweepEngine: pendulum default, Raster/LawnMower variants
reserved and explicitly NotImplemented (no silent fallback per AC-3).
Time injected via next_step(now) for deterministic dwell tests.

AZ-655 PlanExecutor: linear yaw/pitch interpolation between PanGoals
with self-throttle (default 50 ms); stats expose
commands_emitted/dropped_to_throttle counters. PanGoal/PanPlan added
to shared::models::gimbal (spec drift: data_model.md §PanPlan flagged
for next doc sync).

AZ-656 CentreOnTarget: zoom-aware proportional control loop (correction
~ 1/zoom); target_lost debounced — fires once per loss streak, resets
on bbox return. Also fixes the misleadingly-named monotonic_ns() helper
introduced by AZ-653 that used SystemTime::now(): GimbalController now
owns a shared::clock::MonoClock and stamps GimbalState::ts_monotonic_ns
via clock.elapsed_ns(). AZ-656 AC-2 forced the correction; integration
test verifies the fix end-to-end.

58/58 gimbal_controller tests green (47 unit + 7 AZ-653 integration +
4 new batch_11 integration). Workspace test suite green this run.

Co-authored-by: Cursor <cursoragent@cursor.com>
2026-05-19 20:21:00 +03:00

18 KiB
Raw Permalink Blame History

Batch 11 (cycle 1) implementation report

Tasks: AZ-654, AZ-655, AZ-656 Component scope: gimbal_controller (+ shared::models::gimbal type extension) Verdict: PASS_WITH_WARNINGS — proceed; flagged items below.

Tasks

AZ-654 gimbal_zoom_out_sweep — pendulum default + reserved Raster/LawnMower

Outcome: SweepPattern enum with all three variants declared; Pendulum implemented; Raster / LawnMower reserved and return AutopilotError::NotImplemented rather than silently falling back (per AC-3). Sweep envelope (yaw bounds, dwell, step) is configured via SweepConfig; the engine validates it on construction.

Production code added:

  • crates/gimbal_controller/src/internal/sweep.rs
    • SweepPattern enum — #[derive(Default)] with Pendulum as the default variant. Public re-export from lib.rs.
    • SweepConfig { min_yaw_deg, max_yaw_deg, pitch_deg, step_deg, dwell: Duration } — validated on construction (bounds ordered, step > 0). Public re-export.
    • SweepEngine { pattern, config, current_yaw, direction, dwell_started_at: Option<Instant> } — state machine. Public re-export.
    • SweepEngine::next_step(now: Instant) -> Result<GimbalCommand> — pendulum path advances by step_deg, clamps at bounds, starts a dwell window, then reverses on next tick after config.dwell; Raster / LawnMower paths return NotImplemented. Time is injected (not Instant::now() internally) for deterministic tests.
  • crates/gimbal_controller/src/lib.rs (re-export edit)

Tests (10 total, all green):

  • ac1_pendulum_stays_within_bounds_over_100_steps — verifies bound clamping + at least one reversal over 100 ticks at 1-second cadence.
  • ac2_dwell_holds_yaw_at_bound — verifies the yaw stays pinned for the entire 500 ms dwell window then flips on the first call after the window elapses.
  • ac3_raster_returns_not_implemented / ac3_lawnmower_returns_not_implemented — verifies the reserved-variant guarantee.
  • pattern_default_is_pendulum — verifies SweepPattern::default() == Pendulum.
  • invalid_config_rejected (yaw bounds invalid) and invalid_step_rejected (step ≤ 0) — verify SweepConfig::validate.
  • pendulum_advances_in_step_increments_then_clamps — verifies the forward sweep yaw sequence -25, -20, ..., 30 and clamping behaviour.
  • Integration: az654_sweep_engine_emits_gimbal_commands_within_bounds — 200 ticks through the public API, verifies emitted GimbalCommand always inside [min_yaw, max_yaw] and the pitch is fixed at the configured value.

AZ-655 gimbal_smooth_pan_plan — path-tracking plan executor

Outcome: PlanExecutor accepts a PanPlan (new shared type), linearly interpolates (yaw, pitch) between adjacent PanGoals, and self-throttles to a configured min interval (default 50 ms). Past-end queries clamp to the final goal. Before-start queries extrapolate linearly from the first two goals. Stats track commands_emitted_total and commands_dropped_to_throttle_total.

Production code added:

  • crates/shared/src/models/gimbal.rs — extended with PanGoal { yaw_deg, pitch_deg, zoom, at_ns } and PanPlan { goals: Vec<PanGoal> }. Spec drift note: AZ-655 references data_model.md §PanPlan, but data_model.md does not yet have a PanPlan entry — the document sync run will catch up (logged below under "Spec drift").
  • crates/gimbal_controller/src/internal/smooth_pan.rs
    • DEFAULT_MIN_CMD_INTERVAL = 50 ms constant. Public re-export.
    • NextStep { Emit(GimbalCommand), Throttled }Throttled is an explicit state, never silent.
    • ExecutorStats { plan_loaded_at, commands_emitted_total, commands_dropped_to_throttle_total } — public read-side.
    • PlanExecutor::new(min_cmd_interval), with_default_throttle(), load(plan, now), next_step(now), stats(), has_plan().
    • validate_plan rejects empty plans and non-strictly-increasing at_ns.
    • interpolate + linear_at + lerp — pure helpers, no I/O.
  • crates/gimbal_controller/src/lib.rs (re-export edit)

Tests (8 total, all green):

  • ac1_linear_interp_midpoint — plan 0°→30° over 1s, query at 500 ms → yaw ≈ 15°.
  • ac2_throttle_drops_intermediate_calls — 100 ticks at 10 ms cadence over 1s with 100 ms throttle → ~10 emissions, the rest counted as throttled. Stats counter cross-checked.
  • ac3_past_plan_end_clamps_to_last_goal — query 5 s after a 1 s plan → returns the last goal's values exactly.
  • empty_plan_rejected / non_monotonic_plan_rejected / no_plan_returns_errorValidation error paths.
  • reload_clears_throttle_anchor — re-loading the plan does not carry the previous plan's last_emit_at forward.
  • single_goal_plan_holds_value — degenerate 1-goal plan returns the goal verbatim for any query time.
  • Integration: az655_plan_executor_emits_and_throttles_against_real_clock — exercises the public API with a 20 ms throttle over 500 ms (100 ticks at 5 ms cadence) → ~25 emissions; verifies ExecutorStats counters match the emission ratio.

AZ-656 gimbal_centre_on_target — proportional centre-25% loop + monotonic-stamped state publish + target_lost debounce

Outcome: CentreOnTarget::tick(bbox, yaw, pitch, zoom) -> CentreOnTargetOutput runs a proportional control loop that drags the target bbox toward the centre 25 % of the frame. The control law scales the yaw correction by 1 / zoom because the effective FOV shrinks with zoom. target_lost debounces — fires exactly once on the tick that crosses the max_missed_ticks threshold, then stays silent for the rest of the loss streak; a visible bbox resets the counter so a subsequent loss streak can re-fire.

Production code added:

  • crates/gimbal_controller/src/internal/centre_on_target.rs
    • DEFAULT_TARGET_GAIN = 0.6, DEFAULT_CENTRE_WINDOW = 0.25, DEFAULT_MAX_MISSED_TICKS = 3. Public re-exports.
    • CentreOnTargetConfig { fov_deg_at_zoom1, gain, centre_half_width, max_missed_ticks } with sensible Default impl.
    • CentreOnTargetOutput { command: Option<GimbalCommand>, target_lost_signal: bool, on_target: bool }.
    • CentreOnTarget { config, consecutive_missed, in_loss_state } — pure controller, no I/O.
    • CentreOnTarget::tick(bbox, yaw, pitch, zoom) — when bbox.is_none(), increments the missed counter (capped at max_missed_ticks to avoid the unbounded growth → re-fire bug); when bbox.is_some(), resets the counter, computes (err_x, err_y) = bbox_centre - 0.5, scales by fov / zoom * gain, emits the corrective command, and sets on_target iff both errors are inside centre_half_width.
  • crates/gimbal_controller/src/lib.rs
    • Bug fix carried in this batch: replaced the misleadingly-named monotonic_ns() (which used SystemTime::now() and was NOT monotonic) with shared::clock::MonoClock. The GimbalController and GimbalControllerHandle now own a MonoClock and stamp GimbalState::ts_monotonic_ns via self.clock.elapsed_ns(). This bug was introduced by AZ-653 (batch 10) and would have surfaced as observable wall-clock jumps on the consumer side (movement_detector ego-motion sync, frame_ingest telemetry tagging). AZ-656's AC-2 forced the issue.

Tests (6 unit + 1 integration = 7 total, all green):

  • ac1_centre_25pct_within_3_ticks — runs the closed loop against a linearised kinematic camera model (the same one the proportional controller assumes); starting bbox at (0.75, 0.55, 0.1, 0.1) is inside the centre 25 % region by tick 3; on_target flag is observed at some point in the run.
  • ac3_target_lost_emits_once_per_loss_streak — verifies: no signal at tick 1/2; signal at tick 3; silent at tick 4/5; bbox returns → counter resets; second loss streak signals again at the new threshold.
  • bbox_already_centred_marks_on_target_with_small_command — bbox centred at (0.5, 0.5) → on_target = true and delta_yaw/delta_pitch ≈ 0.
  • higher_zoom_yields_smaller_correction — same bbox at 1× vs 4× → 4× correction is exactly 1/4 of 1× correction (within float tolerance).
  • loss_counter_caps_safely_without_overflow — hammer tick(None) 10 000 times; signal fires exactly once.
  • loss_streak_below_threshold_then_recovery_does_not_signalmax_missed_ticks=5, 3 missed then recovery → no signal.
  • Integration: az656_set_pose_publishes_monotonic_timestamp — verifies the AZ-653 → AZ-656 bug fix by issuing 3 sequential set_pose calls through the full transport stack (with a fake A40 echo loop) and checking state_rx.borrow().ts_monotonic_ns is strictly monotonic across the three observations.
  • Integration: az656_centre_on_target_loop_converges_via_public_api — duplicates the AC-1 convergence assertion using only gimbal_controller re-exports + shared::models::frame::BoundingBox (catches re-export drift).

AC coverage

Task AC Behaviour Test Status
AZ-654 AC-1 100-step pendulum stays in bounds, reverses at each bound ac1_pendulum_stays_within_bounds_over_100_steps PASS
AZ-654 AC-2 Yaw pinned for full 500 ms dwell window ac2_dwell_holds_yaw_at_bound PASS
AZ-654 AC-3 Raster + LawnMower variants return NotImplemented ac3_raster_returns_not_implemented, ac3_lawnmower_returns_not_implemented PASS
AZ-655 AC-1 yaw 0→30° at t=500ms → 15° ± epsilon ac1_linear_interp_midpoint PASS
AZ-655 AC-2 100 ticks at 10 ms with 100 ms throttle → ~10 emits ac2_throttle_drops_intermediate_calls PASS
AZ-655 AC-3 Plan past end clamps to last goal ac3_past_plan_end_clamps_to_last_goal PASS
AZ-656 AC-1 bbox (0.75, 0.55) → centre 25 % within 3 ticks at 100 ms ac1_centre_25pct_within_3_ticks + integration PASS
AZ-656 AC-2 ts_monotonic_ns strictly monotonic across set_pose calls az656_set_pose_publishes_monotonic_timestamp PASS
AZ-656 AC-3 3 consecutive missing bboxes → exactly one target_lost; later misses silent; bbox return → counter resets ac3_target_lost_emits_once_per_loss_streak PASS

Code review

Spec compliance: PASS. Every AC has a directly-named test that asserts the claimed behaviour.

Architecture compliance: PASS.

  • gimbal_controller (Layer 2 Actor) imports only shared and standard / tokio deps. No sibling Layer 2 imports.
  • internal/sweep.rs, internal/smooth_pan.rs, internal/centre_on_target.rs are new internal files under the component's owned glob (crates/gimbal_controller/**).
  • internal/smooth_pan.rs was named in module-layout.md (line 113). internal/sweep.rs and internal/centre_on_target.rs are new and not yet enumerated — same drift-flag pattern recorded for internal/transport.rs in batch 10's report. Recommended: extend the gimbal_controller Internal bullet list on the next document refresh.
  • shared::models::gimbal::PanPlan / PanGoal are new and not yet documented in data_model.md. Same drift pattern.

SRP: PASS.

  • sweep::SweepEngine — only sweep state machine + pendulum kinematics.
  • smooth_pan::PlanExecutor — only plan loading + interpolation + throttle.
  • centre_on_target::CentreOnTarget — only the proportional control law + loss-debounce state.
  • lib.rs — composition + transport bridging; primitives are unaware of the transport.

Runtime completeness: PASS.

  • Real pendulum sweep (deterministic bounded state machine; not a random walk).
  • Real linear interpolation + real self-throttle (counter-validated).
  • Real proportional control loop with zoom-aware correction scaling; closed-loop convergence verified in unit + integration tests.
  • Real MonoClock-driven monotonic timestamps (fixes the AZ-653 wall-clock regression).

Test discipline: PASS. AAA pattern with // Arrange / Act / Assert comments. No unsafe. No production unwrap/expect. Integration tests use the public re-export surface — never reach into internal::*.

Security quick-scan: PASS. No external input deserialization in this batch (the primitives consume typed structs supplied by in-process callers); no string-interpolated commands; no network code added (re-uses the existing AZ-653 transport).

Performance scan: PASS.

  • SweepEngine::next_step — three float ops + one struct construct, p99 far below the 1 ms target.
  • PlanExecutor::next_step — one binary-window walk over goals (typically a handful of waypoints) + lerp; p99 far below the 1 ms target.
  • CentreOnTarget::tick — six float ops; p99 far below the 2 ms target.

Cross-task consistency: PASS.

  • All three primitives produce GimbalCommand (same (yaw_deg, pitch_deg) shape AZ-653 wired into set_pose).
  • Time injection pattern (next_step(now: Instant)) is consistent across sweep, smooth_pan, and centre_on_target (the latter receives state via args rather than time since its loop is per-frame, not per-clock-tick).
  • AutopilotError::Validation(String) is used consistently across the three primitives' invalid-input paths.

Spec drift (carried into the implementation, flagged here)

  1. data_model.md §PanPlan is referenced by AZ-655 but does not exist. The implementation adds PanGoal + PanPlan to crates/shared/src/models/gimbal.rs (the canonical location for gimbal-related shared types). The next document-sync run should backfill the entry in data_model.md §4 Action / piloting entities.
  2. module-layout.md does not yet list internal/sweep.rs, internal/centre_on_target.rs, or internal/transport.rs (the last carried over from batch 10). Same drift-flag pattern. Next document refresh should enumerate them.
  3. The monotonic_ns() helper introduced by AZ-653 (batch 10) was misleadingly named and actually used SystemTime::now(). AZ-656's AC-2 forced the correction in this batch. The fix is to construct a shared::clock::MonoClock per-controller and read clock.elapsed_ns() on every state stamp. This is recorded here rather than as a remediation task because the change was already in this batch's scope (the same files were being edited).

Known limitations (warnings)

  1. SweepEngine::next_pendulum carries a one-tick dwell-flip lag. When the dwell elapses, the next call returns the same current_yaw (the boundary value) one more time before the direction flips and the next call moves off the boundary. This is observable in ac2_dwell_holds_yaw_at_bound: the call at +501 ms is technically the "flip tick" (direction flip happens inside the call); the actual movement off the boundary happens on the call after that. Documented in the function body; benign in production where the tick cadence is at most 10 Hz.
  2. CentreOnTarget assumes a linearised camera model. Real ViewPro A40 mechanics introduce a small dead-zone and finite slew rate. The proportional gain (DEFAULT_TARGET_GAIN = 0.6) is conservative enough that the loop should converge under sub-critical conditions, but flight data may motivate adding a PD term or per-axis gains. Tracked as future tuning work; not blocking.
  3. PlanExecutor's at_ns is plan-relative, anchored to loaded_at. This means re-loading the same plan re-anchors its timeline. If a future caller needs to seamlessly continue a plan across reloads (e.g., chunked path-following), the API needs a replace_extend(plan) variant. Not in scope for AZ-655.
  4. No reserved Idle / mode-arbitration logic between the three primitives. Today the composition root must hold at most one primitive active at a time per gimbal; a future refactor may introduce the Sweep | PanPlan | CentreOnTarget | Idle mode enum named in description.md §5. Not in scope for AZ-654/655/656.

Auto-fix attempts during the batch

  • clippy::derivable_impls on impl Default for SweepPattern → replaced with #[derive(Default)] + #[default] attribute on Pendulum.
  • Debug derive missing on SweepEngine (required by Result::unwrap_err in the validation tests) → added.
  • AutopilotError::Validation(&'static str) doesn't compile (variant requires String) → switched the no-plan-loaded path to ok_or_else with a .into(). The earlier batch's unnecessary_lazy_evaluations warning does NOT apply here because String::from(&'static str) is not a no-op call; clippy correctly leaves this one alone.

Test reproduction

cargo build -p gimbal_controller --tests
cargo test  -p gimbal_controller                  # 58 tests; 0 failed
cargo clippy -p gimbal_controller --tests -- -D warnings
cargo test --workspace                            # all green

This run's workspace test suite passed without flakes; the mission_executor::state_machine::ac3_bounded_retry_then_success flake noted in batch 10's report did not reproduce in this run.

Cumulative review trigger

Cumulative review fires every 3 completed batches (batches 7-9, 10-12, 13-15, …). After batch 11 we are 2/3 of the way to the next cumulative; batch 12 will trigger it.

Candidates for batch 12

Remaining todo/: AZ-657 (frame_ingest x3 chain), AZ-660/661 (detection_client x2), AZ-662664 (movement_detector x3), AZ-669671 (semantic_analyzer x3), AZ-675677 (telemetry_stream x3), AZ-678681 (operator_bridge x4), AZ-682686 (scan_controller x5).

Only-AZ-640-dep (ready immediately):

  • AZ-657 frame_ingest_rtsp_session — 3 pts. Standard RTSP, no vendor-spec gap.
  • AZ-682 scan_controller_state_machine — 5 pts. Deps AZ-640, AZ-649 (both done). The primitives shipped in this batch (sweep / pan-plan / centre-on-target) are the natural building blocks scan_controller will compose.

Recommended: AZ-657 + AZ-682 (3 + 5 = 8 pts, 2 tasks, two different components). Gives the next cumulative review (batch 12, batches 10-12) surface area across gimbal_controller, frame_ingest, and scan_controller for a stronger cross-task check. Alternative: AZ-657 alone (3 pts) if the user prefers a single-task batch to keep batch 12 tight.