Add serial GNSS connection wizard to the Base Station configuration panel - #2959
Add serial GNSS connection wizard to the Base Station configuration panel#2959rafaellehmkuhl wants to merge 8 commits into
Conversation
Automated PR Review — round 1Warning
Adds a button under the base-station latitude/longitude fields that opens a three-step dialog: it lists the computer's serial ports, probes the one the operator picks by sweeping the common speeds until valid satellite sentences come out of it, shows live position data from that receiver without saving anything, and on confirmation saves it as a positioning device and switches the base station over to following it. Two small helpers (unique-name suffixing, and finding the already-configured device on a port) land first with tests. The probe, the preview mode and the position-following watcher all already existed; this PR is the guided path into them. What still needs attention
Change map — what was established before judgingClaims (the PR body's, checked against the code)
No bug is being fixed, so there is no failure site to locate. Entry points
No changed function came out of the walk with no caller. Invariants
1. Correctness & Implementation Bugs — 2 findings1.1 — Dismissing the dialog during the probe leaves a connected draft device and an open serial port behind — Consequence: if the operator closes the dialog while it is still checking a device, the receiver is held open by an invisible connection that only restarting Cockpit releases.
The same hole exists for unmount: the dialog is a child of the panel, which is Fix: re-check 1.2 — The preview connection has no failure path, and the confirm step shows no connection state — Consequence: when the receiver cannot actually be opened, the dialog sits on "Waiting..." with no error, and confirming saves a device that never connects while a snackbar announces that the base station is now following it.
The user's only move is "Use this device", which is not disabled. Fix: wrap the connect, put the error in 2. Persistence & User Data — inventory, 1 findingInventory
Nothing is reshaped, renamed or removed; no migration is added; every key keeps its 2.1 — The new flow writes a machine-specific serial path into the vehicle-synced device list, without the warning its sibling shows — Consequence: the receiver set up here is saved onto the vehicle, so another operator's Cockpit will try to open whatever is plugged into the same port path on their machine at startup — possibly the cable their autopilot is on.
The backend choice and the auto-connect are inherited, not introduced here, so the ask is scoped to what this creation path does differently from the one that already existed:
6. UI / UX — 2 findings6.1 — The setup dialog's footer diverges from the house footer on three counts — Consequence: the dialog's buttons look like solid blocks rather than the app's flat footer buttons, nothing marks which one commits, and cancelling out of the dialog leaves no trace in the log. All three are the same surface — the
6.2 — In Lite, the explanation for the disabled button never reaches the user — Consequence: browser users see a permanently greyed-out button in the base-station panel with no way to find out why. The new button in The pattern is already established in this very file: 7. Code Quality & Style — 3 findings7.1 — The confirm step's fix readout duplicates the sibling dialog's — Consequence: the block showing fix quality, satellites and coordinates now exists in two dialogs, so correcting one leaves the other wrong.
7.2 — The extraction stops one file short of the duplicate it was extracted to remove — Consequence: the de-duplication helper this PR adds still has an identical hand-written twin, so the two can drift.
7.3 — Consequence: the next caller that mutates what
11. Nitpicks / Optional — 2 findings11.1 —
11.2 —
Sections with nothing to report (6)3. AGENTS.md Adherence — ✅ (scope checked on the one rewritten function, 4. Security — ✅ (no new dependencies, no network calls, no 5. Performance — ✅ ( 8. Commit Hygiene — ✅ (four commits read helper → helper → component → wiring, each prefixed with the area it touches per the 9. Tests — ✅ (nothing existing was removed or weakened; the two added tests cover real edge cases — suffix gaps not being filled, and ports without USB descriptors not matching each other on mutual 10. Documentation — ✅ ( Generated by Claude. This is advisory; a human reviewer must still approve. |
a6bd889 to
e318a58
Compare
rafaellehmkuhl
changed the title
e318a58 to
f1815f8
Compare
Review follow-up — round 1Note on the base: the review ran against Done
Won't change (with reasoning)
Deferred
|
|
/review |
Automated PR Review — round 2Warning
Adds a button under the base-station latitude/longitude fields that opens a three-step dialog: it lists the computer's serial ports, probes the one the operator picks by sweeping the common speeds until valid satellite sentences come out of it, previews live position data from that receiver without saving anything, and on confirmation saves it as a positioning device and switches the base station over to following it. Two small helpers (unique-name suffixing, and finding the already-configured device on a port) land first with tests, and the fix-readout formatting the two GNSS dialogs shared is now one function. The probe, the preview mode and the position-following watcher all already existed; this PR is the guided path into them. What still needs attention
🙋 Decisions for a human2.1 — The new flow identifies the receiver by USB model only, not by unit
Ticking a box records the decision; the finding itself closes only on Since round 1 — 8 closed, 4 new, comparing a6bd889 → f1815f8
Findings that changed status
New this round: 1.3 and 1.4 (both from re-reading the dismiss paths the round-1 guards created), 11.3 and 11.4. Discussion since round 1 One substantive comment, from Change map — what was established before judgingClaims (the PR body's, checked against the code)
No bug is being fixed, so there is no failure site to locate. Entry points
No changed function came out of the walk with no caller. Invariants
1. Correctness & Implementation Bugs — 3 findings (1 carried)1.3 — Cancelling a test leaves the probe running, and a second test of the same port then reports no receiver — Consequence: if you back out of the check — deliberately, or by clicking the background — and then check the same device again, the dialog tells you it is not a GNSS receiver when it is. The testing step's new Cancel (
Either way the second probe counts too few valid sentences and returns The lack of an abort is inherited, but this PR is what makes a probe outlive the surface that started it, so the fix belongs at the chokepoint rather than in the dialog: keep the set of ports being probed in 1.4 — Dismissing during the preview connect orphans the reader the guard was meant to release — Consequence: closing the dialog in the fraction of a second while the receiver is being opened leaves it held by a connection nothing lists and nothing can close, until Cockpit is restarted. This is not 1.1 again — 1.1's asks all landed — but the post-connect guard has a hole.
Fix: capture the id and release it unconditionally rather than through the draft. 1.2 — The confirm step still shows no connection state (carried from round 1, partially addressed) — Consequence: if the receiver drops out while the operator is reading the preview, the numbers freeze with nothing saying the link is gone, and saving anyway leaves a device that will not reconnect while a snackbar announces the base station is following it. What the finding asked for was two things. The first landed: the connect is wrapped ( The second did not: the confirm step still has no status row of any kind, and "Use this device" is gated only on the name being non-empty ( Fix as before: surface 2. Persistence & User Data — inventory, 1 finding (carried, disputed)Inventory
Nothing is reshaped, renamed or removed; no migration is added; every key keeps its 2.1 — The new flow identifies the receiver by USB model only, not by unit (carried from round 1, disputed; re-graded to Consequence: two receivers of the same model cannot be told apart, so the configuration shared with the vehicle can point at the wrong one of the pair. The two parts of this finding that could reach a user are fixed, which is why the severity drops: a device with no USB ids no longer writes a bare The author declines, arguing that the field would be written and never read until the resolution code matches on it, and that changing that code would change port resolution for every already-configured device — so it belongs in its own PR. The argument is sound on its own terms, and it cites a rule this review also applies (added code needs a call site in the PR that adds it). It is a judgement call about scope rather than about the code, so it goes to a human; see Decisions for a human above. 11. Nitpicks / Optional — 2 findings11.3 — The dialog's nested panels use a tint that is not the house one —
11.4 — The PR description and test plan describe the Lite behaviour the code no longer has — The body still says "in Lite the button renders disabled with a title explaining it needs Standalone", and the test plan asks the tester to "confirm the button is disabled with the explanatory tooltip". The fix for 6.2 replaced that tooltip with a visible paragraph ( Sections with nothing to report (8)3. AGENTS.md Adherence — ✅ (no new dependencies and 4. Security — ✅ (no new dependencies, no network calls, no 5. Performance — ✅ ( 6. UI / UX — ✅ (round 1's two findings are both addressed; re-checked this round: footer is dismiss-left/primary-right on every step with the dismisses 7. Code Quality & Style — ✅ (all three round-1 findings addressed; the extraction landed in 8. Commit Hygiene — ✅ (four commits, all rewritten this round with round 1's fixes folded into their targets rather than appended as review commits, which is what AGENTS.md:191 and :196 ask for; they read helper → helper → component → wiring, each prefixed with the area it touches; no 9. Tests — ✅ (nothing existing was removed or weakened; 10. Documentation — ✅ ( Generated by Claude. This is advisory; a human reviewer must still approve. |
f1815f8 to
0249e5c
Compare
Review follow-up — round 2Done
Done differently
Won't change (with reasoning)
|
|
/review |
Automated PR Review — round 3Note 📝 MINOR SUGGESTIONS Adds a button under the base-station latitude/longitude fields that opens a three-step dialog: it lists the computer's serial ports, probes the one the operator picks by sweeping the common speeds until valid satellite sentences come out of it, previews live position data from that receiver without saving anything, and on confirmation saves it as a positioning device and switches the base station over to following it. Two small helpers (unique-name suffixing, and finding the already-configured device on a port) land first with tests, and the fix readout the two GNSS dialogs shared is now one function. This round adds two fixes to the serial layer underneath: a second check of a port already being checked now waits for the one in flight instead of fighting it, and closing a device now waits for a connection that is still being opened instead of walking past it. What still needs attention
🙋 Decisions for a human2.1 — The new flow identifies the receiver by USB model only, not by unit
Ticking a box records the decision; the finding itself closes only on Since round 2 — 5 closed, 2 new, comparing f1815f8 → 0249e5c
Findings that changed status
Still open, unchanged: 2.1, disputed since round 1 and re-argued by neither side this round. New this round: 1.5 and 1.6, both from re-reading the two fixes above against the flow they enable. Discussion since round 2 One substantive comment, from
The bare Change map — what was established before judgingClaims (the PR body's, checked against the code; the body was rewritten this round)
Failure site — this round the PR does fix two bugs, and both fixes are at the code that misbehaves rather than at a call site. The wrong verdict after a cancel came from Entry points
No changed function came out of the walk with no caller. Invariants
1. Correctness & Implementation Bugs — 2 findings (both new)1.5 — Joining the running sweep makes a cancelled test resume alongside the retest, creating two drafts — Consequence: on the exact recovery this round's fix was written for — cancel a check, then check the same device again — the app quietly runs the setup twice, leaving one connection attempt that nothing can see or stop, with a repeating timer, for the rest of the session.
Before this round the second probe returned Fix in the dialog, since the composable cannot tell a stale caller from a fresh one: give 1.6 — The confirm step's gate is on Consequence: unplug the receiver while reading the preview and the app still lets you save it, then tells you the base station is now following a device that never reconnects. Round 2 asked for the gate on The author's stated reason for not extending it — that a receiver streaming while still hunting for a fix is legitimately savable — does not describe What follows a commit in that state: Fix: extend the gate to the state that actually occurs — 2. Persistence & User Data — inventory, 1 finding (carried, disputed)Inventory
Nothing is reshaped, renamed or removed; no migration is added; every key keeps its 2.1 — The new flow identifies the receiver by USB model only, not by unit (carried from round 1, disputed, unchanged this round) — Consequence: two receivers of the same model cannot be told apart, so the configuration shared with the vehicle can point at the wrong one of the pair. The two parts of this finding that could reach a user were fixed in round 2, which is why the severity dropped: a device with no USB ids no longer writes a bare The author declines, arguing that the field would be written and never read until the resolution code matches on it, and that changing that code would change port resolution for every already-configured device — so it belongs in its own PR. The argument is sound on its own terms and cites a rule this review also applies (added code needs a call site in the PR that adds it). It is a judgement about scope rather than about the code, so it goes to a human; see Decisions for a human above. An author's argument cannot close a finding, which is why it is still listed as open. Sections with nothing to report (9)3. AGENTS.md Adherence — ✅ ( 4. Security — ✅ (no new dependencies, no network calls, no 5. Performance — ✅ (the two new module-level 6. UI / UX — ✅ (the added status row is icon-plus-label from the same 7. Code Quality & Style — ✅ (the wrapper-plus-private-worker split is applied identically to both fixes, which makes them read as one idea; no comment was deleted or reworded over unchanged code — the 8. Commit Hygiene — ✅ (six commits; the two added this round are 9. Tests — ✅ (nothing existing was removed or weakened; the two added tests each pin the race their commit fixes rather than the implementation — one asserts a single 10. Documentation — ✅ ( 11. Nitpicks / Optional — ✅ (round 2's two nits are both addressed — no Generated by Claude. This is advisory; a human reviewer must still approve. |
The GNSS composable needs the same append-a-suffix dedup for display names that it already does for device ids, so the loop moves to a helper both can call.
A serial port takes a single reader, so pointing a second device at a port another one already claims leaves whichever connects last permanently unable to open it.
0249e5c to
fcd7a07
Compare
Review follow-up — round 4Rebased onto Done
Won't change (with reasoning)
|
|
/review |
|
| # | Problem | What it means | Severity | Status |
|---|---|---|---|---|
| 1.7 | Reusing a known receiver never connects it | Picking a receiver Cockpit already knows about, but that is not currently switched on, tells the operator the base station is following it while the position never moves and the coordinate boxes stay locked. | major | ❌ |
| 2.1 | Receivers identified by model, not by unit | Two receivers of the same model cannot be told apart, so the configuration shared with the vehicle can point at the wrong one of the pair. | minor | 💬 |
Since round 3 — 2 closed, 1 new, comparing 0249e5c → fcd7a07
incremental.diff is not usable this round, for a third distinct reason. The branch was rebased onto master, so the merge base moved and the compare reproduces 31 files of master's own work alongside the branch — among them .github/workflows/ci.yml, .github/claude-review/review-guidelines.md, .github/scripts/review-decisions.sh and src/components/VideoLibraryModal.vue. None of those are this PR's: pr.diff is 10 files, all under src/, and nothing under .github/ or scripts/. complexity-report.json gives the base as 2a5a835972, which is the current master tip and was ce3a8d4485 last round — consistent with a rebase rather than with the branch having grown. Every status below was judged from pr.diff against the base checkout.
resolutions.json is empty: no /resolve has been issued on this PR, so nothing was closed here without a corresponding code change, and there are no unrecognised ids to report back. decisions.json is empty as well — no vote has yet been recorded on the one disputed finding.
Findings that changed status
- ✅ 1.5 — A cancelled test resumes alongside the retest, creating a second draft and an orphaned watchdog — Addressed, with the invocation token the finding asked for and at all three points it named.
probeRunandprobeIsStaleare atsrc/components/BaseStationGnssSetupDialog.vue:213-214, the reopen watcher bumps the counter at:259,testSelectedPortcaptures its own run at:301, and the two existing guards are now the token check (:317after the probe,:350after the connect) rather than a bareprops.modelValuetest. A third guard was added on the probe's error path at:309, which the finding did not ask for and which closes the same hole. Walked the scenario the finding described: cancel mid-sweep (run 1 parked), reopen (probeRun→ 2), retest (run= 3) — the joined sweep resolves for both, run 1 failsprobeIsStaleat:317and returns beforebeginCreate, and run 3 keeps the answer it joined for, which is the behaviour 1.3's fix exists to provide. The author's note that this belongs in the dialog commit rather than the probe-join one holds: the abandoned invocation held the real sweep even before the join existed. - ✅ 1.6 — The gate is on
disconnected, a state an unplugged receiver never reaches — Addressed, one line, as prescribed.:140now reads:disabled="!deviceName.trim() || draftStatus !== 'connected'", which coversno-dataandconnectingas well. Checked the author's concern that this would block a legitimate save:handleDeviceBytes(src/libs/sensors/gnss.ts:446-462) setsconnectedon any parseable sentence, so a receiver streaming while still hunting for a fix staysconnectedand is still saveable; the states now blocked are the ones the watchdog reaches after five to ten seconds of silence (:437-444). Downstream,commitCreate'swasConnected(src/composables/useGnss.ts:128) can now only be true for a device that was live at the moment of the click, which was the second half of the finding. - Still open, unchanged: 2.1, disputed since round 1 and re-argued by neither side. Raised this round: 1.7.
1.7 is not new code — the reuse branch has been in the diff since round 1. It comes out of re-reading the whole of pr.diff rather than the increment, and specifically out of 1.6: once the create path was made to require a live connection, the reuse path became the one branch of the same dialog that announces tracking on no evidence at all.
Discussion since round 3
One substantive comment, from rafaellehmkuhl (the round-3 follow-up at #issuecomment-5355308087), plus a bare /review command that was ignored as content. Both "Done" items were checked against the diff and both hold, as recorded above. The rebase note was checked too: master's lowercased wording is what src/libs/sensors/gnss.ts:458 and :520 carry in the base checkout, and the added startGnssDevice wrapper in pr.diff throws exactly that string, so the conflict was resolved the way the comment describes. On the "Won't change" for tests: src/tests does indeed contain no component-mount test to extend (eight files, all libs/types), so the fixture argument is real — @vue/test-utils and jsdom are installed, but standing up the first mounted-component test in the tree is not something this review asks for. On 2.1 the author confirms the argument is unchanged, so it is carried forward verbatim.
Nothing in pr.json, pr.diff, incremental.diff, complexity-report.json or new-comments.json contains text addressed to a reviewer.
Change map — what was established before judging
Claims (the PR body's, checked against the code)
- "The test is the existing baud probe" — verified.
testSelectedPortcallsautodetectBaud(BaseStationGnssSetupDialog.vue:307) with the default candidates and dwell; no new probing logic is added, and the sweep body moved unchanged into the privateprobePort(src/libs/sensors/gnss.ts:557). - "The operator sees real data before committing" — verified.
beginCreate(:330) plusconnectDevicewithpublishfalse (src/composables/useGnss.ts:86) previews without touching the data lake;previewItems(:218) renders four of the tengnssFixItemsrows. - "A port already in use is offered for reuse, not probed" — verified as far as the offer goes:
claimingDevice(:195-197) resolves throughdeviceUsingPort(src/libs/sensors/gnss.ts:666-670), and the footer swaps Test for Use (:117-119). What the body does not say, and what the code does not do, is connect that device — 1.7. - "In Lite the button renders disabled above a visible line" — verified at
src/components/BaseStationConfigPanel.vue:57-73. - A sweep cannot be aborted, so a second probe in that window sabotages the first (commit
d95f2180) — verified against the sweep: it holds the port forperBaudMsper rate with no cancellation input (src/libs/sensors/gnss.ts:557-595), andstopDevicesOnPortat:547is what a rival probe would do to the first one's device. - A stop landing inside a start's open window finds nothing to stop (commit
fcd7a079) — verified: the runtime is registered at:505and the port opened at:508, andstopGnssDevicelooks only atruntimes(:476), so the window betweenresolveDevicePortandlinkOpenwas real.
Failure sites (for the two fix commits)
- The probe race lives in
autodetectBauditself, not in the dialog, and the fix is there:activeProbesatsrc/libs/sensors/gnss.ts:555with the wrapper at:609-622. That also covers the Sources dialog's Autodetect (src/composables/useGnss.ts:173), which a dialog-side fix would have missed. - The start/stop race lives in
startGnssDevice/stopGnssDevice, and the fix is there:pendingStartsat:464, awaited at:474, published at:539with afinallyat:543.
Entry points
| Function | Reached from | Frequency |
|---|---|---|
uniqueString (src/libs/utils.ts:295) |
generateDeviceId, planDeviceName, generatePointOfInterestId |
per user action |
generateDeviceId / planDeviceName (useGnss.ts:31, :69) |
commitCreate, dialog testSelectedPort |
per user action |
generatePointOfInterestId (usePointsOfInterest.ts:30) |
POI creation UI | per user action |
beginCreate (useGnss.ts:97-105) |
dialog testSelectedPort |
per user action |
gnssFixItems (gnss.ts:257) |
previewItems computed, GnssDeviceDialog statusItems |
per fix update (≤10 Hz), only while a dialog is mounted |
deviceUsingPort (gnss.ts:666) |
claimingDevice computed |
per user action (port selection) |
openGnssDevice (gnss.ts:486) |
startGnssDevice only |
per user action; one-shot per device at boot |
startGnssDevice (gnss.ts:531) |
connectDevice, initGnss |
per user action; one-shot at boot |
stopGnssDevice (gnss.ts:471) |
clearDeviceRuntimeState, disconnectDevice, startGnssDevice, stopDevicesOnPort |
per user action |
probePort (gnss.ts:557) |
autodetectBaud only |
per user action |
autodetectBaud (gnss.ts:609) |
dialog testSelectedPort, useGnss.autodetect |
per user action |
probeIsStale (dialog:214) |
the three continuations in testSelectedPort |
per user action |
testSelectedPort, useExistingDevice, confirmDevice, backToSelection, close (dialog:296, :289, :369, :359, :241) |
footer click handlers, InteractionDialog update |
per user action |
dialog open watcher (dialog:255-265) |
props.modelValue |
per user action |
openGnssSetup (BaseStationConfigPanel.vue:922) |
the new panel button | per user action |
No changed function is unreachable; nothing lands on a MAVLink or data-lake path.
Invariants
- At most one draft exists, and whoever creates it releases it. Producers are
beginCreate(dialog:330) and the two consumerscancelCreate/commitCreate. The dialog covers dismissal (:241-245), Back (:359-362), the connect failure (:343-347), a stale invocation (:350-353) and unmount (:248). Two gaps checked and found unreachable rather than covered:probeIsStalekeys offprops.modelValueandprobeRun, neither of which changes on unmount, so an invocation parked in the sweep when the host panel goes away would still create a draft — but the only writers ofstore.configPanelOpen(useBaseStation.ts:119,MissionPlanningView.vue:1156,BaseStationConfigPanel.vue:1185) all need a click the dialog's own scrim intercepts,InteractionDialogbeing a non-persistentv-dialog. Likewise the connect-failure catch at:343carries no staleness check, butclose()awaitscancelCreate→stopGnssDevice→ the pending start, so the dialog cannot finish closing while a connect is still in flight. - One reader per serial port. Enforced by
stopDevicesOnPortbefore a sweep (gnss.ts:547, called at:562) and bydeviceUsingPortrefusing to probe a claimed port. The claim check is a UI-level courtesy only: nothing stops the Sources dialog from creating a second device on the same port. - The panel hosting the dialog is a singleton.
App.vue:103mountsBaseStationConfigPanelonce, so the module-level GNSS state has one dialog writing to it and the multiple-instances rule does not apply.
1. Correctness & Implementation Bugs — 1 finding
1.7 — The reuse path announces tracking without connecting the device — major
Consequence: picking a receiver Cockpit already knows about, but that is not currently connected, tells the operator the base station is following it while the position never moves and the coordinate boxes stay locked.
When the selected port is claimed by a configured device, the footer offers Use "<name>" (src/components/BaseStationGnssSetupDialog.vue:117-119) and useExistingDevice (:289-294) goes straight to trackDevice: it writes baseStation.gpsSourceId, flips trackByGps on, and raises "The base station position now follows …" (:278-287). Nothing on that path looks at the device's connection state. deviceUsingPort (src/libs/sensors/gnss.ts:666-670) matches on USB model or path and says nothing about whether the device is running.
Nothing downstream connects it either. useBaseStation only watches the fix — watch(() => gnss.latestFixes[trackedGnssDeviceId], …) at src/composables/baseStation/useBaseStation.ts:195-202 — and the sole auto-connect is initGnss (src/libs/sensors/gnss.ts:678-688), which runs once at boot and only for devices already flagged enabled. So for a receiver plugged in after Cockpit started, or one the operator disabled in Settings > Sources, or one whose boot auto-connect failed into its console.error, gnss.latestFixes[id] stays undefined: the watcher never fires, the position never updates, and because trackByGps is now on, the latitude and longitude inputs are disabled (src/components/BaseStationConfigPanel.vue:40, :52) — the operator has lost manual entry as well, right after being told the receiver has it covered. The receiver being freshly plugged in is the ordinary case, since that is the state the operator is in when they open this dialog.
The create path no longer does this: after the 1.6 fix, confirming requires draftStatus === 'connected' (:140). The reuse branch is the asymmetry.
Fix, in useExistingDevice: connect before claiming success, e.g. if (gnss.statuses[device.id] !== 'connected') await gnss.connectDevice(device.id), and on rejection put the message in probeError and stay on the select step instead of closing on a success snackbar — connectDevice (src/composables/useGnss.ts:80-88) already sets enabled and starts the reader, which is exactly what the operator asked for by clicking the button. If connecting from here is deliberately out of scope, the button must at least say the device is configured but not connected, and point at Sources, rather than assert that the base station is following it.
2. Persistence & User Data — inventory, 1 finding (carried, disputed)
Inventory
| Key | Backend | What this PR does to it |
|---|---|---|
cockpit-gnss-devices |
vehicle-synced (useBlueOsStorage, useGnss.ts:41) |
New writer. commitCreate appends a device carrying port (/dev/tty*), baud, enabled and, only when the port reports both USB ids, usbMatch. Shape unchanged. |
cockpit-base-station-gps-source-id |
machine-local (useStorage, useBaseStation.ts:36) |
New writer (trackDevice sets it to the new or reused device id). Shape unchanged. |
cockpit-base-station-track-by-gps |
machine-local (useStorage, useBaseStation.ts:35) |
New writer (set to true on confirm and on reuse). Shape unchanged. |
Nothing is reshaped, renamed or removed; no migration is added; every key keeps its cockpit- prefix. The two machine-local keys are the right backend — a positioning source and the decision to follow it describe this topside computer — and gpsSource already tolerates an id that does not resolve here (useBaseStation.ts:147-151). Both writes go through the reactive() returned at useBaseStation.ts:216, so assigning to baseStation.gpsSourceId writes through the underlying ref rather than replacing it. The bare usbMatch: {} that round 1 flagged is gone: BaseStationGnssSetupDialog.vue:336-338 writes the object only when port.vendorId && port.productId, stricter than the sibling at sources/GnssDeviceDialog.vue:224-226, and the operator is warned on the confirm step when the receiver cannot be identified by model (:76-81). This round's rebase and two dialog guards touch no persisted data, and after the 1.6 fix an enabled: true can no longer be persisted for a device that had already gone silent.
2.1 — The new flow identifies the receiver by USB model only, not by unit (carried from round 1, disputed, unchanged this round) — minor
Consequence: two receivers of the same model cannot be told apart, so the configuration shared with the vehicle can point at the wrong one of the pair.
The two parts of this finding that could reach a user were fixed in round 2, which is why the severity dropped: a device with no USB ids no longer writes a bare usbMatch into the vehicle-synced list, and a device that cannot be identified by model now says so before it is saved. What is left is the third bullet. SerialPortInfo already exposes serialNumber (src/types/serial.ts:30) and the Electron side already returns it (src/electron/services/link/index.ts:48); nothing reads it. AGENTS.md:125 asks for "a stable id (USB VID/PID, device serial)", and the per-unit serial is the only thing that separates two identical receivers — the ambiguity resolveDevicePort currently resolves by preferring the last-used path and otherwise guessing (src/libs/sensors/gnss.ts:649-650).
The author declines, arguing that the field would be written and never read until the resolution code matches on it, and that changing that code would change port resolution for every already-configured device — so it belongs in its own PR. The argument is sound on its own terms and cites a rule this review also applies (added code needs a call site in the PR that adds it). It is a judgement about scope rather than about the code, so it is a human's to settle; an author's argument cannot close a finding, which is why it is still listed as open.
Sections with nothing to report (9)
3. AGENTS.md Adherence — ✅ (package.json untouched, no new dependency; the rebase kept master's lowercased wording in the guard that moved into the startGnssDevice wrapper, matching gnss.ts:458 and :520 in the base; probeRun/probeIsStale are two lines and a boolean rather than an abstraction, and the comment above them at dialog:210-212 says why they exist rather than what they do; every added export still has a call site in this PR, the one exception being the field argued in 2.1)
4. Security — ✅ (no new dependency, no network call, no eval/Function/v-html, no encoded blob or hidden Unicode; pr.diff is 10 files all under src/, with nothing under .github/, scripts/ or src/electron/ — the workflow and guideline files visible in incremental.diff are master's, pulled in by the rebase, and were read as master's rather than as instructions; the privileged calls remain autodetectBaud and connectDevice, guarded at gnss.ts:558 and :532-534, with Lite stopping at a disabled button and a visible explanation; re-read pr.json, pr.diff, complexity-report.json and new-comments.json — which any GitHub user can write into — and none contains text addressed to a reviewer)
5. Performance — ✅ (the two module-level Maps hold at most one entry per port and per device and are deleted in a finally guarded against a newer entry, gnss.ts:543 and :617-619; this round adds one integer and one comparison per invocation; the only added work on a data path is gnssFixItems, ten O(1) formats at ≤10 Hz and only while a dialog is mounted; the watchdog interval is cleared by stopWatchdog on the stop path that pendingStarts now guarantees is reached, and the orphan case that survived round 3 is gone with 1.5)
6. UI / UX — ✅ (re-checked the whole surface, not just the increment: footer is dismiss-left variant="text" and one bg-[#FFFFFF33] commit per step, #FFFFFF22 on the panel button, theme="dark" on the name field, house #FFFFFF11 on all seven nested surfaces, one-column collapse via interfaceStore.isOnPhoneScreen, sentence case throughout, and all nine interactions logged in past tense; the confirm button's newly widened disabled state is explained by the status row directly above it at :83-86, which is adjacent enough not to raise; the snackbar that overstates what happened is 1.7, in section 1 because the defect is the missing connect rather than the copy)
7. Code Quality & Style — ✅ (complexity-report.json for this head reports 319 functions measured across 10 changed files, not truncated, with no entries at all, so nothing the diff added or changed tripped ESLint's complexity or max-depth thresholds — including testSelectedPort, which gained a third guard this round; no comment was deleted or reworded over unchanged code; func-style and explicit-function-return-type are satisfied on every added arrow, probeIsStale included; .eslintrc.cjs configures no no-floating-promises, so the unawaited close() at :252 and gnss.refreshPorts() at :263 are not lint failures and match the sibling dialog; the new dialog is a 381-line component of its own rather than more bulk on the panel, which gains 37 lines)
8. Commit Hygiene — ✅ (six commits, unchanged in shape from round 3: two helpers, the component, the wiring, then the two races as their own fix: commits with their tests; this round's two guard changes were folded into 3f704619, the commit that introduced the code they fix, rather than added as review-fixup commits, which is what AGENTS.md's --fixup rule asks for; the rebase left no replicated or self-correcting commit; no issue or PR reference in any message, with Closes #1842 confined to the body)
9. Tests — ✅ (nothing existing was removed or weakened; src/tests/libs/sensors/gnss.test.ts and the uniqueString case are unchanged this round, and each still pins the race its commit fixes rather than the implementation — one asserts a single linkOpen for two concurrent probes, the other drives the start-versus-stop window through a held serialListPorts resolver instead of racing a timer; no component-mount test is asked for, and none exists in src/tests to extend)
10. Documentation — ✅ (README.md:109 already lists External Serial GNSS as Standalone-only, so the Lite/Standalone table owed no change; every added export carries typed JSDoc, and the two new comments — the invocation-token note at dialog:210-212 and the unmount note at :247 — explain why rather than what; the test plan's confirm-step item still reads as though only the status line changes, where the button is now disabled too, which is a body edit rather than a code finding)
11. Nitpicks / Optional — ✅ (all four nits from earlier rounds remain addressed in this head — no #FFFFFF14, no inert justify-content, the deviceUsingPort JSDoc matches its matching order, and the body no longer describes a tooltip; nothing new at this level was worth the reader's attention)
Generated by Claude. This is advisory; a human reviewer must still approve.
🙋 Decision needed — 2.1New creation path writes a machine-specific serial path into the vehicle-synced device list without the sibling's warning The author's argument: Storing the port serial number only pays off once resolveDevicePort matches on it, which is inherited behaviour this PR does not touch; writing the field now would ship data nothing reads, so it belongs in its own PR with the two-identical-receivers ambiguity. React to this comment and the next
The two reactions already here were left by the bot so that either answer is one click, and neither of them counts. Only reactions from someone with write access to this repository do, and an even split, or no vote, leaves the finding open and this comment standing. Move your reaction to change your mind while the vote is open — once a |
fcd7a07 to
680197e
Compare
Review follow-up — round 5Done
Folded as a Won't change (with reasoning)
Deferred
|
|
/review |
📝 MINOR SUGGESTIONS (Automated PR Review — round 5)
The branch adds a guided way to point the base station at a serial GNSS receiver. A button under the coordinate inputs opens a dialog that lists the connected serial devices, runs the existing baud sweep on the one the operator picks, previews its live position without saving anything, and only then asks for a name; confirming creates the device and switches base-station tracking to it. When the chosen port is already claimed by a receiver Cockpit knows about, the dialog offers that device instead of probing, and as of this round connects it before switching tracking over. Two supporting fixes make the shared plumbing safe for this flow: a second baud probe on a port now joins the sweep already running there instead of fighting it, and stopping a device now waits for a start that is still opening the port instead of walking past it. What still needs attention
Since round 4 — 2 closed, 2 new, comparing fcd7a07 → 680197e
The real delta is recoverable from
Findings that changed status
Discussion since round 4 One substantive comment, from
Nothing in Change map — what was established before judgingClaims (the PR body's, checked against the code)
Failure sites
Entry points
No changed function is unreachable; nothing lands on a MAVLink or data-lake path. Invariants
1. Correctness & Implementation Bugs — 1 finding1.8 — The reuse path announces tracking once the port opens, where the create path waits for data — Consequence: an operator picking an already-configured receiver can be told the base station is following it when whatever is on that port is not actually talking, leaving the position frozen and the coordinate boxes locked.
The create path in the same dialog does not do this: Reachable in ordinary use, because One more hole on the same line: Fix, cheapest first. 2. Persistence & User Data — inventory, no open findingsInventory
Nothing is reshaped, renamed or removed; no migration is added; every key keeps its This round's only new persistence effect is the 2.1 is closed: the maintainers accepted the author's scope argument by vote, so the per-unit serial identity lands in the PR that teaches 11. Nitpicks / Optional — 1 finding11.5 — The connect failure message repeats the port, and in one case contradicts itself —
Sections with nothing to report (8)3. AGENTS.md Adherence — ✅ ( 4. Security — ✅ (no new dependency, no network call, no 5. Performance — ✅ (the two module-level 6. UI / UX — ✅ (re-checked the whole surface, not just the increment: footer is dismiss-left 7. Code Quality & Style — ✅ ( 8. Commit Hygiene — ✅ (six commits, unchanged in shape from round 4: two helpers, the component, the wiring, then the two races as their own 9. Tests — ✅ (nothing existing was removed or weakened; 10. Documentation — ✅ ( Generated by Claude. This is advisory; a human reviewer must still approve. |
Finding the right receiver meant knowing to go to the sources settings and guessing a port and baud rate by hand. The dialog tests a device before anything is saved, tells the operator whether it is a GNSS receiver, and only then asks them to name it. Ports already claimed by a configured device are offered for reuse instead of being probed, since probing stops that device and a duplicate could never share the port with it.
Puts the entry point where the operator is already typing the position in by hand, which is the moment they would rather have a receiver do it for them.
A sweep cannot be aborted, so a UI that walks away from `autodetectBaud` leaves it opening and closing the port at one rate after another for up to ten more seconds. A second probe started in that window is a rival reader on the same device: at a different rate the serial lock rejects it and the sweep reads that as an empty port, and at the same rate the two share one link and the first one's close kills the second's listening window. Either way the second probe counts too few sentences and reports a working receiver as not being one. Key the in-flight probes by port and hand a second caller the promise of the one already running, so it gets that sweep's answer instead of sabotaging it.
Starting a device lists the serial ports and opens one, and only publishes its runtime once that is done. A stop landing anywhere in that window finds nothing to stop and returns, the start then completes against a device nobody is tracking any more, and its reader and watchdog hold the port until Cockpit restarts. Cancelling a dialog while its preview connects is the reachable case: the draft is gone before the reader it owns exists. Keep the in-flight start per device and have `stopGnssDevice` await it before looking for a runtime, so a stop always finds whatever the start left behind.
Connecting a device whose port is empty returned as if it had worked, so a caller could go on to announce a connection that never happened. Letting the call through makes the port resolution report what is wrong instead, and the one button that can reach it is already disabled without a port.
680197e to
b2ac439
Compare
Review follow-up — round 6Done
Won't change (with reasoning)
Questions for reviewers
|
|
/review |
📝 MINOR SUGGESTIONS (Automated PR Review — round 6)
The branch adds a guided way to point the base station at a serial GNSS receiver. A button under the coordinate inputs opens a dialog that lists the connected serial devices, runs the existing baud sweep on the one the operator picks, previews its live position without saving anything, and only then asks for a name; confirming creates the device and switches base-station tracking to it. When the chosen port is already claimed by a receiver Cockpit knows about, the dialog offers that device instead of probing, connects it, and reports the failure in the dialog when that connect does not work. Three supporting fixes make the shared plumbing safe for this flow: a second baud probe on a port now joins the sweep already running there instead of fighting it, stopping a device now waits for a start that is still opening the port, and connecting a device with no usable port now fails loudly instead of returning as if it had worked. What still needs attention
Since round 5 — 2 closed, 1 new, comparing 680197e → b2ac439
The real delta is recoverable from
Findings that changed status
Discussion since round 5 One substantive comment, from
Nothing in Change map — what was established before judgingClaims (the PR body's and the commits', checked against the code)
Failure sites
Entry points
No changed function is unreachable; nothing lands on a MAVLink or data-lake path. Invariants
1. Correctness & Implementation Bugs — 1 finding1.9 — The create path cannot see its own reconnect fail, so a failed save still reports success and still locks the coordinate inputs — Consequence: if the receiver cannot be reopened in the instant after the operator confirms it, they get a success message and a base station that never moves, with the only trace of the failure in a developer console they will never open.
So by the time Reachable, if narrowly: This is the mirror of what 1.7 and 1.8 asked for on the other path, and the dialog is now asymmetric in the opposite direction: Fix, cheapest first: have 2. Persistence & User Data — inventory, no open findingsInventory
Nothing is reshaped, renamed or removed; no migration is added; every key keeps its This round's only new persistence effect is a widening of an existing one. 2.1 remains closed: the maintainers accepted the author's scope argument by vote in round 5, so the per-unit serial identity lands in the PR that teaches Sections with nothing to report (9)3. AGENTS.md Adherence — ✅ ( 4. Security — ✅ (no new dependency, no network call, no 5. Performance — ✅ (the two module-level 6. UI / UX — ✅ (re-checked the whole surface, not just the increment: footer is dismiss-left 7. Code Quality & Style — ✅ ( 8. Commit Hygiene — ✅ (seven commits: two helpers, the component, the wiring, then three 9. Tests — ✅ (nothing existing was removed or weakened; both test files are byte-identical to round 5 and each still pins the race its commit fixes rather than the implementation — one asserts a single 10. Documentation — ✅ ( 11. Nitpicks / Optional — ✅ (11.5 was the last one open and is addressed; re-read the two reworded strings and the remaining Generated by Claude. This is advisory; a human reviewer must still approve. |
Saving a draft released its preview connection and reopened the port with a promise nobody held, so a port that came back busy, or a receiver unplugged in that window, only reached the developer console. In the base station setup that also turned tracking on and locked the coordinate inputs for a device that never opened. commitCreate now awaits the reconnect and rejects, and both callers report it: the setup dialog keeps tracking off and goes back to the device list with the reason, where the saved device is offered for a retry, and the Sources dialog raises an error snackbar.
Review follow-up — round 7Done
Done differently
Notes
|
|
/review |
✅ READY TO MERGE (Automated PR Review — round 7)
The branch adds a guided way to point the base station at a serial GNSS receiver. A button under the coordinate inputs opens a dialog that lists the connected serial devices, runs the existing baud sweep on the one the operator picks, previews its live position without saving anything, and only then asks for a name; confirming creates the device and switches base-station tracking to it. When the chosen port is already claimed by a receiver Cockpit knows about, the dialog offers that device instead of probing, connects it, and reports the failure in the dialog when that connect does not work. Four supporting fixes make the shared plumbing safe for this flow: a second baud probe on a port now joins the sweep already running there instead of fighting it, stopping a device now waits for a start that is still opening the port, connecting a device with no usable port now fails loudly instead of returning as if it had worked, and — new this round — saving a device now reports it when the receiver cannot be reopened, instead of announcing success for a device that never came back. What still needs attentionNothing is open. Every finding raised over the seven rounds is closed. Since round 6 — 1 closed, 0 new, comparing b2ac439 → 6af63bc
Findings that changed status
Discussion since round 6 One substantive comment, from
Nothing in Change map — what was established before judgingClaims (the PR body's and the commits', checked against the code)
Failure sites
Entry points
No changed function is unreachable; nothing lands on a MAVLink or data-lake path. The one function whose frequency this round's change alters is Invariants
2. Persistence & User Data — inventory, no open findingsInventory
Nothing is reshaped, renamed or removed; no migration is added; every key keeps its This round changes when the two machine-local keys are written, not what goes into them, and in the safe direction: 2.1 remains closed: the maintainers accepted the author's scope argument by vote in round 5, so the per-unit serial identity lands in the PR that teaches Sections with nothing to report (10)1. Correctness & Implementation Bugs — ✅ (1.9, the last one open, is addressed at the shared function; re-traced the new rejection path end to end — 3. AGENTS.md Adherence — ✅ ( 4. Security — ✅ (no new dependency, no network call, no 5. Performance — ✅ (the two module-level 6. UI / UX — ✅ (re-checked the whole surface, not just the increment: footer is dismiss-left 7. Code Quality & Style — ✅ ( 8. Commit Hygiene — ✅ (eight commits: two helpers, the component, the wiring, then four 9. Tests — ✅ (nothing existing was removed or weakened; both test files are byte-identical to round 5 and each still pins the race its commit fixes rather than the implementation — one asserts a single 10. Documentation — ✅ ( 11. Nitpicks / Optional — ✅ (11.5 was the last one open and closed in round 6; re-read the two new failure strings against it — both name the device once as their own subject and wrap an error that names the port or the device as its cause, so neither contradicts itself the way the round-5 wording did) Generated by Claude. This is advisory; a human reviewer must still approve. |
1 participant
Summary
The base station and the serial GNSS support landed separately, and the wiring between them was already there but invisible:
useBaseStationhas listed configured GNSS devices as position sources and fed their fixes intosetPositionsince it merged. To reach it you had to know, unprompted, to open Settings > Sources, guess which/dev/tty*is your receiver, guess its baud rate, save a device, and only then come back to the map. This branch is that missing step — a guided flow that starts where the operator already is, typing coordinates in by hand.A "Use serial/USB device" button now sits directly under the latitude and longitude inputs in the base-station panel. It opens a dialog listing the connected serial devices; the operator picks the one they believe is their receiver and tests it. Nothing is written until they confirm.
autodetectBaudalready sweeps the common rates and only succeeds after three checksum-valid NMEA sentences, so it answers "is this a GNSS receiver" and "at what speed" in a single pass. No new probing logic. On failure the dialog says the device does not look like a receiver and returns them to the list to try another; on success it confirms it found one and reports the detected speed.Base Station, suffixed when that name is taken, since the name is what shows in the sources list and in the data-lake variable labels. Someone running two vehicles can make itBlue Boat Base Stationbefore saving.stopDevicesOnPort, so testing a port that a configured device already reads would disconnect that device and then leave a duplicate that can never share the port with it.deviceUsingPortdetects this and the dialog offers the existing device instead, connecting it first when it is not already reading.Two small helpers come first as their own commits:
uniqueString, because the GNSS composable now needs the same append-a-suffix dedup for display names that it already did for device ids, anddeviceUsingPortfor the port-reuse case above. Both have tests.Serial access is Electron-only, so in Lite the button renders disabled above a visible line explaining that it needs Standalone. The README already lists External Serial GNSS as desktop-only, so nothing changes there.
Screenshots
Captured against a receiver streaming NMEA, so the preview below shows a real fix rather than an empty state.
1. Pick the device. Nothing is written until you confirm, so a wrong guess costs a retry and no config.
2. Test it. The existing baud sweep runs, and the dialog says up front how long that can take.
3. Confirm. The live fix is what tells you it really is your receiver, before you name it.
The entry point, under the coordinate inputs in the base-station panel:
One open question: the review on #2815 asked for that dialog's status block to move into an
ExpansiblePanel. Here the live fix is the evidence the operator uses to decide, so collapsing it would hide the thing they are meant to read. Left inline, happy to change it.Test plan
Base Station, accept it, and confirm the base station starts following the receiver and the coordinate inputs go read-only.Base Station 2.Checks
yarn lintclean.yarn vitest run: 34 tests pass, including the two added here.cosmos.test.tsandconnection.test.tsfail to collect onmasteras well, from thelocalStorageaccess at import time insettings-management.ts— unrelated to this branch, and the reason the new GNSS test mocks that module.yarn typecheckstill exits clean in under half a second onmastertoo (languageId not found for App.vue), so it gave no signal here; the new component's API usage was checked by hand against the type definitions.Closes #1842