Skip to content

Run GATT callbacks on the main thread and serialize setup - #133

Merged
dzid26 merged 5 commits into
mainfrom
work/gatt-main-thread
Oct 8, 2026
Merged

dzid26 merged 5 commits into
mainfrom
work/gatt-main-thread

Conversation

@dzid26

@dzid26 dzid26 commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

Part of #110. TeslaGattClient passed no Handler to connectGatt, so every BluetoothGattCallback ran on a Binder thread. Those callbacks raced the main-thread send() path and the controller's timers over the write queue, the framer and the VehicleLink maps. Setup also started the CCCD write and requestMtu() back to back.

  • Callbacks run on the main thread. On Android 8.1+ the client connects with the API 26 overload connectGatt(context, false, callback, TRANSPORT_LE, PHY_LE_1M_MASK, handler) and the main-looper Handler. Listener calls, write-queue state and the framer now all run on the main thread, like the controller's Handler runnables and the simulated car. The KDoc on TeslaTransport and TeslaTransport.Listener documents this.
  • Android 8.0 (API 26) is handled differently. On 8.0, BluetoothGatt.onNotify stores the value in the shared characteristic on the Binder thread before it posts the callback to the Handler (AOSP oreo-release). 8.1 (oreo-mr1-release) moved setValue into the posted runnable. With a Handler on 8.0, a queued callback can read the next notification's bytes and corrupt a frame. Nordic's Android-BLE-Library avoids the Handler overload on API 26 for the same reason (their issue Landing page: show only the scanning and car screenshots #54). So on 8.0 only, the client takes callbacks on the Binder thread, reads the value immediately and posts the work to the main thread itself. A guard drops posted work for a BluetoothGatt that was closed or replaced.
  • Setup runs one step at a time. After discovery the order is: MTU exchange, then the CCCD write, then the device-name read, then READY. Each step advances only on its own callback, so a stray onMtuChanged can't skip a step or fire READY twice. Characteristic writes wait until setup finishes. The existing fallbacks and their log lines are unchanged:
    • requestMtu() returning false or a failed MTU callback falls back to default chunking.
    • A missing service, RX characteristic or CCCD descriptor, or a failed setCharacteristicNotification/writeDescriptor, skips to the name read.
    • A missing name characteristic or readCharacteristic() returning false goes straight to READY.
  • One new fallback. If the device-name read completed with an error status, the link used to stay in DISCOVERING forever. It now logs Device name read failed with status N and reports READY, like the other name fallbacks.
  • Write type: unchanged at WRITE_TYPE_NO_RESPONSE (validated on a real car). A comment notes that the Go reference writes with response.

TeslaBleController is unchanged. Two parts of #110 are not in this PR: the test transport that delivers callbacks from a background thread, and serializing readRemoteRssi() with writes.

Type

  • Bug fix
  • Feature
  • Protocol / crypto change
  • Docs / CI / tooling
  • Refactor (no behavior change)

Testing

The app module can't be built in the agent environment because Google Maven is blocked, so CI is the first full Android build and lint. Checked locally:

  • Kotlin compile of TeslaGattClient.kt and TeslaTransport.kt (with BleModels.kt, Vehicle.kt and :core) against Robolectric's android-all API 37 framework jar: no errors.
    • The only warnings: in that jar every connectGatt overload is deprecated behind the gatt_conn_settings feature flag. The replacement is connectGatt(BluetoothGattConnectionSettings, Executor, BluetoothGattCallback).
    • The 4-arg call already on main gets the same warning, so this PR adds no new kind of warning.
  • ktlint and detekt on both files with the repo's .editorconfig and config/detekt/detekt.yml: clean.
  • API levels checked by hand for lint NewApi (minSdk 26). The 6-arg connectGatt and PHY_LE_1M_MASK are API 26 and sit behind SDK_INT >= O_MR1. API 33 calls stay behind the existing TIRAMISU checks.
  • Not run: :app:lintDebug, :app:testDebugUnitTest, :app:assembleDebug (left to CI), and nothing on a device or emulator. The Android 8.0 path is untested.

Needs a real-car smoke test before merge: pair, wake, SOC read, and reconnect after the car drops the link. The log should show TX characteristic: true, RX characteristic: true, then Notifications enabled, then Device name: …, with no requestMtu() returned false or MTU negotiation failed lines.

  • ./gradlew :core:test (core untouched)
  • ./gradlew :app:testDebugUnitTest (left to CI)
  • ./gradlew :app:lintDebug (left to CI)
  • Real car (needed: pair, wake, SOC read, reconnect after the car drops the link)

Checklist

  • Protocol changes ship test vectors; expected.txt regenerated, not hand-edited (n/a, no protocol change)
  • No VINs, keys, or decrypted payloads in logs (the new log line prints only a GATT status code)
  • Docs/master-plan updated where relevant (n/a: KDoc only, no master-plan item)
  • New source files carry SPDX-License-Identifier: AGPL-3.0-only (n/a, no new files)

🤖 Generated with Claude Code

https://claude.ai/code/session_0129RzhbEE2cboMcYyZMuxqX


Generated by Claude Code

TeslaGattClient passed no Handler to connectGatt, so every
BluetoothGattCallback ran on a Binder thread and raced the main-thread
send() path and the controller's timers over the write queue, the
framer and VehicleLink state (#110).

- Connect with the API 26 connectGatt overload and the main-looper
  Handler on Android 8.1+. Android 8.0 stores a notification's value on
  the Binder thread before posting to the Handler, so a queued callback
  can read the next notification's bytes; there the client takes Binder
  callbacks, reads the value at once and posts the work to main.
- Run setup one GATT operation at a time: MTU exchange, then the CCCD
  write, then the device-name read, then READY. Characteristic writes
  wait until setup finishes, and a failed name read still reports READY
  instead of stalling the link.
- Keep write-without-response and note that the Go reference writes
  with response.
- Document the main-thread contract on TeslaTransport.Listener.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0129RzhbEE2cboMcYyZMuxqX
@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Screenshots

App screenshots: light and dark

Captured from the demo build at 5bf55cc.

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Screenshots

App screenshots: light and dark

Captured from the demo build at 57070ed.

Debug APK: zip from Android CI (GitHub sign-in needed)

A value pasted on Windows ends in CRLF, and base64 -d rejects the
carriage return ("invalid input"), which failed the build job before
it produced an APK. Strip all whitespace before decoding; this also
accepts line-wrapped encoder output such as certutil's.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0129RzhbEE2cboMcYyZMuxqX
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

The build re-run failed at "Set up the shared CI debug key": the new CI_DEBUG_KEYSTORE_BASE64 repository variable ends in a Windows line break, which base64 -d rejects. Nothing in this PR's code caused it. 3b7828b ports the one-line fix from #141 (strip whitespace before decoding), so this build now uses the shared key. The change is identical to #141's and merges cleanly whichever lands first.


Generated by Claude Code

@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Screenshots

App screenshots: light and dark

Captured from the demo build at 3b7828b.

Debug APK: zip from Android CI (GitHub sign-in needed)

@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Screenshots

App screenshots: light and dark

Captured from the demo build at 2fee286.

Debug APK: zip from Android CI (GitHub sign-in needed)

@dzid26
dzid26 merged commit 1d71ba0 into main Oct 8, 2026
5 checks passed
@dzid26
dzid26 deleted the work/gatt-main-thread branch October 8, 2026 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants