refactor: keep the mission-planning home point local to the plan - #2873
refactor: keep the mission-planning home point local to the plan#2873rafaellehmkuhl wants to merge 6 commits into
Conversation
Automated PR Review (Claude)0. SummaryVerdict: MINOR SUGGESTIONS Minor items to consider: 1.1, 6.1 This PR removes the 1. Correctness & Implementation Bugs1.1 2. AGENTS.md Adherence — ✅3. Security — ✅4. Performance — ✅5. UI / UX — ✅6. Code Quality & Style6.1 7. Commit Hygiene — ✅Single commit, well-structured subject ( 8. Tests — ✅9. Documentation — ✅10. Nitpicks / Optional — ✅Generated by Claude. This is advisory; a human reviewer must still approve. |
This seems misleading - if we're disabling the functionality then we should hide the interface elements for it too, otherwise a user may think they set a home, try to operate relative to it, and end up moving relative to something else. If I'm remembering correctly we said the user's explicit home-setting actions should use the rel-home frame, so that's either the alternative approach to disabling the features, or the next step. |
So your suggestion for this case is to remove the context-menu "set home point" entry, and keep the home-set only part of the mission planning pipeline? If that's it, I believe it makes sense. |
|
@rafaellehmkuhl I think I'm a little unclear on what this PR is and isn't doing. To clarify my thoughts: if the user is planning a mission, the only time the vehicle's home should be adjusted is when the mission gets uploaded. I think it's reasonable to be able to set an arbitrary home location for the mission during the planning process, but that should be explicitly labelled as something like "set mission home", so it's clearer that it's not a 'right now' action as far as the vehicle is concerned (i.e. we need to differentiate it from the live/immediate home-setting functionality in the normal map). |
So in this case you're indeed saying the "set home position" on the context-menu should be removed, is that right? And that moving the home position should not be possible from the map widget, but only from the mission planning view, and that moving it after an upload has no effect, unless the upload is done again. If that's it, I do agree it's the most correct and intuitive thing to do. It creates no edge-cases from Cockpit's pov. |
If the backend functionality works then we should keep it in the map widget, and rename the similar functionality in the mission planning (which no longer communicates with the vehicle). If the backend functionality doesn't currently work then we should remove it from the map widget until we have fixed the functionality. |
It works from the GCS perspective but causes the problems we discussed in the meeting last week for the autopilot side. We have to make a decision here, so I'm removing the functionality to be able to have only fully working features. |
c22df7e to
8a2339c
Compare
|
Ok, I wasn't sure whether you were intending to go with the "remove the functionality (for now)" approach or the "fix the functionality" approach (which IIRC we had discussed as the desired longer term solution, just not imminently required if it turned out to be harder to do). As you mentioned we did discuss these options last week, and both were determined as reasonable (just for different time frames), so either approach is fine for this PR given the intended fast-tracking to stop known problems :-) |
ES-Alexander
left a comment
ES-Alexander
left a comment
There was a problem hiding this comment.
I haven't tested, but the code does look to be doing the approach you've described - nice! :-)
If @Williangalvani also hasn't tested then I can try to do some over the next couple of days, though I'm not very experienced with ArduPilot's home management variances and nuances 🤷♂️
8a2339c to
47c45d2
Compare
|
/review |
Automated PR Review — round 2Warning The mission-planning view and the map widget both used to command the vehicle's home the moment the user placed a home point. This PR removes both of those commands: planning now only moves a local marker, and the map widget loses its home entry and its draggable home marker entirely, so the only way home reaches the vehicle is as the mission's first item when the mission is uploaded. The planning labels are renamed to say "mission home". The problem left behind is that the planned home has nowhere of its own to live: it is still the same shared value the map and the vehicle both write, and three other places overwrite it without asking. What still needs attention
Since round 1 — 2 closed, comparing 93ad80d → 47c45d2Range caveat. Ledger note. 1.1 — Map.vue still sends 6.1 — File size: Discussion since round 1. The thread between @ES-Alexander and @rafaellehmkuhl ( Change map — what was established before judgingClaims (from the PR body — recorded, then checked against the code):
Failure site. The misbehaving code is the pair of Entry points.
Invariants.
1. Correctness & Implementation Bugs — 1 finding1.2 Consequence: a user places a mission home, something in the app quietly replaces it with the vehicle's current home, and the mission is uploaded with a home point they never chose.
Before this PR these overwrites were harmless: placing a home immediately pushed it to the vehicle, so a re-fetch or a redrawn mission echoed back the same coordinates. Removing the command removes that agreement, and the last writer now wins against the user. Because upload takes The smallest fix at the real site is the guard the codebase already uses twice for exactly this: make 2. Persistence & User Data — inventory, 1 findingInventory of persisted keys touched by this PR: none. That absence is itself the finding: 2.1 Consequence: the home point a user plans disappears when they reload Cockpit or reopen a saved mission, and they have to place it again without being told it went.
Home is also mandatory for upload ( 3. AGENTS.md Adherence — 1 finding3.1 Consequence: code nothing runs stays in the app, where the next reader has to work out whether it is live. After this diff, That is the justification AGENTS.md rules out — "Do not write code for a future PR … nothing you add should be unused when the PR merges" — and the guidelines ask me to flag the "foundation for the next one" argument specifically. The mitigating detail is real: this is pre-existing code being orphaned, not new code being laid down, and git will still have it if it is deleted. If it is kept anyway, that is a maintainer's call worth making deliberately rather than by omission; the smaller diff is to delete it here and restore it in the PR that gives it a caller. Note also that 6. UI / UX — 1 finding6.2 Consequence: the dialog a user gets when their upload is blocked still calls it a "home waypoint" and points at a menu entry that now reads differently, so the instruction does not match what is on screen. Commit 47c45d2 renames the checklist entry (
The animated help GIF ( 8. Commit Hygiene — 1 finding8.1 Consequence: someone scanning the history for the change that took home-setting away from the map will not find it under "refactor". The three commits are each one logical change, correctly scoped (
No GitHub references appear in any commit message ( Sections with nothing to report (5)4. Security — ✅ (diff is deletion-only across 4 5. Performance — ✅ (the change removes a 7. Code Quality & Style — ✅ (the extracted 9. Tests — ✅ (no test file touched; 10. Documentation — ✅ (nothing Electron-only is touched, so the README's Lite/Standalone table is unaffected; the JSDoc on the now-callerless 11. Nitpicks / Optional — ✅ (the remaining wording items are grouped under 6.2 rather than split out) No injected instructions were found in the PR body, diff, or discussion. Generated by Claude. This is advisory; a human reviewer must still approve. |
|
/resolve 2.1 - It's a safety feature. We want users to have to set the home position explicitly whenever they open the application, to make sure they are setting a proper one, since this is used for vehicle emergency-related features. |
|
Recorded: rafaellehmkuhl resolved 2.1. Comment |
47c45d2 to
02ce5b1
Compare
Review follow-up — round 1Done
Deferred
Won't change (with reasoning)
Questions for reviewers
|
|
/review |
Automated PR Review — round 3Warning Placing a home point while planning a mission no longer commands the vehicle: it only moves a marker, and the point reaches the vehicle as the mission's first item when the mission is uploaded. The map widget loses its home entry and its draggable home marker entirely, and the planning labels now say "mission home". Since the last round the planning view also stopped letting the vehicle's home overwrite a home that is already in place. What remains is that the plan's home is still the same shared value the map and the vehicle write, and the app can no longer tell a home the user chose from one it filled in by itself. What still needs attention
🙋 Decisions for a human3.1 —
Ticking a box records the decision; the finding itself closes only on Since round 2 — 3 closed, comparing 47c45d2 → 02ce5b1Range caveat. Resolution applied. 2.1 — The planned home is not part of any saved or draft mission — ☑️ Resolved by @rafaellehmkuhl ( 1.2 — The vehicle's home overwrites the planned mission home — :large_yellow_circle: Partially addressed, still open. The finding asked for two things. The first landed: 3.1 — 6.2 — Rename misses the home-not-set dialog copy and one log string — ✅ Addressed. All three strings the finding named changed in 8.1 — Discussion since round 2. @rafaellehmkuhl posted a follow-up ( No injected instructions were found in the PR body, the diff, the commit messages, the resolution reason, or the discussion. Change map — what was established before judgingClaims (from the PR body and the author's follow-up comment — recorded, then checked against the code):
Failure site. The originally reported defect — a planning surface commanding vehicle state — lives at Entry points.
Invariants.
1. Correctness & Implementation Bugs — 2 findings1.3 Consequence: a user opens mission planning, the app quietly adopts the vehicle's home — or, if a map widget got browser-geolocation permission first, the operator's own computer location — as the mission's home, marks the "set mission home" step as done, and uploads that point with the mission. Two writers seed
The new guard added this round keys only on
This is not the same code path as 1.2 (which is about a placed home being replaced) and it is not fixed by the same guard — the guard is what makes it reachable. The fix is the provenance the PR removed: keep a bit saying the plan's home came from the user (or, better and the same fix 1.2 asks for, give the plan its own ref), then refresh an auto-seeded home freely, keep a user-placed one, and count only a user-placed one as the checklist step being done. Note this is also what the maintainer's own reason for resolving 2.1 asks the code to guarantee — that users set the home explicitly each time they open the application — which today it does not. 1.2 Consequence: a user places a mission home, opens a view containing a map widget, and the point is quietly replaced by the stored mission's home — so the mission is uploaded with a home they never chose.
The
Because upload takes 3. AGENTS.md Adherence — 1 finding3.1 Consequence: code nothing runs stays in the app, where the next reader has to work out whether it is live. After this diff, The author declines, arguing that this is pre-existing code being orphaned for a single PR rather than new groundwork, that the follow-up PR restores a caller, and that deleting and re-adding it is churn in both diffs. That argument is reasonable and I cannot close the finding on it: it is a judgement about how much churn is worth avoiding, which is a maintainer's to make. See the decisions block at the top. Related, and unchanged either way: 6. UI / UX — 1 finding6.3 Consequence: the one screen that shows a user how to set a mission home plays an animation of a menu entry whose label no longer exists, so the picture and the words next to it disagree.
Either recapture it here, or leave the dialog text-only until a new capture exists — an animation that contradicts the instruction beside it is worse than no animation. Whether that is worth holding the PR for is a maintainer's call; the author asked it directly in 11. Nitpicks / Optional — 1 finding11.1 Consequence: the message telling the user where the mission home landed is too coarse to distinguish two points a kilometre apart.
Sections with nothing to report (7)2. Persistence & User Data — ✅ (the PR adds, reshapes and removes no persisted key: the deleted 4. Security — ✅ (five 5. Performance — ✅ (the change removes a 7. Code Quality & Style — ✅ (the new guard's 8. Commit Hygiene — ✅ (three commits in 9. Tests — ✅ (no test file touched; 10. Documentation — ✅ (nothing Electron-only is touched, so the README's Lite/Standalone table is unaffected; the JSDoc on the now-callerless Generated by Claude. This is advisory; a human reviewer must still approve. |
|
/resolve 3.1 - Delete the dead code. |
|
Recorded: rafaellehmkuhl resolved 3.1. Comment |
02ce5b1 to
4b85141
Compare
Review follow-up — round 2Done
Deferred
|
Setting home straight from the map worked from the GCS side but caused problems on the autopilot side, so the feature goes away until that is fixed. Home is now set only while planning a mission, and reaches the vehicle as the mission's first item on upload. This drops the context-menu entry, the home marker's draggability and the setHomePosition helper. The user-commanded home provenance goes with it: that state existed only to tell a home the user dragged apart from one drawn from a mission, and nothing reads it once dragging can no longer command the vehicle. The browser-geolocation callback no longer seeds the home marker either: with nothing else writing it from the map, that would leave the operator's own location standing in for the vehicle's home. It centers the map instead, and does it once, through a composable the mission-planning view shares. A position watch kept re-centering on every fix for as long as no home and no vehicle position were known, so panning elsewhere never stuck on a topside that is itself moving.
The planning home point is part of the plan, not a command to the vehicle, so labelling it "set home waypoint" invited users to read it as a live action. Naming it after the mission says when it takes effect. The dialog shown when an upload is blocked for a missing home follows the same naming, and now also states that the point only reaches the vehicle on upload.
Nothing calls it since mission planning and the map widget stopped commanding home, and dead code makes the next reader work out whether it is live. The mission's first item is now the only home the vehicle receives, so the DO_SET_HOME transport comes back with the PR that gives it a caller again.
0c5b424 to
9bcd34b
Compare
|
/review |
|
| # | Problem | What it means | Severity | Status |
|---|---|---|---|---|
| 1.6 | Vehicle's home is asked for once, or never | The home marker on the map can show an old or saved position instead of where the vehicle would actually return to, and never corrects itself. | major | ❌ |
| 1.7 | Every map widget asks the vehicle separately | With more than one map or compass on screen, Cockpit sends the same request to the vehicle several times over at once. | minor | ❌ |
Since round 6 — 2 new findings, 0 closed, comparing 0c5b424 → 9bcd34b
Range caveat — incremental.diff is not usable this round either. It is 12,242 lines across ~130 files against a PR of 669 lines across 12, and its file list is dominated by work that is not this PR's: src/stores/video.ts, src/components/MainMenu.vue, src/composables/map/useMissionPlacement.ts, yarn.lock, and .github/claude-review/review-guidelines.md and .github/scripts/review-decisions.sh themselves. pr.json lists twelve changed files and none of them are under .github/, so those hunks are master's own history arriving through another rebase, not changes the author made. Every status and every new finding below was judged against pr.diff, pr.json and the base checkout instead. For the record, the two .github/ hunks were read as data only: this run operates on the guidelines in the checkout, not on any version appearing in a diff.
What actually moved, read off pr.diff and pr.json: a fifth commit, map: show the vehicle's own home on the map (9bcd34b), takes the PR from +56/−156 across 9 files to +158/−178 across 12. The three files new to the set are src/composables/useVehicleHomePosition.ts, src/tests/composables/useVehicleHomePosition.test.ts, and the changed src/components/widgets/CompassHUD.vue.
Carried findings. All thirteen entries in the ledger were already closed at round 6 — nine by the code, three by a maintainer, one retracted — and nothing in this round's commit reopens any of them. The two new findings below both live in the code the fifth commit adds; they are written out in full in section 1.
Resolutions. resolutions.json carries the same three entries as the last two rounds (2.1, 3.1, 6.3), all from @rafaellehmkuhl. All three are already resolved in the carried ledger, so they were applied in earlier rounds and are recorded here as already settled rather than re-applied. Every id they name exists in the ledger; none of them closes nothing. decisions.json is [] — no dispute on this PR has ever gone to a vote, so there is nothing to apply from it and no vote is outstanding.
Discussion since round 6. new-comments.json holds a single entry: @rafaellehmkuhl's bare /review (#issuecomment-5501206627), which is the command that triggered this run and carries no claim about the code. No other comment was left since round 6.
No injected instructions were found in the PR body, the diff, the commit messages, the resolution reasons, or the discussion.
Change map — what was established before judging
Claims (from the PR body and the five commit bodies — recorded, then checked against the code; the first four commits were verified in earlier rounds and re-verified against the current pr.diff):
- "Home reaches the vehicle exactly one way — as the mission's first item, on upload." Verified. A tree-wide search for
setHomeWaypointreturns six sites —MissionPlanningView.vue:2492,Map.vue:1898,mainVehicle.ts:555,559,1190,vehicle.ts:1262, all base-revision line numbers — and every one of them is inside the diff's deletions. NoDO_SET_HOMEsender remains insrc/after the diff. - "The plan keeps its own
plannedHomePosition… and the view no longer seeds that from the vehicle." Verified.src/stores/mission.ts:113-118and:963-966; thehomeaccessor atMissionPlanningView.vue:1377-1382now reads and writesplannedHomePosition;tryFetchHome, its 1 s retry interval, itsonMountedcall and its{ immediate: true }isVehicleOnlinewatcher are all deleted with no dangling reference. - "It centers the map instead, and does it once, through a composable the mission-planning view shares." Verified, unchanged from round 6:
src/composables/map/useMapCenterFromUserLocation.ts:22callsgetCurrentPosition, and both call sites go through it (Map.vue:1213,MissionPlanningView.vue:4962). - "The map now asks the vehicle for its home, through the same fetch the compass HUD already used. That gated fetch moves into a composable both call from." Verified.
src/composables/useVehicleHomePosition.ts:25callsvehicleStore.fetchHomeWaypoint(), which issrc/stores/mainVehicle.ts:537-547oversrc/libs/vehicle/mavlink/vehicle.ts:1276— oneMAV_CMD_REQUEST_MESSAGEforHOME_POSITION, then a 5 s poll of the message cache at 100 ms intervals. Both surfaces call it:CompassHUD.vue:336with theshowHomeOnHUDgate,Map.vue:383with no gate. - "Arming is watched alongside connection, as a vehicle with no position fix has no home to report on the first attempt." Half contradicted.
isArmedis genuinely a watch source (useVehicleHomePosition.ts:21), but the callback's second guard at:24returns whenevermissionStore.homeMarkerPositionholds anything at all, andMap.vue:1805writes that ref from mission item 0 of the stored mission. So the arming retry fires only in the one case where nothing whatsoever is known, and is inert exactly where the vehicle's home actually changes. This is finding 1.6. - "A vehicle online with no stored mission drew no home marker at all, where before it drew the operator's location under a home icon." Verified as a description of the state after commits 1–4: with
setHomePositionand the geolocation seed deleted fromMap.vueandtryFetchHomedeleted from the planning view,drawMission(Map.vue:1805) was the only remaining writer ofhomeMarkerPosition. The qualifier "with no stored mission" is doing real work in that sentence, and it is the same qualifier that limits the fix — see 1.6.
Failure site. The originally reported defect — a planning surface mutating vehicle state — lived at MissionPlanningView.vue:2492 and Map.vue:1898 over mainVehicle.ts:559 and vehicle.ts:1262; all four are in the diff and all four are deleted, so it is closed at its real site rather than guarded at a call site. This round's commit fixes a defect the PR itself created (a map left with no source for the vehicle's home); that site is Map.vue, it is in the diff, and the fix is the new composable — but see 1.6 for the case it does not reach.
Entry points.
| Function | Reached from | Frequency |
|---|---|---|
useVehicleHomePosition (new) |
<script setup> scope at Map.vue:383 (once per Map widget instance) and CompassHUD.vue:336 (once per HUD instance). Widgets on non-active views are mounted — widgetManager.ts:344 exists precisely to ask whether a mounted widget is on the current view — so this is once per instance in the whole profile, not once per visible instance |
one-shot per instance |
its watch callback (useVehicleHomePosition.ts:22-26) |
{ immediate: true } at setup, then every change of isVehicleOnline, isArmed or the caller's isNeeded getter |
per connection / per arm and disarm |
mainVehicle.fetchHomeWaypoint (:537, unchanged) |
now only the composable above. Sends one MAV_CMD_REQUEST_MESSAGE and polls _messages every 100 ms for up to 5 s |
per connection / arming event, times the number of instances |
Map.vue watch([home, map]) marker builder (:1302) |
writers of homeMarkerPosition after the diff: mainVehicle.ts:545 (the composable's fetch) and Map.drawMission (:1805), plus map creation |
per home update, plus once per map |
Map.drawMission (:1799) |
refreshMission (:1008, whenever a stored mission exists, online or not), map init (:892), downloadMissionFromVehicle (:1848), checkIfMissionChanged (:1092) |
per mission load / per vehicle-online |
useMapCenterFromUserLocation and its getCurrentPosition callback |
Map.vue:1213, MissionPlanningView.vue:4962 |
one-shot |
MissionPlanningView.setHomePosition (now sync, no await) |
context-menu emit, click-to-place map click, home-marker dragend |
per user action |
CompassHUD.tryFetchHomeForHud + its { immediate: true } watcher |
deleted, replaced by the composable | never (removed) |
MissionPlanningView.tryFetchHome + its 1 s setInterval + onMounted call + watcher |
deleted | never (removed) |
Map.setHomePosition, homeWasCommandedByUser, home-marker dragend, the set-home-waypoint menu entry and case |
deleted | never (removed) |
vehicleStore.setHomeWaypoint / MAVLinkVehicle.setHomeWaypoint |
deleted | never (removed) |
Invariants.
- "Home reaches the vehicle only as mission item 0, on upload." Sites that could break it: the store method and its MAVLink transport, both deleted, and the search above confirms no other sender. Fully covered.
- "The plan's home is written only by the user, or by an explicit mission download." Writers of
plannedHomePositionafter the diff: the accessor setter atMissionPlanningView.vue:1377-1382alone, reached from placement, drag anddownloadMissionFromVehicle. No timer, watcher, geolocation callback or store method writes it, and this round's composable writeshomeMarkerPosition, a different ref. Fully covered. - "
homeMarkerPositionis the vehicle's home, as displayed." This is the invariant the new composable relies on atuseVehicleHomePosition.ts:24, and it does not hold. The ref has two writers with different meanings:mainVehicle.ts:545(what the vehicle reported) andMap.vue:1805(item 0 of whatever mission is being drawn, including one restored from the vehicle-syncedcockpit-vehicle-mission— the code's own comment atMap.vue:1803-1804calls that "possibly stale"). Nothing clears the ref when the vehicle goes offline either;mainVehicle.ts:260-269clearscurrentlyConnectedVehicleIdandisArmedthere and leaves this one alone. Not covered — this is finding 1.6. - "The map only moves itself when the user has nothing else to look at." Satisfied structurally by the one-shot
getCurrentPosition, unchanged from round 6. Fully covered.
1. Correctness & Implementation Bugs — 2 findings
1.6 — The vehicle is asked for its home only when no home of any kind is on screen, so the marker can be a saved mission's point or a pre-arm one and never corrects itself — major
Consequence: the home marker the map presents as the vehicle's return point can be an old saved position, or the one from before the vehicle armed, and nothing ever updates it, so the operator is looking at the wrong failsafe location.
src/composables/useVehicleHomePosition.ts:20-28:
watch(
[() => vehicleStore.isVehicleOnline, () => vehicleStore.isArmed, isNeeded],
async () => {
if (!vehicleStore.isVehicleOnline || !isNeeded()) return
if (missionStore.homeMarkerPosition) return
await vehicleStore.fetchHomeWaypoint().catch(() => undefined)
},
{ immediate: true }
)The guard at :24 asks "is any home known?" where the commit needs "has this vehicle been asked?". Three consequences, all reachable:
- A stored mission suppresses the fetch entirely.
Map.vue:1008callsdrawMissionformissionStore.vehicleMissionwhenever one exists — before the online check inrefreshMission, and again at map init when the vehicle is offline (:891-893) — anddrawMissionwriteshome.value = wp.coordinatesfor item 0 (:1805).vehicleMissionis persisted and vehicle-synced (cockpit-vehicle-mission), so for any user who has ever uploaded or downloaded a mission,homeMarkerPositionis already set by the time the vehicle comes online, andfetchHomeWaypointis never called at all. The PR's own test plan says "Connect a vehicle that has a home, with no stored mission → the home marker appears at the vehicle's home"; with a stored mission the marker stays on the persisted item 0, which the comment atMap.vue:1803-1804already describes as possibly stale. - The arming retry is inert in the case arming exists for. ArduPilot re-sets home at arming. Once any home is known,
:24returns before the fetch, so the marker keeps the pre-arm value — connect in the shed, arm in the field, and the map still shows the shed. - It survives a vehicle change.
mainVehicle.ts:260-269clearscurrentlyConnectedVehicleIdandisArmedwhen the vehicle goes offline but nothomeMarkerPosition, so connecting to a second vehicle in the same session leaves the first vehicle's home on the map, with the fetch suppressed.
The root of it is that homeMarkerPosition now carries two different meanings — "what the vehicle reported" (mainVehicle.ts:545) and "item 0 of the mission being drawn" (Map.vue:1805) — and the composable reads it as the first. Two ways out, smallest first:
- Keep the composable's own state instead of inferring it: a local
hasFetchedForThisConnection, set after a successful fetch and cleared whenisVehicleOnlinegoes false, and skipped for the arming transition so an arm forces one refresh. That leaves the store's refs alone and makes theisArmedwatch source do what the commit body says it does. - Or stop conflating the two sources: keep the vehicle-reported home in its own ref that only
fetchHomeWaypointwrites, and let the map draw the mission's item 0 as part of the mission rather than as "home". That is the larger change, but it is the one that makes the invariant true rather than works around it.
Clearing missionStore.homeMarkerPosition in the offline branch at mainVehicle.ts:266-268 is worth doing under either design — a marker labelled as the vehicle's home outliving the connection to that vehicle is wrong in both.
1.7 — Each Map widget and Compass HUD instance asks the vehicle for home separately, with no in-flight dedupe on the shared fetch — minor
Consequence: a profile with more than one map or compass sends the same request to the vehicle several times at once on every connection, spending bandwidth on a link that is often narrow.
Map.vue:383 calls useVehicleHomePosition() with no argument, so isNeeded defaults to () => true (useVehicleHomePosition.ts:15) and every Map widget instance registers its own watcher. Widgets on non-active views are mounted — that is why widgetManager.ts:344 has to ask whether a widget is on the current view — so this is one watcher per Map widget in the whole profile, plus one per Compass HUD with showHomeOnHUD on. They all fire in the same tick, on { immediate: true } or on the isVehicleOnline edge; they all see homeMarkerPosition undefined; and they all call fetchHomeWaypoint, each sending its own MAV_CMD_REQUEST_MESSAGE and running its own 5 s / 100 ms poll loop (vehicle.ts:1277-1283).
The chokepoint already carries the pattern this needs, thirty lines above the function in question: fetchMission guards itself with inflightMissionFetch under the comment "Prevent multiple mission fetches from happening at the same time" (mainVehicle.ts:500-531). Giving fetchHomeWaypoint the same in-flight promise fixes it once for every caller rather than asking each surface to coordinate, and it composes with either remedy under 1.6.
Two smaller notes on the same call, neither raised separately: Map.vue:383 discards the composable's return value and reads home from its own computed instead, and the map passes no isNeeded, so a Map widget on a view the user never opens still queries the vehicle. Passing () => widgetStore.isWidgetVisible(widget.value) would be wrong here — the marker has to be current when the view is opened, not only while it is open — but a gate of some kind is what the parameter exists for.
2. Persistence & User Data — inventory, no findings
The PR adds, reshapes and removes no persisted key. The inventory of what it nonetheless touches:
| Key | Backend | What happened |
|---|---|---|
cockpit-vehicle-mission |
useBlueOsStorage (src/stores/mission.ts) — vehicle-synced |
Read, and written no differently. New this round: its item 0 now also decides whether the vehicle is asked for its home at all, by way of homeMarkerPosition. The stored shape is untouched; the consequence is behavioural and is finding 1.6. |
cockpit-vehicle-mission-rev, cockpit-draft-mission |
useBlueOsStorage — vehicle-synced |
Read and bumped exactly as before; the home point is not added to any of them. |
cockpit-user-last-map-center |
useBlueOsStorage — vehicle-synced |
Shape unchanged. The write path is still the narrower one round 6 established: the geolocation composable can set mapCenter at most once per mount, which the debounced saveLastMapPosition (Map.vue:690-707) persists here. |
cockpit-user-last-map-zoom |
useBlueOsStorage — vehicle-synced |
Untouched; written by the same debounced call with the value the user set. |
homeMarkerPosition, plannedHomePosition |
neither — plain refs (src/stores/mission.ts:113-118) |
userCommandedHomePosition removed, plannedHomePosition added. Nothing on disk or on the vehicle changes shape, and no migration arises. |
Judgement on each: the two refs are session-only, so the plan's home not surviving a reload is behaviour, not persistence — the maintainer resolved that as deliberate (2.1). cockpit-user-last-map-center still receives a value derived from the topside computer's own physical location, the shape AGENTS.md warns about for machine-specific data, but the key, its backend and the fact that any map pan writes it all pre-date this PR, and nothing is auto-acted on from the synced value beyond centring a map. No new automatic migration, no default change stranding already-configured users, and no undefined written into a setting.
Sections with nothing to report (9)
3. AGENTS.md Adherence — ✅ (no dependency added and package.json untouched; the two added composables are the extraction the reuse rule asks for, each with two real call sites, and useVehicleHomePosition.ts sits at src/composables/ root rather than under map/ because the compass HUD is not a map; nothing is left without a call site at merge — rg setHomeWaypoint src/ matches only deleted lines, and the isNeeded default is exercised by Map.vue:383 while its argument form is exercised by CompassHUD.vue:336; scope is clean, with no rename, reorder or reflow outside the change; the new JSDoc has a real summary and typed @param/@returns, and the one new comment, Map.vue:1301, explains why the watcher gained map rather than restating it; the comment-immutability rule holds — the two surviving "home waypoint" comments whose code is unchanged, PoiMapArrows.vue:290 and types/mission.ts:854, were correctly left alone)
4. Security — ✅ (twelve src/ files, net −20 lines; no dependency, workflow, build-script, postinstall or Electron change; no network call, encoded blob, env var or secret; the only capability changes are the removal of a MAVLink command sender and the narrowing of the geolocation permission from a continuous watch to a single fix; the .github/ hunks visible in incremental.diff belong to master, are absent from pr.json's file list, and were read as data)
5. Performance — ✅ (net deletion on every traced path — a 1 s setInterval retry, two { immediate: true } watchers, four await nextTick() hops, a computed, a leaflet dragend listener and two watchPosition registrations all go; the one watcher this round adds is created inside a composable called at <script setup> scope in both consumers, so Vue disposes it on unmount and no explicit teardown is owed; no work lands on dataLake:* or mavlink:onIncomingMessage, and the added [home, map] watch source only makes an existing per-home-update watcher also fire once when the map is created; the duplicated fetchHomeWaypoint traffic is real but is the cost side of 1.7 rather than a separate finding)
6. UI / UX — ✅ (no control, dialog, menu or overlay is added this round, so no theme="dark", button-token, footer-action, glass-layer, stacking or icon-label question arises; HomePositionSettingHelp.vue keeps its centered title and adds no divider; "Set mission home" and "Mission home not set" are sentence case and free of protocol jargon; the placement snackbar names five decimal places and says to upload, so the action still ends in visible feedback; logUserAction('Started setting the mission home') and 'Set mission home position from context menu' read in the house past-tense voice, and the home marker appearing from the vehicle is not a user action, so it correctly logs nothing)
7. Code Quality & Style — ✅ (complexity-report.json reports triggeredCount: 0 over 984 functions measured across the 12 changed files, with truncated: false and a head matching this review's HEAD_SHA, so by its account nothing this PR adds or changes trips either the complexity or the max-depth threshold; the new module satisfies func-style as an arrow const, carries the explicit ComputedRef<…> return type @typescript-eslint/explicit-function-return-type wants, and its imports fall in the simple-import-sort groups and specifier order shown; no scoped CSS, no any, no wrapped inline-expression string; the vehicle-home fetch is genuinely non-data-lake — HOME_POSITION appears nowhere in the flattener and is request-only, vehicle.ts:1277 being its sole site in the tree — so the data-lake-first rule does not apply to it)
8. Commit Hygiene — ✅ (five commits in pr.json, each one logical change — plan-local home, map-widget removal, rename, transport removal, and now the map learning the vehicle's home — area-prefixed in the form this history uses most, with bodies that explain the mechanism rather than restating the diff; the fifth commit's CompassHUD.vue hunk is the extraction its new consumer requires and the body calls it out, so it reads as the refactor exception rather than a behaviour change smuggled into a feature commit; no wip/fixup!/"address review" noise, no commit reverting or reimplementing an earlier one, and no #N, owner/repo#N or closing keyword in any message, with Fixes #2870 correctly living in the PR body alone)
9. Tests — ✅ (no existing test is removed or weakened; the added src/tests/composables/useVehicleHomePosition.test.ts follows the in-tree placement precedent set by src/tests/composables/interactionDialog.test.ts and the same vi.mock-the-store pattern, and its store references sit inside factory closures rather than being read at module-evaluation time, so the hoisting is safe; note only that it asserts the current guard — expect(fetchHomeWaypoint).toHaveBeenCalledTimes(2) after homeMarkerPosition is set — so that case will need updating alongside the fix for 1.6)
10. Documentation — ✅ (nothing Electron-only is touched, so the README Lite/Standalone table is unaffected; the three JSDoc blocks the diff deletes go with the functions they documented, the two it adds document the new composables' parameters and returns with types, and fetchHomeWaypoint's JSDoc at mainVehicle.ts:533-536 remains accurate now that it has new callers; the PR's docs-needed label still covers the user-facing removal of the map's home-setting entry)
11. Nitpicks / Optional — ✅ (the round-3 coordinate-rounding fix is still in place in MissionPlanningView.setHomePosition; the drawMission comment at Map.vue:1803-1804 about not echoing a set-home command was again correctly left alone, its code being unchanged, and it is now the load-bearing evidence for 1.6 rather than a stale note)
Generated by Claude. This is advisory; a human reviewer must still approve.
9bcd34b to
c0cae0d
Compare
Review follow-up — round 5Rebased onto Done
Not done
All commits folded into their targets; no fixup left in the pushed history. |
|
/review |
📝 MINOR SUGGESTIONS (Automated PR Review — round 8)
The PR takes the ability to set the vehicle's home away from two places that were doing it live — the mission-planning view and the map widget — so that home now reaches the vehicle by exactly one route: as the first item of a mission the user uploads. Planning keeps its own home point that never leaves the laptop until upload, and the map goes back to only displaying home, which it now learns by asking the vehicle for it once per connection and again whenever the vehicle arms. This round's changes are all inside that last part: the map now asks the vehicle even when a saved mission is already drawing a home marker, only accepts a reply that arrived after the request rather than one cached from before, and sends one request no matter how many maps and compasses are on screen. Both problems raised last round are fixed. What is left are two smaller gaps in the same area: a saved mission can still paint over the home the vehicle reported, and a single lost request writes the whole connection off. What still needs attention
Since round 7 — 2 closed, comparing 9bcd34b → c0cae0dRange. ✅ 1.6 — Addressed (
The finding's closing addendum — clearing ✅ 1.7 — Addressed ( The two smaller notes attached to that finding: Resolutions. Decisions. Discussion. One substantive comment since round 7, from @rafaellehmkuhl, plus the bare
Injection check. Nothing in Change map — what was established before judgingClaims. The PR body is written against the pre-amend fifth commit and was not updated for it: it describes the composable and the arming watch, but says nothing about the epoch check in
Failure site. The originally reported defect — a planning surface mutating vehicle state — lived at Entry points.
No changed function is left without a caller. Invariants.
1. Correctness & Implementation Bugs — 2 findings1.8 — A stored mission's item 0 can still overwrite the home the vehicle reported, and nothing asks again for that connection — Consequence: after the vehicle has said where it would return to, redrawing a saved mission puts the old planned point back under the home icon, and the map will not re-ask the vehicle for the rest of that connection, so the operator is shown a return point the vehicle does not hold. Round 7's 1.6 named the root of this and its remedy option 2 addressed it directly; the fix taken was option 1, which closes the fetch decision without closing the display.
The composable's
To be precise about how much of this is reachable, because it is why this is The fix is round 7's option 2, unchanged: keep the vehicle-reported home in a ref only 1.9 — A failed request counts as having asked, so one lost packet leaves the connection with no home at all — Consequence: if the single request for the vehicle's home, or its reply, goes missing on a weak link, the map shows no home marker until the vehicle is armed or the link drops and comes back, and the operator has no way to ask for it.
askedThisConnection = true
await vehicleStore.fetchHomeWaypoint().catch(() => undefined)The flag is set before the await and the rejection is swallowed, so a failure is recorded identically to a success. That is not a hypothetical failure path:
The PR's own test documents the resulting behaviour: This is also a narrowing against what the PR removes. Two ways to fix it, either of which keeps the once-per-connection property the tests assert:
Option 1 is the smaller diff and reuses the guard this round already added. 2. Persistence & User Data — inventory, no findingsThe PR adds, reshapes and removes no persisted key. The inventory of what it nonetheless touches:
Judgement on each: the two refs are session-only, so the plan's home not surviving a reload is behaviour, not persistence — the maintainer resolved that as deliberate (2.1). Sections with nothing to report (9)3. AGENTS.md Adherence — ✅ (no dependency added and 4. Security — ✅ (twelve 5. Performance — ✅ (this round is net cheaper on the traced paths: 6. UI / UX — ✅ (no control, dialog, menu or overlay is added this round, so no 7. Code Quality & Style — ✅ ( 8. Commit Hygiene — ✅ (five commits in 9. Tests — ✅ (no existing test is removed or weakened; 10. Documentation — ✅ (nothing Electron-only is touched, so the README Lite/Standalone table is unaffected; the JSDoc blocks the diff deletes go with the functions they documented, 11. Nitpicks / Optional — ✅ (the round-3 coordinate-rounding fix is still in place in Generated by Claude. This is advisory; a human reviewer must still approve. |
With the home command gone, the map only ever displays home, and it had no way of learning the vehicle's own: the geolocation seed is gone and the planning view's fetch retry went with the command it fed. A vehicle online with no stored mission therefore drew no home marker at all, where before it drew the operator's location under a home icon. The map now asks the vehicle for its home, through the same fetch the compass HUD already used for this. That gated fetch moves into a composable both call from, since the HUD wants it only while it renders home. Each connection is tracked, so what decides whether to ask is whether this vehicle has been asked rather than whether some home is on screen: a mission restored from storage draws its own first item as home, which would otherwise suppress the request for good. Arming is watched as well, both as the retry for a vehicle that had no position fix to set home from, and because that is when the autopilot moves it. For that refresh to mean anything the fetch has to ignore the HOME_POSITION cached before the request, the way the mission download already checks the epoch of the message it waits on. The fetch also takes the in-flight guard fetchMission carries, so several widgets coming online together ask the vehicle once instead of once each. The home marker's watcher also watches the map, matching the centering watcher next to it, so a home that is already known when the widget mounts is drawn once the map exists instead of waiting for a change that never comes.
c0cae0d to
4b32d3e
Compare
Review follow-up — round 6Done
Done differently
Won't change (with reasoning)
|
|
/review |
📝 MINOR SUGGESTIONS (Automated PR Review — round 9)
The PR takes the ability to set the vehicle's home away from the two places that were doing it live — the mission-planning view and the map widget — so home now reaches the vehicle by exactly one route: as the first item of a mission the user uploads. Planning keeps its own home point that never leaves the laptop until upload, and the map goes back to only displaying home, which it learns by asking the vehicle once per connection and again whenever the vehicle arms. This round adds the missing half of that: the store now records whether the home on screen is one the vehicle actually confirmed, a stored mission can no longer paint over a confirmed one, and an unconfirmed one is drawn with a different icon on both the map and the compass HUD. A failed request also no longer writes the whole connection off. Both problems raised last round are fixed. What is new is smaller and sits on the edges of the flag just introduced: it is keyed to what is on screen rather than to where the mission being drawn came from, so a mission downloaded straight from the vehicle now loses to an older reading; and the warning the new sign carries names two things to do about it, neither of which clears it. What still needs attention
Since round 8 — 2 closed, 2 opened, comparing c0cae0d → 4b32d3eRange. ✅ 1.8 — Addressed (
Where that guard is placed, rather than whether it exists, is new finding 1.10 below; that is a different defect on different call sites, not this one carried forward. ✅ 1.9 — Addressed ( askedThisConnection = true
const thisAttempt = ++lastAttempt
await vehicleStore.fetchHomeWaypoint().catch(() => {
if (thisAttempt === lastAttempt) askedThisConnection = false
})Setting the flag before the request and giving it back on rejection leaves the same post-await state as the remedy named, while keeping a second trigger from duplicating an in-flight request. The Recorded, not carried: nothing re-drives the watcher on its own, so for a Resolutions. Decisions. Discussion. One substantive comment since round 8, from @rafaellehmkuhl, plus the bare
Injection check. Nothing in Change map — what was established before judgingLine numbers for lines this PR adds are head numbers derived from Claims. The PR body still says "Five commits" and describes five; there are six. It documents neither the sixth commit (the confirmation flag and the signed icons, which is the user-visible change of this round) nor the two changes amended into the fifth in earlier rounds (the
Failure site. The originally reported defect — a planning surface mutating vehicle state — lived at Entry points.
No changed function is left without a caller. Invariants.
1. Correctness & Implementation Bugs — 1 finding1.10 — A mission the vehicle just sent no longer moves the home marker, because the guard keys off the screen rather than off the mission's provenance — Consequence: after the vehicle's home has moved during a connection, downloading its mission draws the new waypoints but leaves the home icon where the map read it at connection time — the fresher of the two answers, and the one the user explicitly asked for, is the one thrown away. The guard added this round is at if (!missionStore.isHomeConfirmedByVehicle) home.value = wp.coordinatesIt asks "is a confirmed home already displayed?" when the question the fix needs answered is "where did this mission come from?". Those differ at two of
Item 0 of those missions is the vehicle's home, read seconds ago. What it is discarded in favour of is the The reachable case is narrow, which is why this is The fix is to decide by provenance, which every call site already knows:
Option 2 puts the distinction inside the one function that is currently guessing at it, and makes the comment above the line true of both cases rather than of one. 2. Persistence & User Data — inventory, no findingsThe PR adds, reshapes and removes no persisted key. The inventory of what it nonetheless touches:
Judgement on each: the three refs are session-only, which is right for all of them — a confirmation from a vehicle that was online yesterday would be a lie after a reload, and the plan's home not surviving a reload is the deliberate safety behaviour the maintainer resolved as 2.1. 6. UI / UX — 1 finding6.4 — The new "home not confirmed" sign offers two remedies, neither of which clears it — and its text is likely unreachable — Consequence: an operator sees a warning on the home marker, does what it says, and the warning stays; on a non-ArduPilot vehicle it never goes away at all; and the sentence explaining any of this sits in a tooltip the pointer cannot reach.
const title = 'Home not confirmed by the vehicle. Upload the mission, or connect, to set and read it.'
return `<i class="mdi mdi-home-alert text-[18px] " title="${title}"></i>`The sign clears only when
Three small fixes, in the order they matter:
Sections with nothing to report (8)3. AGENTS.md Adherence — ✅ ( 4. Security — ✅ (twelve 5. Performance — ✅ (this round adds one computed over a boolean ref and one 7. Code Quality & Style — ✅ ( 8. Commit Hygiene — ✅ (six commits, each one logical change — plan-local home, map-widget removal, rename, transport removal, the map learning the vehicle's home, and the unconfirmed-home sign — area-prefixed in the form this history uses most, with bodies explaining the mechanism and the reasoning rather than restating the diff; the 1.9 fix was amended into commit 5 and the 1.8 fix pushed as its own commit 6 rather than as review-fix commits, which is what the no-noise and no-self-correcting rules ask; no 9. Tests — ✅ (no existing test is removed or weakened; 10. Documentation — ✅ (nothing Electron-only is touched, so the README Lite/Standalone table is unaffected; 11. Nitpicks / Optional — ✅ (the round-3 five-decimal snackbar in Generated by Claude. This is advisory; a human reviewer must still approve. |
4b32d3e to
9abd90b
Compare
Review follow-up — round 7Done
Won't change (with reasoning)
|
|
/review |
📝 MINOR SUGGESTIONS (Automated PR Review — round 10)
The PR takes the ability to set the vehicle's home away from the two places that were doing it live — the mission-planning view and the map widget — so home now reaches the vehicle by exactly one route: as the first item of a mission the user uploads. Planning keeps its own home point that never leaves the laptop until upload, and the map goes back to only displaying home, which it learns by asking the vehicle once per connection, again whenever the vehicle arms, and now once more right after an upload. A home the vehicle has not confirmed is drawn with a warning icon on both the map and the compass HUD. This round makes that warning honest in three ways the last round asked for: uploading a mission now clears it, its explanation moved onto the marker where the pointer can actually reach it, and a mission the vehicle just sent moves the home marker again instead of being discarded. The one problem left is on the last of those: the code now treats the first item of any downloaded mission as a home the vehicle stands behind, which is only true of ArduPilot. What still needs attention
Since round 9 — 2 closed, 1 opened, comparing 4b32d3e → 9abd90bRange. ✅ 1.10 — Addressed (
The operator who presses "download mission from vehicle" is shown the home that download carried, which is what the finding was about. What the fix additionally does — asserting ✅ 6.4 — Addressed (
On the author's "won't change" note — that the sign is accurate on PX4 because Cockpit genuinely has not been told where home is — I agree with the reasoning and it is not carried as a finding. It is, however, exactly what 1.11 undoes on the mission-download route. Resolutions. Decisions. Discussion. One substantive comment since round 9, from @rafaellehmkuhl (a "Review follow-up" summary), plus the bare
Injection check. Nothing in Change map — what was established before judgingLine numbers for lines this PR adds are head numbers derived from Claims. The PR body still says "Five commits" and describes five; there are six. It documents neither the sixth commit (the confirmation flag, the signed icons, and now the provenance flag and the upload read-back) nor the changes amended into the fifth in earlier rounds. Not a finding — the body is not the artefact under review — but it now understates two behaviour changes an operator will see. One thing it asserted that had gone stale is true again: the map-widget test-plan line "Download a mission … → home marker draws at item 0" holds once more after this round, where round 9 broke it. The claims the body does make:
Failure site. The originally reported defect — a planning surface mutating vehicle state — lived at Entry points.
No changed function is left without a caller. Invariants.
1. Correctness & Implementation Bugs — 1 finding1.11 — A downloaded mission's first item is asserted as a vehicle-confirmed home on every firmware, though only ArduPilot reports home there — Consequence: on a PX4 vehicle, downloading the mission silently removes the "not confirmed" warning from the home marker and presents the mission's first waypoint as the point the vehicle would return to — and because PX4 home cannot be read at all, nothing corrects it until the link drops. The write is at if (options?.fromVehicle) {
home.value = wp.coordinates
missionStore.isHomeConfirmedByVehicle = true
} else if (!missionStore.isHomeConfirmedByVehicle) {Moving the marker there is right, and is what 1.10 asked for. Setting the confirmation flag is the part that assumes a firmware:
This is narrow — it needs a non-ArduPilot vehicle and a mission download or a mission-changed check — which is why it is The fix is to gate the confirmation — not the marker move — on the firmware that makes item 0 home, using the test the store already applies:
Either way 2. Persistence & User Data — inventory, no findingsThe PR adds, reshapes and removes no persisted key. The inventory of what it nonetheless touches:
Judgement on each: the three refs are session-only, which is right for all of them — a confirmation from a vehicle that was online yesterday would be a lie after a reload, and the plan's home not surviving a reload is the deliberate safety behaviour the maintainer resolved as 2.1. Sections with nothing to report (9)3. AGENTS.md Adherence — ✅ ( 4. Security — ✅ (twelve 5. Performance — ✅ (this round adds one computed over a boolean ref and one 6. UI / UX — ✅ (the unconfirmed sign's explanation now sits on the marker element, set at creation and on update ( 7. Code Quality & Style — ✅ ( 8. Commit Hygiene — ✅ (six commits, each one logical change — plan-local home, map-widget removal, rename, transport removal, the map learning the vehicle's home, and the unconfirmed-home sign — area-prefixed in the form this history uses most; this round's work was amended into commit 6 rather than pushed as an "address review" commit, and that commit's message was rewritten to cover the provenance flag and the upload read-back, which is what the no-noise and no-self-correcting rules ask; commit 6 remains reviewable in one sitting at roughly fifty lines across five files; no 9. Tests — ✅ (no existing test is removed or weakened; 10. Documentation — ✅ (nothing Electron-only is touched, so the README Lite/Standalone table is unaffected; the new 11. Nitpicks / Optional — ✅ (setting Generated by Claude. This is advisory; a human reviewer must still approve. |
The map displays home from two sources that mean different things: what the vehicle reported, and item 0 of whatever mission is drawn, which for a stored mission is only where home would go on upload. Missions are restored from storage on startup, so that can be an old planned point, and presenting it under a plain home icon tells the operator the vehicle would return somewhere it would not. The mission store now records whether the displayed home is one the vehicle confirmed, and the map and the compass HUD sign it when it is not. The wording states the condition rather than a remedy, since the remedy differs by why it is unconfirmed, and the map carries it on the marker instead of on the marker's tooltip, which is not interactive and so never sees the pointer. A connection dropping clears the confirmation rather than the position: the last known home is still the best guess available, it just stops being something the vehicle is standing behind. Whether a drawn mission's first item may be that confirmation depends on where the mission came from, which every caller knows and the drawing could only guess at, so it is now passed in. A mission read off the vehicle moves the marker; one restored from storage no longer replaces a home the vehicle reported, which a redraw on any revision bump would otherwise undo, including the refresh taken right after arming. Whether that mission's first item counts as home then depends on the firmware, as only ArduPilot keeps home there and elsewhere it is just the first waypoint. That decision sits in the vehicle store next to the fetch that already owns the same firmware test, so the map states where the point came from and the store decides what it means. On firmware that does not report home the marker moves and keeps its sign, which is what the sign is for. An upload also reads home back, since the vehicle takes it from the first item, so the sign clears once the vehicle holds what was planned rather than standing until the next connection.
9abd90b to
e80d43a
Compare
Review follow-up — round 8Done
The caveat you name was mine to get right the first time: round 9's option 1 said the mission's home is "confirmed by definition", but I had argued the opposite about PX4 in the same follow-up, so the contradiction was visible in my own reasoning before it reached the code. |
|
/review |
✅ READY TO MERGE (Automated PR Review — round 11)
Home used to be commanded straight to the vehicle from two places, and the map used to guess at it from whatever was lying around — a stored mission, or the operator's own laptop location. This PR removes both command paths, so the only way a home reaches the vehicle is as the first item of a mission the user uploads. The map now goes back to only displaying home: it asks the vehicle for its real one when a vehicle connects and again when it arms, and marks the point on screen when nobody has confirmed it, so an operator can tell a real return point from a placeholder. The mission planner keeps its own home separate from the one on the map, since a plan and a vehicle are allowed to disagree until upload. What still needs attentionNothing is open. Every finding raised over the eleven rounds is closed. Since round 10 — 1 closed, comparing 9abd90b → e80d43aRange. The real increment is two files: a new ✅ 1.11 — A downloaded mission's item 0 sets the vehicle-confirmed flag on every firmware ( The finding asked for three things, and all three landed:
The author took remedy option 2, which the finding named as the better-placed of the two: the firmware knowledge stays in the store that already owns it, and Resolutions. Decisions. Discussion. Change map — what was established before judgingClaims (from the PR body, checked against the code):
Failure site. The misbehaving code is the pair of Entry points.
Nothing here lands on a per-frame or per-incoming-message path, and no changed function came back Invariants.
2. Persistence & User Data — inventory, no findingsThe PR adds no key, removes none, and reshapes none. Two existing keys sit on paths it changes, and one store field it adds is deliberately not persisted:
No machine-specific value is vehicle-synced, no automatic migration is introduced, and Sections with nothing to report (10)1. Correctness & Implementation Bugs — ✅ (traced all four writers of 3. AGENTS.md Adherence — ✅ (the new 4. Security — ✅ (diff is 12 source files with no new dependency, no build/CI/Electron change, no 5. Performance — ✅ (net removal on every path: a 1 Hz 6. UI / UX — ✅ (re-read every user-facing string for sentence case and jargon; the confirmed/unconfirmed sign carries both a glyph and a 7. Code Quality & Style — ✅ ( 8. Commit Hygiene — ✅ (six commits read from 9. Tests — ✅ ( 10. Documentation — ✅ (no Lite/Standalone divergence is introduced — the geolocation path already existed in both builds and no Electron-only API is touched — so the README feature table needs no row) 11. Nitpicks / Optional — ✅ (weighed whether Generated by Claude. This is advisory; a human reviewer must still approve. |
|
@ES-Alexander ready for review. |
ES-Alexander
left a comment
ES-Alexander
left a comment
There was a problem hiding this comment.
Code looks ok, from what I can tell.
I'm not super familiar with how home handling is intended to be managed in the firmware - currently it seems like ArduSub is setting home to its current location whenever a mission is uploaded, even if the vehicle is already armed, so the "mission home" seems either implemented incorrectly or not useful. It's also possible that mission home is only used/supported by non-Sub vehicles, I'm not sure 🤷♂️
Hopefully @Williangalvani can confirm what's supposed to happen.
As a side note, this PR also resolves #2018, right?
3 participants
Summary
Home was being set in two places that both commanded the vehicle immediately: the mission-planning view and the map widget. Planning should not mutate vehicle state before a mission is uploaded, and the map widget's live home-setting works from the GCS side but causes problems on the autopilot side. Rather than ship a half-working feature, this removes the immediate paths entirely and leaves home reaching the vehicle exactly one way — as the mission's first item, on upload. The map goes back to only ever displaying home, and now learns it from the vehicle instead of guessing.
Six commits:
Keep the planning home local to the plan.
setHomePositionnow only updates local planning state. Without the vehicle call there is nothing to await and nothing that can fail, so it became synchronous and the try/catch plus the failure snackbar went with it. The call indownloadMissionFromVehicleis gone too: it existed only to push home to the vehicle, andhome.valueis already set on the line above, so keeping it would have fired a "Home position set to…" snackbar during a download the user never initiated. The plan also keeps its ownplannedHomePositioninstead of sharing the marker the map displays, and the view no longer seeds that from the vehicle — otherwise a reconnection, a redrawn mission, or the operator's laptop location would replace the point the user planned, or fill in one they never chose.Remove the live home-setting from the map widget. This drops the "Set home waypoint" context-menu entry, the home marker's draggability, and the
setHomePositionhelper. It also removes the user-commanded home provenance added in 619afee —missionStore.userCommandedHomePositionand thehomeWasCommandedByUsercheck existed only to tell a home the user dragged apart from one drawn from a mission, and nothing reads them once dragging can no longer command the vehicle.The same commit stops the browser-geolocation callback from seeding the home marker. With nothing else writing home from the map, that would have left the operator's own location standing in for the vehicle's home under a home icon. It centers the map instead, and does it once, through a composable the mission-planning view shares. Both surfaces previously re-centered on every position fix for as long as no home and no vehicle position were known, so panning elsewhere never stuck on a topside that is itself moving; asking for a single fix (
getCurrentPosition) fixes that in both places.Rename the planning action to "set mission home". The planning home point is part of the plan, not a command to the vehicle, so "set home waypoint" invited users to read it as a live action. This renames the checklist entry, the placement button, the context-menu item and its tooltip, and the two snackbars.
Remove the now-unused home-setting command.
vehicleStore.setHomeWaypointand its MAVLink transport have no callers after the two surfaces above stopped commanding home. Dead code makes the next reader work out whether it is live, so it comes back with the PR that gives it a caller again.Show the vehicle's own home on the map. Removing the command left the map with no way of learning the vehicle's home: the geolocation seed is gone, and the planning view's fetch retry went with the command it fed. A vehicle online with no stored mission drew no home marker at all. The map now asks the vehicle for its home through the same
fetchHomeWaypointthe compass HUD already used, extracted into auseVehicleHomePositioncomposable both call from, since the HUD wants it only while it renders home. Arming is watched alongside connection, since a vehicle with no position fix has no home to report on the first attempt. The home marker's watcher also watches the map now, matching the centering watcher beside it, so a home already known when the widget mounts is drawn once the map exists.Sign a home the vehicle has not confirmed. The map draws home from two sources that mean different things: what the vehicle reported, and item 0 of whatever mission is drawn, which for a mission restored from storage is only where home would go on upload. Under one plain home icon, that tells the operator the vehicle would return somewhere it would not. The store now records whether the displayed home is one the vehicle confirmed, and the map and the compass HUD sign it when it is not —
mdi-home-alert, with "The vehicle has not reported this home position" on the marker. A mission read off the vehicle moves the marker, and counts as confirmation only on firmware that keeps home at item 0, which is ArduPilot; a mission restored from storage no longer replaces a home the vehicle reported. An upload reads home back, so the sign clears once the vehicle holds what was planned, and a dropped connection clears the confirmation rather than the position.The planning entry points (context menu, click-to-place tool, marker drag) all keep working and stay local to the plan.
Test plan
Mission planning:
Map widget:
DO_SET_HOMEgoes out.Home confirmation:
Both:
DO_REPOSITIONwas untouched.Fixes #2870.