Repository navigation
perf(android): exponential BLE reconnect backoff for out-of-range retry loops - #21404
Git-on-my-level merged 6 commits into
Conversation
Git-on-my-level
added
the
ci:full
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b32f0e78ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
b32f0e7 to
d6d0b45
Compare
|
Ack — not implemented in this PR, intentionally. Verified in-tree: no firmware in this repository advertises the Friend Pendant service UUID ( |
|
Monitor run summary (context
|
…ry loops A fixed 3s retry keeps the BLE radio awake while a pendant stays out of range. Stretch that wait up to 60s, and return to the fast interval when the link comes up, the app is foregrounded, or the user connects. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Codex review: onAppForegrounded compared the originally scheduled duration instead of the time left, so resuming 5s into a 6s wait replaced a callback due in 1s with a fresh 3s wait — opening the app postponed reconnect. Record a monotonic (uptime) deadline with each pending retry and pull the timer forward only when the remaining wait still exceeds the fastest interval. BleReconnectBackoff gains remainingDelayMs(); verified locally by compiling the backoff + tests with Kotlin 2.2.20 and running the 6 JVM tests (5 prior + 1 new). Full Gradle suite runs in CI.
…tion INV-CAP-1: the escalated reconnect schedule is only licensed for firmware that verifiably retains audio while disconnected. Service discovery now records whether the device advertises the storage-drain GATT service (storageDataStreamServiceUuid, registered by Omi-family firmware built with CONFIG_OMI_ENABLE_OFFLINE_STORAGE). Devices without it — e.g. Friend Pendant, whose retention the app cannot verify — keep the legacy flat 3s retry cadence, so out-of-range audio is never parked behind a battery wait it cannot afford. CCCD recovery keeps its own budget unchanged. Addresses codex review thread 4259444843 (P1). Verification: BleReconnectBackoffTest scenarios (6 original + 3 new: flat cadence never escalates, no stale step before later escalation, CCCD pass-through independent of the gate) exercised against the production class via kotlinc 2.4.21 — all checks passed. Full :app:testDevDebugUnitTest runs in CI.
d6d0b45 to
d4f5775
Compare
|
Rebased onto latest
Verification: bounded pre-push gate passed against |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4f5775aa8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Independent pre-merge review — VERDICT: CHANGES (Codex gpt-6-luna, read-only, on head d4f5775; not an author of any commit in this PR) P1 — The retention gate trusts a service that can be advertised when storage failed to initialize. The fix is being prepared (runtime probe of the storage-drain read characteristic). |
…backoff Independent review P1: the storage-drain GATT service is advertised whenever offline storage is compiled in, even when storage_init() failed at boot (main.c continues non-critical), so the advertisement alone can license a 6-60s backoff on a device whose audio actually has no durable destination. After discovery sees the service, read the storage status characteristic (30295782, [used, unread, free, rtc] LE uint32s); only a successful read with a nonzero total confirms retention at runtime. Anything else - missing service, failed read, all-zero status - keeps the legacy flat 3s cadence. Probe decision extracted as a pure function with regression tests for the advertised-but-dead-storage case.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0eca7c1fdd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Second review round P2: retentionProbeInFlight was only cleared by the read callback, but OmiBleManager drops callbacks for a disconnected GATT, so one interrupted probe could keep the device on flat retries forever. Probe now has a bounded timeout, GATT disconnect retires the in-flight probe and resets runtime verification, and a stale or late completion still cannot verify a replacement connection. Lifecycle regression tests cover timeout, disconnect interruption, recovery on the next probe, and stale results.
|
Independent pre-merge review round 3 — VERDICT: APPROVE (Codex gpt-6-luna, read-only, on head 2a423d6; not an author of any commit in this PR) No P1/P2 findings. Verbatim summary:
Review history on this PR: round 1 (gpt-6-luna) CHANGES (advertisement can exist with failed storage init) → fixed by 0eca7c1 runtime status probe; round 2 (gpt-6-luna) CHANGES (interrupted probe could permanently disable escalation) → fixed by 2a423d6 probe lifecycle. All review threads have fix/evidence replies. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a423d6cac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ange retry loops (#21404)" (#21484) ## Summary Reverts merge b7e38cf (#21404, "exponential BLE reconnect backoff for out-of-range retry loops") ahead of the 1.0.559 train, per the review-debt ruling of 2026-10-14. Why revert rather than fix forward: #21475 (the fix-forward for #21404's four unanswered P1s) resolved the three verifiable findings, but a focused second-model arbitration of the remaining design disagreement (invalidate retention proof per new GATT connection vs. per attempt) ruled the conservative design audio-loss-critical, not battery-efficiency-only: with a stale proof surviving failed attempts, a device that rebooted into broken storage and then returns to range mid-gap is reached later (6-60s schedule instead of 3s), extending the connected-but-unsubscribed window over audio the broken device is discarding — exactly the class INV-CAP-1's own history rejected ("a trade that silently deletes user recordings is not acceptable at any battery saving"). The conservative design eliminates the escalation feature entirely, so the honest disposition is revert; the fixes and the gate design (per-link proof, RTC-validity gate, subscribe-before-probe) are preserved in #21475 for a re-land only after the escalation gate gets a retention guarantee that covers the disconnected interval. ## Product invariants - INV-CAP-1 (pendant-audio-retention): the revert removes the battery policy whose gate could not prove retention across the disconnected interval. Reconnect pacing returns to the pre-#21404 steady cadence. ## Failure class Failure-Class: none ## Test plan - [x] `git revert -m 1 b7e38cf` — clean (it reverses the original merge wholesale, including its tests and changelog fragment) - [x] No remaining references to BleReconnectBackoff / RetentionProbeLifecycle in main or test sources - [ ] Full CI green on this exact head, then merge to clear the train <!-- This is an auto-generated description by cubic. --> <a href="https://cubic.dev/pr/BasedHardware/omi/pull/21484?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="View guided diff" src="https://www.cubic.dev/buttons/review-in-cubic-light.svg"></picture></a> <a href="https://www.cubic.dev/action/auto-fix/pr/BasedHardware/omi/21484?returnTo=https%3A%2F%2Fgithub.com%2FBasedHardware%2Fomi%2Fpull%2F21484&source=description" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/turn-on-auto-fix-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/turn-on-auto-fix-light.svg"><img alt="Turn on auto-fix" src="https://www.cubic.dev/buttons/turn-on-auto-fix-light.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
…ange retry loops (BasedHardware#21404)" This reverts commit b7e38cf, reversing changes made to 39bc7b8.
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img](https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"></picture></a](https://www.cubic.dev/buttons/review-in-cubic-light.svg"></picture></a){kind=link}
![https://www.cubic.dev/buttons/turn-on-auto-fix-dark.svg"><source](https://www.cubic.dev/buttons/turn-on-auto-fix-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/turn-on-auto-fix-light.svg"><img](https://www.cubic.dev/buttons/turn-on-auto-fix-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/turn-on-auto-fix-light.svg"></picture></a](https://www.cubic.dev/buttons/turn-on-auto-fix-light.svg"></picture></a){kind=link}
1 participant
What changed and why
Android ordinary BLE reconnects used a fixed 3 second delay and retried for as long as the pendant stayed out of range, which keeps the radio and CPU awake. Failed attempts now wait 3s, 6s, 12s, 30s, then 60s. A successful connection, the app returning to the foreground, or an explicit connect request returns the schedule to 3 seconds, and an explicit connect runs immediately instead of sitting out the current wait.
CCCD timeout recovery is unchanged: it still backs off and stops after its existing two-reconnect budget. Packet handling, subscriptions, and WAL buffering are unchanged. Reconnect still happens; only the wait between failed attempts changes.
Product invariants affected
INV-CAP-1
This paces the gap between failed connect attempts. It does not unsubscribe a live uplink, close a connected socket, or drop frames the phone is already receiving. Update (d4f5775, review P1): the escalated schedule is now gated on verified offline retention. Service discovery records whether the device advertises the storage-drain GATT service (Omi-family firmware built with CONFIG_OMI_ENABLE_OFFLINE_STORAGE); only those devices escalate 3/6/12/30/60s, because their firmware retains audio while disconnected. Devices without the advertised capability (Friend Pendant included) keep the legacy flat 3s retry, so stream-only out-of-range audio is never parked behind a battery wait.
How it was verified
Gradle 8.14.2, JDK 21. BUILD SUCCESSFUL.
BleReconnectBackoffTest: 5 tests, 0 failures. Covers the 3s→6s→12s→30s→60s sequence and the cap, per-device isolation, reset on success and explicit retry, foreground pull-forward of a long pending wait, and CCCD recovery leaving the ordinary schedule alone.CccdReconnectPolicyTest: 5 tests, 0 failures. The existing CCCD budget and backoff are unchanged.No Flutter widget tests were run because the reconnect UI was not changed. A physical out-of-range pendant was not exercised here.
Tests
app/android/app/src/test/kotlin/com/friend/ios/ble/BleReconnectBackoffTest.ktUI evidence
UI-Evidence: none -- Android retry pacing only; no widget or screen change.
Failure class (fixes)
Failure-Class: none