-
Notifications
You must be signed in to change notification settings - Fork 1
feat: ugv_strike vehicle profile for full-capability UGV testing #317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -293,6 +293,32 @@ autonomous.platform_role = ground_isr | |
| autonomous.safe_mode = HOLD | ||
| autonomous.default_features = detect,mavlink,tak_output,logging | ||
|
|
||
| [vehicle.ugv_strike] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Useful? React with 👍 / 👎. |
||
| ; Vehicle profiles do NOT inherit from each other — bootstrap.py reads only | ||
| ; the one [vehicle.<name>] section named by HYDRA_VEHICLE. reserved_channels | ||
| ; is a vehicle-LOCAL key (no section prefix); omitting it here would skip the | ||
| ; servo/light-bar channel-conflict check entirely. 1,3 are the truck's | ||
| ; steering and throttle. | ||
| reserved_channels = 1,3 | ||
| autonomous.post_drop_mode = SMART_RTL | ||
| autonomous.default_features = detect,mavlink,tak_output,logging,autonomy,servo | ||
| autonomous.enabled = true | ||
| autonomous.geofence_lat = 35.15 | ||
| autonomous.geofence_lon = -79.52 | ||
| autonomous.geofence_radius_m = 25000 | ||
| autonomous.allowed_classes = truck, car | ||
| autonomous.allowed_vehicle_modes = GUIDED | ||
| autonomous.require_operator_lock = true | ||
| autonomous.arm_channel = 0 | ||
| autonomous.hardware_arm_channel = 0 | ||
| autonomous.post_strike_mode = HOLD | ||
| autonomous.safe_mode = HOLD | ||
| autonomous.platform_role = ground_isr | ||
| servo_tracking.enabled = true | ||
| approach.strike_approach_m = 0.5 | ||
| approach.abort_mode = HOLD | ||
| approach.waypoint_interval = 0.1 | ||
|
|
||
| [vehicle.drone_10in] | ||
| reserved_channels = 1,2,3,4 | ||
| autonomous.post_drop_mode = DOGLEG_RTL | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,129 @@ | ||
| # Adversarial review — PR #317 (ugv_strike vehicle profile) | ||
|
|
||
| Touched adversarial path: `hydra_detect/pipeline/facade.py`. | ||
|
|
||
| Reviewer: Claude (orchestrator). Implementation drafted by Codex (gpt-5.6-sol), | ||
| reviewed and corrected here. Codex could not execute Python in its environment, | ||
| so **every test result in this report was produced by the reviewer**, not the | ||
| implementer. All red-green claims below were observed directly. | ||
|
|
||
| ## Round 1 — behavior change in the autonomous strike path | ||
|
|
||
| The PR rewires which callback `AutonomousController.evaluate()` receives. That | ||
| is the callback that fires a real engagement, so the review started there. | ||
|
|
||
| **Finding 1.1 (CONFIRMED, fixed): strike re-entry produces false failure records.** | ||
|
|
||
| `autonomous.py` updates `_last_strike_time` only when `strike_cb` returns | ||
| truthy (line ~482). The cooldown gate (line ~241) is the only re-entry control. | ||
| `_handle_approach_strike_command` returned `False` whenever | ||
| `approach.mode != IDLE`. | ||
|
|
||
| Sequence on the new profile: | ||
| 1. Gates pass, `start_strike()` succeeds, returns True, cooldown starts (30 s). | ||
| 2. Cooldown correctly suppresses for 30 s. | ||
| 3. Cooldown expires while the approach is *still* in `STRIKE` — normal for a | ||
| moving truck. Every qualifying frame now calls `strike_cb`, gets `False`, | ||
| never refreshes `_last_strike_time`, and emits | ||
| `AUTONOMOUS STRIKE FAILED` plus a reject decision record. | ||
|
|
||
| Impact: the audit log fills with strike failures during a *working* strike. | ||
| Anyone reading the audit trail afterward would conclude the engagement failed | ||
| repeatedly when it succeeded once and stayed active. | ||
|
|
||
| Fix: re-striking the track already under an active approach returns True | ||
| without restarting. A different track still returns False — that refusal is | ||
| load-bearing and was preserved. | ||
|
|
||
| Verified red: `test_same_active_strike_after_cooldown_is_not_callback_failure` | ||
| fails against pre-fix code with the predicted | ||
| `AUTONOMOUS STRIKE FAILED: track_id=7 (strike_cb returned False)` in the log. | ||
|
|
||
| ## Round 2 — what the profile silently fails to inherit | ||
|
|
||
| **Finding 2.1 (CONFIRMED, fixed): `reserved_channels` was dropped.** | ||
|
|
||
| `bootstrap.py` resolves `f"vehicle.{vehicle}"` and reads **only** that section. | ||
| Vehicle profiles do not inherit. `reserved_channels` is a vehicle-local key | ||
| (`_VEHICLE_LOCAL_KEYS`), read in `facade.py` with `fallback=""`. | ||
|
|
||
| Absence does not fail loudly — it skips the entire servo and light-bar | ||
| channel-conflict block. That block is what prints | ||
| `SAFETY: ... conflicts with reserved channels — servo tracking DISABLED` and | ||
| sets `_servo_tracker = None`. | ||
|
|
||
| `[vehicle.ugv]` declares `1,3`. The new profile omitted them while also setting | ||
| `servo_tracking.enabled = true`. On the truck, 1 is steering and 3 is throttle. | ||
|
|
||
| No live collision today (pan 10, strike 11), so this is a latent hole rather | ||
| than an active fault: the guard that would catch a future channel edit was not | ||
| running on this profile. Fixed by declaring `reserved_channels` explicitly, | ||
| pinned by a test that asserts parity with `[vehicle.ugv]`. | ||
|
|
||
| Verified red: commenting the line out fails | ||
| `test_ugv_strike_reserves_the_same_channels_as_ugv`. | ||
|
|
||
| **Finding 2.2 (considered, not a defect): `strike_approach_m = 0.5`.** | ||
| Schema minimum, closes to contact. Intended for this profile — the platform is | ||
| meant to physically reach the object. No payload and no arm channel is wired | ||
| (`arm_channel = 0`), so `start_strike()` skips its software-arm servo write. | ||
|
|
||
| ## Round 3 — the new selector as a failure surface | ||
|
|
||
| `_autonomous_strike_handler()` is invoked inside the detection loop on every | ||
| frame. Anything that can raise there stops detection. | ||
|
|
||
| **Finding 3.1 (CONFIRMED, fixed): loop-killing `AttributeError`.** | ||
|
|
||
| The drafted selector read `self._vehicle` directly. `_refuse_approach_for_fw` | ||
| in the same file uses `getattr(self, "_vehicle", None)` with a comment stating | ||
| that pre-existing fixtures construct `Pipeline` by patching `__init__` and | ||
| never set `_vehicle`. The new code did not honor that established contract, | ||
| in a hotter code path than the one that documented it. | ||
|
|
||
| Fixed with the same idiom; missing profile falls back to the one-shot handler, | ||
| which is pre-existing behavior for every other profile. | ||
|
|
||
| Verified red: `test_strike_handler_survives_pipeline_without_vehicle_attr` | ||
| raises `AttributeError` against the naive version. | ||
|
|
||
| **Finding 3.2 (CONFIRMED, fixed): case-sensitive profile match.** | ||
|
|
||
| `_is_fw_profile` normalizes with `.strip().lower()`. The drafted selector used | ||
| `==` against a literal. `HYDRA_VEHICLE=UGV_STRIKE` would silently select the | ||
| one-shot handler — a quiet downgrade of the engagement path, not an error. | ||
| Now normalized through `_is_ugv_strike_profile`, matching the file's contract. | ||
|
|
||
| Verified red: `test_strike_handler_profile_match_is_case_insensitive`. | ||
|
|
||
| **Finding 3.3 (accepted, not fixed): TOCTOU between two property reads.** | ||
|
|
||
| `_handle_approach_strike_command` reads `self._approach.mode` and | ||
| `self._approach.target_track_id` under two separate lock acquisitions. The mode | ||
| can change between them. Both outcomes are benign: either the idempotent True | ||
| (approach just completed, cooldown refreshes, next frame re-evaluates) or fall | ||
| through to the existing `!= IDLE` refusal. Fixing it would mean a combined | ||
| locked accessor on `ApproachController`, widening that class's public surface | ||
| for no behavioral gain. Documented rather than changed. | ||
|
|
||
| ## Pre-existing issue surfaced, deliberately not fixed here | ||
|
|
||
| `autonomous.notify_strike_complete()` (autonomous.py:565) clears | ||
| `_strike_in_progress`. Repository-wide search finds **no callers**, and | ||
| `_strike_in_progress` is never read as a gate — it is set at line 478 and | ||
| otherwise write-only. The cooldown is the only real re-entry control, which is | ||
| precisely why finding 1.1 had teeth. Wiring a completion signal is a larger | ||
| change to shared autonomous logic affecting every profile; out of scope for a | ||
| profile addition. Should be filed. | ||
|
|
||
| ## Verification summary | ||
|
|
||
| - `2654 passed`, flake8 clean on `hydra_detect/ tests/`. | ||
| - 19 failures / 10 errors in `test_ota_verify_pull.py` and | ||
| `test_tak_pytak_emitter.py` are environment-only (missing `pytak` on the | ||
| Windows dev box) and reproduce with identical counts on a stashed clean tree. | ||
| - Two tests in `TestUGVStrikePipeline` are `fcntl`-gated and skip on Windows; | ||
| they execute in Linux CI only and were not observed locally. Flagged rather | ||
| than claimed. | ||
| - `_eval_drop` and `_eval_rf_hunt` unmodified; drop and RF hunt remain blocked | ||
| on #147. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| # UGV strike profile | ||
|
|
||
| ## Select the profile | ||
|
|
||
| On each kit, set `/etc/hydra/vehicle.env` to: | ||
|
|
||
| ```sh | ||
| HYDRA_VEHICLE=ugv_strike | ||
| ``` | ||
|
|
||
| Then reload the service: | ||
|
|
||
| ```sh | ||
| sudo systemctl restart hydra-detect | ||
| ``` | ||
|
|
||
| The profile enables autonomous evaluation, servo tracking, truck/car target | ||
| selection, GUIDED operation, operator-selected target locking, a 25 km circular | ||
| geofence centered at 35.15, -79.52, and continuous strike approach updates at | ||
| 0.1-second intervals to a 0.5 m close-approach estimate. Arm channels remain | ||
| disabled. Post-strike, abort, and autonomous fallback modes are HOLD. | ||
|
|
||
| ## Profiles do not inherit | ||
|
|
||
| `hydra_detect/pipeline/bootstrap.py` reads only the single `[vehicle.<name>]` | ||
| section named by `HYDRA_VEHICLE`. `[vehicle.ugv_strike]` gets nothing from | ||
| `[vehicle.ugv]`. Any key you need must be restated in full. | ||
|
|
||
| This matters most for `reserved_channels`, which is a vehicle-local key with no | ||
| section prefix. `facade.py` reads it with `fallback=""`, so a missing value | ||
| skips the servo and light-bar channel-conflict check rather than failing | ||
| loudly. On the truck, channels 1 and 3 are steering and throttle. | ||
|
|
||
| ## Capability preconditions | ||
|
|
||
| - **Follow** checks MAVLink connectivity, GPS fix type 3 or higher, and a locked | ||
| track in `hydra_detect/capability_status.py::_eval_follow`. | ||
| - **Servo Tracking** checks that a servo controller has claimed its channel. A | ||
| claimed controller with no locked track reports WARN; a claimed controller | ||
| with a locked track reports READY. The check is in | ||
| `hydra_detect/capability_status.py::_eval_servo_tracking`. | ||
| - **Autonomy Live** checks MAVLink connectivity, GPS fix type 3 or higher, a | ||
| valid configured geofence, `autonomous.enabled=true`, and operating mode | ||
| `ARMED`. The check is in | ||
| `hydra_detect/capability_status.py::_eval_autonomy_live`. | ||
|
|
||
| ## Strike paths | ||
|
|
||
| `hydra_detect/pipeline/facade.py::_handle_strike_command` sends one GUIDED | ||
| waypoint at the estimated target GPS position. Other profiles retain this | ||
| one-shot autonomous path. | ||
|
|
||
| `hydra_detect/pipeline/facade.py::_handle_approach_strike_command` calls | ||
| `approach.start_strike()` and enters `ApproachMode.STRIKE`. That path updates a | ||
| GUIDED waypoint continuously from the current locked track and commands twice | ||
| the configured follow maximum speed. Use the continuous path for a moving | ||
| target. | ||
|
|
||
| For `ugv_strike`, the autonomous controller is wired to the continuous handler | ||
| by `hydra_detect/pipeline/facade.py::_autonomous_strike_handler`. | ||
|
|
||
| Payload drop and RF hunt remain unconditionally BLOCKED in | ||
| `hydra_detect/capability_status.py::_eval_drop` and `_eval_rf_hunt` on issue | ||
| #147. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new UGV profile does not set
shared_battery = true. Because vehicle profiles do not inherit andPipelinedefaults_shared_batteryto false when the active[vehicle.<name>]section omits the key, shared-pack UGV/truck kits selected withHYDRA_VEHICLE=ugv_strikewill not wire the BatteryMonitor LOW-transition callback, so a low propulsion pack only gets the STATUSTEXT path instead of commanding HOLD and safing the servo tracker like the existing[vehicle.ugv]profile. Addshared_battery = truehere unless this strike truck has a separate companion battery.Useful? React with 👍 / 👎.