feat(app): support ring-buffer offline sync for fw 3.0.20+#7218
feat(app): support ring-buffer offline sync for fw 3.0.20+#7218mdmohsin7 wants to merge 12 commits into
Conversation
…viceConnection Adds data classes and abstract method surface for the ring-buffer storage protocol (firmware 3.0.20+, omi PR #7216). Same BLE service UUID as the existing multi-file protocol; subclasses fill in the new opcodes. The 444-byte record format ([ts:4 BE][audio:440]) is identical to the multi-file protocol on main, so the audio parser will be reused. Only the control-plane (read-by-sequence, advance-by-sequence) is new.
Implements the wire protocol for fw 3.0.20+ ring storage on the existing 30295780 service: - performGetRingStatus: 16-byte read of status char (used/unread/free/rtcValid) - performGetRingInfo: CMD_RING_INFO (0x10) on control char, awaits NOTIFY_INFO (0x02) and decodes the 31-byte payload (u64/u64/u32/u64/u16 BE) - performReadRingFromSeq: CMD_RING_READ (0x11) with u64 BE start_seq and optional u32 BE packet_count; data flows through the existing storage bytes listener (NOTIFY_DATA chunks) handled by the sync layer - performAdvanceRing: CMD_RING_ADVANCE (0x12) with u64 BE seq, awaits NOTIFY_ACK (0x01) so callers can verify the commit succeeded before treating the chunk as durably consumed - performClearRing: CMD_RING_CLEAR (0x13), awaits NOTIFY_ACK
Mirrors StorageSyncImpl but talks the ring protocol from omi PR #7216: - refreshWalsFromDevice: 16B status read; emits ONE virtual Wal covering the entire unread region (the ring is a single logical stream, not a list of files). Skipped when <10s of audio, mirroring the file-based threshold. - _syncRing: snapshots ring state via getRingInfo, sends CMD_RING_READ, demuxes notifications by opcode (ACK/INFO/DATA/DONE/READ_BEGIN), and reassembles the unaligned NOTIFY_DATA byte stream into 444B records. Per record, strips the 4B BE timestamp and feeds the 440B audio payload through the same packed [size:1][frame:size] parser used by the multi-file path. - Chunks flush to disk + register with LocalWalSync as we go (60s chunks per sdcardChunkSizeSecs) so a mid-stream BLE drop still lands the early audio in the cloud. - CMD_RING_ADVANCE is sent ONLY after NOTIFY_DONE arrives with status=0 AND every chunk has been registered with LocalWalSync. Any failure (cancel, BLE drop, flush error, DONE with non-zero status) leaves the ring untouched — the next sync resumes from the same read_seq. This is the data-safety invariant. - rtc_valid handling: if the device RTC isn't synced yet, fall back to now-duration like the legacy code does. Otherwise the per-record timestamp from the first record anchors timerStart. - dropped_packets > 0 surfaces as a DebugLogManager warning; sync proceeds with whatever the firmware still has.
Routes the offline storage sync to the right protocol based on firmware version, with no impact on devices outside the ring-firmware range: - WalSyncs.isRingBufferFirmware(version) static helper for the >=3.0.20 cutover, parallel to DeviceProvider._isFirmwareVersionSupported (>=3.0.17) for the multi-file path. - New WalSyncs.setDevice() so the sync orchestrator knows the firmware version at syncAll() time without re-querying BLE. - Phase 0 in syncAll() now branches: fw>=3.0.20 calls _ringSync, fw 3.0.17-.19 keeps the existing _storageSync path. fw<3.0.17 falls through to Phase 1a legacy SD-card sync as before. - _ringSync threaded through deleteWal, deleteAllSynced/Pending, getMissing, getAll, start, stop, cancelSync, isStorageSyncing, storageSpeedKBps so the rest of the codebase observes ring sync state through the same surfaces it already uses for storage sync. DeviceProvider: - syncs.setDevice(device) called alongside the existing per-sync fanout so WalSyncs has the firmware version locally. - syncs.ring.setDevice(device) so the ring path can read its device. - _checkAndStartAutoSync branches on isRingBufferFirmware: ring-capable devices use connection.getRingStatus() (16B status read) instead of the multi-file file-list endpoint, which the ring firmware no longer serves. Without this, the auto-sync banner would never appear on fw>=3.0.20 devices.
Hoists status / NOTIFY_INFO / NOTIFY_DONE / NOTIFY_READ_BEGIN parsing, CMD_RING_READ / CMD_RING_ADVANCE encoding, the 4-byte BE timestamp extractor, and the [size:1][frame:size] audio-payload parser into a new RingProtocol module. Adds RingRecordReassembler for the unaligned NOTIFY_DATA byte stream. OmiDeviceConnection and RingStorageSyncImpl now call into these helpers instead of inlining ByteData ops. No behavior change — the helpers are byte-for-byte equivalent to the previous inline parsers. The split makes the wire protocol unit-testable without BLE mocking; tests are added in the next commit.
30 tests covering: - 16-byte status read parse (4x u32 LE) including rtc_valid surfacing - NOTIFY_INFO (0x02), NOTIFY_DONE (0x04), NOTIFY_READ_BEGIN (0x05) parsers including opcode and length validation - CMD_RING_READ encoding with and without packet_count, u64 BE on the wire - CMD_RING_ADVANCE encoding, u64 BE - 4-byte BE timestamp extraction - packed [size:1][frame:size] audio payload parsing including zero-padding, multi-frame, truncated trailing frame - RingRecordReassembler boundary, mid-record split, multi-record drain, byte-by-byte streaming, and a stream of two records with a mid-record cut - end-to-end: a 444B record split across two NOTIFY_DATA chunks reassembles cleanly and decodes the audio payload
Greptile SummaryThis PR adds app-side support for the ring-buffer NAND storage protocol introduced in firmware 3.0.20, routing devices on older firmware versions to the existing LittleFS or SD-card paths based on the firmware version comparison in
Confidence Score: 3/5Two correctness bugs in the new sync path should be fixed before merging: one causes failed syncs to be silently marked complete (breaking retries), the other can assign identical timestamps to consecutive audio chunks. The WAL-status inversion in app/lib/services/wals/ring_storage_sync.dart warrants the most scrutiny — it contains both correctness issues and drives the entire ring sync flow. Important Files Changed
Sequence DiagramsequenceDiagram
participant DP as DeviceProvider
participant WS as WalSyncs
participant RS as RingStorageSyncImpl
participant OC as OmiDeviceConnection
participant FW as Firmware (3.0.20+)
DP->>WS: setDevice(device)
DP->>WS: syncAll()
WS->>WS: isRingBufferFirmware(fwVersion)?
alt "fw >= 3.0.20 (ring path)"
WS->>RS: refreshWalsFromDevice()
RS->>OC: getRingStatus()
OC->>FW: Read status char (30295782)
FW-->>OC: 16-byte LE [used, unread, free, rtcValid]
OC-->>RS: RingStatus
WS->>RS: syncAll()
RS->>OC: getRingInfo()
OC->>FW: Write CMD_INFO (0x10)
FW-->>OC: NOTIFY_INFO (0x02)
RS->>OC: readRingFromSeq(read_seq)
OC->>FW: Write CMD_RING_READ (0x11)
FW-->>OC: NOTIFY_READ_BEGIN (0x05)
loop NOTIFY_DATA chunks
FW-->>OC: NOTIFY_DATA (0x03)[raw_bytes...]
OC-->>RS: onStorageBytesReceived
RS->>RS: reassembler.append() then drainRecords()
RS->>RS: flushChunks() then _flushToDisk + _registerWithLocalSync
end
FW-->>OC: NOTIFY_DONE (0x04)[status, next_seq]
alt "status==0 AND no flush error"
RS->>OC: advanceRing(next_seq)
OC->>FW: Write CMD_RING_ADVANCE (0x12)
FW-->>OC: NOTIFY_ACK (0x01)[status]
else failure
RS->>RS: skip advance, ring unchanged
end
else fw 3.0.17-3.0.19
WS->>WS: StorageSync.syncAll()
end
Reviews (1): Last reviewed commit: "test(app): unit tests for ring-buffer wi..." | Re-trigger Greptile |
morpheus review — omi#7218 (ring-buffer sync, fw 3.0.20+)Scope: 8 files, +1486/-17, 6 atomic commits, 30 unit tests. App-side ring-buffer protocol for the new NAND storage firmware. Clean commit structure, no co-author trailers, no forward-references. P2 —
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| final complete = await _syncRing(wal, progress: progress); | ||
| wal.status = WalStatus.synced; | ||
| if (!complete) { | ||
| Logger.debug('RingStorageSync: Ring transfer incomplete; ring untouched, will resume next sync'); | ||
| listener.onWalUpdated(); | ||
| break; | ||
| } | ||
| listener.onWalUpdated(); |
There was a problem hiding this comment.
synced unconditionally before the completion check, so an incomplete transfer still removes the WAL from the retry queue. The next call to getMissingWals() (which filters on WalStatus.miss) will not see this WAL again, silently defeating the "resume from same read_seq" guarantee described in the PR.
| final complete = await _syncRing(wal, progress: progress); | |
| wal.status = WalStatus.synced; | |
| if (!complete) { | |
| Logger.debug('RingStorageSync: Ring transfer incomplete; ring untouched, will resume next sync'); | |
| listener.onWalUpdated(); | |
| break; | |
| } | |
| listener.onWalUpdated(); | |
| final complete = await _syncRing(wal, progress: progress); | |
| if (!complete) { | |
| Logger.debug('RingStorageSync: Ring transfer incomplete; ring untouched, will resume next sync'); | |
| listener.onWalUpdated(); | |
| break; | |
| } | |
| wal.status = WalStatus.synced; | |
| listener.onWalUpdated(); |
There was a problem hiding this comment.
Fixed in a792720 — wal.status only flips to synced after _syncRing returns true. On incomplete transfers the wal stays in WalStatus.miss so getMissingWals returns it on the next pass, pairing with the no-advance-on-failure guard so the ring also stays untouched.
| // Flush full chunks as we go (data safety: even if BLE drops mid-stream, | ||
| // already-flushed chunks land in LocalWalSync and reach the cloud). | ||
| if (bytesData.length >= chunkFrames) { | ||
| unawaited(() async { | ||
| try { | ||
| await flushChunks(finalFlush: false); | ||
| } catch (_) { | ||
| if (!completer.isCompleted) completer.complete(false); | ||
| } | ||
| }()); | ||
| } |
There was a problem hiding this comment.
chunkTimerStart
Each call to unawaited(() async { await flushChunks(...) }()) spawns an independent async closure that captures and mutates the same chunkTimerStart variable. Because flushChunks suspends at await _flushToDisk and await _registerWithLocalSync, a second BLE notification can arrive mid-flush, satisfy bytesData.length >= chunkFrames, and launch a second concurrent flush before the first one updates chunkTimerStart. Both flushes then pass the same chunkTimerStart value to _registerWithLocalSync, producing overlapping timestamps for consecutive audio chunks in LocalWalSync.
There was a problem hiding this comment.
Fixed in 7eb800c — replaced the unawaited closure with a single in-flight Future? inFlightFlush. New flushes only launch when the slot is free; the in-flight task's while loop absorbs additional NOTIFY_DATA arriving during its awaits. After NOTIFY_DONE / stream close, the post-loop code awaits the in-flight flush before draining the tail with finalFlush:true so the two never race on bytesData or chunkTimerStart.
| if (offset + 1 + size >= audio.length) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
>= audio.length rejects a frame whose data ends exactly at the last byte of the buffer (offset + 1 + size == audio.length), even though the frame is fully present. For a 440-byte audio payload where a frame occupies the last byte exactly, the final frame is silently dropped. Should be > audio.length.
| if (offset + 1 + size >= audio.length) { | |
| break; | |
| } | |
| if (offset + 1 + size > audio.length) { | |
| break; | |
| } |
There was a problem hiding this comment.
Fixed in 1b98973 — boundary check now uses '>'. A frame whose data ends exactly at audio.length is parsed correctly. Added two tests: a 3-byte payload with one boundary-fitting frame, and a 440B payload tightly packed with 40 frames of 10 bytes (the 40th frame would have been silently dropped under '>='). Legacy parsers in storage_sync.dart and sdcard_wal_sync.dart kept on '>=' since they work for current codecs (opus 80B, opusFS320 160B never hit the boundary) and changing them risks regression in flows out of scope here.
WalSyncs.deleteWal and WalSyncs.deleteAllPendingWals fan out the call to every sub-sync regardless of which one owns the wal. The ring impl was unconditionally clearing the entire ring buffer on any cascading delete, so deleting a phone or sdcard wal from the UI would wipe all unsynced ring data the user didn't intend to delete. Membership-guard both methods: deleteWal returns early if the wal id isn't ours; deleteAllPendingWals returns early if we have no pending wals.
morpheus re-review — fix commit 635bb11P2 resolved. Both Approved. 7 commits, +1500/-17, 30/30 tests, analyzer clean. |
syncAll set wal.status = WalStatus.synced unconditionally before checking the result of _syncRing. An incomplete transfer (BLE drop, non-zero NOTIFY_DONE status, cancel) was therefore removed from the in-memory retry queue (getMissingWals filters on WalStatus.miss), silently defeating the resume-from-read_seq guarantee for any UI flow that relies on the cached _wals list between refreshes. In practice the next refreshWalsFromDevice() call would have rebuilt the list anyway, but the wal status flapping to 'synced' is observable by listeners and contradicts the documented data-safety invariant. Pair with the existing no-advance-on-failure guard in _syncRing — the ring stays untouched on the device, and the wal stays in WalStatus.miss on the phone.
The original NOTIFY_DATA handler launched a fresh unawaited flushChunks closure every time bytesData crossed chunkFrames. Each closure captures and mutates the shared chunkTimerStart variable, but chunkTimerStart is only updated after the awaits inside flushChunks return. Two concurrent flushes therefore both observed the same chunkTimerStart value and passed it to _registerWithLocalSync, producing overlapping timestamps for consecutive audio chunks. Hold a single Future<void>? inFlightFlush reference. Only launch a new flush when the slot is free; flushChunks's existing while-loop naturally absorbs additional NOTIFY_DATA arriving during a flush on its next iteration. After NOTIFY_DONE / stream close, await the in-flight flush before draining the tail with the final flushChunks(finalFlush: true) so the two never race on bytesData or chunkTimerStart.
The previous boundary check 'offset + 1 + size >= audio.length' rejects a frame whose data ends exactly at the last byte of the buffer, even though the frame is fully present. For tightly-packed payloads where (frameSize + 1) divides 440 evenly the final frame would be silently dropped. In practice no current codec hits the boundary — opus packs 5 80-byte frames + padding (405B used) and opusFS320 packs 2 160-byte frames + padding (322B used) — but a future codec with a 10-byte frame size would lose every 40th frame. Switch to '>'. The legacy parsers in storage_sync.dart and sdcard_wal_sync.dart still use '>=' but are not changed: they're working for current frame sizes and changing them risks regression in flows we're not touching here. Adds two unit tests: a 3-byte payload with one boundary-fitting frame, and a 440-byte payload tightly packed with 40 frames of 10 bytes (verifies the 40th frame is preserved).
morpheus re-review — 3 fix commits (a792720, 7eb800c, 1b98973)All three Greptile findings addressed correctly:
32/32 tests, analyzer clean, 10 atomic commits. Approved. Self-note: I explicitly reviewed the flush concurrency and called it safe in my first pass. I traced data ownership correctly (removeRange is sync) but missed the timestamp race (chunkTimerStart update happens after the yield). Lesson: trace all shared mutable state through yield points, not just the primary data structure. |
1 participant
Summary
App-side support for the ring-buffer NAND storage protocol introduced in firmware PR #7216 (TuEmb). Devices on fw >= 3.0.20 will use this path; fw 3.0.17–.19 keep the existing multi-file LittleFS path; fw < 3.0.17 stay on the legacy SD-card path.
The 444-byte record format (
[ts:4 BE][audio:440]) is identical to the multi-file protocol on main — only the control plane changes (read by u64 sequence, advance by sequence). The audio parser is reused unchanged.Commits (atomic)
RingStatus/RingInfodata classes + abstract method surface onDeviceConnectionOmiDeviceConnection(CMD_RING_INFO/READ/ADVANCE/CLEAR + NOTIFY_INFO/ACK/READ_BEGIN/DATA/DONE handling)RingStorageSyncImplmirroringStorageSyncImplbut talking ring (NOTIFY_DATA reassembly across BLE chunks, advance only after LocalWalSync confirms chunk landed)WalSyncs.syncAll()by firmware version; route_checkAndStartAutoSyncthroughgetRingStatus()for ring-capable devicesRingProtocolso they're testable without BLE mockingData-safety invariant
CMD_RING_ADVANCEis sent only afterNOTIFY_DONEarrives with status=0 and every chunk has been registered withLocalWalSync. Any failure (cancel, BLE drop, flush error, non-zero DONE status) leaves the ring untouched — the next sync resumes from the sameread_seq. This is the data-loss win versus the file-based protocol's delete-after-read semantics.RTC handling
If
status.rtc_valid == 0(device RTC not yet synced viaperformSyncTime), per-record timestamps are treated as unreliable and timerStart falls back tonow - estimated_duration, matching the legacy code path. Ifrtc_valid == 1, the first record's BE u32 timestamp anchors timerStart.Out of scope
LocalWalSyncis reused as-is; no API changesTest plan
cd app && flutter test test/unit/ring_protocol_test.dart— 30/30 passflutter analyze— 0 new issues introduced (pre-existing warnings on main untouched)read_seqCloses nothing yet (firmware PR #7216 is the dependency).