diff --git a/.gitignore b/.gitignore index 2db27f4..42f5336 100644 --- a/.gitignore +++ b/.gitignore @@ -14,3 +14,4 @@ DerivedData/ # macOS .DS_Store +.gstack/ diff --git a/Sources/KitchenSyncApp/Discovery/DeviceMatch.swift b/Sources/KitchenSyncApp/Discovery/DeviceMatch.swift new file mode 100644 index 0000000..459832b --- /dev/null +++ b/Sources/KitchenSyncApp/Discovery/DeviceMatch.swift @@ -0,0 +1,40 @@ +import Foundation + +/// Decides whether a Bonjour result is one of ours. +/// +/// Pure, so it can be tested without standing up an `NWBrowser` — the browser glue in +/// `KitchenSyncDiscovery` stays dumb and this gets the tests. +enum DeviceMatch { + /// The TXT key the firmware will publish (`link-devices` ESP-031). + static let identityKey = "dev" + static let identityValue = "kitchensync" + + /// **TXT is the truth; the hostname is a guess.** + /// + /// When a TXT record is present, identity is a FACT and the hostname is not consulted at all — + /// including when the hostname *would* have passed. A stranger's box at a venue named + /// `kitchensync-whatever` is exactly the false positive the prefix heuristic cannot prevent + /// and TXT can. + /// + /// When TXT is ABSENT we fall back to the hostname, because units in the field have no TXT + /// record and won't until they're reflashed. Matching on TXT alone would make every existing + /// device vanish — a worse bug than the one this fixes. + static func isKitchenSync(serviceName: String, txt: [String: String]?) -> Bool { + if let txt, let dev = txt[identityKey] { + return dev == identityValue + } + return matchesKnownHostname(serviceName) + } + + /// The FALLBACK. A heuristic, and deliberately labelled as one — it exists to keep un-updated + /// units working, not because it is correct. + /// + /// The firmware advertises generic `_http._tcp` with no TXT record, so the hostname is all a + /// client has. `ks_hostname()` builds `-`: `kitchensync-a4f2` on + /// the P4, `kstouch-dfd0` on the Touch. Missing `kstouch` is what made a real device invisible. + private static func matchesKnownHostname(_ name: String) -> Bool { + knownHostPrefixes.contains { name.hasPrefix($0) } + } + + private static let knownHostPrefixes = ["kitchensync", "kstouch"] +} diff --git a/Sources/KitchenSyncApp/Discovery/KitchenSyncDiscovery.swift b/Sources/KitchenSyncApp/Discovery/KitchenSyncDiscovery.swift index 8291b86..8a76485 100644 --- a/Sources/KitchenSyncApp/Discovery/KitchenSyncDiscovery.swift +++ b/Sources/KitchenSyncApp/Discovery/KitchenSyncDiscovery.swift @@ -32,11 +32,14 @@ final class KitchenSyncDiscovery { parameters.includePeerToPeer = false let browser = NWBrowser(for: .bonjour(type: "_http._tcp", domain: nil), using: parameters) browser.browseResultsChangedHandler = { [weak self] results, _ in - let names = results.compactMap { result -> String? in + let matched = results.compactMap { result -> String? in guard case let .service(name, _, _, _) = result.endpoint else { return nil } - return name + // The decision lives in `DeviceMatch` — a pure function with tests. This glue + // only extracts the two inputs. TXT wins when present; the hostname is the + // fallback for units that haven't been reflashed (see `DeviceMatch`). + return DeviceMatch.isKitchenSync(serviceName: name, txt: Self.txt(of: result)) + ? name : nil } - let matched = names.filter { $0 == "kitchensync" || $0.hasPrefix("kitchensync-") } self?.onHostnamesChanged?(Set(matched)) } browser.start(queue: queue) @@ -47,4 +50,18 @@ final class KitchenSyncDiscovery { browser?.cancel() browser = nil } + + /// The TXT record, if the responder published one. Firmware doesn't yet (`link-devices` + /// ESP-031), so this is `nil` for every unit in the field today — which is exactly why + /// `DeviceMatch` keeps a hostname fallback. + private static func txt(of result: NWBrowser.Result) -> [String: String]? { + guard case let .bonjour(record) = result.metadata else { return nil } + var entries: [String: String] = [:] + for (key, entry) in record { + if case let .string(value) = entry { + entries[key] = value + } + } + return entries.isEmpty ? nil : entries + } } diff --git a/Sources/KitchenSyncApp/ViewModels/DeviceDetailViewModel.swift b/Sources/KitchenSyncApp/ViewModels/DeviceDetailViewModel.swift index f9a077b..698ae4a 100644 --- a/Sources/KitchenSyncApp/ViewModels/DeviceDetailViewModel.swift +++ b/Sources/KitchenSyncApp/ViewModels/DeviceDetailViewModel.swift @@ -42,12 +42,40 @@ final class DeviceDetailViewModel: ObservableObject { pollTask = nil } + /// Why we do or don't have this device's telemetry. + /// + /// A DECODE failure and a TRANSPORT failure are different problems with different fixes, and + /// collapsing them is how a live device gets reported as dead. Found on real hardware: a + /// KitchenSync Touch answers `/status` with HTTP 200 in a dialect this app can't read + /// (`link-devices` ESP-029), and the app called it UNREACHABLE — sending the user off to debug + /// a network that was working perfectly. + enum StatusAvailability: Equatable { + case unknown + case available + /// It ANSWERED, but we couldn't decode it. The device is fine; its firmware speaks a + /// different `/status`. The way out is a firmware update, not a network fix. + case unsupported + /// It didn't answer at all. + case unreachable + } + + @Published private(set) var statusAvailability: StatusAvailability = .unknown + func refreshStatus() async { - let fresh = try? await client.fetchStatus() - status = fresh - // A failed poll while rebooting is the device still being down — which is - // exactly what we're waiting out. Only an ANSWER clears the flag. - if fresh != nil { isRebooting = false } + do { + let fresh = try await client.fetchStatus() + status = fresh + statusAvailability = .available + isRebooting = false // the device answering IS the signal + } catch is DecodingError { + // It's there. We just can't read it. + statusAvailability = .unsupported + } catch { + status = nil + statusAvailability = .unreachable + // A failed poll while rebooting is the device still being down — which is + // exactly what we're waiting out. Only an ANSWER clears that flag. + } } /// Why we do or don't have the device's settings. diff --git a/Sources/KitchenSyncApp/ViewModels/DeviceListViewModel.swift b/Sources/KitchenSyncApp/ViewModels/DeviceListViewModel.swift index 57aa65a..d040ee4 100644 --- a/Sources/KitchenSyncApp/ViewModels/DeviceListViewModel.swift +++ b/Sources/KitchenSyncApp/ViewModels/DeviceListViewModel.swift @@ -16,6 +16,13 @@ final class DeviceListViewModel: ObservableObject { enum Reachability: Equatable { case reachable case unreachable + /// It ANSWERS — we just can't decode what it says. Emphatically not "gone". + /// + /// Found on real hardware: a KitchenSync Touch replies to every poll with HTTP 200 in a + /// `/status` dialect this app can't read (`link-devices` ESP-029). Counting those as + /// polling misses declared a live, healthy device dead and sent the user off to debug a + /// network that was working perfectly. A device that keeps answering is never unreachable. + case unsupported } private static let missesBeforeUnreachable = 3 @@ -25,6 +32,9 @@ final class DeviceListViewModel: ObservableObject { private let session: URLSession private var pollTask: Task? private var consecutiveMisses: [String: Int] = [:] + /// Devices that ANSWER but whose `/status` we can't decode. Kept apart from the miss counter + /// on purpose — they are present, not absent. + private var undecodable: Set = [] init(store: ManualDeviceStore = ManualDeviceStore(), session: URLSession = .shared) { self.store = store @@ -32,7 +42,10 @@ final class DeviceListViewModel: ObservableObject { } func reachability(of id: String) -> Reachability { - (consecutiveMisses[id] ?? 0) >= Self.missesBeforeUnreachable ? .unreachable : .reachable + // Answering-but-undecodable outranks the miss counter: a device that is plainly THERE + // must never be reported as gone, however long we've failed to read it. + if undecodable.contains(id) { return .unsupported } + return (consecutiveMisses[id] ?? 0) >= Self.missesBeforeUnreachable ? .unreachable : .reachable } func start() { @@ -91,22 +104,45 @@ final class DeviceListViewModel: ObservableObject { await refreshAll() } + /// The outcome of one poll. A DECODE failure and a TRANSPORT failure are different problems + /// with different fixes; collapsing them into `nil` is what let a live device be reported dead. + private enum PollResult { + case ok(KsStatus) + case undecodable // it answered; we can't read it + case noAnswer // it didn't answer + } + private func refreshAll() async { let snapshot = devices let session = session - await withTaskGroup(of: (String, KsStatus?).self) { group in + await withTaskGroup(of: (String, PollResult).self) { group in for device in snapshot { group.addTask { let client = KitchenSyncClient(host: device.host, session: session) - let status = try? await client.fetchStatus() - return (device.id, status) + do { + return (device.id, .ok(try await client.fetchStatus())) + } catch is DecodingError { + return (device.id, .undecodable) + } catch { + return (device.id, .noAnswer) + } } } - for await (id, status) in group { - if let status { + for await (id, result) in group { + switch result { + case .ok(let status): statuses[id] = status consecutiveMisses[id] = 0 - } else { + undecodable.remove(id) + + case .undecodable: + // It is THERE. Not a miss — don't let the counter creep toward "gone". + undecodable.insert(id) + consecutiveMisses[id] = 0 + statuses[id] = nil + + case .noAnswer: + undecodable.remove(id) let misses = (consecutiveMisses[id] ?? 0) + 1 consecutiveMisses[id] = misses // Below the threshold the last known status is still the best we diff --git a/Sources/KitchenSyncApp/Views/DeviceListView.swift b/Sources/KitchenSyncApp/Views/DeviceListView.swift index ec27b99..76536c2 100644 --- a/Sources/KitchenSyncApp/Views/DeviceListView.swift +++ b/Sources/KitchenSyncApp/Views/DeviceListView.swift @@ -163,7 +163,13 @@ private struct DeviceRow: View { .lineLimit(1) } - if reachability == .unreachable { + if reachability == .unsupported { + // It ANSWERS — we just can't read it. Saying "unreachable" here would send + // the user off to debug a network that is working perfectly. The fix is a + // firmware update, and the label has to point at that. + KSPill(text: "UNSUPPORTED FW", color: KS.amber) + .padding(.top, 2) + } else if reachability == .unreachable { // An expected-to-be-there device that has missed three polls in a // row. Say so — the whole point of the app is telling you the state // of hardware on stage, and "silently stale" is the worst answer. @@ -184,7 +190,9 @@ private struct DeviceRow: View { KSGlass(bpm: status?.bpm ?? 0, size: 26) // "0 PEERS" for a device that isn't answering is a lie dressed as // a measurement. If there's no status, we don't know the peer count. - Text(status.map { "\($0.peers) PEERS" } ?? "NO SIGNAL") + // And "NO SIGNAL" is wrong for a device that's answering fine — it's + // signalling perfectly, in a dialect we can't read. + Text(peersCaption) .font(.ksMono(10.5)) .tracking(1.6) .foregroundStyle(KS.mut) @@ -196,6 +204,11 @@ private struct DeviceRow: View { .listRowSeparatorTint(KS.line) } + private var peersCaption: String { + if let status { return "\(status.peers) PEERS" } + return reachability == .unsupported ? "CAN'T READ" : "NO SIGNAL" + } + @ViewBuilder private func summaryPill(_ summary: KsStatus.TransportSummary) -> some View { switch summary { diff --git a/Tests/KitchenSyncAppTests/DeviceMatchTests.swift b/Tests/KitchenSyncAppTests/DeviceMatchTests.swift new file mode 100644 index 0000000..914ea1b --- /dev/null +++ b/Tests/KitchenSyncAppTests/DeviceMatchTests.swift @@ -0,0 +1,50 @@ +import XCTest +@testable import KitchenSyncApp + +/// TDD: written before `DeviceMatch` exists. +/// +/// **This is the bug a real device found.** On 2026-07-14 a KitchenSync Touch was put on the LAN. +/// It advertises as `kstouch-dfd0`. `KitchenSyncDiscovery` filtered with +/// `hasPrefix("kitchensync-")`, so the device was not shown wrong or shown offline — it was +/// **invisible**. The prefix heuristic failed on the first real hardware the app ever met. +/// +/// The decision lives here, as a pure function over (serviceName, txt), so it can be asserted +/// without standing up Bonjour. The `NWBrowser` glue stays dumb. +final class DeviceMatchTests: XCTestCase { + func test_a_touch_is_matched_by_its_hostname_when_there_is_no_txt_record() { + XCTAssertTrue(DeviceMatch.isKitchenSync(serviceName: "kstouch-dfd0", txt: nil), + "a real Touch on the bench was invisible to the app because of this") + } + + /// Units already in the field have no TXT record and won't until they're reflashed. Breaking + /// them would be a worse bug than the one being fixed. + func test_the_existing_devices_still_match_on_hostname() { + XCTAssertTrue(DeviceMatch.isKitchenSync(serviceName: "kitchensync-a4f2", txt: nil)) + XCTAssertTrue(DeviceMatch.isKitchenSync(serviceName: "kitchensync", txt: nil), + "the delegated single-board alias") + } + + func test_someone_elses_http_responder_is_not_adopted() { + XCTAssertFalse(DeviceMatch.isKitchenSync(serviceName: "brother-printer", txt: nil)) + XCTAssertFalse(DeviceMatch.isKitchenSync(serviceName: "", txt: nil)) + } + + // MARK: TXT beats the hostname (link-devices ESP-031) + // + // Once firmware advertises `dev=kitchensync`, identity is a FACT rather than a guess. The + // hostname stops being evidence at all. + + func test_txt_identifies_a_device_whatever_it_is_called() { + XCTAssertTrue(DeviceMatch.isKitchenSync(serviceName: "some-renamed-box", + txt: ["dev": "kitchensync"]), + "TXT is the truth; the hostname is not consulted when it's present") + } + + /// **The failure the prefix heuristic can't prevent.** At a venue, a stranger's box named + /// `kitchensync-whatever` gets adopted straight into the fleet. Once TXT exists, it must not. + func test_a_stranger_named_like_us_is_rejected_once_txt_is_available() { + XCTAssertFalse(DeviceMatch.isKitchenSync(serviceName: "kitchensync-imposter", + txt: ["dev": "some-other-product"]), + "a TXT record that says it ISN'T ours must beat a hostname that says it is") + } +} diff --git a/Tests/KitchenSyncAppTests/UnsupportedDeviceTests.swift b/Tests/KitchenSyncAppTests/UnsupportedDeviceTests.swift new file mode 100644 index 0000000..ed02e8e --- /dev/null +++ b/Tests/KitchenSyncAppTests/UnsupportedDeviceTests.swift @@ -0,0 +1,87 @@ +import XCTest +@testable import KitchenSyncApp + +/// TDD: written before the app can tell "I can't understand this device" apart from +/// "this device is gone". +/// +/// **Found on real hardware, 2026-07-14.** With discovery fixed (T-014), the app finally sees a +/// real KitchenSync Touch — and then labels it UNREACHABLE, while `curl` gets HTTP 200 from that +/// exact host. The device is answering perfectly; the app just can't decode what it says. +/// +/// `refreshStatus()` collapses a DECODE failure and a TRANSPORT failure into the same `nil`, so +/// three polls later T-010's reachability counter declares a live device dead. Telling the user +/// their device is unreachable sends them to debug a network that is working fine. +@MainActor +final class UnsupportedDeviceTests: XCTestCase { + override func setUp() { + super.setUp() + StubURLProtocol.reset() + } + + /// The REAL Touch's `/status`, captured verbatim from `kstouch-dfd0` on 2026-07-14 (current + /// `master` firmware). It is missing 11 keys `KsStatus` requires — `min` first, then `usb`, + /// `tx`, `fw`, `follow_*`, `launch`, `playing`, `link_owns`. + /// + /// This fixture goes GREEN the day `link-devices` ESP-029 ships and the Touch shares + /// `ks_status.c`. Keep it. + static let realTouchStatusJSON = """ + {"bpm":0.0,"sync":-1,"peers":0,"clock":1,"transport":0,"cue":0,"tfail":0,"tzero":0, + "ccancel":0,"drop":0,"burst":0,"gap":1486,"work":93,"over":0,"core":1, + "wbeats":49,"wclock":1,"wtport":43,"beats":0.00,"locked":0,"bsactive":0, + "btn":-1,"btnlows":0,"btnpress":0,"clk":"silent","pulses":0} + """.data(using: .utf8)! + + private func makeVM() -> DeviceDetailViewModel { + DeviceDetailViewModel( + client: KitchenSyncClient(host: "kstouch-dfd0.local", session: StubURLProtocol.session()) + ) + } + + /// A device that ANSWERS but speaks a dialect we don't understand is `.unsupported`. + /// It is emphatically not "gone". + func test_a_device_that_answers_but_cannot_be_decoded_is_unsupported_not_offline() async { + StubURLProtocol.routes["GET /status"] = .init(body: Self.realTouchStatusJSON) + let vm = makeVM() + + await vm.refreshStatus() + + XCTAssertEqual(vm.statusAvailability, .unsupported, + "the device answered HTTP 200 — it is not unreachable, we just can't read it") + } + + // MARK: The fleet list — where UNREACHABLE is actually displayed + + private func makeListVM() -> DeviceListViewModel { + let defaults = UserDefaults(suiteName: "UnsupportedDeviceTests.\(UUID().uuidString)")! + return DeviceListViewModel(store: ManualDeviceStore(defaults: defaults), + session: StubURLProtocol.session()) + } + + /// **The exact bug seen on the bench.** The Touch answers every poll with HTTP 200, so it is + /// plainly not gone — but the app decoded nothing, counted three misses, and rendered + /// UNREACHABLE. A device that keeps answering must never be declared dead. + func test_a_device_that_keeps_answering_is_never_declared_unreachable() async { + StubURLProtocol.routes["GET /status"] = .init(body: Self.realTouchStatusJSON) + let vm = makeListVM() + vm.addManualDevice(host: "kstouch-dfd0.local") + + for _ in 0..<5 { await vm.refreshNow() } // well past T-010's 3-miss threshold + + XCTAssertNotEqual(vm.reachability(of: "kstouch-dfd0.local"), .unreachable, + "it answered HTTP 200 five times — calling it unreachable is a lie") + XCTAssertEqual(vm.reachability(of: "kstouch-dfd0.local"), .unsupported) + } + + /// And the T-010 behaviour still holds: a device that genuinely stops answering IS unreachable. + func test_a_device_that_stops_answering_is_still_unreachable() async { + StubURLProtocol.routes["GET /status"] = .init(body: Self.realTouchStatusJSON) + let vm = makeListVM() + vm.addManualDevice(host: "kstouch-dfd0.local") + await vm.refreshNow() + + StubURLProtocol.routes = [:] // device drops off the network + for _ in 0..<3 { await vm.refreshNow() } + + XCTAssertEqual(vm.reachability(of: "kstouch-dfd0.local"), .unreachable) + } +} diff --git a/tasks/T-014.md b/tasks/T-014.md index 08d5c42..76d9543 100644 --- a/tasks/T-014.md +++ b/tasks/T-014.md @@ -2,7 +2,7 @@ id: T-014 title: Discovery must find devices that aren't named kitchensync-* (TXT, with fallback) -status: todo +status: done assignee: null priority: high tags: diff --git a/tasks/T-016.md b/tasks/T-016.md new file mode 100644 index 0000000..f07ea62 --- /dev/null +++ b/tasks/T-016.md @@ -0,0 +1,69 @@ +--- +id: T-016 +title: A device we can't decode is not 'unreachable' — don't conflate the two +status: done +assignee: null +priority: high +tags: + - protocol + - hardware +depends_on: [] +created_at: 2026-07-14 +updated_at: 2026-07-14 +--- + +## Goal + +**Found on real hardware, 2026-07-14.** With T-014 landed, the app finally discovers a real +KitchenSync Touch — and then immediately labels it **UNREACHABLE**, while `curl` gets **HTTP 200** +from that same host. + +The device is answering perfectly. The app just can't understand what it says. + +``` +GET http://kstouch-dfd0.local/status -> HTTP 200 +KsStatus.init(from:) -> THROWS on the first required key, "min" + (11 required keys absent: min, usb, tx, fw, + follow_*, launch, playing, link_owns) +``` + +`refreshStatus()` does `try? await client.fetchStatus()`, so a **decode** failure and a +**transport** failure collapse into the same `nil`. Three of those and T-010's reachability +counter declares the device gone. + +**Telling a user their device is unreachable sends them to debug a network that is working fine.** +The real problem is a protocol mismatch. Same class of bug as T-009 (a `/config.json` 404 is not a +dead network) — this is the identical mistake, one endpoint over. + +## Acceptance Criteria + +- [ ] A decode failure is distinguished from a transport failure at the client boundary +- [ ] A device that answers but can't be decoded is NOT counted as a reachability miss +- [ ] The list shows something honest for it — e.g. `UNSUPPORTED`, not `UNREACHABLE` +- [ ] The detail screen explains the real cause: this device speaks a different `/status`, its + firmware needs updating (`link-devices` ESP-029), and points at the OTA path +- [ ] A genuinely absent device still reads `UNREACHABLE` (T-010's behaviour is preserved) +- [ ] A fixture built from the REAL Touch `/status` response is in the test suite +- [ ] Written test-first per the /tdd rule + +## Notes + +The real device's `/status` (captured 2026-07-14, current `master` firmware — keep this as a +fixture): + +```json +{"bpm":0.0,"sync":-1,"peers":0,"clock":1,"transport":0,"cue":0,"tfail":0,"tzero":0, + "ccancel":0,"drop":0,"burst":0,"gap":1486,"work":93,"over":0,"core":1, + "wbeats":49,"wclock":1,"wtport":43,"beats":0.00,"locked":0,"bsactive":0, + "btn":-1,"btnlows":0,"btnpress":0,"clk":"silent","pulses":0} +``` + +When `link-devices` ESP-029 lands (Touch shares `ks_status.c`), this device will decode and the +state disappears on its own. **That is not a reason to skip this.** The app should never claim a +device is gone when it is sitting there answering — and it should say what's actually wrong, so the +user's next action is "update the firmware", not "check the WiFi". + +Note the sequencing gift: the fixture above is a regression test that goes GREEN the day ESP-029 +ships. Keep it. + +## Progress