hotfix - miximum 50 files for each syncing time#7163
hotfix - miximum 50 files for each syncing time#7163TuEmb wants to merge 3 commits intoBasedHardware:mainfrom
Conversation
Greptile SummaryThis PR fixes an MTU overflow bug in
Confidence Score: 3/5Not safe to merge as-is; the MTU-based secondary cap can silently break the pagination contract, causing files to be permanently unsynced on low-MTU links. One P1 logic bug: omi/firmware/omi/src/lib/core/storage.c — the Important Files Changed
Sequence DiagramsequenceDiagram
participant App
participant Device as Device (storage.c)
App->>Device: CMD_LIST_FILES (0x10)
Device->>Device: refresh_file_list_cache()
Device->>Device: compute att_payload from bt_gatt_get_mtu()
Device->>Device: max_files_by_payload = (att_payload-1)/8
Device->>Device: response_count = MIN(file_count, 50, max_files_by_payload)
Device-->>App: [count][ts1][sz1]...[tsN][szN]
alt response_count == 50
loop for each file in batch
App->>Device: CMD_READ_FILE [index]
Device-->>App: file data chunks
App->>Device: CMD_DELETE_FILE [index]
Device-->>App: 0x00 ok
end
App->>Device: CMD_LIST_FILES (request next batch)
else response_count < 50
Note over App: App assumes sync complete — stops requesting
end
Reviews (1): Last reviewed commit: "miximum 50 files for each syncing" | Re-trigger Greptile |
| int max_files_by_payload = (att_payload > 1U) ? ((att_payload - 1U) / STORAGE_FILE_LIST_ENTRY_SIZE) : 0; | ||
| int response_file_count = MIN(sync_file_count, STORAGE_LIST_MAX_FILES_PER_RESPONSE); | ||
| response_file_count = MIN(response_file_count, max_files_by_payload); | ||
|
|
||
| if (response_file_count < 0) { | ||
| response_file_count = 0; | ||
| } |
There was a problem hiding this comment.
Payload-based cap can silently break pagination protocol
max_files_by_payload is used as a hard upper bound on response_file_count, but the app's "request more" trigger is exactly response_file_count == STORAGE_LIST_MAX_FILES_PER_RESPONSE (50). If the negotiated MTU yields a payload where max_files_by_payload < 50 (any MTU below ~404 bytes, i.e., att_payload < 401), the device will send fewer than 50 entries even when 50+ files exist. The app treats anything less than 50 as "done" and stops requesting, leaving remaining files stranded on the device.
For example: MTU = 350 → att_payload = 347 → max_files_by_payload = (347-1)/8 = 43. With 60 files on disk, response_file_count = min(60, 50, 43) = 43. The app receives 43, concludes syncing is complete, and never retrieves the remaining 17 files.
The 50-file cap alone already guarantees the payload stays within 1 + 50×8 = 401 bytes (plus ATT overhead of 3), fitting any MTU ≥ 404. The max_files_by_payload secondary cap is redundant in the common case and silently breaks pagination in the edge case. Consider removing it, or adding a "more files available" flag so the app can continue paginating regardless of batch size.
| if (response_file_count < 0) { | ||
| response_file_count = 0; | ||
| } |
There was a problem hiding this comment.
response_file_count is the result of two successive MIN calls where both operands are always non-negative (sync_file_count >= 1 at this point, STORAGE_LIST_MAX_FILES_PER_RESPONSE = 50 > 0, and max_files_by_payload is derived from an unsigned division). The < 0 branch can never be taken.
| if (response_file_count < 0) { | |
| response_file_count = 0; | |
| } | |
| /* response_file_count is always >= 0 here; no clamp needed */ |
The device is not able to send file list when total file list is over 61 files ( > maximum MTU size - 498):
For each sync cycle, the device sends up to the 50 oldest files. If the app receives exactly 50 files, it will request more after the sync completes. If fewer than 50 files are received, no further requests