feat - remove wifi sync and update new SD sync method for Omi CV1#5905
feat - remove wifi sync and update new SD sync method for Omi CV1#5905TuEmb wants to merge 2 commits intoBasedHardware:mainfrom
Conversation
Greptile SummaryThis PR removes WiFi-based SD card sync entirely and replaces it with a new multi-file BLE protocol for Omi CV1, adds real-time upload progress tracking through the HTTP multipart layer, and cleans up the sync UI by removing WiFi-specific dialogs and error handling. Key changes:
Issues found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant App as Flutter App
participant DP as DeviceProvider
participant SDSync as SDCardWalSync
participant Conn as OmiDeviceConnection
participant FW as Omi CV1 Firmware
App->>DP: device connected
DP->>DP: check autoSyncEnabled pref
alt autoSync disabled
DP->>Conn: stopStorageSync()
Conn->>FW: STOP_COMMAND [0x03]
end
App->>SDSync: getMissingWals()
SDSync->>Conn: stopStorageSync()
Conn->>FW: STOP_COMMAND [0x03]
note over SDSync,FW: 500ms delay
SDSync->>Conn: listFiles()
Conn->>FW: CMD_LIST_FILES [0x10]
FW-->>Conn: [count][ts1:4BE][sz1:4BE]...
Conn-->>SDSync: List<StorageFile>
SDSync->>SDSync: _buildWalsFromFileList()
App->>SDSync: syncAll()
loop for each WAL (oldest→newest)
SDSync->>Conn: _readStorageBytesToFile(wal)
Conn->>FW: CMD_READ_FILE [0x11, fileIndex, offset]
FW-->>Conn: [ts:4BE][audio_data...]
note over Conn,SDSync: onPacketReceived → progress updates
Conn-->>SDSync: all chunks received
SDSync->>SDSync: upload to cloud via syncLocalFiles()
SDSync->>SDSync: onUploadProgress → UI speed/progress
SDSync->>Conn: deleteFile(fileNum)
Conn->>FW: CMD_DELETE_FILE [0x12, fileIndex]
FW-->>Conn: [0x00] success
SDSync->>SDSync: decrement fileNum for higher-index WALs
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| var pd = await _device!.getDeviceInfo(connection); | ||
| String deviceModel = pd.modelNumber.isNotEmpty ? pd.modelNumber : "Omi"; | ||
|
|
There was a problem hiding this comment.
_buildWalsFromFileList
ServiceManager.instance().device.ensureConnection() returns DeviceConnection?, but the result is passed directly to _device!.getDeviceInfo(connection) without a null check. If the BLE connection drops between the earlier ensureConnection call in _getMissingWals and this secondary call inside _buildWalsFromFileList, connection will be null, and the getDeviceInfo implementation will crash at runtime when it tries to invoke methods on the null connection.
The secondary ensureConnection call is also redundant — the caller already holds a live connection at this point. Consider passing the connection reference in from the call site, or at minimum add a null guard:
Future<List<Wal>> _buildWalsFromFileList(String deviceId, BleAudioCodec codec, List<StorageFile> files) async {
var connection = await ServiceManager.instance().device.ensureConnection(deviceId);
if (connection == null) {
Logger.debug("SDCardWalSync: _buildWalsFromFileList - connection lost, returning empty");
return [];
}
var pd = await _device!.getDeviceInfo(connection);
String deviceModel = pd.modelNumber.isNotEmpty ? pd.modelNumber : "Omi";
| // After deletion, file indices shift for all files that had higher index. | ||
| for (var j = 0; j < wals.length; j++) { | ||
| if (j == i) continue; | ||
| if (wals[j].fileNum > wal.fileNum) { | ||
| wals[j].fileNum = wals[j].fileNum - 1; | ||
| } | ||
| } | ||
| } | ||
| } catch (e) { | ||
| Logger.debug("SDCardWalSync: Failed to delete file ${wal.fileNum}: $e (data is safe on phone)"); |
There was a problem hiding this comment.
After successfully deleting a file at index wal.fileNum, the code decrements the fileNum of every remaining WAL whose index is higher than the deleted one. This is only correct if the firmware re-indexes all remaining files after each deletion (i.e., deleting file 0 causes firmware to rename file 1→0, file 2→1, etc.).
If the firmware instead keeps stable/fixed indices (a common FAT-style approach), this in-memory decrement will cause all subsequent _writeToStorage and deleteFile calls to target the wrong file index. The data would still be safe on the phone, but leftover SD card files could never be deleted, and the next sync would attempt to read from the wrong file.
This assumption about firmware behavior needs to be explicitly verified and documented — it's a protocol contract that should appear in a comment or be validated with a connection-level assertion.
// After deletion, file indices shift for all files that had higher index.
// NOTE: this relies on the firmware shifting its file index table after
// CMD_DELETE_FILE. If firmware uses stable/fixed indices, remove this block.
for (var j = 0; j < wals.length; j++) {| Future<List<StorageFile>> performListFiles() async { | ||
| try { | ||
| final completer = Completer<List<StorageFile>>(); | ||
| StreamSubscription? sub; | ||
|
|
||
| final stream = | ||
| transport.getCharacteristicStream(storageDataStreamServiceUuid, storageDataStreamCharacteristicUuid); | ||
|
|
||
| sub = stream.listen((value) { | ||
| if (completer.isCompleted) return; | ||
| if (value.isEmpty) return; | ||
|
|
||
| int count = value[0]; | ||
| int expectedLen = 1 + count * 8; | ||
|
|
||
| // Empty file list | ||
| if (count == 0 && value.length == 1) { | ||
| completer.complete([]); | ||
| return; | ||
| } | ||
|
|
||
| // Validate this looks like a file list response (not a data packet or status byte) | ||
| if (value.length >= expectedLen && count > 0 && count <= 128) { | ||
| List<StorageFile> files = []; | ||
| for (int i = 0; i < count; i++) { | ||
| int base = 1 + i * 8; | ||
| if (base + 8 > value.length) break; | ||
| int timestamp = (value[base] << 24) | (value[base + 1] << 16) | (value[base + 2] << 8) | value[base + 3]; | ||
| int size = (value[base + 4] << 24) | (value[base + 5] << 16) | (value[base + 6] << 8) | value[base + 7]; | ||
| files.add(StorageFile(index: i, timestamp: timestamp, size: size)); | ||
| } | ||
| Logger.debug('OmiDeviceConnection: Listed ${files.length} files'); | ||
| completer.complete(files); | ||
| } | ||
| }); | ||
|
|
||
| // Send CMD_LIST_FILES | ||
| await transport.writeCharacteristic(storageDataStreamServiceUuid, storageDataStreamCharacteristicUuid, [0x10]); | ||
|
|
||
| final result = await completer.future.timeout( | ||
| const Duration(seconds: 10), | ||
| onTimeout: () { | ||
| Logger.debug('OmiDeviceConnection: listFiles timeout'); | ||
| return <StorageFile>[]; | ||
| }, | ||
| ); | ||
|
|
||
| await sub.cancel(); | ||
| return result; | ||
| } catch (e) { | ||
| Logger.debug('OmiDeviceConnection: Error listing files: $e'); | ||
| return []; | ||
| } |
There was a problem hiding this comment.
In both performListFiles and performDeleteFile, sub is assigned inside the try block before writeCharacteristic is called. If writeCharacteristic throws (e.g. BLE write error), the catch block returns immediately without calling sub.cancel(). The subscription stays alive on the characteristic until the BLE connection closes, which can cause stale listeners to fire during a later operation on the same characteristic.
A simple fix is to wrap the post-listen work in a try/finally:
sub = stream.listen((value) { ... });
try {
await transport.writeCharacteristic(..., [0x10]);
final result = await completer.future.timeout(...);
return result;
} finally {
await sub.cancel();
}The same pattern applies to performDeleteFile (lines 357–378).
| Future<bool> performDeleteFile(int fileIndex) async { | ||
| try { | ||
| final completer = Completer<bool>(); | ||
| StreamSubscription? sub; | ||
|
|
||
| final stream = | ||
| transport.getCharacteristicStream(storageDataStreamServiceUuid, storageDataStreamCharacteristicUuid); | ||
|
|
||
| sub = stream.listen((value) { | ||
| if (completer.isCompleted) return; | ||
| if (value.length == 1) { | ||
| // 0 = success, other values = error codes | ||
| Logger.debug('OmiDeviceConnection: deleteFile result: ${value[0]}'); | ||
| completer.complete(value[0] == 0); | ||
| } | ||
| }); | ||
|
|
||
| // Send CMD_DELETE_FILE [0x12, fileIndex] | ||
| await transport.writeCharacteristic( | ||
| storageDataStreamServiceUuid, storageDataStreamCharacteristicUuid, [0x12, fileIndex & 0xFF]); | ||
|
|
||
| final result = await completer.future.timeout( | ||
| const Duration(seconds: 5), | ||
| onTimeout: () { | ||
| Logger.debug('OmiDeviceConnection: deleteFile timeout'); | ||
| return false; | ||
| }, | ||
| ); | ||
|
|
||
| await sub.cancel(); | ||
| return result; | ||
| } catch (e) { | ||
| Logger.debug('OmiDeviceConnection: Error deleting file: $e'); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
performDeleteFile may misinterpret a stale 1-byte notification
The delete response listener fires on any 1-byte packet on storageDataStreamCharacteristicUuid:
if (value.length == 1) {
completer.complete(value[0] == 0);
}The BLE GATT notification queue may still hold a single-byte status byte from the preceding stopStorageSync call (byte 0x00 = success) or from the listFiles trailing empty-list response (count = 0). Both are 1-byte payloads with value 0, indistinguishable from a successful delete confirmation. This could cause performDeleteFile to return true before the firmware has actually processed the delete command.
Consider introducing a command-tag prefix in the delete response, or adding a short drain period (await Future.delayed(const Duration(milliseconds: 50))) before setting up the listener to flush pending notifications.
| } | ||
| } | ||
|
|
||
| if (onUploadProgress != null) { | ||
| var totalFileBytes = 0; | ||
| for (var file in files) { | ||
| totalFileBytes += await file.length(); | ||
| } | ||
| if (totalFileBytes > 0) { | ||
| onUploadProgress(totalFileBytes, totalFileBytes, null); | ||
| } | ||
| } |
There was a problem hiding this comment.
totalFileBytes re-computed via async I/O after response is received
After a successful response, the code calculates totalFileBytes a second time by iterating over files and calling await file.length() for each file. This is the same calculation already performed inside _buildMultipartRequest. For large batches this adds unnecessary async I/O after the upload is already complete.
The 100%-complete callback could instead reuse the total calculated earlier, e.g. by returning it from _buildMultipartRequest or computing it once before the request is built.
// Current (after-response block):
if (onUploadProgress != null) {
var totalFileBytes = 0;
for (var file in files) {
totalFileBytes += await file.length(); // redundant I/O
}
if (totalFileBytes > 0) {
onUploadProgress(totalFileBytes, totalFileBytes, null);
}
}
|
Hey @TuEmb 👋 Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request. After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:
Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out. Thank you for being part of the Omi community! 💜 |
1 participant
Note: This PR is 100% AI generation
This pull request introduces several improvements and refactorings related to file sync and UI feedback for syncing in the application. The main changes include adding upload progress tracking to backend file sync operations, simplifying the sync page UI by removing WiFi-specific sync logic, and enhancing the user experience with animated progress and speed indicators during sync.
Backend & Progress Tracking:
makeMultipartApiCalland related multipart request functions, allowing real-time tracking of bytes sent, total bytes, and upload speed (KB/s) during file uploads. This includes propagating theonUploadProgresscallback through all relevant layers (syncLocalFiles,_buildMultipartRequest, etc.) [1] [2] [3] [4] [5] [6]Sync Page UI Simplification & Improvements:
Removed all WiFi-specific sync options, dialogs, and error handling from
sync_page.dart, consolidating the sync flow and error reporting. The settings card and sync initiation logic no longer reference or branch on WiFi/fast transfer methods [1] [2] [3] [4] [5] [6].Updated the sync progress UI to use animated transitions for both speed (KB/s) and progress percentage, providing a smoother and more visually appealing user experience. This includes using
TweenAnimationBuilderfor speed and progress indicators, and animating the progress bar itself [1] [2] [3].Preferences Utility:
SharedPreferencesUtilfor per-device auto-sync settings, allowing the app to store and retrieve whether auto-sync is enabled for a specific device.Code Cleanup:
sync_page.dartfor better maintainability [1] [2].These changes collectively streamline the sync experience, provide better feedback to users, and lay groundwork for more robust file upload handling.