Repository navigation
feat(app): Settings › Knob pane, USB setup sheet, knob settings - #228
Conversation
Improv Serial and CINDER1 codecs pinned by shared test vectors, a byte demuxer for frames, JSON lines and logs, a KnobLink transport seam with a termios serial link that never toggles DTR/RTS, IOKit port discovery for 303a:1001, the setup state machine, and the /v1/devices client and model with merge-PUT autosave.
tarakanof
left a comment
tarakanof
left a comment
There was a problem hiding this comment.
Review of #228 (Knob pane + USB setup). swift test --package-path macos: 722 tests pass. I didn't open the real serial port; the serial findings come from pty harnesses built against the PR's own SerialPortLink/KnobSession.
What checks out:
- Improv frames: I rebuilt all 14 vectors in
improv.jsonfrom the spec in Python (header, version 1, type, length,cmd,len,len-prefixed strings, low-byte sum checksum). All 14 match, and so do the 5 invalid cases. - Demuxer: I fuzzed 3000 streams cut at random byte boundaries, mixing ANSI log lines, printable garbage, Improv frames with
\ninside their strings, text before a frame on the same line, and CINDER1 events. No frame or line was lost. - Merge PUT: the patch carries changed fields of nested objects only, sends
pageswhole and never adds unknown keys. That matchesmergeKnobSettings+DisallowUnknownFields.normalized()matchesvalidate(). - The token and password are never logged. New UI strings are in the catalog.
Findings (ranked)
1. High: a replaced knob is almost never forgotten
KnobSetupSheet.swift:42-46,85-87. replaces is a computed property. It is read again inside didSetUp, when setup finishes, not when the user confirms. KnobPane's .task calls knob.load() every 15 s while the sheet is up. Once the mint has created the new record, KnobDevice.current (newest created_at) is the new knob, so replaces comes back nil and didSetUp(_, replacing: nil) forgets nothing. A setup takes 30-90 s, so this hits nearly every time. The old knob stays registered with a live token and is hidden by the one-knob UI.
Fix: capture the replaced knob once, when Set Up / Replace is pressed:
private func send() {
let old = replaces
model?.send { await env.knob.didSetUp($0, replacing: old) }
}Do the same for remint.
2. High: "Re-mint Token" always fails with "The knob was disconnected"
KnobSetupModel.swift:173-183, 208-223; KnobProvisioner.swift:280-282, 303-314. emberUnauthorized only happens after join has followed the reboot and opened a new session. run stores next only on success. On failure, self.session is still the pre-reboot session, which is closed. remint → setEmber → KnobSession.send throws .closed → .disconnected. Even with a live session, remint doesn't drain() before waitForEmber, so a queued {"ev":"ember","state":"unauthorized"} from before set_ember matches at once and the re-mint fails again.
The session opened by the reconnect is also never closed on any error path, and SerialPortLink has no deinit. In the harness each dropped session leaked one fd (/dev/fd 6 → 7) for the life of the process.
Fix: let the provisioner report the live session on failure, e.g. struct KnobSetupFailure: Error { let error: KnobSetupError; let session: KnobSession? }, or an onSession callback. Have the model adopt it in run's catch. Add await session.drain() before waitForEmber in remint, and deinit { close() } on SerialPortLink.
3. High (dev workflow): probes open every 303a:1001 port with no exclusivity, and two readers split the byte stream
SerialPortLink.swift:22-30, KnobModel.swift:178-190, AppEnvironment.feedKnobPorts. Ember probes every Espressif USB-Serial/JTAG port at launch and on every re-enumeration. Each probe writes IMPROV… and a CINDER1 info line, then reads for up to 5 s. Nothing takes TIOCEXCL or flock. I opened two SerialPortLinks on one pty and wrote 40 lines: one got 1, the other 39. Concrete cases:
- During
idf.py monitor, every knob reboot re-enumerates the port, and Ember swallows the boot log. - The ROM download mode is also
303a:1001, so duringidf.py flashthe probe can eat esptool's SLIP replies. - If the user hits "Set Up Knob…" while the USB row still says "Checking the board…", the sheet and the in-flight probe split the replies.
portBusyis only checked before a probe starts. The sheet'sidentifythen times out and shows "isn't running cinder".
Fix: right after open:
if ioctl(fd, TIOCEXCL) != 0 || flock(fd, LOCK_EX | LOCK_NB) != 0 {
Darwin.close(fd); throw KnobLinkError.openFailed("busy") // → .unavailable
}Pyserial's exclusive=True uses the same flock. Also have the sheet wait for any in-flight probe of the port before it connects. Consider probing only while Settings › Knob is open, or after the port has been stable for a few seconds.
4. Medium: a failed setup of a different board leaves an orphan record that becomes "the knob"
POST /v1/devices creates the record before set_ember and Wi-Fi. If either fails (wrong password, timeout), the new record stays. KnobDevice.current picks the newest created_at, so the pane switches to the board that never checked in. The working knob drops out of the UI and can't be forgotten there, but its token stays valid.
Fix: in provision, if the mint created a new record (minted.device.lastCheckin == nil and its hwID differs from the registered knob's) and a later step throws, DELETE it before rethrowing.
Separately, re-setting up the same knob revokes its working token at mint time. A set_ember failure then leaves the knob on 401 until setup succeeds. The server design allows nothing better, but the error text should say to run setup again.
5. Medium: the check-in test can report Done for a token that never worked
KnobProvisioner.swift:105-108, 187, 331. Re-provisioning keeps last_checkin, and the check is seenAt >= started - 5s, comparing the Mac clock with the server clock. A check-in on the old token in the 5 s before started (there's one every 60 s or on each epoch bump), or a server clock running ahead of the Mac, makes waitForEmber return at once. Example: a typo in the Ember URL still ends in "Done".
Fix: use the server's own clock as the baseline: let base = minted.device.lastCheckin?.seenAt, and succeed only when seenAt > base (or when base == nil and any check-in exists). Drop the -5.
6. Low: setup work ignores cancellation
KnobLink.swift:101-103 (withCheckedContinuation ignores cancellation) and SerialPortLink.swift:122-128 (reopens at once, without checking). If model.close() runs while .sending (app quit, Settings window closed), join sees .closed, calls reconnect, reopens the port and carries on. It can even reach didSetUp/forget.
Fix: wrap the waiter in withTaskCancellationHandler and resume it with nil on cancel. Call try Task.checkCancellation() at the top of reconnect and of the join and waitForEmber loops.
7. Low: probe results are single-shot and cached
KnobModel.swift:181. USB enumerates at ROM time, before the firmware's listener runs. One device-info RPC with a 2 s timeout at first-match can catch a knob that is still booting. .notCinder/.unavailable is then cached until replug, and no setup notification is posted. Fix: retry the RPC 2-3 times, treat an unsolicited CINDER1 {"ev":"boot"} as cinder, and re-probe non-cinder ports when the pane appears.
DTR/RTS (I can't verify this without hardware)
The link never issues TIOCMBIS/TIOCMBIC/TIOCMSET and clears HUPCL (tested on a pty). macOS asserts DTR/RTS on open, and userspace can't prevent that, so the "doesn't reset on open" item in the spec's hardware test plan is still required before merge. Minor: when configure fails, init closes the fd with HUPCL still set (SerialPortLink.swift:25-28), which drops the lines. Clear HUPCL before any other termios step, or leave the fd open until the clear succeeds.
Nits
Package.swift: SwiftPM warns about 2 unhandled files (testdata/knob/*.json). Addexclude: ["testdata"]to the test target. The vectors aren't actually shared with cinder yet (nofirmware/test/vectors), so add a sync note to avoid drift.ImprovCodec.Statelacks the spec's0x00 Stoppedand includes the BLE-only0x01;ErrorCode.notAuthorized 0x04is also BLE-only. Both are harmless.- The sheet doesn't check WPA password length (8-63). A short password costs a reboot plus ~45 s before "Couldn't join".
- Accessibility: progress rows in the setup sheet are an unlabeled SF Symbol plus text. Use
.accessibilityElement(children: .combine)and.accessibilityValue("done/in progress/pending").
…tups Review of #228: take the port exclusively, close dropped links, keep the rebooted session for re-mint, delete records that never worked, judge success by a server-side checkin baseline, honour cancellation, and retry device info for a knob that is still booting.
The port is shared with idf.py monitor and esptool, so detection stays passive and probes run only while Settings › Knob is visible. The setup sheet keeps the knob it replaces from the moment the user confirms, waits for a probe in flight, and checks WPA password length.
|
Review fixes (f5752f2, 9c8d9a9). I wrote the tests first;
|
|
Re-check of f5752f2 and 9c8d9a9. Locally, I checked each fix by mutating a scratch copy of the code and seeing whether a test fails:
Verdict: the fixes are correct. Make CI green first. The test gaps (drain, remint baseline, sheet capture, deeper cancellation) and the DELETE on cancel can go in a follow-up. |
TIOCEXCL lives on the tty and outlasts our fd while anything else holds the tty open, and the fd itself closed later on the dispatch queue, so an immediate reopen could fail with EBUSY (seen on CI).
The orphan DELETE runs unstructured so a cancelled setup still sends it, a cancelled setup never sends Wi-Fi, and the setup model captures the knob it replaces at send time. Tests now fail if the remint drain, its baseline, the capture or the join cancellation check go missing.
|
Re-verify fixes (3a1cd33, 8f12d62). CI is green and
|
http-only lower-case URL with an IPv4 or [a-z0-9.-] host, 32-byte name, self-restart after set_ember, Wi-Fi rejection and ember connecting.
The knob has no TLS and a 32-byte name, restarts itself when its Ember URL changes, and reports rejected Wi-Fi settings separately from a failed join, so the setup validates before minting and handles each case.
|
Firmware contract fixes from the cinder#16/#17 review (d1daac8 tests first, then 9564974). CI is green and
|
1 participant
Closes #6
Phase 1 of knob provisioning: settings move from compiled-in Kconfig to NVS.
Changes
components/cinder_cfg(pure C, host-tested):cfg_tfield table (NVS keys), validation (URLhttp(s)://host[:port], WPA2 password, header-safe token, device id), one-time seed from the build config, hardware ID from the MAC, log line without secrets. Alsoreset_gesture(factory-reset state machine).main/config_store.c(ESP only):nvs_flash_init, load namespacecinder(wifi_ssid,wifi_pass,ember_url,dev_id,dev_tok,name;settingsblob +settings_verreserved for docs(unraid): data-dir chown (UID 65532) + :3627 preset in install steps #8), copies in PSRAM.CONFIG_CINDER_WIFI_SSIDis set, the Kconfig values are written once. The currentEMBER_TOKENbecomesdev_tokuntil docs(unraid): data-dir chown (UID 65532) + :3627 preset in install steps #8.CINDER_EMBER_URLdefault is now empty, so release builds hold no secrets.config_store.ID 61FC8C; Wi-Fi not started.cinder+esp_wifi_restore()+ reboot (core-1 task, no log in the LVGL task). Release / left detent / 5 s without a right detent cancels; that press is then no push, long push or page change. Normal input unchanged (push vs long push on release, push-and-turn = page).docs/llm.md,docs/firmware-plan.md.Verification
firmware/test/host/run.shall pass (newtest_cfg.c: keys, URL/password/token validation, seed, IDs, no secrets in logs, gesture confirm/cancel/timeout, push/long push/page change untouched).idf.py build: 0 warnings./dev/cu.usbmodem1101):.bssmade the second 56 KB LVGL draw buffer fail (alloc secondary buffer 56640 bytes failed, boot loop). Fixed by keeping config copies in PSRAM / on existing stacks;.bssis now +8 B vs main.esp_wifi_set_storage(WIFI_STORAGE_RAM); there the first association failed (reason 203) and the IP came ~5 s late. The final commit drops that line (driver keeps its default flash copy, like main; factory reset clears it). The knob stopped answering on USB (no ROM output, esptool cannot connect) right after flashing that last one-line change, so the final build is not hardware-verified: needs a replug and a reflash.Spec: Obsidian
Superpowers Specs/cinder/2026-10-04-knob-provisioning-design.md. Deviation: Wi-Fi credentials live in namespacecinder(single source of truth), not only in the driver's storage.