Repository navigation
ble: stop repeating a BUSY-refused write that was delivered, + the diagnostics that pinned it (#797, #791) - #795
Merged
Conversation
…des (#791) Two diagnostics aimed at the same reported bug: on a Galaxy S24 a single GET_DATA_RANGE produced three CRC-valid responses with advancing strap-side sequence numbers, always correlating with a burst of busy-retries and never on ticks without them. A write the stack reported as refused had been delivered, and the retry duplicated it. Neither of the two facts needed to diagnose that was being recorded. The write status code. writeCharacteristicCompat collapses Android 13+'s int result to a Boolean, discarding which refusal occurred. That distinction is the whole question: ERROR_GATT_WRITE_REQUEST_BUSY (201) is documented as "not initiated" and therefore safe to retry, so either this stack returns it while still delivering, or it returns something else. RealGattOps now keeps the raw status and the busy-retry log line reports it, along with which command is being retried — the harm from a duplicate depends entirely on which command got sent twice. The 4.0 response result code. The 5/MG path has always exposed resp_seq and result; the 4.0 path exposed neither, so a 4.0 strap log could not say whether a command SUCCEEDED or FAILED. The reporter had to hand-decode result=0x00 out of raw hex to establish that their strap answers GET_EXTENDED_BATTERY_INFO with FAILURE rather than with an empty success — a materially different conclusion, and the difference between "battery is unreachable here" and "our decode is wrong". The echo also makes a duplicated write self-evident, since one send now shows the same resp_seq two or three times instead of requiring frame archaeology. The offsets are grounded in real captures, not inferred: an answered GET_DATA_RANGE carries payload[1] = 0x01 (SUCCESS), the empty extended-battery reply carries 0x00 (FAILURE), and the long-standing GET_BATTERY_LEVEL decode has always read its value from payload[2..4] — already skipping both bytes. commandResultLabel's doc no longer claims to be 5/MG-only. Tests use the reporter's real frames as vectors rather than synthesised ones. A draft included a hand-built short-frame case whose payload offsets I got wrong and whose CRC was invented; it was removed rather than shipped unverifiable. Diagnostics only — no retry behaviour is changed. Fixing the duplication needs to know which status the stack returns first, and dropping the retry is not available: it exists because #77/#312 showed a dropped frame silently breaks live HR, the clock, or the backfill. Android-only by nature: CoreBluetooth has no equivalent busy-refusal contract, and the 4.0 result byte is surfaced here to match what the Kotlin 5/MG path already did.
…#797) drainWriteQueue treats writeCharacteristicCompat returning false as proof the frame was not delivered, holds it in pendingRetry and re-writes it. On a Galaxy S24 the write was delivered anyway: one GET_DATA_RANGE drew three CRC-valid responses whose strap-side sequence advanced while the origin-seq echo stayed unchanged, so the strap received the command three times. Only ever after a burst of busy-retries, never on quiet ticks. The reason the retry was never called off is narrow: pendingRetry was cleared only when the drain consumed it, or on reset. Nothing cleared it on a write-completion callback — which is precisely the evidence that the "refused" write landed. So the completion released the in-flight slot, left the retry armed, and the retry sent the frame a second time. A completion for a frame still held for retry now cancels it. This cannot swallow a legitimate retry: pendingRetry is non-null only when the most recent attempt returned BUSY, and the drain never starts a write while one is in flight, so no other outstanding command-channel write could own the completion. A frame the stack truly refused produces no completion at all, so its retry still fires — the #77/#312 protection against a silently dropped TOGGLE_REALTIME_HR / SET_CLOCK / offload-ack is untouched, which is why the retry itself was left in place rather than removed. Not airtight by construction: a completion arriving after the retry has already fired (12 ms base delay) is too late, so this removes the observed case rather than making duplication impossible. The decision is a pure companion predicate because the instance path cannot be unit-tested here — the constructor needs a real Looper and Context, and there is no Robolectric. That is the constraint GattCrashSafetyTest documents and the pattern it follows. Cancellation runs on the main looper, since pendingRetry is main-looper-only state, and is posted without delay so it precedes the retry it cancels.
…#797) Re-review of the previous commit found the safety argument overstated. It claimed no other outstanding command-channel write could own a completion, because the drain never starts a write while one is in flight. writeBondFrame is exactly such a write: it writes the same characteristic DELIBERATELY outside the queue, sets writeInFlight itself, and never consumes pendingRetry. Its ack is therefore not evidence that a held frame went out, and acting on it would cancel the retry for a frame that never left — turning the duplicate this fixes into the silent loss #77/#312 is about, which is strictly worse than a duplicate. The bond write is now tracked and its completion excluded, with the flag consumed on the callback so a later completion cannot inherit it, and cleared in reset() so a stale flag cannot suppress a legitimate cancellation in the next session. Also corrects the scope claim. Only WITH-response writes reach this path at all, because a without-response write gets no completion callback — the drain frees its own slot after a pacing gap. That happens to cover the reported command and the ones where a duplicate actually harms: GET_DATA_RANGE, SET_CLOCK, the historical acks, haptics and RUN_ALARM are all sent withResponse = true. But it is a real limit rather than a general guarantee, and the previous commit read as though the fix applied to every write.
ryanbr
added a commit
that referenced
this pull request
Jul 25, 2026
MARKETING_VERSION/versionName move together per the versioning rule; build numbers increment independently (207->208 Apple, 298->299 Android). The staging workflow reads MARKETING_VERSION for its title and asset names, so main has to carry 9.1.1 for the rolling testing-latest release to be labelled as such — the previous 9.1.1 staging build was cut from a commit that never landed on main. This is also the first compile of the app-target changes merged today (#783/#784/#785, #787, #788, #793, #795, #519/#798, #523, #778), which CI does not cover: app-build.yml and android.yml are disabled and only the Swift packages and the i18n gate run on PRs.
simoncad7
pushed a commit
to simoncad7/noop
that referenced
this pull request
Aug 17, 2026
…agnostics that pinned it (ryanbr#797) (ryanbr#795) Fixes ryanbr#797, reported as "finding 1" inside the battery report in issue 791. THE BUG. One GET_DATA_RANGE produced three CRC-valid responses whose strap-side sequence advanced (0x7e/0x7f/0x80) while the origin-seq echo stayed 0x0a, so the strap received the command three times. Only ever after a burst of busy-retries, never on quiet ticks; reproduced three times in one session on a Galaxy S24 Ultra. drainWriteQueue treated writeCharacteristicCompat returning false as proof of non-delivery. The reason the retry was never called off is narrow: pendingRetry was cleared only when the drain consumed it, or on reset. Nothing cleared it on a write-completion callback — which is precisely the evidence that the "refused" write landed. The completion released the in-flight slot, left the retry armed, and the retry sent the frame again. THE FIX. A completion for a frame still held for retry cancels that retry. A frame the stack truly refused produces no completion at all, so its retry still fires and the ryanbr#77/ryanbr#312 protection against a silently dropped TOGGLE_REALTIME_HR / SET_CLOCK / offload-ack is untouched — which is why the retry was left in place rather than removed. Trading a duplicate for a silent loss would be worse. Two limits, both found by re-review rather than assumed away: The out-of-queue bond write is excluded. writeBondFrame writes the same characteristic deliberately outside the queue and never consumes pendingRetry, so its ack is not evidence about a held frame; acting on it would cancel a retry for a frame that never left. The flag is consumed on the callback so a later completion cannot inherit it, and cleared in reset(). Only WITH-response writes reach this path, because a without-response write gets no completion callback. That covers the reported command and every case where a duplicate actually harms — GET_DATA_RANGE, SET_CLOCK, the historical acks, haptics and RUN_ALARM are all sent withResponse = true — but it is a limit, not a general guarantee. A completion arriving after the retry has already fired is also too late. THE DIAGNOSTICS that made this provable. The Android 13+ write status code is now logged instead of being collapsed into a Boolean, since ERROR_GATT_WRITE_REQUEST_BUSY is documented as "not initiated" and knowing which code came back is the difference between a safe retry and a duplicate. And the 4.0 COMMAND_RESPONSE now exposes resp_seq and result, which the 5/MG path always did: a 4.0 log previously could not say whether a command succeeded, and the reporter had to hand-decode result=0x00 to establish that their strap answers GET_EXTENDED_BATTERY_INFO with FAILURE rather than an empty success. Offsets verified against their real frames. Merged without a compile or a device run: no JDK on the authoring machine and android.yml is disabled. The decision logic is pinned by pure predicates in the repo's established pattern for BLE code, and the decode offsets were verified against real capture frames, but the write-path behaviour has not been exercised on hardware.
simoncad7
pushed a commit
to simoncad7/noop
that referenced
this pull request
Aug 17, 2026
MARKETING_VERSION/versionName move together per the versioning rule; build numbers increment independently (207->208 Apple, 298->299 Android). The staging workflow reads MARKETING_VERSION for its title and asset names, so main has to carry 9.1.1 for the rolling testing-latest release to be labelled as such — the previous 9.1.1 staging build was cut from a commit that never landed on main. This is also the first compile of the app-target changes merged today (ryanbr#783/ryanbr#784/ryanbr#785, ryanbr#787, ryanbr#788, ryanbr#793, ryanbr#795, ryanbr#519/ryanbr#798, ryanbr#523, ryanbr#778), which CI does not cover: app-build.yml and android.yml are disabled and only the Swift packages and the i18n gate run on PRs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #797. Refs #791, where this was reported as "finding 1" inside a battery report.
Two commits at deliberately different risk levels — read the second one carefully.
e312f3a8diagnostics — write status code, 4.0 responseresult/resp_seq2eeb8e06the fix — cancel a retry the completion callback disprovedThey are together because the fix depends on the first commit: its predicate sits beside
writeStatusLabeland its test asserts it. Splitting would have meant a stacked PR, which in this repo has already cost one
branch.
The bug
One
GET_DATA_RANGEproduced three CRC-valid responses whose strap-side sequence advanced (0x7e/0x7f/0x80)while the origin-seq echo stayed
0x0a— so the strap received the command three times. Only ever after aburst of busy-retries, never on quiet ticks. Reproduced three times in one session.
drainWriteQueuetreatswriteCharacteristicCompatreturningfalseas proof of non-delivery. On this stackthe write was delivered anyway. The reason the retry is never called off is narrow:
Nothing cleared it on a write-completion callback — and that callback is exactly the evidence that the
"refused" write landed. The completion released the in-flight slot, left the retry armed, and the retry sent
the frame again.
The fix
A completion for a frame still held for retry cancels it.
It cannot swallow a legitimate retry:
pendingRetryis non-null only when the most recent attempt returnedBUSY, and the drain never starts a write while one is in flight, so no other outstanding command-channel write
could own that completion. A frame the stack truly refused produces no completion at all, so its retry
still fires — which is why the retry was left in place rather than removed. #77/#312 established that a
dropped
TOGGLE_REALTIME_HR/SET_CLOCK/ offload-ack silently breaks live HR, the clock, or the backfill,and trading a duplicate for a silent loss would be worse.
Not airtight by construction. If the completion arrives after the retry has fired (12 ms base delay) the
duplicate is already out. This removes the observed case; it does not make duplication impossible.
Diagnostics (first commit)
writeCharacteristicCompatcollapsed Android 13+'sintinto aBoolean.ERROR_GATT_WRITE_REQUEST_BUSY(201) is documented as not initiated, so which code the stack returns isthe difference between "safe retry" and "we just sent it twice". Now logged, with the command name.
resultandresp_seq. The 5/MG path always exposed both; the 4.0 path exposed neither, so a 4.0log could not say whether a command succeeded. The reporter had to hand-decode
result=0x00from raw hex tolearn their strap answers
GET_EXTENDED_BATTERY_INFOwith FAILURE rather than an empty success — thedifference between "battery unreachable here" and "our decode is wrong". Verified against their real frames:
answered
GET_DATA_RANGE→resp_seq=0x0A,SUCCESS(1); empty extended-battery →resp_seq=0x22,FAILURE(0). Corroborated byGET_BATTERY_LEVELhaving always read frompayload[2..4], already skippingboth bytes.
Testing
The decision is a pure companion predicate because the instance path cannot be unit-tested here — the
constructor needs a real
LooperandContextand the project ships no Robolectric. That is the constraintGattCrashSafetyTestdocuments and the pattern it follows: pin the pure decision the behaviour is built on.Tests cover: a completion cancelling a held retry on both families' command channels; a completion with
nothing held being a no-op; a completion on another characteristic (or a null UUID) never cancelling, since
doing so would convert a duplicate into the silent loss #77/#312 is about; and the refusal-code labels.
The 4.0 decode tests use the reporter's real frames as vectors.
Statically verified: UUID literals in the test match the production constants byte for byte, the predicate and
writeStatusLabelare public companion members, no stale call site remains from the signature change, bracesbalanced,
Tools/i18n_audit.py --ci origin/main→ exit 0.Corrected during re-review
Two claims in the section above were wrong, and a third commit fixes them.
The safety argument was overstated. It said no other outstanding command-channel write could own a
completion, because the drain never starts a write while one is in flight.
writeBondFrameis exactly such awrite — same characteristic, deliberately outside the queue, sets
writeInFlightitself, never consumespendingRetry. Its ack proves nothing about a held frame, and acting on it would cancel a retry for a framethat never left: the duplicate becomes the silent loss #77/#312 exists to prevent, which is worse. Now tracked
and excluded, with the flag consumed on the callback so a later completion cannot inherit it, and cleared in
reset()so a stale flag cannot suppress a legitimate cancellation next session.The fix covers only WITH-response writes. A without-response write gets no completion callback — the drain
frees its own slot after a pacing gap — so there is no signal to act on. It happens to cover the reported
command and every case where a duplicate actually harms (
GET_DATA_RANGE,SET_CLOCK, the historical acks,haptics,
RUN_ALARMare allwithResponse = true), which I verified rather than assumed — but that isfortunate, not by design, and the original text implied generality.
So precisely: a with-response command frame is no longer repeated when a completion proves the BUSY-refused
write was delivered, except where that completion belongs to the bond write. Without-response frames, and
completions arriving after the retry has fired, are unchanged.
What this needs before merge
Not run: the JUnit suite — no JDK on this machine and
android.ymlis disabled, so nothing compiled it.More importantly, the fix wants a device test. It changes when a GATT write is repeated, and the failure
mode if the reasoning is wrong is a silently dropped command — worse than the duplicate it removes. Worth
confirming on hardware that live HR, the clock handshake and the backfill all still work across a reconnect,
ideally on a stack that produces busy-retries. @bhargav8963 offered to run probe builds and has the device that
reproduces it.