feat: add hardware-validated YCBT R10M support - #31
Conversation
|
We just had a massive update. Could you please resolve the conflicts? |
|
Absolutely, on it. |
Review notesA few things I'd want addressed or answered before merge, grouped by severity. High1. if (packets != expectedPackets || bytes != expectedBytes || bytes != buffer.size) {
return emptyList()
}On a metadata/byte-count mismatch this returns with no block ACK and no Medium2. CONNECTED no longer clears real sleep for any family — The CONNECTED handler now calls 3. DELETE FROM measurements WHERE rowid NOT IN (SELECT MAX(rowid) ... GROUP BY kindRaw, timestamp, sourceRaw)then 4. Both paths went from 5. RESPIRATORY_RATE / VO2MAX are decoded then unconditionally filtered out —
Low / polish6. 7. 8. 9. 10. 11. 12. In |
Protocol / wire-layer passWent through the framing, CRC16, opcode tables, history record offsets, and the support-function bitmap separately — the wire layer looks correct, so nothing blocking there. Two small things worth a look: Unverified/possibly-wrong mode constants — Inferred conversions with no hard ground truth — the blood-sugar tenths-of-mmol → mg/dL conversion ( Minor: the |
|
Addressed all review notes in
Wire-layer follow-ups:
Additional lifecycle hardening found during device testing:
Verification:
|
…r shadowing Address the confirmed issues from the PR foureight84#31 review: - YCBTHistoryTransfer: publish watchdog-driven events OUTSIDE the transfer monitor. Holding it while `onOutOfBandEvents` re-enters the sync engine (which calls back into `append`) inverted the engine->transfer lock order the GATT-callback path uses — a classic AB/BA deadlock that could wedge all BLE notification processing during history sync. - YCBTCoordinator: narrow name matching to R10M only. It sits ahead of TK5Coordinator/ColmiSmartHealthCoordinator in the registry, so matching the TK5/T50/SR0x/R0x prefixes by name shadowed uncataloged units of those families and mis-bound them to RingDeviceType.YCBT. The be940000 service stays a positive signal (R10M-exclusive; not advertised by TK5/SmartHealth). - PulseEventBus: isolate each emit so one throwing event can't kill the single, non-restarting dispatcher and silently stop every subscriber. - RingBLEClient: read `inFlightOp` under `opLock` in the op-timeout path so a completion landing at the timeout can't trigger a spurious reconnect. - YCBTDriver: mark the cross-thread `capabilities` field @volatile. - RingDecodedEvent: correct the stale MeasurementRejected doc (it now maps to PulseEvent.MeasurementRejected via RingEventBridge). Add regression tests locking in the R10M-only coordinator scope. Left as-is by design: onCharacteristicWrite's immediate recoverWedgedLink on a write failure (it can't tell a failed required-handshake write from a normal one, and advancing risks retiring a successor on the shared callback channel).
# Conflicts: # app/src/main/java/com/pulseloop/ring/RingEventBridge.kt # app/src/main/java/com/pulseloop/service/EventPersistenceSubscriber.kt # app/src/main/java/com/pulseloop/service/RingSyncCoordinator.kt # app/src/main/java/com/pulseloop/ui/screens/DebugScreen.kt
Colmi HRV *history* used to persist as an `HrvSample` — random id, sourceRaw 'live' — and now decodes to a `HistoryMeasurement`, keyed on `history:hrv:<timestamp>`. `adoptStableMeasurementIdentities` re-keyed the equivalent TEMPERATURE rows but not HRV, so the old rows never collided with the new ones: a re-sync wrote a second row at every timestamp already stored, and `range()` filters on kindRaw + timestamp (never sourceRaw), so both came back and the HRV series doubled. Adds the missing 'live' pass for HRV, mirroring TEMPERATURE, plus a v14 → v15 migration so a test APK already on v14 re-runs the (idempotent) adoption instead of keeping its un-keyed rows. Also fixes the R10M catalog entry: it no longer borrows Colmi R10 product art (it is a YCBT-protocol ring, not a Colmi one — it now falls back to the generic silhouette like TK5/TK18), and leads with the model number the ring actually advertises, "R10M (LittleMeatball)", since that is what every reseller of this white-label ODM shares while the brand is what a buyer recognises.
…lated Before measurements had stable ids, a ring's history replay was persisted with a fresh random id on every sync, so each re-sync appended another row at a slot already stored. On a real Colmi R10 that meant HRV, stress and temperature growing by one row per slot per sync, permanently — nothing prunes this table. A device pulled for this change held 1197 measurement rows covering only 414 distinct slots: 783 rows, 65% of the table, were replay copies. They are not merely wasted space. `dailyAggregates`/`hourlyAggregates` compute AVG(value) over raw rows with no sourceRaw filter, so a slot the ring replayed more often than its neighbours drags the daily average toward its value. `adoptStableMeasurementIdentities` already stops the growth by giving one row per slot the canonical `history:<key>:<timestamp>` id that later syncs upsert onto, but it deliberately leaves the accumulated copies behind. This removes them, restricted to rows that are provably redundant: a non-canonical row goes only when a canonical row exists for the same (kindRaw, timestamp) AND holds the same value. A differing value is a distinct reading and is always kept, so no information can be destroyed. Interruption safety — one statement, so SQLite's journal makes it all-or- nothing; a kill can never leave the table half-deleted. Room additionally runs migrations inside SQLiteOpenHelper's onUpgrade transaction, so the version bump and the delete commit together and an interrupted upgrade rolls back to v15 and re-runs next launch. The statement is idempotent, so that retry is safe. Verified against the real device database: 1197 -> 414 rows with every distinct (kind, timestamp, value) triple preserved, a second run a no-op, a rolled-back run leaving all 1197 intact and its retry completing cleanly, and PRAGMA integrity_check ok. Then confirmed on-device: every kind now holds exactly one row per slot.
Summary
Hardware validation
Validated on R10M FCF4, firmware 2.32:
The implementation intentionally does not advertise or query capabilities this firmware does not declare, including temperature, glucose, HRV, stress, fatigue, Find Device, and dedicated SpO2 history.
Verification
TZ=UTC ./gradlew --no-daemon testDebugUnitTest --rerun-tasksTZ=Europe/Skopje ./gradlew --no-daemon testDebugUnitTest./gradlew --no-daemon :app:assembleDebugDebug APK:
app/build/outputs/apk/debug/app-universal-debug.apk