feat: parse timestamp markers and fix WAL timing in SD card sync (app)#5572
feat: parse timestamp markers and fix WAL timing in SD card sync (app)#5572
Conversation
Greptile SummaryThis PR adds inline Key changes:
Issues found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant FW as Firmware (SD Card)
participant BLE as BLE / WiFi Transport
participant Parser as Packet Parser
participant Markers as timestampMarkers[]
participant Flusher as Segment Flusher
participant WAL as LocalWalSync
FW->>BLE: Raw bytes (440-byte blocks)<br/>[size][frame_data] … [0xFF][epoch LE 4B] …
BLE->>Parser: onStorageBytesReceived(value)
Parser->>Parser: Walk packets<br/>packageSize == 0 → skip<br/>packageSize == 0xFF → parse epoch<br/>else → decode audio frame
Parser->>Markers: timestampMarkers.add(MapEntry(frameIndex, epoch))
Parser->>Parser: bytesData.add(frame)
Note over Parser,Flusher: BLE: flush at marker boundary during streaming
Parser->>Flusher: nextMarkerIdx = first marker > bytesLeft
Flusher->>Flusher: while frames >= chunkSize AND within marker boundary → flush chunk
Flusher->>Flusher: if reached marker → flush remainder, apply marker epoch
Note over Parser,Flusher: WiFi: buffer all, flush segments post-transfer
Parser-->>Flusher: (after transfer complete)
Flusher->>Flusher: Build segments from timestampMarkers<br/>[segStart, segEnd, epoch]
loop each segment
Flusher->>Flusher: chunk-flush full chunkSize blocks
Flusher->>Flusher: flush partial remainder
end
Flusher->>WAL: _registerSingleChunk(wal, file, timerStart, chunkFrames)
WAL->>WAL: Wal(timerStart, seconds=chunkFrames÷fps, totalFrames=chunkFrames)
Last reviewed commit: a5c0ca5 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Find the next marker boundary (if any) after bytesLeft | ||
| int nextMarkerIdx = bytesData.length; | ||
| for (var m in timestampMarkers) { | ||
| if (m.key > bytesLeft) { | ||
| nextMarkerIdx = m.key; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Chunk up to the next marker boundary or chunkSize, whichever comes first | ||
| while (bytesData.length - bytesLeft >= chunkSize && bytesLeft + chunkSize <= nextMarkerIdx) { | ||
| var chunk = bytesData.sublist(bytesLeft, bytesLeft + chunkSize); | ||
| var chunkFrames = chunk.length; | ||
| var chunkSecs = chunkFrames ~/ wal.codec.getFramesPerSecond(); | ||
| bytesLeft += chunkSize; | ||
| timerStart += sdcardChunkSizeSecs; | ||
| try { | ||
| var file = await _flushToDisk(wal, chunk, timerStart); | ||
| await callback(file, offset, timerStart); | ||
| await callback(file, offset, timerStart, chunkFrames); | ||
| } catch (e) { | ||
| Logger.debug('Error in callback during chunking: $e'); | ||
| hasError = true; | ||
| if (!completer.isCompleted) { | ||
| completer.completeError(e); | ||
| } | ||
| } | ||
| timerStart += chunkSecs; | ||
| } | ||
|
|
||
| // If we've reached a marker boundary, flush remaining frames before it and advance timerStart | ||
| if (nextMarkerIdx <= bytesData.length && bytesLeft < nextMarkerIdx) { | ||
| // Only flush if there are enough frames to be meaningful (> 0) | ||
| var chunk = bytesData.sublist(bytesLeft, nextMarkerIdx); | ||
| var chunkFrames = chunk.length; | ||
| if (chunkFrames > 0) { | ||
| var chunkSecs = chunkFrames ~/ wal.codec.getFramesPerSecond(); | ||
| bytesLeft = nextMarkerIdx; | ||
| try { | ||
| var file = await _flushToDisk(wal, chunk, timerStart); | ||
| await callback(file, offset, timerStart, chunkFrames); | ||
| } catch (e) { | ||
| Logger.debug('Error flushing segment at marker: $e'); | ||
| hasError = true; | ||
| if (!completer.isCompleted) completer.completeError(e); | ||
| } | ||
| timerStart += chunkSecs; | ||
| } else { | ||
| bytesLeft = nextMarkerIdx; | ||
| } | ||
| // Apply the marker's epoch | ||
| for (var m in timestampMarkers) { | ||
| if (m.key == nextMarkerIdx) { | ||
| timerStart = m.value; | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Regression: tiny WAL segments created on every BLE notification when no markers present
When no timestamp markers exist (e.g. older firmware), nextMarkerIdx is set to bytesData.length (line 386). The condition at line 414 then becomes bytesData.length <= bytesData.length && bytesLeft < bytesData.length, which is true whenever any unprocessed frames exist. This triggers a "marker boundary flush" on every single BLE notification, emitting a tiny WAL segment (often < 1 second) for the handful of frames received in that notification.
The original code only flushed full chunkSize blocks during streaming and deferred the remainder to the final flush at end-of-download. The new logic accidentally removes that deferral for the no-marker case.
A simple guard would prevent this:
// Only flush at a real marker boundary, not when nextMarkerIdx is just bytesData.length with no marker
bool hasRealMarker = timestampMarkers.any((m) => m.key == nextMarkerIdx);
if (hasRealMarker && nextMarkerIdx <= bytesData.length && bytesLeft < nextMarkerIdx) {| print( | ||
| 'SDCardWalSync: totalBytes=$totalBytes storageOffset=$storageOffset frameLengthInBytes=${codec.getFramesLengthInBytes()} fps=${codec.getFramesPerSecond()} calculatedSeconds=$seconds timerStart=$timerStart now=${DateTime.now().millisecondsSinceEpoch ~/ 1000}'); |
There was a problem hiding this comment.
print() instead of Logger.debug() left in production code
This debug print() call will leak verbose internal state to stdout in release builds. All other logging in this file consistently uses Logger.debug().
| print( | |
| 'SDCardWalSync: totalBytes=$totalBytes storageOffset=$storageOffset frameLengthInBytes=${codec.getFramesLengthInBytes()} fps=${codec.getFramesPerSecond()} calculatedSeconds=$seconds timerStart=$timerStart now=${DateTime.now().millisecondsSinceEpoch ~/ 1000}'); | |
| Logger.debug( | |
| 'SDCardWalSync: totalBytes=$totalBytes storageOffset=$storageOffset frameLengthInBytes=${codec.getFramesLengthInBytes()} fps=${codec.getFramesPerSecond()} calculatedSeconds=$seconds timerStart=$timerStart now=${DateTime.now().millisecondsSinceEpoch ~/ 1000}'); |
| bytesLeft = nextMarkerIdx; | ||
| try { | ||
| var file = await _flushToDisk(wal, chunk, timerStart); | ||
| await callback(file, offset, timerStart, chunkFrames); | ||
| } catch (e) { | ||
| Logger.debug('Error flushing segment at marker: $e'); | ||
| hasError = true; | ||
| if (!completer.isCompleted) completer.completeError(e); | ||
| } | ||
| timerStart += chunkSecs; | ||
| } else { | ||
| bytesLeft = nextMarkerIdx; |
There was a problem hiding this comment.
Dead code: timerStart += chunkSecs is always overwritten by the marker epoch
After the flush at line 422-423, timerStart is incremented by chunkSecs (line 429). However, the loop immediately below (lines 434–439) overwrites timerStart with m.value (the actual device epoch) whenever a real marker is found. Since nextMarkerIdx is always derived from a marker entry, the increment is a no-op and can mislead future maintainers into thinking this is meaningful timing logic.
If the intent is that timerStart should be set to the device-provided epoch unconditionally (which makes sense — the device epoch is authoritative), the += chunkSecs line should be removed:
| bytesLeft = nextMarkerIdx; | |
| try { | |
| var file = await _flushToDisk(wal, chunk, timerStart); | |
| await callback(file, offset, timerStart, chunkFrames); | |
| } catch (e) { | |
| Logger.debug('Error flushing segment at marker: $e'); | |
| hasError = true; | |
| if (!completer.isCompleted) completer.completeError(e); | |
| } | |
| timerStart += chunkSecs; | |
| } else { | |
| bytesLeft = nextMarkerIdx; | |
| bytesLeft = nextMarkerIdx; | |
| try { | |
| var file = await _flushToDisk(wal, chunk, timerStart); | |
| await callback(file, offset, timerStart, chunkFrames); | |
| } catch (e) { | |
| Logger.debug('Error flushing segment at marker: $e'); | |
| hasError = true; | |
| if (!completer.isCompleted) completer.completeError(e); | |
| } | |
| } else { |
| // Timestamp marker: 0xFF followed by 4-byte little-endian epoch | ||
| if (packageSize == 0xFF && packageOffset + 5 <= value.length) { | ||
| var epoch = value[packageOffset + 1] | | ||
| (value[packageOffset + 2] << 8) | | ||
| (value[packageOffset + 3] << 16) | | ||
| (value[packageOffset + 4] << 24); | ||
| packageOffset += 5; | ||
| if (epoch > 0) { | ||
| timestampMarkers.add(MapEntry(bytesData.length, epoch)); | ||
| Logger.debug('Timestamp marker: epoch=$epoch at frame ${bytesData.length}'); | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Partial 0xFF timestamp marker at BLE packet boundary is silently dropped
When packageSize == 0xFF but packageOffset + 5 > value.length (the marker straddles a BLE notification boundary), the marker check's && short-circuits and the code falls through. With packageSize = 255, the subsequent guard if (packageOffset + 1 + packageSize >= value.length) is almost certainly true, causing a break. The 5-byte marker is lost entirely, and its epoch is never recorded.
If the firmware guarantees timestamp markers are always contained within a single 440-byte BLE packet, this is safe. If not, the partial marker bytes should be buffered and completed on the next notification — otherwise a recording boundary will have an incorrect timerStart for its segment.
Parse inline timestamp markers in both BLE and WiFi sync paths to split audio into properly-timed WAL segments. Also fix chunk size calculation to use actual codec FPS instead of hardcoded values, and derive WAL duration from actual frame count rather than a fixed 60-second assumption.
Instead of flushing immediately when a 0xFF timestamp marker is encountered during BLE streaming, record markers and their frame indices. Split segments at marker boundaries only during the normal chunking phase, preventing tiny 5-second WAL fragments.
mdmohsin7
force-pushed
the
feat/offline-audio-timestamp-app
branch
from
e20ce23 to
a5c0ca5
Compare
|
@greptile-apps re-review |
| if (packageSize == 0xFF && packageOffset + 5 <= bufferLength) { | ||
| var epoch = tcpBuffer[packageOffset + 1] | | ||
| (tcpBuffer[packageOffset + 2] << 8) | | ||
| (tcpBuffer[packageOffset + 3] << 16) | | ||
| (tcpBuffer[packageOffset + 4] << 24); | ||
| packageOffset += 5; | ||
| bytesProcessed = packageOffset; | ||
| if (epoch > 0) { | ||
| // Record frame index and epoch for segment splitting during flush | ||
| timestampMarkers.add(MapEntry(bytesData.length, epoch)); | ||
| Logger.debug('SDCardWalSync WiFi: Timestamp marker: epoch=$epoch at frame ${bytesData.length}'); | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
WiFi path: 0xFF marker check ignores 440-byte block boundary alignment
The guard packageOffset + 5 <= bufferLength uses the total accumulated TCP buffer size (potentially thousands of bytes), not the remaining bytes in the current 440-byte logical block. This means a 0xFF timestamp marker that starts in the last few bytes of a block (where bytesRemainingInBlock < 12) will bypass the block-boundary padding logic at line 1148 because the timestamp check fires first.
When the 5-byte marker straddles a block boundary, globalBytePosition advances by 5 bytes that cross into the next block. The subsequent frame's posInBlock calculation becomes (globalBytePosition + 0) % 440 ≠ 0, causing all following frames in that block to be misaligned. This corrupts frame parsing for the rest of the segment.
Compare with the BLE path which uses packageOffset + 5 <= value.length (where value.length == 440), correctly preventing the marker from being consumed if it would exceed the packet boundary.
A fix would be to also check that the marker fits within the current block:
if (packageSize == 0xFF &&
packageOffset + 5 <= bufferLength &&
(posInBlock == 0 || posInBlock + 5 <= 440)) {| @@ -498,7 +588,7 @@ class SDCardWalSyncImpl implements SDCardWalSync { | |||
| device: wal.device, | |||
| deviceModel: wal.deviceModel, | |||
| seconds: chunkSeconds, | |||
| totalFrames: chunkSeconds * wal.codec.getFramesPerSecond(), | |||
| totalFrames: chunkFrames, | |||
| syncedFrameOffset: 0, | |||
There was a problem hiding this comment.
Zero-second WAL created for sub-FPS remainder segments
chunkSeconds = chunkFrames ~/ wal.codec.getFramesPerSecond() uses integer division, so any chunk with fewer frames than the codec FPS (e.g. 50 frames at 100 fps, or a short segment just before a timestamp marker boundary) yields chunkSeconds = 0.
A Wal with seconds: 0 can cause two problems:
- Data loss: downstream processors that guard on
wal.seconds > 0(or use duration to slice audio) may silently discard the segment. - Incorrect conversation timestamps: if
chunkSecondsis ever used to advancetimerStartbefore the marker epoch is applied, a sub-1-second remainder produces no time advance, potentially overlapping the next segment's start time with the previous one.
This is most likely to occur for the "pre-marker" flush in the BLE path (lines 419–430) and for the WiFi partial-segment remainder. Consider rounding up instead of truncating, or enforcing a minimum of 1 second:
int chunkSeconds = (chunkFrames / wal.codec.getFramesPerSecond()).ceil().clamp(1, chunkFrames);| int segStart = bytesLeft; | ||
| int segEpoch = timerStart; | ||
| for (var marker in timestampMarkers) { | ||
| int frameIdx = marker.key; | ||
| int epoch = marker.value; | ||
| if (frameIdx > segStart) { | ||
| segments.add([segStart, frameIdx, segEpoch]); | ||
| } | ||
| segStart = frameIdx; | ||
| segEpoch = epoch; | ||
| } | ||
| // Final segment from last marker (or start) to end | ||
| if (segStart < bytesData.length) { | ||
| segments.add([segStart, bytesData.length, segEpoch]); |
There was a problem hiding this comment.
WiFi path silently drops frames when a marker's frameIdx equals segStart
The condition if (frameIdx > segStart) skips adding a segment when two markers share the same frame index, or (in theory) when a marker appears at exactly bytesLeft == 0. In that case segStart is advanced to frameIdx and segEpoch is updated, but no segment is emitted for the frames between the old segStart and this frameIdx. Those frames are simply lost.
While duplicate markers are an edge case, a marker at frameIdx == segStart would silently drop frames. Using >= instead of > for the guard, or asserting uniqueness, would make the invariant explicit and prevent silent data loss:
if (frameIdx >= segStart) {
segments.add([segStart, frameIdx, segEpoch]);
}
segStart = frameIdx;
segEpoch = epoch;Note: the same pattern exists in the BLE post-transfer flush (lines 472–478) and should be reviewed there as well.
Use _supportsTimestampMarkers() to branch BLE and WiFi sync paths: - Old firmware: uses original legacy parsing (chunkSize = secs * 100, no marker detection, simple timerStart increment) - New firmware (>= 3.0.16): uses marker-aware parsing with segment splitting and device-provided epochs Also fixes print() -> Logger.debug() for production logging.
|
@greptile-apps re-review |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
With timestamp markers providing session boundaries, 1-min chunks are unnecessary. Use 2-min max chunks (or session marker, whichever first) to reduce file count and overhead. Legacy path stays at 1-min chunks.
This reverts commit 088d64f.
1 participant
Summary
0xFFtimestamp markers from the offline audio data stream so each recording segment has an exact UTC start timeSplit from #4851 — app changes only. See #5571 for the firmware counterpart.
App changes
>= 3.0.16using_supportsTimestampMarkers(). Old firmware uses the original legacy parsing path unchanged. New firmware uses marker-aware parsing in both BLE and WiFi sync paths, splitting audio into correctly-timed WAL segments. Also fixes chunk size fromsdcardChunkSizeSecs * 100tosdcardChunkSizeSecs * codec.getFramesPerSecond()(new firmware path only).Firmware version gating
_readStorageBytesToFileLegacy()path — identical to currentmainbranch behavior. No marker detection, no new chunking logic._readStorageBytesToFileWithMarkers()path — parses0xFFmarkers, splits segments by timestamp boundaries, uses device-provided recording start time.Test plan
🤖 Generated with Claude Code