From 00c9f5fcaaa22fdc0905146a5c25221feae166da Mon Sep 17 00:00:00 2001 From: Khoa Truong Date: Fri, 31 Jul 2026 19:42:46 -0700 Subject: [PATCH 1/3] fix(ring): re-route jring rings off the wrong driver, correct the CRP firmware opcode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent issue #29 reports. Colmi R09 stuck on "Required ring indication channel unavailable: BE940001, BE940003" (itspuia). connectTo's honorSelection treats a JRING classification of the generic "SMART_RING" advertisement as a fallback guess and lets the pairing carousel pick win. That is backwards for a jring-firmware ring sold under a Colmi badge: picking "Colmi / Yawell (SmartHealth app)" installs the YCBT driver, whose be940001/be940003 channels do not exist on the ring, so topologyFailure() hard- failed the connect and the app retried forever — while 000056ff was present in the GATT table the whole time and the ring works in the official JRing app. Adds the inverse of the existing Colmi and CRP post-connect re-routes: when the jring service is present and every service the active driver declared is absent, route back to the jring driver instead of failing. The "active driver's own services missing" guard is what stops it stealing a ring that genuinely speaks its selected protocol. Policy lives in DriverReroute so it is unit-testable — the two existing re-routes are welded into the GATT callback and have no tests. R11 firmware version never read (zaggash). Not the ring ignoring us: q.b(7,1) is the vendor's querySavedGomoreKey (d1/b.java:613), not a firmware query. All of b1/r is the Gomore analytics module, and our constants had paired its methods with opcodes positionally (a->0, b->1, c->13) — but jadx alphabetises method names, so letter order carries no meaning. That mislabelled all three "device info" queries, and separately mislabelled 3/1 (shutDown) as CMD_RESTART; restart is 3/14. The real query is 3/3 (b1/l.k -> d1/b.queryFirmwareVersion), answering with a bare UTF-8 string via g1/a.i1 — MOY-R1K3-2.1.6 on zaggash's R11, matching the vendor app's Firmware-information screen. Decoded into RingDecodedEvent.Status (firmware), the path YCBT and LuckRing already use to reach the device record, because RingDecodedEvent.FirmwareVersion carries an Int and cannot hold it. Drops queryDeviceInfo/queryDeviceSN, which framed Gomore opcodes and had no callers. AGENTS.md gains the rule this mistake argues for: resolve every CRP opcode through its d1/b.java caller, never by position in the builder class. Neither fix is hardware-validated — the re-route is reasoned from itspuia's service list, the firmware query from the decompile. Both need a real device. Bumps versionCode to 32 so testers can be pointed at a build. --- AGENTS.md | 18 +++- app/build.gradle.kts | 2 +- .../java/com/pulseloop/ring/CRPDecoder.kt | 33 +++++-- .../java/com/pulseloop/ring/CRPProtocol.kt | 43 +++++---- .../java/com/pulseloop/ring/CRPSyncEngine.kt | 6 +- .../java/com/pulseloop/ring/DriverReroute.kt | 40 +++++++++ .../java/com/pulseloop/ring/RingBLEClient.kt | 23 +++++ .../java/com/pulseloop/ring/CRPDecoderTest.kt | 40 +++++++++ .../com/pulseloop/ring/CRPSyncEngineTest.kt | 6 +- .../com/pulseloop/ring/DriverRerouteTest.kt | 88 +++++++++++++++++++ 10 files changed, 269 insertions(+), 30 deletions(-) create mode 100644 app/src/main/java/com/pulseloop/ring/DriverReroute.kt create mode 100644 app/src/test/java/com/pulseloop/ring/DriverRerouteTest.kt diff --git a/AGENTS.md b/AGENTS.md index 2eec6dd..73d318f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -89,8 +89,24 @@ supporting evidence as the cause. NOT_SUPPORT / SLEEP_OXYGEN / TIMING_OXYGEN, and the monitor-state queries `2/6` HR, `2/7` HRV, `2/8` SpO2, `2/45` stress, `2/21` temp each report the configured interval (`0` = off). These are how you tell "the monitor is switched off" apart from "this ring lacks the sensor" — the open - question for stress (`2/47`), temperature and firmware (`7/1`), all 23-sent/0-answered. Send them + question for stress (`2/47`) and temperature, both 23-sent/0-answered. Send them **once per connection**, not per poll pass: `runStartup` is also the ~30-minute background sync. +- **Group 7 is Gomore, not device info — an opcode read off a decompiled builder is a guess until + you check its caller.** Firmware was queried on `7/1` and never answered (23 sends, 0 replies), + which read like ring firmware ignoring a valid vendor command. It wasn't: every builder in `b1/r` + resolves to a Gomore call in `d1/b.java` (`7/0` querySupportGomore, `7/1` **querySavedGomoreKey**, + `7/2` queryGomoreEUID, `7/3` sendGomoreKey, `7/13` queryGomoreVersion). The constants had been + built by pairing `b1/r`'s methods with opcodes *positionally* (a→0, b→1, c→13) — but jadx + alphabetises method names, so letter order carries no meaning. The same slip mislabelled `3/1` + (`shutDown`) as `CMD_RESTART`; restart is `3/14`. **Resolve every opcode through its `d1/b.java` + caller, never by position in the builder class.** +- **Firmware version is `3/3`**, replying with a bare UTF-8 string (`g1/a.i1`: + `onVersion(new String(payload, UTF_8))`) — `MOY-R1K3-2.1.6` on zaggash's R11, matching the vendor + app's Firmware-information screen. Decoded into `RingDecodedEvent.Status(firmware = …)`, the same + path YCBT/LuckRing already use to reach the device record, because `RingDecodedEvent.FirmwareVersion` + carries an `Int` and can't hold this. Sibling group-3 queries confirmed from their callers: + `3/0` reset, `3/1` shutDown, `3/4` firmware hash, `3/6` real-time battery, `3/7` wear state, + `3/14` restart, `3/22` binding reminder. - **Temperature history is `2/22`, not `2/48`.** `q.b(2,48)` is the vendor's `querySleepState` (`d1/b.java` line 650); real temp history is `i0.b(day, frameIndex)` = `q.c(2,22,[day,idx])`, the same shape as the other timing histories. Its sample layout is still unconfirmed — no non-empty diff --git a/app/build.gradle.kts b/app/build.gradle.kts index d56899f..5ed9793 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -19,7 +19,7 @@ android { // versionCode/versionName are overridable from Gradle properties so the release CI // can drive them straight from the git tag (e.g. -PappVersionCode=5 -PappVersionName=1.0.0). // Local builds fall back to the literals below. - versionCode = (project.findProperty("appVersionCode") as String?)?.toIntOrNull() ?: 31 + versionCode = (project.findProperty("appVersionCode") as String?)?.toIntOrNull() ?: 32 versionName = (project.findProperty("appVersionName") as String?) ?: "1.0.0" testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner" diff --git a/app/src/main/java/com/pulseloop/ring/CRPDecoder.kt b/app/src/main/java/com/pulseloop/ring/CRPDecoder.kt index f451c4b..7077ccc 100644 --- a/app/src/main/java/com/pulseloop/ring/CRPDecoder.kt +++ b/app/src/main/java/com/pulseloop/ring/CRPDecoder.kt @@ -99,9 +99,10 @@ object CRPDecoder { return decodeVitalResult(cmd, payload, now) } - // Group 7: history queries + device info (decompiled `b1/e0` + `b1/r`). - if (group == CRPCommands.GROUP_DEVICE_INFO) { - return decodeHistoryOrDeviceInfoResponse(cmd, payload, now) + // Group 7: the vendor's Gomore module (`b1/r`). Nothing we send lands here any more; kept so + // an unsolicited Gomore frame in a capture is still recorded rather than dropped. + if (group == CRPCommands.GROUP_GOMORE) { + return decodeGomoreResponse(cmd) } // Group 2: sleep + the all-day "timing" vital timelines + temperature history. @@ -127,6 +128,9 @@ object CRPDecoder { if (cmd == CRPCommands.CMD_WEAR_STATE && payload.isNotEmpty()) { return listOf(RingDecodedEvent.WearingStatus(worn = (payload[0].toInt() and 0xFF) != 0, _timestamp = now)) } + if (cmd == CRPCommands.CMD_QUERY_FIRMWARE_VERSION && payload.isNotEmpty()) { + return decodeFirmwareVersion(payload) + } return listOf(RingDecodedEvent.CommandAck(commandId = ((group shl 4) or (cmd and 0x0F)).toUByte())) } @@ -173,14 +177,25 @@ object CRPDecoder { } } + /** Group-7 (Gomore) replies. No layout is decoded — acked so the raw-packet feed records them. */ + private fun decodeGomoreResponse(cmd: Int): List { + return listOf(RingDecodedEvent.CommandAck(commandId = ((CRPCommands.GROUP_GOMORE shl 4) or (cmd and 0x0F)).toUByte())) + } + /** - * Decode group-7 responses: history queries (cmd 4–7, 14, 48) and device info (cmd 0, 1, 13). - * History layouts are unconfirmed against hardware — emit as CommandAck so the raw-packet feed - * records them without inventing metric values. Extend [decodeHistoryOrDeviceInfoResponse] - * as more layouts are confirmed. + * The firmware version string (`group 3 / cmd 3`). Vendor `g1/a.i1`: + * `onVersion(new String(payload, StandardCharsets.UTF_8))` — a bare UTF-8 string with no + * length prefix or terminator, e.g. `MOY-R1K3-2.1.6` on zaggash's R11 (issue #29). + * + * Surfaced as [RingDecodedEvent.Status] rather than [RingDecodedEvent.FirmwareVersion] because + * that event carries an `Int` — it models the jring 0xF6 numeric version and can't hold this. + * `Status.firmware` is the same path YCBT and LuckRing already use to reach the device record. */ - private fun decodeHistoryOrDeviceInfoResponse(cmd: Int, payload: ByteArray, now: Instant): List { - return listOf(RingDecodedEvent.CommandAck(commandId = ((CRPCommands.GROUP_DEVICE_INFO shl 4) or (cmd and 0x0F)).toUByte())) + private fun decodeFirmwareVersion(payload: ByteArray): List { + // Trims NUL padding as well as whitespace: some firmwares pad the frame to a fixed width. + val version = String(payload, Charsets.UTF_8).trim { it <= ' ' } + if (version.isEmpty()) return emptyList() + return listOf(RingDecodedEvent.Status(address = null, firmware = version)) } /** diff --git a/app/src/main/java/com/pulseloop/ring/CRPProtocol.kt b/app/src/main/java/com/pulseloop/ring/CRPProtocol.kt index 6877f36..a0d3b3f 100644 --- a/app/src/main/java/com/pulseloop/ring/CRPProtocol.kt +++ b/app/src/main/java/com/pulseloop/ring/CRPProtocol.kt @@ -83,10 +83,19 @@ object CRPCommands { // Group 7 — history queries + device info (decompiled b1/e0 + b1/r). // NOTE: most history queries are group 7 (b1/e0 builders use q.b(7,…)/q.c(7,…)), but sleep // and temp are the exception — they live on group 2 (see GROUP_HISTORY below). - const val GROUP_DEVICE_INFO = 7 - const val CMD_QUERY_DEVICE_INFO = 0 // b1/r.a: q.b(7,0) - const val CMD_QUERY_FIRMWARE_VERSION = 1 // b1/r.b: q.b(7,1) - const val CMD_QUERY_DEVICE_SN = 13 // b1/r.c: q.b(7,13) + // Group 7 is the vendor's **Gomore** group (the licensed activity-analytics module), NOT device + // info — every builder in `b1/r` resolves to a Gomore call in `d1/b.java`: + // q.b(7,0)=querySupportGomore q.b(7,1)=querySavedGomoreKey q.b(7,2)=queryGomoreEUID + // q.c(7,3,str)=sendGomoreKey q.b(7,13)=queryGomoreVersion + // The earlier constants here paired `b1/r`'s methods with opcodes positionally (a→0, b→1, c→13) + // and mislabelled all three as device info; `queryFirmwareVersion` was really + // `querySavedGomoreKey`, which is why the R11 answered none of the 23 sends (issue #29). + // Real device queries live on group 3 — see GROUP_DEVICE_CONTROL below. Kept only so a capture + // containing these frames is still identifiable; nothing sends them. + const val GROUP_GOMORE = 7 + const val CMD_QUERY_SUPPORT_GOMORE = 0 // b1/r.e: q.b(7,0) → d1/b.querySupportGomore + const val CMD_QUERY_SAVED_GOMORE_KEY = 1 // b1/r.d: q.b(7,1) → d1/b.querySavedGomoreKey + const val CMD_QUERY_GOMORE_VERSION = 13 // b1/r.c: q.b(7,13) → d1/b.queryGomoreVersion // Group 2 — the day's stored vital timelines. The all-day "timing" histories the vendor's sync // pass actually pulls (`u3/g1.java`) live here with a [day, 0] payload, NOT on group 7: the ring @@ -121,14 +130,22 @@ object CRPCommands { const val CMD_QUERY_TIMING_TEMP_STATE = 21 // b1/i0.a: q.b(2,21) → onTimingState(type, state) const val CMD_QUERY_TIMING_STRESS_STATE = 45 // b1/h0.e: q.b(2,45) - // Group 3 — power control + device-state pushes. + // Group 3 — device control, identity queries, and device-state pushes. Opcodes read off the + // `b1/l` builders via their `d1/b.java` callers (the method letters are alphabetised by the + // decompiler and carry no ordering, so each one is resolved by its caller, not by position). const val GROUP_POWER = 3 - const val CMD_FACTORY_RESET = 0 // b1/l.v: q.b(3,0) - const val CMD_RESTART = 1 // b1/l.w: q.b(3,1) + const val CMD_FACTORY_RESET = 0 // b1/l.v: q.b(3,0) → d1/b.reset + const val CMD_SHUT_DOWN = 1 // b1/l.y: q.b(3,1) → d1/b.shutDown (was mislabelled RESTART) + // Firmware identity — the pair the vendor's own "Firmware information" screen shows + // (`FirmwareInformationActivity`): version is a bare UTF-8 string, hash a hex code. + const val CMD_QUERY_FIRMWARE_VERSION = 3 // b1/l.k: q.b(3,3) → d1/b.queryFirmwareVersion + const val CMD_QUERY_FIRMWARE_HASH = 4 // b1/l.j: q.b(3,4) → d1/b.queryFirmwareHash + const val CMD_QUERY_REALTIME_BATTERY = 6 // b1/l.f: q.b(3,6) → d1/b.queryRealTimeBattery // Autonomous wear-state push: vendor `g1/a.java` case 3→7 → onWearStateChange(payload[0] > 0). // payload[0] == 0 ⇒ ring not on finger / no skin contact (issue #29: an optical spot measure // returns nothing while this is 0; we surface it instead of spinning the full window). const val CMD_WEAR_STATE = 7 + const val CMD_RESTART = 14 // b1/l.w: q.b(3,14) → d1/b.restart // Group 9 — device actions. const val GROUP_ACTION = 9 @@ -309,14 +326,10 @@ object CRPProtocol { fun queryTimingTempState(): ByteArray = frame(CRPCommands.GROUP_HISTORY, CRPCommands.CMD_QUERY_TIMING_TEMP_STATE) - // ---- Device info queries (group 7) ---- - - fun queryDeviceInfo(): ByteArray = - frame(CRPCommands.GROUP_DEVICE_INFO, CRPCommands.CMD_QUERY_DEVICE_INFO) + // ---- Device identity queries (group 3) ---- + // `queryDeviceInfo`/`queryDeviceSN` are gone: they framed group-7 Gomore opcodes, which the ring + // never answers. Firmware version is the one the vendor's Firmware-information screen reads. fun queryFirmwareVersion(): ByteArray = - frame(CRPCommands.GROUP_DEVICE_INFO, CRPCommands.CMD_QUERY_FIRMWARE_VERSION) - - fun queryDeviceSN(): ByteArray = - frame(CRPCommands.GROUP_DEVICE_INFO, CRPCommands.CMD_QUERY_DEVICE_SN) + frame(CRPCommands.GROUP_POWER, CRPCommands.CMD_QUERY_FIRMWARE_VERSION) } diff --git a/app/src/main/java/com/pulseloop/ring/CRPSyncEngine.kt b/app/src/main/java/com/pulseloop/ring/CRPSyncEngine.kt index 78d2a96..cf81997 100644 --- a/app/src/main/java/com/pulseloop/ring/CRPSyncEngine.kt +++ b/app/src/main/java/com/pulseloop/ring/CRPSyncEngine.kt @@ -32,8 +32,10 @@ class CRPSyncEngine(private val writer: RingCommandWriter?) : RingSyncEngine { // the ring's step/calorie algorithm has real inputs. send(CRPProtocol.setTime()) // Query firmware version so the UI doesn't show "Firmware: reading" (zaggash's report). - // NOTE: still unanswered on his R11 — 23 sends, 0 replies in the 2026-07-25 capture — so the - // panel keeps showing "?". The group-7 opcode is the vendor's, but this ring ignores it. + // The 23-sends/0-replies in the 2026-07-25 capture were our fault, not the ring's: the old + // opcode was group 7 cmd 1, which the vendor SDK uses for `querySavedGomoreKey`, not + // firmware. The real query is group 3 cmd 3 (`b1/l.k` → `d1/b.queryFirmwareVersion`), and + // it answers with a UTF-8 string — `MOY-R1K3-2.1.6` on zaggash's R11. send(CRPProtocol.queryFirmwareVersion()) profile?.let { send(userInfoFrame(it)) } sendConnectionReadBacks() diff --git a/app/src/main/java/com/pulseloop/ring/DriverReroute.kt b/app/src/main/java/com/pulseloop/ring/DriverReroute.kt new file mode 100644 index 0000000..83b796f --- /dev/null +++ b/app/src/main/java/com/pulseloop/ring/DriverReroute.kt @@ -0,0 +1,40 @@ +package com.pulseloop.ring + +/** + * Pure post-connect driver-reroute policy. Android GATT objects stay in RingBLEClient; this holds + * the decision so it can be unit-tested (issue #29). + */ +internal object DriverReroute { + + /** + * True when the connection should be moved back to the jring driver. + * + * `RingBLEClient.connectTo`'s `honorSelection` treats a JRING classification of the generic + * "SMART_RING" advertisement as a fallback guess and lets the user's carousel pick win. For a + * jring-firmware ring sold under a Colmi badge that guess is wrong — picking "Colmi / Yawell + * (SmartHealth app)" installs the YCBT driver against a ring that only speaks 000056ff, and the + * connect hard-fails on the missing be940001/be940003 channels. + * + * The advertisement can't distinguish the two, but the discovered GATT table can: the jring + * service present *and* every service the active driver declared absent means the carousel pick + * was wrong about this ring. Requiring the active driver's own services to be missing is what + * keeps this from stealing a ring that genuinely speaks its selected protocol. + * + * @param discoveredServices service UUIDs from the connected GATT table + * @param activeDeclaredServices `serviceUUIDs` of the currently installed driver + * @param activeDeviceType family of the currently installed coordinator + */ + fun shouldRerouteToJring( + discoveredServices: Collection, + activeDeclaredServices: Collection, + activeDeviceType: RingDeviceType?, + ): Boolean { + if (activeDeviceType == RingDeviceType.JRING) return false + val present = discoveredServices.map { it.lowercase() }.toSet() + if (RingUUIDs.SERVICE.lowercase() !in present) return false + // An empty declaration would make "none present" vacuously true; a driver that declares no + // service of its own gives us no evidence the pick was wrong, so leave it alone. + if (activeDeclaredServices.isEmpty()) return false + return activeDeclaredServices.none { it.lowercase() in present } + } +} diff --git a/app/src/main/java/com/pulseloop/ring/RingBLEClient.kt b/app/src/main/java/com/pulseloop/ring/RingBLEClient.kt index effe7d9..81aa834 100644 --- a/app/src/main/java/com/pulseloop/ring/RingBLEClient.kt +++ b/app/src/main/java/com/pulseloop/ring/RingBLEClient.kt @@ -1346,6 +1346,29 @@ class RingBLEClient( updateState { copy(activeWearableModelID = remodel) } } + // Post-connect re-route (issue #29), the *inverse* of the two above: `connectTo`'s + // `honorSelection` lets the carousel pick override a generic-"SMART_RING" JRING + // detection, which is wrong for a jring-firmware ring sold under a Colmi badge + // (itspuia's R09 — the YCBT driver's be940001/be940003 don't exist on it, so + // `topologyFailure()` below hard-failed the connect). Policy in [DriverReroute]; runs + // after the Colmi/CRP blocks so a ring exposing both keeps its more specific family. + if (DriverReroute.shouldRerouteToJring( + discoveredServices = serviceUuids, + activeDeclaredServices = activeDriver?.serviceUUIDs.orEmpty(), + activeDeviceType = activeCoordinator?.deviceType, + ) + ) { + Log.i("RingBLEClient", "Discovered the jring (56ff) service and none of the " + + "${activeCoordinator?.deviceType} driver's own services — re-routing to the jring driver") + installDriver(JringCoordinator) + val remodel = com.pulseloop.wearables.WearableModel.resolve( + advertisedName = activeAdvertisedName, + selectedModelID = _state.value.activeWearableModelID, + family = RingDeviceType.JRING, + )?.id ?: com.pulseloop.wearables.WearableModel.JRING.id + updateState { copy(activeWearableModelID = remodel) } + } + val driver = activeDriver ?: return // Bind the ring's own service first and enable its notifications BEFORE any diff --git a/app/src/test/java/com/pulseloop/ring/CRPDecoderTest.kt b/app/src/test/java/com/pulseloop/ring/CRPDecoderTest.kt index 5efadd9..5eab618 100644 --- a/app/src/test/java/com/pulseloop/ring/CRPDecoderTest.kt +++ b/app/src/test/java/com/pulseloop/ring/CRPDecoderTest.kt @@ -105,6 +105,46 @@ class CRPDecoderTest { assertEquals(false, (worn.single() as PulseEvent.WearState).worn) } + @Test + fun `firmware version decodes as the UTF-8 string the vendor reads`() { + // zaggash's R11, shown as "MOY-R1K3-2.1.6" by the official app's Firmware-information + // screen. Vendor `g1/a.i1`: new String(payload, UTF_8) on group 3 / cmd 3. + val payload = "MOY-R1K3-2.1.6".toByteArray(Charsets.UTF_8) + val frame = CRPProtocol.frame(3, CRPCommands.CMD_QUERY_FIRMWARE_VERSION, payload) + val ev = CRPDecoder.decode(frame, fdd3).single() as RingDecodedEvent.Status + assertEquals("MOY-R1K3-2.1.6", ev.firmware) + assertNull(ev.address) + } + + @Test + fun `firmware version reaches the device record via DeviceStateChanged`() { + val decoded = RingDecodedEvent.Status(address = null, firmware = "MOY-R1K3-2.1.6") + val ev = RingEventBridge.eventsFor(decoded).single() as PulseEvent.DeviceStateChanged + assertEquals("MOY-R1K3-2.1.6", ev.firmware) + } + + @Test + fun `firmware version tolerates NUL padding`() { + val payload = "MOY-R1K3-2.1.6".toByteArray(Charsets.UTF_8) + byteArrayOf(0, 0) + val frame = CRPProtocol.frame(3, CRPCommands.CMD_QUERY_FIRMWARE_VERSION, payload) + val ev = CRPDecoder.decode(frame, fdd3).single() as RingDecodedEvent.Status + assertEquals("MOY-R1K3-2.1.6", ev.firmware) + } + + @Test + fun `empty firmware payload yields no event rather than a blank version`() { + val frame = CRPProtocol.frame(3, CRPCommands.CMD_QUERY_FIRMWARE_VERSION, byteArrayOf(0)) + assertTrue(CRPDecoder.decode(frame, fdd3).none { it is RingDecodedEvent.Status }) + } + + @Test + fun `the firmware query targets group 3 cmd 3, not the group-7 Gomore opcode`() { + // The old query framed q.b(7,1) = querySavedGomoreKey, which the ring never answers. + val sent = CRPProtocol.queryFirmwareVersion() + assertEquals(3, sent[4].toInt() and 0xFF) + assertEquals(3, sent[5].toInt() and 0xFF) + } + @Test fun `unrecognised group3 cmd is acked, not dropped`() { val ev = CRPDecoder.decode(CRPProtocol.frame(3, 2, byteArrayOf(0)), fdd3)[0] diff --git a/app/src/test/java/com/pulseloop/ring/CRPSyncEngineTest.kt b/app/src/test/java/com/pulseloop/ring/CRPSyncEngineTest.kt index 8ae302d..ddbeff6 100644 --- a/app/src/test/java/com/pulseloop/ring/CRPSyncEngineTest.kt +++ b/app/src/test/java/com/pulseloop/ring/CRPSyncEngineTest.kt @@ -31,6 +31,8 @@ class CRPSyncEngineTest { @Test fun `runStartup sends set-time, firmware query, user info, default monitor enables, then the history pull`() { + // The firmware query is 3/3 (`b1/l.k` -> d1/b.queryFirmwareVersion), NOT the 7/1 it used to + // send -- that opcode is the vendor's `querySavedGomoreKey` and the R11 never answers it. val w = FakeWriter() val engine = CRPSyncEngine(w) engine.runStartup() @@ -40,7 +42,7 @@ class CRPSyncEngineTest { // the enables force everything on moments later. Asking afterwards would only describe the // state we just imposed. If this assertion fails, move the call site back — don't reorder the // expectation. See CRPSyncEngine.sendConnectionReadBacks. - assertEquals(listOf(1 to 1, 7 to 1) + readBackQueries + timingEnables + historyQueries, w.opcodes()) + assertEquals(listOf(1 to 1, 3 to 3) + readBackQueries + timingEnables + historyQueries, w.opcodes()) w.sent.clear() engine.setUserProfile( @@ -49,7 +51,7 @@ class CRPSyncEngineTest { engine.runStartup() // A second pass on the same connection re-sends the poll work but NOT the read-backs — // what the ring supports cannot change between syncs. - assertEquals(listOf(1 to 1, 7 to 1, 1 to 0) + timingEnables + historyQueries, w.opcodes()) + assertEquals(listOf(1 to 1, 3 to 3, 1 to 0) + timingEnables + historyQueries, w.opcodes()) } /** diff --git a/app/src/test/java/com/pulseloop/ring/DriverRerouteTest.kt b/app/src/test/java/com/pulseloop/ring/DriverRerouteTest.kt new file mode 100644 index 0000000..025f4e1 --- /dev/null +++ b/app/src/test/java/com/pulseloop/ring/DriverRerouteTest.kt @@ -0,0 +1,88 @@ +package com.pulseloop.ring + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +class DriverRerouteTest { + + /** Exactly what itspuia's R09 reported in `onSearchComplete` (issue #29 diagnostics). */ + private val r09Services = listOf( + "0000fef5-0000-1000-8000-00805f9b34fb", + "000056ff-0000-1000-8000-00805f9b34fb", + "0000ff12-0000-1000-8000-00805f9b34fb", + "00001800-0000-1000-8000-00805f9b34fb", + "00001801-0000-1000-8000-00805f9b34fb", + "0000180f-0000-1000-8000-00805f9b34fb", + "0000180a-0000-1000-8000-00805f9b34fb", + "00001812-0000-1000-8000-00805f9b34fb", + ) + + @Test + fun `re-routes a 56ff ring stranded on the YCBT driver by the carousel pick`() { + assertTrue( + DriverReroute.shouldRerouteToJring( + discoveredServices = r09Services, + activeDeclaredServices = listOf(YCBTUUIDs.SERVICE), + activeDeviceType = RingDeviceType.COLMI_SMART_HEALTH, + ) + ) + } + + @Test + fun `leaves a ring that genuinely speaks its selected protocol alone`() { + // Both services present: the YCBT pick is correct, so 56ff must not steal it. + assertFalse( + DriverReroute.shouldRerouteToJring( + discoveredServices = r09Services + YCBTUUIDs.SERVICE, + activeDeclaredServices = listOf(YCBTUUIDs.SERVICE), + activeDeviceType = RingDeviceType.COLMI_SMART_HEALTH, + ) + ) + } + + @Test + fun `no-ops when the jring driver is already installed`() { + assertFalse( + DriverReroute.shouldRerouteToJring( + discoveredServices = r09Services, + activeDeclaredServices = listOf(RingUUIDs.SERVICE), + activeDeviceType = RingDeviceType.JRING, + ) + ) + } + + @Test + fun `does not fire for a ring with no 56ff service`() { + // zaggash's CRP R11: the CRP block re-routes it; this one must stay out of the way. + assertFalse( + DriverReroute.shouldRerouteToJring( + discoveredServices = listOf(CRPUUIDs.SERVICE, "0000180a-0000-1000-8000-00805f9b34fb"), + activeDeclaredServices = listOf(CRPUUIDs.SERVICE), + activeDeviceType = RingDeviceType.CRP, + ) + ) + } + + @Test + fun `matches service UUIDs case-insensitively`() { + assertTrue( + DriverReroute.shouldRerouteToJring( + discoveredServices = r09Services.map { it.uppercase() }, + activeDeclaredServices = listOf(YCBTUUIDs.SERVICE.uppercase()), + activeDeviceType = RingDeviceType.COLMI_SMART_HEALTH, + ) + ) + } + + @Test + fun `a driver declaring no service of its own is left alone`() { + assertFalse( + DriverReroute.shouldRerouteToJring( + discoveredServices = r09Services, + activeDeclaredServices = emptyList(), + activeDeviceType = RingDeviceType.COLMI_SMART_HEALTH, + ) + ) + } +} From faaa6b87c30034231491b0a16a3b3daf8e8aa29d Mon Sep 17 00:00:00 2001 From: Khoa Truong Date: Fri, 31 Jul 2026 21:55:52 -0700 Subject: [PATCH 2/3] fix(ring): keep the firmware reply off the connection-state path, scope the jring re-route MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects found reviewing the changes on this branch. The CRP firmware string was decoded into RingDecodedEvent.Status, which bridges to DeviceStateChanged(CONNECTED, ...). EventPersistenceSubscriber reads every CONNECTED as "a connection was just established" and, for any family outside preservesSleepOnConnect — CRP among them — answers by running unscoped DELETEs on sleep_sessions and sleep_stage_blocks. CRPSyncEngine.runStartup re-queries firmware on every pass, including the ~30-minute background sync, so each pass would have wiped all stored sleep and then depended on that same pass re-pulling it: an interrupted pass, or a sleep query that comes back empty, loses everything past the ring's 14-day retention. The bug lands exactly when the firmware fix works. Adds RingDecodedEvent.FirmwareRevision / PulseEvent.FirmwareRevision — neither existing event fits, FirmwareVersion carrying the jring 0xF6 Int and Status meaning a connection change — with a persistence branch that stamps firmwareVersion on an existing device row and touches nothing else. An all-padding payload now acks instead of decoding to nothing, so the frame still gets a label in the raw feed. LuckRingDecoder has the same Status shape and the same latent wipe; left alone as pre-existing and out of scope here. DriverReroute.shouldRerouteToJring fired for any non-JRING driver whose declared services were all absent, though its rationale is specifically about honorSelection overriding a generic-"SMART_RING" JRING guess. The gap matters for the Colmi family: the R09/R11 UART profile (6e40fff0/de5bf728) is suspected to be gated behind the OS bond, so an unbonded first connect can present a table without it — and re-routing there is self-sealing, because the model re-resolves to generic JRING whose requiresOsBond is false, the bond that would reveal the Colmi profile never fires, and CONNECTED persists the jring family to LAST_WEARABLE_MODEL_KEY so every later reconnect starts there too. Not recoverable through the carousel. Scopes it to scanDetectedType == JRING, threaded through beginConnect from the two call sites that have a real scan classification. itspuia's R09 is unaffected (his log is the honorSelection line). The trade: a jring-firmware ring advertising a Colmi name pattern rather than SMART_RING is no longer rescued — no such unit reported, and rescuing it would mean overriding a confident scan match, the line connectTo already draws. Also corrects docs this branch left inconsistent: the group-7 comment block in CRPProtocol contradicted the Gomore block directly below it, "see GROUP_DEVICE_CONTROL" named no existing symbol, and two CRPDecoder comments still described group 7 as device info. AGENTS.md documented the Status routing that caused the first defect; rewritten with the reason, plus a note under the bonding allowlist that a driver re-route can revoke a bond the same way widening the allowlist condition once did. 790 unit tests pass. 3 new DriverRerouteTest cases (confident scan match, bond-gated Colmi table, absent scan type); the CRPDecoderTest case that asserted DeviceStateChanged now asserts its absence. --- AGENTS.md | 22 ++++++-- .../java/com/pulseloop/ring/CRPDecoder.kt | 29 +++++++---- .../java/com/pulseloop/ring/CRPProtocol.kt | 12 ++--- .../java/com/pulseloop/ring/DriverReroute.kt | 20 ++++++++ .../java/com/pulseloop/ring/PulseEventBus.kt | 5 ++ .../java/com/pulseloop/ring/RingBLEClient.kt | 17 ++++++- .../com/pulseloop/ring/RingDecodedEvent.kt | 18 +++++++ .../com/pulseloop/ring/RingEventBridge.kt | 5 ++ .../service/EventPersistenceSubscriber.kt | 18 +++++++ .../com/pulseloop/ui/screens/DebugScreen.kt | 4 +- .../java/com/pulseloop/ring/CRPDecoderTest.kt | 30 +++++++---- .../com/pulseloop/ring/DriverRerouteTest.kt | 50 +++++++++++++++++++ 12 files changed, 198 insertions(+), 32 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 73d318f..71e98e9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -37,6 +37,16 @@ to need an OS bond, add it to `WearableModel.requiresOsBond`'s allowlist by name the condition to "whenever `supportBlePair` is set" to match the vendor app — that is exactly the change that caused the regression, and it will cause it again for the R10. +**A driver re-route can silently revoke a bond, too.** `DriverReroute.shouldRerouteToJring` moves a +ring off its selected driver post-connect, and re-resolving the model against the JRING family lands +on the generic `JRING` entry, whose `requiresOsBond` is `false`. On an R09/R11 that is self-sealing: +the Colmi UART (`6e40fff0`/`de5bf728`) is suspected to be gated behind the bond (root `AGENTS.md`), +so a re-route fired on a table that doesn't show it prevents the very bond that would reveal it — +and CONNECTED persists the jring family to `LAST_WEARABLE_MODEL_KEY`, so every later reconnect +starts there and the carousel can't undo it. That is why the re-route is scoped to +`scanDetectedType == JRING`: only connections where a generic-"SMART_RING" guess was actually +overridden. Don't widen it to "any driver whose services are missing." + See `docs/qring-ble-adoption.md` §5a for the full history and the decompiled source references. ## Colmi R11 (CRP "Da Rings") — diagnose from the capture, and decode wear state before blaming code @@ -102,9 +112,15 @@ supporting evidence as the cause. caller, never by position in the builder class.** - **Firmware version is `3/3`**, replying with a bare UTF-8 string (`g1/a.i1`: `onVersion(new String(payload, UTF_8))`) — `MOY-R1K3-2.1.6` on zaggash's R11, matching the vendor - app's Firmware-information screen. Decoded into `RingDecodedEvent.Status(firmware = …)`, the same - path YCBT/LuckRing already use to reach the device record, because `RingDecodedEvent.FirmwareVersion` - carries an `Int` and can't hold this. Sibling group-3 queries confirmed from their callers: + app's Firmware-information screen. Decoded into `RingDecodedEvent.FirmwareRevision`, which exists + because neither older event fits: `FirmwareVersion` carries an `Int` (the jring `0xF6` build), and + `Status` — the path YCBT/LuckRing use — bridges to `DeviceStateChanged(CONNECTED, …)`. **Never + route a firmware reply through `Status`.** Persistence reads every CONNECTED as "a connection was + just established" and, for any family outside `preservesSleepOnConnect` (CRP included), answers by + running unscoped `DELETE`s on `sleep_sessions` + `sleep_stage_blocks`. `runStartup` re-queries + firmware on **every** pass, including the ~30-minute background sync, so that wiring would wipe all + stored sleep on each pass and depend on the same pass re-pulling it — losing everything past the + ring's 14-day retention whenever it didn't. Sibling group-3 queries confirmed from their callers: `3/0` reset, `3/1` shutDown, `3/4` firmware hash, `3/6` real-time battery, `3/7` wear state, `3/14` restart, `3/22` binding reminder. - **Temperature history is `2/22`, not `2/48`.** `q.b(2,48)` is the vendor's `querySleepState` diff --git a/app/src/main/java/com/pulseloop/ring/CRPDecoder.kt b/app/src/main/java/com/pulseloop/ring/CRPDecoder.kt index 7077ccc..02700f9 100644 --- a/app/src/main/java/com/pulseloop/ring/CRPDecoder.kt +++ b/app/src/main/java/com/pulseloop/ring/CRPDecoder.kt @@ -86,7 +86,8 @@ object CRPDecoder { /** * Framed `fdd3` reply: `FD DA 10 `. - * Real-time vital results come on group 1; history queries on group 7; device info on group 7. + * Real-time vital results come on group 1; sleep/all-day history on group 2; device identity + * and state pushes on group 3. Group 7 is the vendor's Gomore module, not device info. */ private fun decodeFramedReply(frame: ByteArray, now: Instant, zone: ZoneId): List { if (frame.size < CRPProtocol.HEADER_SIZE) return emptyList() @@ -121,9 +122,10 @@ object CRPDecoder { return listOf(RingDecodedEvent.CommandAck(commandId = ((group shl 4) or (cmd and 0x0F)).toUByte())) } - // Group 3: power control + the autonomous wear-state push (vendor `g1/a.java` case 3→7, - // `onWearStateChange(payload[0] > 0)`). Confirmed against zaggash's R11 (issue #29): a spot - // measure returns nothing while `payload[0] == 0` (ring off the finger). + // Group 3: device control, the firmware-version string (cmd 3), and the autonomous + // wear-state push (vendor `g1/a.java` case 3→7, `onWearStateChange(payload[0] > 0)`). + // Confirmed against zaggash's R11 (issue #29): a spot measure returns nothing while + // `payload[0] == 0` (ring off the finger). if (group == CRPCommands.GROUP_POWER) { if (cmd == CRPCommands.CMD_WEAR_STATE && payload.isNotEmpty()) { return listOf(RingDecodedEvent.WearingStatus(worn = (payload[0].toInt() and 0xFF) != 0, _timestamp = now)) @@ -187,15 +189,24 @@ object CRPDecoder { * `onVersion(new String(payload, StandardCharsets.UTF_8))` — a bare UTF-8 string with no * length prefix or terminator, e.g. `MOY-R1K3-2.1.6` on zaggash's R11 (issue #29). * - * Surfaced as [RingDecodedEvent.Status] rather than [RingDecodedEvent.FirmwareVersion] because - * that event carries an `Int` — it models the jring 0xF6 numeric version and can't hold this. - * `Status.firmware` is the same path YCBT and LuckRing already use to reach the device record. + * Surfaced as [RingDecodedEvent.FirmwareRevision], not [RingDecodedEvent.FirmwareVersion] + * (which carries an `Int` — the jring 0xF6 numeric build — and can't hold this) and not + * [RingDecodedEvent.Status] (which bridges to `DeviceStateChanged(CONNECTED, …)`; persistence + * rebuilds the sleep tables on every one of those, and [CRPSyncEngine.runStartup] re-queries + * firmware on every sync pass). */ private fun decodeFirmwareVersion(payload: ByteArray): List { // Trims NUL padding as well as whitespace: some firmwares pad the frame to a fixed width. val version = String(payload, Charsets.UTF_8).trim { it <= ' ' } - if (version.isEmpty()) return emptyList() - return listOf(RingDecodedEvent.Status(address = null, firmware = version)) + if (version.isEmpty()) { + // Still ack it, so an all-padding reply is labelled in the raw-packet feed + // rather than showing up as an undecoded frame. + return listOf(RingDecodedEvent.CommandAck( + commandId = ((CRPCommands.GROUP_POWER shl 4) or + (CRPCommands.CMD_QUERY_FIRMWARE_VERSION and 0x0F)).toUByte() + )) + } + return listOf(RingDecodedEvent.FirmwareRevision(version)) } /** diff --git a/app/src/main/java/com/pulseloop/ring/CRPProtocol.kt b/app/src/main/java/com/pulseloop/ring/CRPProtocol.kt index a0d3b3f..5b1fb9a 100644 --- a/app/src/main/java/com/pulseloop/ring/CRPProtocol.kt +++ b/app/src/main/java/com/pulseloop/ring/CRPProtocol.kt @@ -80,9 +80,6 @@ object CRPCommands { const val CMD_ENABLE_TIMING_TEMP = 13 // b1/i0.c: q.c(1,13, [enable]) — all-day temp timing on/off // NOTE: temp's spot-measure toggle is a DIFFERENT opcode (cmd 32, b1/i0.d) — see CMD_MEASURE_TEMP. - // Group 7 — history queries + device info (decompiled b1/e0 + b1/r). - // NOTE: most history queries are group 7 (b1/e0 builders use q.b(7,…)/q.c(7,…)), but sleep - // and temp are the exception — they live on group 2 (see GROUP_HISTORY below). // Group 7 is the vendor's **Gomore** group (the licensed activity-analytics module), NOT device // info — every builder in `b1/r` resolves to a Gomore call in `d1/b.java`: // q.b(7,0)=querySupportGomore q.b(7,1)=querySavedGomoreKey q.b(7,2)=queryGomoreEUID @@ -90,7 +87,7 @@ object CRPCommands { // The earlier constants here paired `b1/r`'s methods with opcodes positionally (a→0, b→1, c→13) // and mislabelled all three as device info; `queryFirmwareVersion` was really // `querySavedGomoreKey`, which is why the R11 answered none of the 23 sends (issue #29). - // Real device queries live on group 3 — see GROUP_DEVICE_CONTROL below. Kept only so a capture + // Real device queries live on group 3 — see GROUP_POWER below. Kept only so a capture // containing these frames is still identifiable; nothing sends them. const val GROUP_GOMORE = 7 const val CMD_QUERY_SUPPORT_GOMORE = 0 // b1/r.e: q.b(7,0) → d1/b.querySupportGomore @@ -130,9 +127,10 @@ object CRPCommands { const val CMD_QUERY_TIMING_TEMP_STATE = 21 // b1/i0.a: q.b(2,21) → onTimingState(type, state) const val CMD_QUERY_TIMING_STRESS_STATE = 45 // b1/h0.e: q.b(2,45) - // Group 3 — device control, identity queries, and device-state pushes. Opcodes read off the - // `b1/l` builders via their `d1/b.java` callers (the method letters are alphabetised by the - // decompiler and carry no ordering, so each one is resolved by its caller, not by position). + // Group 3 — device control, identity queries, and device-state pushes. (Named GROUP_POWER from + // when only the two power opcodes were known; the group is broader than the name.) Opcodes read + // off the `b1/l` builders via their `d1/b.java` callers (the method letters are alphabetised by + // the decompiler and carry no ordering, so each one is resolved by its caller, not by position). const val GROUP_POWER = 3 const val CMD_FACTORY_RESET = 0 // b1/l.v: q.b(3,0) → d1/b.reset const val CMD_SHUT_DOWN = 1 // b1/l.y: q.b(3,1) → d1/b.shutDown (was mislabelled RESTART) diff --git a/app/src/main/java/com/pulseloop/ring/DriverReroute.kt b/app/src/main/java/com/pulseloop/ring/DriverReroute.kt index 83b796f..23923c8 100644 --- a/app/src/main/java/com/pulseloop/ring/DriverReroute.kt +++ b/app/src/main/java/com/pulseloop/ring/DriverReroute.kt @@ -20,16 +20,36 @@ internal object DriverReroute { * was wrong about this ring. Requiring the active driver's own services to be missing is what * keeps this from stealing a ring that genuinely speaks its selected protocol. * + * Scoped to `scanDetectedType == JRING`, i.e. only where `honorSelection` actually overrode a + * generic-"SMART_RING" guess. Two reasons: + * - It matches the rationale. A *confident* scan match (a Colmi name pattern, an advertised + * service UUID) is evidence about the hardware that this function has none of; `connectTo` + * already draws that ambiguous-vs-confident line and only overrides the JRING fallback. + * - Without it, any driver whose services are missing from the table is fair game — including + * the Colmi driver on an R09/R11, whose UART profile (`6e40fff0`/`de5bf728`) is suspected to + * be gated behind the OS bond (root `AGENTS.md`). Re-routing there would be self-sealing: + * the model re-resolves to generic JRING, whose `requiresOsBond` is false, so the bond that + * would reveal the Colmi profile never fires — and the jring family is persisted to + * `LAST_WEARABLE_MODEL_KEY` on CONNECTED, so every later reconnect starts there too. + * + * Cost of the guard: a jring-firmware ring that advertises a *Colmi* name pattern (`R09_ABCD` + * rather than `SMART_RING`) won't be rescued. No such unit has been reported — itspuia's + * advertises the generic name — and rescuing it would mean overriding a confident match. + * * @param discoveredServices service UUIDs from the connected GATT table * @param activeDeclaredServices `serviceUUIDs` of the currently installed driver * @param activeDeviceType family of the currently installed coordinator + * @param scanDetectedType family the *scanner* classified this ring as, or null when the + * connection didn't come from a fresh scan match (a direct reconnect to a stored address) */ fun shouldRerouteToJring( discoveredServices: Collection, activeDeclaredServices: Collection, activeDeviceType: RingDeviceType?, + scanDetectedType: RingDeviceType?, ): Boolean { if (activeDeviceType == RingDeviceType.JRING) return false + if (scanDetectedType != RingDeviceType.JRING) return false val present = discoveredServices.map { it.lowercase() }.toSet() if (RingUUIDs.SERVICE.lowercase() !in present) return false // An empty declaration would make "none present" vacuously true; a driver that declares no diff --git a/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt b/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt index 19f7c7e..254dd9a 100644 --- a/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt +++ b/app/src/main/java/com/pulseloop/ring/PulseEventBus.kt @@ -69,6 +69,11 @@ sealed class PulseEvent { ) : PulseEvent() data class SyncProgress(val stage: String) : PulseEvent() data class FirmwareVersion(val version: Int?) : PulseEvent() + /** The ring's firmware version as a display string (CRP `3/3` → `MOY-R1K3-2.1.6`). Distinct + * from [FirmwareVersion], which carries the jring `0xF6` numeric build, and deliberately not + * folded into [DeviceStateChanged]: that event means the *connection state* changed, and + * persistence rebuilds the sleep tables on every CONNECTED it sees. */ + data class FirmwareRevision(val version: String) : PulseEvent() /** The ring's on-finger / skin-contact state changed. `worn == false` ⇒ an optical spot * measurement can't produce a reading, so the coordinator fast-fails it (issue #29). */ data class WearState(val worn: Boolean) : PulseEvent() diff --git a/app/src/main/java/com/pulseloop/ring/RingBLEClient.kt b/app/src/main/java/com/pulseloop/ring/RingBLEClient.kt index 81aa834..36877b7 100644 --- a/app/src/main/java/com/pulseloop/ring/RingBLEClient.kt +++ b/app/src/main/java/com/pulseloop/ring/RingBLEClient.kt @@ -130,6 +130,10 @@ class RingBLEClient( private var activeSyncEngine: RingSyncEngine? = null // Advertised name of the connection being established, for exact-model resolution + events. private var activeAdvertisedName: String? = null + /** Scanner's family classification for the in-flight connection, before any carousel override. + * Read post-connect by the jring re-route, which must only fire against the ambiguous + * generic-"SMART_RING" guess. See [beginConnect]. */ + private var activeScanDetectedType: RingDeviceType? = null // Set while a "Forget" is waiting for the ring's UNBOND_ACK (0x4B) before teardown. private val forgetLock = Any() @@ -301,6 +305,7 @@ class RingBLEClient( selectedModelID = if (honorSelection) selectedModelID else discoveredRing?.wearableModelID ?: selectedModelID, advertisedName = discoveredRing?.name ?: target.name, + scanDetectedType = detectedType, ) } @@ -671,6 +676,7 @@ class RingBLEClient( .remove(USER_DISCONNECTED_KEY) .apply() activeAdvertisedName = null + activeScanDetectedType = null updateState { copy( connectionState = RingConnectionState.IDLE, @@ -729,6 +735,10 @@ class RingBLEClient( deviceType: RingDeviceType?, selectedModelID: String? = null, advertisedName: String? = null, + /** What the *scanner* classified this ring as, before any carousel override — null when + * the attempt didn't come from a fresh scan match. [DriverReroute.shouldRerouteToJring] + * needs it to tell a generic-"SMART_RING" guess apart from a confident family match. */ + scanDetectedType: RingDeviceType? = null, ) { if (watchdogReconnectPaused) return if (synchronized(forgetLock) { forgetPending || forgetFinalizing }) return @@ -740,7 +750,7 @@ class RingBLEClient( ownershipRetryJob = scope.launch { delay(PROCESS_OWNER_RETRY_MS) ownershipRetryJob = null - beginConnect(target, deviceType, selectedModelID, advertisedName) + beginConnect(target, deviceType, selectedModelID, advertisedName, scanDetectedType) } } return @@ -765,6 +775,7 @@ class RingBLEClient( // Resolve the exact catalog model for this connection: Bluetooth identity wins over the // user's carousel selection; family mismatches are rejected (iOS #49 beginConnect). activeAdvertisedName = advertisedName + activeScanDetectedType = scanDetectedType val resolvedModelID = com.pulseloop.wearables.WearableModel.resolve( advertisedName = advertisedName, selectedModelID = selectedModelID, @@ -1190,6 +1201,7 @@ class RingBLEClient( matchedType ?: lastKnownDeviceType, selectedModelID = lastKnownWearableModelID, advertisedName = name, + scanDetectedType = matchedType, ) return } @@ -1352,10 +1364,13 @@ class RingBLEClient( // (itspuia's R09 — the YCBT driver's be940001/be940003 don't exist on it, so // `topologyFailure()` below hard-failed the connect). Policy in [DriverReroute]; runs // after the Colmi/CRP blocks so a ring exposing both keeps its more specific family. + // Scoped to the connections `honorSelection` actually overrode — see the guard notes + // on [DriverReroute.shouldRerouteToJring] for why a confident scan match is off-limits. if (DriverReroute.shouldRerouteToJring( discoveredServices = serviceUuids, activeDeclaredServices = activeDriver?.serviceUUIDs.orEmpty(), activeDeviceType = activeCoordinator?.deviceType, + scanDetectedType = activeScanDetectedType, ) ) { Log.i("RingBLEClient", "Discovered the jring (56ff) service and none of the " + diff --git a/app/src/main/java/com/pulseloop/ring/RingDecodedEvent.kt b/app/src/main/java/com/pulseloop/ring/RingDecodedEvent.kt index 484d20e..497c202 100644 --- a/app/src/main/java/com/pulseloop/ring/RingDecodedEvent.kt +++ b/app/src/main/java/com/pulseloop/ring/RingDecodedEvent.kt @@ -84,6 +84,7 @@ sealed class RingDecodedEvent { is TimeSyncAck -> this._timestamp is CommandAck -> Instant.EPOCH is FirmwareVersion -> Instant.EPOCH + is FirmwareRevision -> Instant.EPOCH is BindNotify -> Instant.EPOCH is BandFunction -> Instant.EPOCH is SupportFunctions -> Instant.EPOCH @@ -347,6 +348,23 @@ sealed class RingDecodedEvent { override val debugJSON = """{"version":${version ?: 0}}""" } + /** + * A firmware version *string* — e.g. the CRP R11's `MOY-R1K3-2.1.6` (group `3/3`). [FirmwareVersion] + * above carries the jring `0xF6` numeric build instead and can't hold this. + * + * Deliberately its own event rather than [Status]: `Status` bridges to + * `DeviceStateChanged(CONNECTED, …)`, which persistence reads as "a connection was just + * established" and answers by rebuilding the sleep tables from scratch. A firmware reply says + * nothing about the connection, and the CRP engine re-queries it on every ~30-minute sync pass. + */ + data class FirmwareRevision( + val version: String, + ) : RingDecodedEvent() { + override val kind = "firmware_revision" + override val confidence = DecodeConfidence.KNOWN + override val debugJSON = """{"version":"$version"}""" + } + data class TimeSyncAck( val _timestamp: Instant ) : RingDecodedEvent() { diff --git a/app/src/main/java/com/pulseloop/ring/RingEventBridge.kt b/app/src/main/java/com/pulseloop/ring/RingEventBridge.kt index 2c0c70a..0ecf489 100644 --- a/app/src/main/java/com/pulseloop/ring/RingEventBridge.kt +++ b/app/src/main/java/com/pulseloop/ring/RingEventBridge.kt @@ -89,6 +89,11 @@ object RingEventBridge { is RingDecodedEvent.FirmwareVersion -> listOf(PulseEvent.FirmwareVersion(decoded.version)) + is RingDecodedEvent.FirmwareRevision -> { + if (decoded.version.isBlank()) emptyList() + else listOf(PulseEvent.FirmwareRevision(decoded.version)) + } + is RingDecodedEvent.Spo2Complete -> listOf(PulseEvent.Spo2Complete(decoded._timestamp)) diff --git a/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt b/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt index 6f95dfc..4215690 100644 --- a/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt +++ b/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt @@ -295,6 +295,24 @@ class EventPersistenceSubscriber( // requested on every connect via runStartup(), with DIS 0x2A26 as a fallback, so // 0xF6 is never needed here. Kept as a decoded event purely for diagnostics. } + is PulseEvent.FirmwareRevision -> { + // Stamp the version onto an existing row only — never create one, so a late reply + // can't resurrect a forgotten ring (same rule as DeviceStateChanged above). + // + // This is why the firmware string does NOT travel as DeviceStateChanged: that + // branch treats every CONNECTED as "a connection was just established" and, for + // families outside preservesSleepOnConnect (CRP among them), answers by clearing + // sleep_sessions + sleep_stage_blocks outright. CRPSyncEngine.runStartup re-queries + // firmware on every pass — including the ~30-minute background sync — so routing it + // through there would wipe all stored sleep on each pass and rely on the same pass + // re-pulling it, losing everything past the ring's 14-day retention if it didn't. + val device = db.deviceDao().currentReal() ?: return + if (event.version.isBlank() || event.version == device.firmwareVersion) return + db.deviceDao().upsert(device.copy( + firmwareVersion = event.version, + updatedAt = System.currentTimeMillis(), + )) + } } } diff --git a/app/src/main/java/com/pulseloop/ui/screens/DebugScreen.kt b/app/src/main/java/com/pulseloop/ui/screens/DebugScreen.kt index f892a1d..04a2ac9 100644 --- a/app/src/main/java/com/pulseloop/ui/screens/DebugScreen.kt +++ b/app/src/main/java/com/pulseloop/ui/screens/DebugScreen.kt @@ -278,6 +278,7 @@ private fun labelFor(event: PulseEvent): String = when (event) { is PulseEvent.DeviceForgotten -> "Forgot" is PulseEvent.SyncProgress -> "Sync" is PulseEvent.FirmwareVersion -> "FW Ver" + is PulseEvent.FirmwareRevision -> "FW Rev" is PulseEvent.RawPacket -> "Raw Pkt" is PulseEvent.ActivitySyncReset -> "Act Reset" } @@ -300,6 +301,7 @@ private fun detailFor(event: PulseEvent): String = when (event) { ?: event.deviceType.displayName is PulseEvent.SyncProgress -> event.stage is PulseEvent.FirmwareVersion -> "V${event.version ?: 0}" + is PulseEvent.FirmwareRevision -> event.version is PulseEvent.RawPacket -> hexDump(event.data) + " ${event.direction.name}" else -> "" } @@ -335,6 +337,6 @@ private fun colorFor(event: PulseEvent): Color = when (event) { is PulseEvent.DeviceStateChanged -> Color(0xFF90A4AE) is PulseEvent.DeviceIdentified -> Color(0xFF42A5F5) is PulseEvent.SyncProgress -> Color(0xFF78909C) - is PulseEvent.FirmwareVersion -> Color(0xFF90A4AE) + is PulseEvent.FirmwareVersion, is PulseEvent.FirmwareRevision -> Color(0xFF90A4AE) else -> Color(0xFFBDBDBD) } diff --git a/app/src/test/java/com/pulseloop/ring/CRPDecoderTest.kt b/app/src/test/java/com/pulseloop/ring/CRPDecoderTest.kt index 5eab618..e83ef4a 100644 --- a/app/src/test/java/com/pulseloop/ring/CRPDecoderTest.kt +++ b/app/src/test/java/com/pulseloop/ring/CRPDecoderTest.kt @@ -111,30 +111,38 @@ class CRPDecoderTest { // screen. Vendor `g1/a.i1`: new String(payload, UTF_8) on group 3 / cmd 3. val payload = "MOY-R1K3-2.1.6".toByteArray(Charsets.UTF_8) val frame = CRPProtocol.frame(3, CRPCommands.CMD_QUERY_FIRMWARE_VERSION, payload) - val ev = CRPDecoder.decode(frame, fdd3).single() as RingDecodedEvent.Status - assertEquals("MOY-R1K3-2.1.6", ev.firmware) - assertNull(ev.address) + val ev = CRPDecoder.decode(frame, fdd3).single() as RingDecodedEvent.FirmwareRevision + assertEquals("MOY-R1K3-2.1.6", ev.version) } @Test - fun `firmware version reaches the device record via DeviceStateChanged`() { - val decoded = RingDecodedEvent.Status(address = null, firmware = "MOY-R1K3-2.1.6") - val ev = RingEventBridge.eventsFor(decoded).single() as PulseEvent.DeviceStateChanged - assertEquals("MOY-R1K3-2.1.6", ev.firmware) + fun `firmware version reaches the device record as its own event, not a connection change`() { + // NOT RingDecodedEvent.Status: that bridges to DeviceStateChanged(CONNECTED), which + // persistence answers by clearing sleep_sessions for families outside + // preservesSleepOnConnect — CRP among them — and runStartup re-queries firmware on every + // ~30-minute sync pass, so that path would wipe stored sleep on each one. + val decoded = RingDecodedEvent.FirmwareRevision("MOY-R1K3-2.1.6") + val events = RingEventBridge.eventsFor(decoded) + assertEquals("MOY-R1K3-2.1.6", (events.single() as PulseEvent.FirmwareRevision).version) + assertTrue(events.none { it is PulseEvent.DeviceStateChanged }) } @Test fun `firmware version tolerates NUL padding`() { val payload = "MOY-R1K3-2.1.6".toByteArray(Charsets.UTF_8) + byteArrayOf(0, 0) val frame = CRPProtocol.frame(3, CRPCommands.CMD_QUERY_FIRMWARE_VERSION, payload) - val ev = CRPDecoder.decode(frame, fdd3).single() as RingDecodedEvent.Status - assertEquals("MOY-R1K3-2.1.6", ev.firmware) + val ev = CRPDecoder.decode(frame, fdd3).single() as RingDecodedEvent.FirmwareRevision + assertEquals("MOY-R1K3-2.1.6", ev.version) } @Test - fun `empty firmware payload yields no event rather than a blank version`() { + fun `empty firmware payload is acked, not reported as a blank version`() { val frame = CRPProtocol.frame(3, CRPCommands.CMD_QUERY_FIRMWARE_VERSION, byteArrayOf(0)) - assertTrue(CRPDecoder.decode(frame, fdd3).none { it is RingDecodedEvent.Status }) + val events = CRPDecoder.decode(frame, fdd3) + assertTrue(events.none { it is RingDecodedEvent.FirmwareRevision }) + assertTrue(events.single() is RingDecodedEvent.CommandAck) + // ...and nothing downstream mistakes the ack for a firmware reading. + assertTrue(RingEventBridge.eventsFor(events.single()).isEmpty()) } @Test diff --git a/app/src/test/java/com/pulseloop/ring/DriverRerouteTest.kt b/app/src/test/java/com/pulseloop/ring/DriverRerouteTest.kt index 025f4e1..cc16f8b 100644 --- a/app/src/test/java/com/pulseloop/ring/DriverRerouteTest.kt +++ b/app/src/test/java/com/pulseloop/ring/DriverRerouteTest.kt @@ -25,6 +25,7 @@ class DriverRerouteTest { discoveredServices = r09Services, activeDeclaredServices = listOf(YCBTUUIDs.SERVICE), activeDeviceType = RingDeviceType.COLMI_SMART_HEALTH, + scanDetectedType = RingDeviceType.JRING, ) ) } @@ -37,6 +38,7 @@ class DriverRerouteTest { discoveredServices = r09Services + YCBTUUIDs.SERVICE, activeDeclaredServices = listOf(YCBTUUIDs.SERVICE), activeDeviceType = RingDeviceType.COLMI_SMART_HEALTH, + scanDetectedType = RingDeviceType.JRING, ) ) } @@ -48,6 +50,7 @@ class DriverRerouteTest { discoveredServices = r09Services, activeDeclaredServices = listOf(RingUUIDs.SERVICE), activeDeviceType = RingDeviceType.JRING, + scanDetectedType = RingDeviceType.JRING, ) ) } @@ -60,6 +63,7 @@ class DriverRerouteTest { discoveredServices = listOf(CRPUUIDs.SERVICE, "0000180a-0000-1000-8000-00805f9b34fb"), activeDeclaredServices = listOf(CRPUUIDs.SERVICE), activeDeviceType = RingDeviceType.CRP, + scanDetectedType = RingDeviceType.JRING, ) ) } @@ -71,6 +75,7 @@ class DriverRerouteTest { discoveredServices = r09Services.map { it.uppercase() }, activeDeclaredServices = listOf(YCBTUUIDs.SERVICE.uppercase()), activeDeviceType = RingDeviceType.COLMI_SMART_HEALTH, + scanDetectedType = RingDeviceType.JRING, ) ) } @@ -82,6 +87,51 @@ class DriverRerouteTest { discoveredServices = r09Services, activeDeclaredServices = emptyList(), activeDeviceType = RingDeviceType.COLMI_SMART_HEALTH, + scanDetectedType = RingDeviceType.JRING, + ) + ) + } + + @Test + fun `never steals a ring the scanner confidently matched to its own family`() { + // A ring whose advertisement carried a real Colmi signature (name pattern or service UUID) + // is not the ambiguous "SMART_RING" case this re-route exists for — `connectTo` never + // overrode anything here, so there is no wrong guess to undo. + assertFalse( + DriverReroute.shouldRerouteToJring( + discoveredServices = r09Services, + activeDeclaredServices = listOf(YCBTUUIDs.SERVICE), + activeDeviceType = RingDeviceType.COLMI_SMART_HEALTH, + scanDetectedType = RingDeviceType.COLMI_SMART_HEALTH, + ) + ) + } + + @Test + fun `never strands a Colmi R09-R11 whose UART profile is hidden pre-bond`() { + // The regression this guard exists to prevent. The Colmi UART (6e40fff0/de5bf728) is + // suspected to be bond-gated on the R09/R11 (root AGENTS.md), so a first, unbonded connect + // can show a table without it. Re-routing to jring here would be self-sealing: the model + // re-resolves to generic JRING, requiresOsBond goes false, the bond that would reveal the + // Colmi profile never fires, and the jring family is persisted for every later reconnect. + assertFalse( + DriverReroute.shouldRerouteToJring( + discoveredServices = r09Services, + activeDeclaredServices = listOf(ColmiUUIDs.SERVICE_V1, ColmiUUIDs.SERVICE_V2), + activeDeviceType = RingDeviceType.COLMI_R02, + scanDetectedType = RingDeviceType.COLMI_R02, + ) + ) + } + + @Test + fun `stays out of a direct reconnect that had no scan classification`() { + assertFalse( + DriverReroute.shouldRerouteToJring( + discoveredServices = r09Services, + activeDeclaredServices = listOf(YCBTUUIDs.SERVICE), + activeDeviceType = RingDeviceType.COLMI_SMART_HEALTH, + scanDetectedType = null, ) ) } From c3b669eef8d441c5cc1bb65fa454cd49ffa729b1 Mon Sep 17 00:00:00 2001 From: Khoa Truong Date: Fri, 31 Jul 2026 22:25:31 -0700 Subject: [PATCH 3/3] fix(persistence): gate the connect rebuild on a real transition, not on any CONNECTED MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review of faaa6b8 found the fix was the wrong shape. It moved the CRP firmware reply off RingDecodedEvent.Status so it would stop reaching the CONNECTED branch, and called jring and LuckRing "pre-existing, out of scope" — but the destructive rebuild is triggered by the CONNECTED state itself, not by the firmware field, so moving one field fixed one family and left the other two broken while AGENTS.md gained a rule ("never route a firmware reply through Status") that the shipping code violated. Two things publish DeviceStateChanged(CONNECTED), and deviceType separates them exactly: RingBLEClient's own connect always carries the resolved family (installDriver assigns activeCoordinator before the CCCD write that gates CONNECTED, and it is never nulled), while RingEventBridge maps every decoder's Status to CONNECTED and never sets it. The second kind are ordinary device-info replies — jring 0x0C, LuckRing dev-info, YCBT status packets — and JringSyncEngine sends 0x0C as the first command of runStartup, which is also the ~30-minute background sync. So on jring and LuckRing every sync pass ran unscoped DELETEs on sleep_sessions and sleep_stage_blocks and depended on that same pass re-pulling them, losing anything past the ring's retention when it did not. jring is the app's original family; the earlier review understated this as a LuckRing footnote. isConnectTransition(event.deviceType) gates the rebuild at the source, which covers every family at once. preservesSleepOnConnect was an attempt to solve this per-family and could not work: the set of families that re-assert CONNECTED is most of them. The CRP FirmwareRevision routing stays — a firmware string is not a connection event regardless — but it is no longer load-bearing, so the decoders for jring and LuckRing are left alone rather than churned. Also from the same review: - Tests now cover the invariant in EventPersistenceIdentityTest, next to the preservesSleepOnConnect case it depends on. The previous commit asserted only that the bridge emits no DeviceStateChanged — a proxy for the real rule — and left the persistence behaviour untested while criticising PR #42 for a coverage gap. - Narrowed the bond-gating claim. Root AGENTS.md hedges it for the R11 alone ("appears to be"); the DriverReroute KDoc, this AGENTS.md and a test name had all restated it as fact about the R09 and R11 both. Writing a guess down as fact is the exact defect PR #42 exists to correct. - decodeFirmwareVersion returns null for an unreadable payload and lets the caller ack, instead of rebuilding the identical 0x33 CommandAck the generic group-3 fallthrough already produces. Dropped one of three redundant blank checks; the bridge keeps the gate, matching how it gates every other event. Not addressed: PulseLoopApp's DIS fallback only preserves a stored firmware string containing "V" (jring's 003A002AV138), so MOY-R1K3-2.1.6 is overwritable and the two writers can ping-pong by completion order. Pre-existing, and only reachable if a CRP ring exposes DIS 0x2A26/0x2A28 — zaggash's reported "?" suggests his does not. Wants hardware before being changed. 793 unit tests pass. Behaviour note: the firmware reply no longer refreshes DeviceEntity.lastSyncAt, which matches every other family and matches CRP before PR #42. --- AGENTS.md | 51 ++++++++++++++----- .../java/com/pulseloop/ring/CRPDecoder.kt | 16 ++---- .../java/com/pulseloop/ring/DriverReroute.kt | 13 +++-- .../service/EventPersistenceSubscriber.kt | 51 ++++++++++++++----- .../com/pulseloop/ring/DriverRerouteTest.kt | 13 ++--- .../service/EventPersistenceIdentityTest.kt | 31 +++++++++++ 6 files changed, 127 insertions(+), 48 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 71e98e9..dfd9c4b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -39,16 +39,44 @@ the change that caused the regression, and it will cause it again for the R10. **A driver re-route can silently revoke a bond, too.** `DriverReroute.shouldRerouteToJring` moves a ring off its selected driver post-connect, and re-resolving the model against the JRING family lands -on the generic `JRING` entry, whose `requiresOsBond` is `false`. On an R09/R11 that is self-sealing: -the Colmi UART (`6e40fff0`/`de5bf728`) is suspected to be gated behind the bond (root `AGENTS.md`), -so a re-route fired on a table that doesn't show it prevents the very bond that would reveal it — -and CONNECTED persists the jring family to `LAST_WEARABLE_MODEL_KEY`, so every later reconnect -starts there and the carousel can't undo it. That is why the re-route is scoped to +on the generic `JRING` entry, whose `requiresOsBond` is `false`. Root `AGENTS.md` records one hedged +suspicion — that the **R11**'s full Colmi UART profile (`6e40fff0`/`de5bf728`) *appears* to be gated +behind an OS bond. Unproven, and about one model, but if it holds anywhere then a re-route fired on +a table missing that profile is self-sealing: it prevents the very bond that would reveal it, and +CONNECTED persists the jring family to `LAST_WEARABLE_MODEL_KEY`, so every later reconnect starts +there and the carousel can't undo it. That is why the re-route is scoped to `scanDetectedType == JRING`: only connections where a generic-"SMART_RING" guess was actually overridden. Don't widen it to "any driver whose services are missing." See `docs/qring-ble-adoption.md` §5a for the full history and the decompiled source references. +## Only the client's own connect may rebuild stored data + +**Read this before touching `EventPersistenceSubscriber`'s `DeviceStateChanged` branch, or before +adding a family to `preservesSleepOnConnect`.** + +`RingConnectionState.CONNECTED` arrives from two unrelated places, and only one of them means a +connection was established: + +- `RingBLEClient`'s own connect event — always carries `deviceType` (`activeCoordinator` is set by + `installDriver`, which runs before the CCCD write that gates CONNECTED). +- `RingEventBridge`, which maps **every** decoder's `RingDecodedEvent.Status` to CONNECTED and never + sets `deviceType`. These are ordinary device-info replies: jring `0x0C`, LuckRing dev-info, YCBT + status packets. `runStartup` re-sends them, and `runStartup` is also the ~30-minute background + sync — so they recur for the whole life of a connection. + +The CONNECTED branch clears and rebuilds (unscoped `DELETE FROM sleep_sessions` / +`sleep_stage_blocks` for families outside `preservesSleepOnConnect`). Ungated, every background sync +pass on jring or LuckRing wiped all stored sleep and depended on that same pass re-pulling it — +losing anything past the ring's retention when the pass was interrupted or came back empty. +`isConnectTransition(event.deviceType)` is the gate. **Don't remove it, and don't try to fix this +family-by-family** — `preservesSleepOnConnect` was an attempt at that, and the set of families that +re-assert CONNECTED turned out to be most of them. + +Corollary for new protocol work: a reply that merely reports something about the device (firmware, +serial, capabilities) is not a connection event. Give it its own `RingDecodedEvent` — as +`FirmwareRevision` does — rather than hanging it off `Status`. + ## Colmi R11 (CRP "Da Rings") — diagnose from the capture, and decode wear state before blaming code **Read this before changing anything in `CRP*` startup, sync, all-day-monitoring, or history code — @@ -114,15 +142,10 @@ supporting evidence as the cause. `onVersion(new String(payload, UTF_8))`) — `MOY-R1K3-2.1.6` on zaggash's R11, matching the vendor app's Firmware-information screen. Decoded into `RingDecodedEvent.FirmwareRevision`, which exists because neither older event fits: `FirmwareVersion` carries an `Int` (the jring `0xF6` build), and - `Status` — the path YCBT/LuckRing use — bridges to `DeviceStateChanged(CONNECTED, …)`. **Never - route a firmware reply through `Status`.** Persistence reads every CONNECTED as "a connection was - just established" and, for any family outside `preservesSleepOnConnect` (CRP included), answers by - running unscoped `DELETE`s on `sleep_sessions` + `sleep_stage_blocks`. `runStartup` re-queries - firmware on **every** pass, including the ~30-minute background sync, so that wiring would wipe all - stored sleep on each pass and depend on the same pass re-pulling it — losing everything past the - ring's 14-day retention whenever it didn't. Sibling group-3 queries confirmed from their callers: - `3/0` reset, `3/1` shutDown, `3/4` firmware hash, `3/6` real-time battery, `3/7` wear state, - `3/14` restart, `3/22` binding reminder. + `Status` bridges to `DeviceStateChanged(CONNECTED, …)` — a connection-state event, which a + firmware string is not. Sibling group-3 queries confirmed from their callers: `3/0` reset, + `3/1` shutDown, `3/4` firmware hash, `3/6` real-time battery, `3/7` wear state, `3/14` restart, + `3/22` binding reminder. - **Temperature history is `2/22`, not `2/48`.** `q.b(2,48)` is the vendor's `querySleepState` (`d1/b.java` line 650); real temp history is `i0.b(day, frameIndex)` = `q.c(2,22,[day,idx])`, the same shape as the other timing histories. Its sample layout is still unconfirmed — no non-empty diff --git a/app/src/main/java/com/pulseloop/ring/CRPDecoder.kt b/app/src/main/java/com/pulseloop/ring/CRPDecoder.kt index 02700f9..88d963b 100644 --- a/app/src/main/java/com/pulseloop/ring/CRPDecoder.kt +++ b/app/src/main/java/com/pulseloop/ring/CRPDecoder.kt @@ -130,8 +130,9 @@ object CRPDecoder { if (cmd == CRPCommands.CMD_WEAR_STATE && payload.isNotEmpty()) { return listOf(RingDecodedEvent.WearingStatus(worn = (payload[0].toInt() and 0xFF) != 0, _timestamp = now)) } - if (cmd == CRPCommands.CMD_QUERY_FIRMWARE_VERSION && payload.isNotEmpty()) { - return decodeFirmwareVersion(payload) + if (cmd == CRPCommands.CMD_QUERY_FIRMWARE_VERSION) { + // null ⇒ nothing readable in the payload; fall through to the ack below. + decodeFirmwareVersion(payload)?.let { return it } } return listOf(RingDecodedEvent.CommandAck(commandId = ((group shl 4) or (cmd and 0x0F)).toUByte())) } @@ -195,17 +196,10 @@ object CRPDecoder { * rebuilds the sleep tables on every one of those, and [CRPSyncEngine.runStartup] re-queries * firmware on every sync pass). */ - private fun decodeFirmwareVersion(payload: ByteArray): List { + private fun decodeFirmwareVersion(payload: ByteArray): List? { // Trims NUL padding as well as whitespace: some firmwares pad the frame to a fixed width. val version = String(payload, Charsets.UTF_8).trim { it <= ' ' } - if (version.isEmpty()) { - // Still ack it, so an all-padding reply is labelled in the raw-packet feed - // rather than showing up as an undecoded frame. - return listOf(RingDecodedEvent.CommandAck( - commandId = ((CRPCommands.GROUP_POWER shl 4) or - (CRPCommands.CMD_QUERY_FIRMWARE_VERSION and 0x0F)).toUByte() - )) - } + if (version.isEmpty()) return null // empty or all-padding — the caller acks it instead return listOf(RingDecodedEvent.FirmwareRevision(version)) } diff --git a/app/src/main/java/com/pulseloop/ring/DriverReroute.kt b/app/src/main/java/com/pulseloop/ring/DriverReroute.kt index 23923c8..c3074ad 100644 --- a/app/src/main/java/com/pulseloop/ring/DriverReroute.kt +++ b/app/src/main/java/com/pulseloop/ring/DriverReroute.kt @@ -26,11 +26,14 @@ internal object DriverReroute { * service UUID) is evidence about the hardware that this function has none of; `connectTo` * already draws that ambiguous-vs-confident line and only overrides the JRING fallback. * - Without it, any driver whose services are missing from the table is fair game — including - * the Colmi driver on an R09/R11, whose UART profile (`6e40fff0`/`de5bf728`) is suspected to - * be gated behind the OS bond (root `AGENTS.md`). Re-routing there would be self-sealing: - * the model re-resolves to generic JRING, whose `requiresOsBond` is false, so the bond that - * would reveal the Colmi profile never fires — and the jring family is persisted to - * `LAST_WEARABLE_MODEL_KEY` on CONNECTED, so every later reconnect starts there too. + * the Colmi driver on a ring whose UART profile (`6e40fff0`/`de5bf728`) isn't in the table + * yet. Root `AGENTS.md` records exactly one such suspicion, hedged, for the **R11**: its full + * Colmi UART profile *appears* to be gated behind an OS bond. That is a hypothesis about one + * model, not an established fact about the family — but re-routing on it would be + * self-sealing, which is what makes it worth guarding against: the model re-resolves to + * generic JRING, whose `requiresOsBond` is false, so the bond that would reveal the Colmi + * profile never fires — and the jring family is persisted to `LAST_WEARABLE_MODEL_KEY` on + * CONNECTED, so every later reconnect starts there too. * * Cost of the guard: a jring-firmware ring that advertises a *Colmi* name pattern (`R09_ABCD` * rather than `SMART_RING`) won't be rescued. No such unit has been reported — itspuia's diff --git a/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt b/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt index 4215690..a9603e3 100644 --- a/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt +++ b/app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt @@ -80,17 +80,26 @@ class EventPersistenceSubscriber( val device = existing ?: DeviceEntity() val state = when (event.state) { RingConnectionState.CONNECTED -> { - db.measurementDao().clearDemo() - db.activityDailyDao().clearDemo() - if (preservesSleepOnConnect(event.deviceType, existing?.deviceType)) { - // YCBT status packets re-emit CONNECTED while history is still arriving. - db.sleepStageBlockDao().clearDemo() - db.sleepSessionDao().clearDemo() - } else { - // Packet-based families rebuild sleep on connect. Clear blocks with - // sessions so legacy midnight-keyed blocks cannot contaminate a rebuild. - db.sleepStageBlockDao().clear() - db.sleepSessionDao().clear() + // Only the *client's own* connect event is a connection transition, and it + // is the only CONNECTED that may run the destructive rebuild below. Decoders + // re-assert CONNECTED mid-session from ordinary device-info replies — jring + // `0x0C`, LuckRing dev-info, YCBT status packets — and those replies are + // re-sent by `runStartup`, which is also the ~30-minute background sync. Left + // ungated, each pass would wipe every stored sleep session and then depend on + // that same pass re-pulling it. See [isConnectTransition]. + if (isConnectTransition(event.deviceType)) { + db.measurementDao().clearDemo() + db.activityDailyDao().clearDemo() + if (preservesSleepOnConnect(event.deviceType, existing?.deviceType)) { + // YCBT status packets re-emit CONNECTED while history is still arriving. + db.sleepStageBlockDao().clearDemo() + db.sleepSessionDao().clearDemo() + } else { + // Packet-based families rebuild sleep on connect. Clear blocks with + // sessions so legacy midnight-keyed blocks cannot contaminate a rebuild. + db.sleepStageBlockDao().clear() + db.sleepSessionDao().clear() + } } "CONNECTED" } @@ -307,7 +316,7 @@ class EventPersistenceSubscriber( // through there would wipe all stored sleep on each pass and rely on the same pass // re-pulling it, losing everything past the ring's 14-day retention if it didn't. val device = db.deviceDao().currentReal() ?: return - if (event.version.isBlank() || event.version == device.firmwareVersion) return + if (event.version == device.firmwareVersion) return // blank is gated in the bridge db.deviceDao().upsert(device.copy( firmwareVersion = event.version, updatedAt = System.currentTimeMillis(), @@ -619,6 +628,24 @@ class EventPersistenceSubscriber( internal fun historyMeasurementId(kind: MeasurementKind, timestamp: Long): String = "history:${kind.key}:$timestamp" +/** + * True when a `DeviceStateChanged(CONNECTED, …)` is a real connection transition rather than a + * mid-session re-assertion, and may therefore run the clear-and-rebuild in [EventPersistenceSubscriber]. + * + * Exactly two things publish a CONNECTED event, and `deviceType` separates them cleanly: + * - `RingBLEClient`'s own connect always passes `deviceType = activeCoordinator.deviceType`, which + * is non-null by then (`installDriver` ran before the CCCD write that gates CONNECTED). + * - `RingEventBridge` maps every decoder's `RingDecodedEvent.Status` to CONNECTED and never sets + * `deviceType`. Those are ordinary device-info replies — jring `0x0C`, LuckRing dev-info, YCBT + * status packets — re-sent by `runStartup` on every sync pass, not connection events. + * + * Before this gate, each of those replies ran the rebuild, so a background sync pass on jring or + * LuckRing dropped every `sleep_sessions` / `sleep_stage_blocks` row (unscoped `DELETE`s) and relied + * on the same pass re-pulling them — losing anything past the ring's retention when it didn't. + * A per-family allowlist can't fix this: the families that re-assert CONNECTED are most of them. + */ +internal fun isConnectTransition(eventDeviceType: RingDeviceType?): Boolean = eventDeviceType != null + internal fun preservesSleepOnConnect( eventDeviceType: RingDeviceType?, persistedDeviceType: RingDeviceType? = null, diff --git a/app/src/test/java/com/pulseloop/ring/DriverRerouteTest.kt b/app/src/test/java/com/pulseloop/ring/DriverRerouteTest.kt index cc16f8b..c7898fc 100644 --- a/app/src/test/java/com/pulseloop/ring/DriverRerouteTest.kt +++ b/app/src/test/java/com/pulseloop/ring/DriverRerouteTest.kt @@ -108,12 +108,13 @@ class DriverRerouteTest { } @Test - fun `never strands a Colmi R09-R11 whose UART profile is hidden pre-bond`() { - // The regression this guard exists to prevent. The Colmi UART (6e40fff0/de5bf728) is - // suspected to be bond-gated on the R09/R11 (root AGENTS.md), so a first, unbonded connect - // can show a table without it. Re-routing to jring here would be self-sealing: the model - // re-resolves to generic JRING, requiresOsBond goes false, the bond that would reveal the - // Colmi profile never fires, and the jring family is persisted for every later reconnect. + fun `never strands a Colmi ring whose UART profile is missing from the table`() { + // The regression this guard exists to prevent. Root AGENTS.md records one hedged suspicion, + // for the R11, that the Colmi UART (6e40fff0/de5bf728) is gated behind the OS bond — so an + // unbonded first connect could show a table without it. Unproven and single-model, but the + // failure would be self-sealing: the model re-resolves to generic JRING, requiresOsBond goes + // false, the bond that would reveal the Colmi profile never fires, and the jring family is + // persisted for every later reconnect. assertFalse( DriverReroute.shouldRerouteToJring( discoveredServices = r09Services, diff --git a/app/src/test/java/com/pulseloop/service/EventPersistenceIdentityTest.kt b/app/src/test/java/com/pulseloop/service/EventPersistenceIdentityTest.kt index d35124b..a26f97f 100644 --- a/app/src/test/java/com/pulseloop/service/EventPersistenceIdentityTest.kt +++ b/app/src/test/java/com/pulseloop/service/EventPersistenceIdentityTest.kt @@ -22,6 +22,37 @@ class EventPersistenceIdentityTest { assertFalse(preservesSleepOnConnect(null)) } + @Test + fun `only the client's own connect event is a connection transition`() { + // RingBLEClient always stamps the resolved family on its connect event... + assertTrue(isConnectTransition(RingDeviceType.JRING)) + assertTrue(isConnectTransition(RingDeviceType.CRP)) + assertTrue(isConnectTransition(RingDeviceType.YCBT)) + // ...and RingEventBridge never does, for any decoder's Status. + assertFalse(isConnectTransition(null)) + } + + @Test + fun `a decoder's Status never reaches the rebuild, whatever family it belongs to`() { + // The pairing that used to wipe sleep: no deviceType (so it's a mid-session re-assertion + // from jring 0x0C / LuckRing dev-info) AND a family outside preservesSleepOnConnect (so the + // rebuild branch runs unscoped DELETEs). The transition gate is what breaks it. + assertFalse(preservesSleepOnConnect(null, RingDeviceType.JRING)) + assertFalse(preservesSleepOnConnect(null, RingDeviceType.LUCK_RING)) + assertFalse(preservesSleepOnConnect(null, RingDeviceType.CRP)) + assertFalse(isConnectTransition(null)) + } + + @Test + fun `a real connect still rebuilds for the packet-based families`() { + // The gate must not suppress the behaviour it is protecting: a genuine connect on a + // non-YCBT family still clears and re-pulls. + assertTrue(isConnectTransition(RingDeviceType.JRING)) + assertFalse(preservesSleepOnConnect(RingDeviceType.JRING)) + assertTrue(isConnectTransition(RingDeviceType.CRP)) + assertFalse(preservesSleepOnConnect(RingDeviceType.CRP)) + } + @Test fun `history identity is stable across repeated syncs`() { val timestamp = 1_721_234_567_000L