Skip to content

BLE: GATT callbacks mutate shared state from Binder threads, and GATT operations aren't serialized #110

Closed

Description

@dzid26

Problem

TeslaGattClient.connect opens the link with connectGatt(context, false, callback, TRANSPORT_LE) (TeslaGattClient.kt#L55) and passes no Handler, so every BluetoothGattCallback runs on a Binder thread. Those callbacks then:

  • mutate the client's unsynchronized state: writeQueue (ArrayDeque), writeInProgress, framer, txCharacteristic (L33-L34, `processWriteQueue` [L92-L122](https://github.com/dzid26/TeslaBatteryBLE/blob/18f1b464d84c65a0de00261ea02a2a48334532f9/app/src/main/java/com/dzid26/teslable/ble/TeslaGattClient.kt#L92-L122),`` onCharacteristicWrite L272-L284).`` send() and the `writeTimeout` runnable touch the same fields from the main thread.
  • call straight into VehicleLink through the transport listener (TeslaBleController.kt#L1383-L1441).`` onMessage mutates `sessions`, `pendingSessions`, `pendingCommands` (plain `HashMap`s), `keySlotQueue`, the pairing state and the controller's `vehicles` map (via `rememberVehicle`). The main-thread `Handler` runnables (`poll`, `sessionRetry`, `whitelistPoll`, `pairingTimeout`, `wakeRefresh`) and UI calls mutate the same fields.

Two concrete races:

  1. onCharacteristicWrite → processWriteQueue() on a Binder thread runs at the same time as send() → processWriteQueue() on main. Both can see writeInProgress == false and start two writes. If the first is rejected and re-queued with addFirst, the chunks of a frame go out in the wrong order and the car receives a corrupt frame.
  2. sendAuthenticated inserts into pendingCommands on main (L1059) while `handleEncryptedResponse` removes from it on a Binder thread ([L1071](https://github.com/dzid26/TeslaBatteryBLE/blob/18f1b464d84c65a0de00261ea02a2a48334532f9/app/src/main/java/com/dzid26/teslable/ble/TeslaBleController.kt#L1071)).`` Nothing synchronizes the HashMap.

The simulated car (FakeTeslaTransport) posts every callback on the main looper. That means the demo, the screenshot runs and FakeCarProtocolTest never exercise the real threading model, which is why none of them catch this.

GATT operations are also issued concurrently

Android allows one outstanding GATT operation per connection.

  • onServicesDiscovered starts the CCCD writeDescriptor (via subscribe, L190) and then calls requestMtu right away (L194).`` When requestMtu() is rejected, the code silently stays on the 23-byte default MTU and sends 20-byte chunks.
  • readRemoteRssi() is polled every 500 ms while the UI is visible, and every 10 s in poll. Those reads interleave with characteristic writes.
  • Tesla's Go client does these steps one after another: subscribe, then MTU exchange (pkg/connector/ble/ble.go). It also writes chunks with response (WriteCharacteristic(…, false)). The app uses WRITE_TYPE_NO_RESPONSE (L104).``

Proposal

  • Deliver all callbacks on one thread. Use the API 26+ overload connectGatt(context, false, callback, TRANSPORT_LE, PHY_LE_1M_MASK, handler) (minSdk is 26) with the controller's main-looper Handler or a dedicated HandlerThread. Document the threading contract on TeslaTransport.Listener.
  • Add a small GATT operation queue (CCCD write → MTU → device-name read → writes and RSSI reads) so only one operation is in flight. Request the MTU right after connecting, before service discovery, or after the CCCD write completes.
  • Choose the write type on purpose: match the reference (write with response), or document why write-without-response is safe on the TX characteristic.
  • Add a test transport that delivers callbacks from a background thread, so a regression shows up in CI.

Moving the per-vehicle state machine into core with a single-threaded coroutine model would fix this structurally (architecture item in #109).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions