fix(ring): re-route jring rings off the wrong driver, correct the CRP firmware opcode - #42
Merged
Merged
Conversation
… firmware opcode 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.
…pe the jring re-route 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.
…on any CONNECTED 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two independent reports from issue #29. Both are provable from source + the diagnostics log; neither is hardware-validated yet.
1. Colmi R09 can't connect (@itspuia)
connectTo'shonorSelectiontreats aJRINGclassification of the genericSMART_RINGadvertisement as a fallback guess and lets the pairing-carousel pick win. That's backwards for a jring-firmware ring sold under a Colmi badge: picking "Colmi / Yawell (SmartHealth app)" installs the YCBT driver, whosebe940001/be940003don't exist on the ring, sotopologyFailure()hard-failed the connect and the app retried forever.000056ff— the jring service — was in the GATT table the whole time, which is why the ring works fine in the official JRing app. From the attached diagnostics:Fix: the inverse of the two existing post-connect re-routes (JRING→Colmi, JRING/Colmi→CRP). When the jring service is present and every service the active driver declared is absent, route back to the jring driver instead of failing. That second guard is what keeps it from stealing a ring that genuinely speaks its selected protocol.
The decision is pure, so it lives in
DriverReroutewith 6 tests built on itspuia's exactonSearchCompleteservice list. The two existing re-routes are welded into the GATT callback and have no tests.2. R11 firmware version never read (@zaggash)
Diagnostics showed
"firmware": "?"; AGENTS.md recorded the query as 23-sent / 0-answered and assumed the ring was ignoring a valid vendor command.It wasn't.
q.b(7,1)isquerySavedGomoreKey(d1/b.java:613) — the wholeb1/rclass is the vendor's Gomore analytics module:7/0querySupportGomore7/1querySavedGomoreKey← we called this "firmware version"7/2queryGomoreEUID7/3sendGomoreKey7/13queryGomoreVersionThe constants had been built by pairing
b1/r's methods with opcodes positionally (a→0, b→1, c→13). jadx alphabetises method names, so that ordering is meaningless — it mislabelled all three "device info" queries. The same slip mislabelled3/1(shutDown) asCMD_RESTART; restart is3/14. Corrected the labels; didn't wire up new shutdown behavior.The real query is
3/3(b1/l.k→d1/b.queryFirmwareVersion), answering with a bare UTF-8 string viag1/a.i1:Expected value on zaggash's ring:
MOY-R1K3-2.1.6, 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 — becauseRingDecodedEvent.FirmwareVersioncarries anIntand can't hold this string.queryDeviceInfo()/queryDeviceSN()are removed rather than repointed: they framed Gomore opcodes, had no callers, and I found no group-3 equivalents.Docs
AGENTS.md gains the rule this mistake argues for: resolve every CRP opcode through its
d1/b.javacaller, never by position in the builder class. That single slip produced four wrong constants.Testing
854 unit tests pass. 11 new (6
DriverRerouteTest, 5CRPDecoderTest);CRPSyncEngineTest's startup-sequence assertion updated7 to 1→3 to 3.Not hardware-validated. The re-route is reasoned from itspuia's service list, the firmware query from the decompile. Per AGENTS.md, both want a real device before being trusted —
versionCodebumped to 32 so testers can be pointed at a build.Not included
@liamscic05's walking-activity report (0.01 mi over 1:13, no HR) is still open — no log was attached, and the two candidate causes (
WorkoutForegroundServicelackingforegroundServiceType="location", andrestartWorkoutHeartRateIfActive()having no reconnect call site) need a capture spanning the workout to confirm.