docs: quickstart guide for a fresh laptop + remote over Tailscale - #114
docs: quickstart guide for a fresh laptop + remote over Tailscale#114iliabaranov wants to merge 4 commits into
Conversation
Adds docs/QUICKSTART.md: Tailscale account and auth key, laptop join, USB tether, build/flash, first boot, ROS 2 machine node, pairing, and a bench test through STOP and ARM. Appendices: plain-C runner, PoE/LAN/WiFi, what persists across reflash vs erase-flash. Fixes the traps the walkthrough surfaced: - Kconfig ML_TAILSCALE_AUTH_KEY help said "reusable + ephemeral"; the firmware re-sends the key on every re-register and persists the node identity, so it must be reusable, NOT ephemeral, pre-approved. - sdkconfig.credentials.example regrouped required/optional; documents that sdkconfig must be regenerated after editing it. - API.md: document GET/POST /api/role (the second arming gate) and the admin auth + ?confirm=1 on /api/enter_download. - Remove the stale "factory-default peer 10.42.0.1:8890" claim from USB_NCM_SETUP, host/README and TROUBLESHOOTING: a fresh unit has had no peer since #65. Fix the verify snippet that curled the host IP. - ASSEMBLY.md linked to a README anchor that does not exist. - CONTRIBUTING: idf.py monitor goes quiet once the USB tether starts. - README "Build and run" is now a pointer plus the short version. - TAILSCALE_ISOLATION: example email instead of a real one.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good, straightforward documentation change. Reviewed the new docs/QUICKSTART.md and the updated cross-references (README.md, CONTRIBUTING.md, ASSEMBLY.md, host/README.md all now point at it correctly); checked the "factory-default peer" removal in USB_NCM_SETUP.md/TROUBLESHOOTING.md/host/README.md for consistency; verified the new /api/role and /api/enter_download?confirm=1 docs in API.md against the actual handlers in firmware/components/dcs_support/src/dcs_admin_pages.c (admin auth + confirm param match); and confirmed the ROS 2 package/executable names and operators/[[operator]] config keys referenced in the guide exist in ros2/protective_stop_machine and host/machine.toml.
Extended reasoning...
Overview
This PR is documentation-only: it adds docs/QUICKSTART.md (a 290-line end-to-end walkthrough) and updates README.md, CONTRIBUTING.md, hardware/ASSEMBLY.md, host/README.md, docs/API.md, docs/USB_NCM_SETUP.md, docs/TROUBLESHOOTING.md, docs/TAILSCALE_ISOLATION.md, components/microlink/Kconfig, and firmware/sdkconfig.credentials.example. No source code changes are included.
Verification performed
I cross-checked several factual claims in the new/updated docs against the actual firmware and ROS 2 source (which was touched in the immediately preceding commit e67b5c5, "remote-announced operator/stop-only role"):
docs/API.md's new/api/role(GET/POST, admin-gated) and updated/api/enter_download?confirm=1(admin-gated) descriptions matchapi_role_get,api_role_post, andapi_enter_downloadinfirmware/components/dcs_support/src/dcs_admin_pages.cexactly (auth checks, query params, behavior).- The removed "factory-default machine peer at 10.42.0.1:8890" claim is consistently removed across USB_NCM_SETUP.md, TROUBLESHOOTING.md, and host/README.md, replaced with "fresh unit has no peer" language, and the internal anchor fix in hardware/ASSEMBLY.md (
README.md#quickstart→docs/QUICKSTART.md) is correct — no other stale anchors remain. - ROS 2 package name (
protective_stop_machine), executable (machine_bridge_node), topics (~/machine_state,~/remotes,/pstop_hb), and theoperatorsparameter all exist as referenced inros2/protective_stop_machine. host/machine.toml's[[operator]]table andstop_onlyfield match what Appendix A of the quickstart describes.- The Kconfig and sdkconfig.credentials.example changes (reusable/non-ephemeral Tailscale key guidance, required-vs-optional grouping) are consistent with the role/NVS persistence behavior in the firmware.
The guide itself is honest about what hasn't been verified on hardware, marking timing/ring-color/AP-gateway claims with <!-- VERIFY --> comments rather than asserting them as fact.
Security risks
None. This is prose/config-comment documentation; no code paths, auth logic, or credentials are altered. The one example credential file only gains a comment structure and empty-string placeholders (no real secrets introduced).
Level of scrutiny
Low-to-moderate is appropriate here — accuracy-of-documentation review rather than code-correctness review. I spent effort cross-referencing the new API and config claims against actual source to make sure the guide doesn't mislead readers, and found no discrepancies.
Other factors
No CODEOWNERS restriction applies to these paths, the bug-hunting pass reported no findings, and the PR author's own "traps found" table lines up with what the diff actually contains. Given the diligence already visible in the diff and my own source cross-checks, this is a good candidate for approval without requiring further human review.
…pdated The scripted software remote still used the v1 40-byte layout with message codes 0..3, so every datagram was dropped by the size check in the current pstop_c (48 bytes, codewords 0x55/0x92/0xAD/0x6A, two padding words) and "bond failed" was the only possible outcome. It also never announced a role, so even with a fixed codec the AND-rule would have kept it stop-only. Now: v2 codec, aux-channel role in padding1 (--role operator|stop_only, default operator), length-checked decode. --role stop_only asserts the inverse contract (a full press-and-release must NOT arm). Verified against host/machine_app_runner (5/5 + stop_only) and the ROS 2 machine_bridge_node with operators: [16909185] (5/5). TESTING.md documents both targets and the stop_only mode; QUICKSTART gets a one-line pointer for hardware-free bring-up; RECOVERY_PLAYBOOK no longer calls the vendored pstop_c a submodule.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I traced the new v2 wire-protocol simulation in tools/pstop_test_remote.py against common/pstop_aux_channel.h — the padding1 encoding (version in bits 0-7, role in bits 8-15) and struct layout match bit-for-bit, so the --role/aux-channel test logic is exercising the real machine-side gate correctly.
Extended reasoning...
This run's new commit (54c966b) adds the substantive part of the diff: updating tools/pstop_test_remote.py from the v0/40-byte wire format to the v2/48-byte format with the aux-channel role announcement, plus doc touch-ups. I compared the script's encode()/decode() and the padding1 aux-channel bit layout against the actual wire-format definition in common/pstop_aux_channel.h (added in the base commit e67b5c5) and confirmed they agree exactly (schema version in low byte, role in high byte of padding1), and that the new length guard in decode() correctly handles short/malformed UDP datagrams instead of raising, which is an improvement over the previous behavior. This is a test-only script, not firmware or application logic, so the blast radius of any bug here is limited to the bench test tool's own assertions.
One finding (docs/QUICKSTART.md Appendix C persistence table incorrectly lumping the machine-side operator allowlist in with remote-side NVS state) has already been confirmed and will be posted as an inline comment. That is a documentation accuracy issue only, not a functional or security defect in the codebase. Since a finding already exists, per policy I should not restate it or produce a "what was reviewed" summary; instead I'm using the narrow ruled-out-note exception to record the additional wire-format cross-check I performed on the new commit, which was not part of the prior review (posted before this commit was pushed).
…te NVS Review: Appendix C grouped 'operators' with the remote's NVS-persisted peer and role. The allowlist lives in the machine's params (pstop_machine.yaml, machine.toml, or /api/operators on machn); reflashing or erasing the remote never touches it.
There was a problem hiding this comment.
LGTM — the only outstanding item from my prior review (Appendix C's persistence table conflating the remote's NVS-backed peer/role with the machine-side operator allowlist) has been correctly fixed by splitting that row.
What was reviewed: the only new commit since the last review (6549ad0) touches docs/QUICKSTART.md's persistence table; confirmed it now attributes operators to laptop-side config (pstop_machine.yaml/machine.toml), consistent with docs/API.md, and keeps Machine peer, self-role as the actual remote NVS state. tools/pstop_test_remote.py's v2 48-byte frame struct layout, codewords, and aux-channel role encoding are unchanged from the previously-reviewed revision.
Extended reasoning...
Overview
This run's diff against the last-reviewed revision is a single new commit (6549ad0) touching only docs/QUICKSTART.md. It splits the Appendix C persistence table row that previously grouped "Machine peer, role, operators" as remote NVS state, into two rows: Machine peer, self-role (remote NVS, correct) and Operator allowlist (operators) (laptop-side pstop_machine.yaml/machine.toml, not on the remote at all).
Security risks
None introduced by this change — it is a documentation-only correction. It actually reduces a real risk: the prior wording could have led an operator to believe re-flashing/erasing the remote affects who is authorized to re-arm the system, which is a safety-relevant misunderstanding for a protective-stop device. The fix correctly clarifies that authorization state lives off the remote.
Level of scrutiny
Low — this is a one-line-table documentation fix that directly and accurately addresses a specific inline comment I raised in a prior review of this PR. I re-checked it against docs/API.md's description of where operators is configured, and it is consistent. No code paths changed in this commit.
Other factors
No other code changed since the last reviewed revision (tools/pstop_test_remote.py and all other docs are identical to what was already reviewed twice). The bug hunting system reported no findings this run, and my own re-check of the fixed table found nothing further to raise.
… behaviour Bench 2026-09-04 on PSTOP06: POST /api/iface/usb disables the tether (DHCP Release + TinyUSB removed) rather than selecting it; API.md claimed 'select'. Six Eth<->USB transitions with a bonded, armed ROS 2 machine: 0 stops, 0 rebonds; Eth->USB costs one ~5 s re-register.
2 participants
Summary
docs/QUICKSTART.md: a copy-paste walkthrough from an empty Ubuntu 24.04 laptop to a bench test through STOP and ARM, over the user's own tailnet.Sections: Tailscale account/key -> laptop joins -> USB tether (
host/setup/install.sh) -> build + flash -> first boot -> ROS 2 Jazzy machine node -> pair -> test-station table -> troubleshooting table. Appendices: plain-C runner, PoE/LAN/WiFi, what persists across reflash vs erase-flash.Traps found while tracing the code, fixed here
components/microlink/Kconfigsdkconfig.credentials.examplesdkconfigmust be regenerated after editing (silent stale-config pitfall, previously only inops/HANDOFF.md).docs/API.mdGET/POST /api/rolewas undocumented; it is one of the two arming gates./api/enter_downloadis admin +?confirm=1.USB_NCM_SETUP.md,host/README.md,TROUBLESHOOTING.md10.42.0.1:8890" claim; fresh NVS has had no peer since #65 (ring white, not red). Fixed the verify snippet that curled the host's own IP.hardware/ASSEMBLY.mdREADME.md#quickstart, which does not exist.CONTRIBUTING.mdidf.py flash monitorimplies working serial logs; the console goes quiet once TinyUSB takes the port.README.mdTAILSCALE_ISOLATION.mdTo verify on hardware (marked
<!-- VERIFY -->in the guide)pstop-01xxxxxxintailscale statusover the USB tether (guide says ~60 s).192.168.4.1).Also in this PR (second commit)
tools/pstop_test_remote.pyspoke the v1 40-byte frame with message codes 0..3; the currentpstop_cis 48-byte with codewords0x55/0x92/0xAD/0x6A, so the node dropped every datagram and the script could only ever print "bond failed". Now v2 codec + aux-channel role (--role operator|stop_only), verified 5/5 againsthost/machine_app_runnerand against the ROS 2machine_bridge_node(operators: [16909185]), plus thestop_onlyinverse assertion.TESTING.mddocuments both targets.Not in this PR
/api/healthand one verify line to this guide.pre-commit run --all-filespasses.