Skip to content

Handle already-connected BLE devices gracefully - #928

Merged
PieterAelse merged 5 commits into
PhilipsHue:masterfrom
utkuvrs:master
Nov 11, 2026
Merged

PieterAelse merged 5 commits into
PhilipsHue:masterfrom
utkuvrs:master

Conversation

@utkuvrs

@utkuvrs utkuvrs commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

Detect and surface a clear, actionable error when RxAndroidBle already holds a connection that the plugin does not track (e.g. after a hot restart).

  • ReactiveBleClient.establishConnection: if the underlying RxBleDevice is CONNECTED but no DeviceConnector is tracked, return an EstablishConnectionFailure Observable with a helpful message instead of triggering BleAlreadyConnectedException.
  • PluginController: map discover-services errors and return a "device_already_connected" code and detailed message when the underlying error indicates an already-connected device.
  • Tests: adjust connectionState stubbing and add a unit test verifying the actionable error is surfaced for untracked OS-level connections.

This avoids confusing BleAlreadyConnected exceptions and guides callers to disconnect the device (or restart Bluetooth) before retrying.

Detect and surface a clear, actionable error when RxAndroidBle already holds a connection that the plugin does not track (e.g. after a hot restart).

- ReactiveBleClient.establishConnection: if the underlying RxBleDevice is CONNECTED but no DeviceConnector is tracked, return an EstablishConnectionFailure Observable with a helpful message instead of triggering BleAlreadyConnectedException.
- PluginController: map discover-services errors and return a "device_already_connected" code and detailed message when the underlying error indicates an already-connected device.
- Tests: adjust connectionState stubbing and add a unit test verifying the actionable error is surfaced for untracked OS-level connections.

This avoids confusing BleAlreadyConnected exceptions and guides callers to disconnect the device (or restart Bluetooth) before retrying.
@utkuvrs

utkuvrs commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

@werediver hello, could you please take a look at this? It's meant for an overall improvement to exception handling.

If you are okay on reviewing this, I'll make the necessary commits to improve it. Thanks for your time.

@werediver

Copy link
Copy Markdown
Collaborator

@utkuvrs My expertise is primarily iOS & Flutter, but I poked some relevant people who may want to look at this p/r.

In fact, they said "looks good" while I was typing this. I will approve and integrate, if the CI validation passes, but a new release will happen later, can't give a timeline on this.

Detect and surface a clear, actionable error when RxAndroidBle already holds a connection that the plugin does not track (e.g. after a hot restart).

- ReactiveBleClient.establishConnection: if the underlying RxBleDevice is CONNECTED but no DeviceConnector is tracked, return an EstablishConnectionFailure Observable with a helpful message instead of triggering BleAlreadyConnectedException.
- PluginController: map discover-services errors and return a "device_already_connected" code and detailed message when the underlying error indicates an already-connected device.
- Tests: adjust connectionState stubbing and add a unit test verifying the actionable error is surfaced for untracked OS-level connections.

This avoids confusing BleAlreadyConnected exceptions and guides callers to disconnect the device (or restart Bluetooth) before retrying.
@utkuvrs

utkuvrs commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks a lot @werediver I appreciate it.

Please do let me know if there's anything that requires any changes on my end.

@utkuvrs utkuvrs closed this Oct 9, 2026
@utkuvrs utkuvrs reopened this Oct 9, 2026
@utkuvrs

utkuvrs commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

@werediver thank you for your time regarding this PR, is it ready for merge I'm unable to see the contents of the workflow. Please do let me know if there is anything I can fix regarding this. If not, should I close the PR?

@werediver

Copy link
Copy Markdown
Collaborator

@PieterAelse I think you're comparatively active here. Is the CI broken permanently and we don't care anymore or is it something someone can fix? We have a p/r with an improvement here (mostly development experience), but the CI doesn't validate it.

@PieterAelse

PieterAelse commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

@werediver / @utkuvrs (thanks for your PR!) I've ran the tests/build and it came back with (link) one (stupid?) detekt error:

style - 10min debt

  • What went wrong:
    ReturnCount - [Function getConnection has 3 return statements which exceeds the limit of 2.] at /Users/runner/work/flutter_reactive_ble/flutter_reactive_ble/packages/reactive_ble_mobile/android/src/main/kotlin/com/signify/hue/flutterreactiveble/ble/ReactiveBleClient.kt:281:17
    Execution failed for task ':reactive_ble_mobile:detekt'.

Which is due to 2 new returns added for method getConnection. I see 3 viable/easy options:

  1. We change the detekt rule of max return count from 2 to 3. (see here )
  2. We annotate the method to be the exception (e.g. @Suppress("Detekt.ReturnCount") )
  3. We change the code to not trigger detekt to complain.

I'm all fine for 1, but I think 2 narrows the scope so is better (and 3 is not needed IMO).

(sorry for closing/reopening, I mispressed 🥲)

@PieterAelse PieterAelse reopened this Oct 16, 2026
@PieterAelse

Copy link
Copy Markdown
Collaborator

@utkuvrs I've discussed internally, could you update the PR to add option 2 (if there's such an annotation)

@utkuvrs

utkuvrs commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

@utkuvrs I've discussed internally, could you update the PR to add option 2 (if there's such an annotation)

Thank you I will start implementing this and I will get back with updates. Have a good day for now.

@utkuvrs

utkuvrs commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

I've also ran the tests on my local with Gradle 9.8.2 here are the results:

Test results

Command (run from example/android, Gradle 9.8.2):

gradle :reactive_ble_mobile:testDebugUnitTest --tests '*ReactiveBleClientTest*' :reactive_ble_mobile:detekt

Result: BUILD SUCCESSFUL in 2m 59s (29 actionable tasks executed).

detekt

  • 23 Kotlin files analyzed
  • 0 code smells
  • ReturnCount violation in ReactiveBleClient.getConnection resolved via @Suppress("ReturnCount")

Unit tests (ReactiveBleClientTest): 26 run, 0 failed, 0 errors, 0 skipped

Writing reading and subscribing to characteristics (11/11)

  • PASS should report failure in case writing characteristic fails
  • PASS should report failure in case reading characteristic fails
  • PASS should incorporate the value in case writecharacteristic succeeds
  • PASS should not call readcharacteristic in case the connection is not established
  • PASS should not call writecharacteristicResponse in case the connection is not established
  • PASS should call writecharacteristicWithoutResponse in case the connection is established
  • PASS should call writecharacteristicResponse in case the connection is established
  • PASS should not call writecharacteristicWithoutResponse in case the connection is not established
  • PASS should not call writecharacteristicWithoutResponse in case the connection is established
  • PASS should incorporate the value in case readcharacteristic succeeds
  • PASS should call readcharacteristic in case the connection is established

Change priority (2/2)

  • PASS returns false when connectionfailed
  • PASS returns prioritysuccess when completed

Discover services (3/3)

  • PASS It returns failure when connectionfailed
  • PASS It surfaces an actionable error when device is connected at OS level but untracked
  • PASS It returns success in case services can be discovered

Establishing a connection (1/1)

  • PASS should use deviceconnector when connecting to a device

Resolving a connection (3/3) — new

  • PASS should reuse the tracked connector even when the OS reports connected
  • PASS should create and track a connector for an untracked disconnected device
  • PASS should return an actionable failure when connected at OS level but untracked

Negotiate mtu (2/2)

  • PASS should return mtunegotiatefailed in case it fails
  • PASS should return mtunegotiatesuccesful in case it succeeds

Observe status (2/2)

  • PASS observes status changes
  • PASS starts with current state

Read RSSI (2/2)

  • PASS should return RSSI in case it succeeds
  • PASS should return error in case it fails

@utkuvrs

utkuvrs commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

@PieterAelse sorry for the late reply. I've applied the 2nd option and also included unit tests.

@PieterAelse
PieterAelse merged commit 939ffbe into PhilipsHue:master Nov 11, 2026
2 checks passed
@PieterAelse

Copy link
Copy Markdown
Collaborator

Thanks @utkuvrs ! Everything was now green so I've (squashed and) merged it 🎉 thanks for you contribution.

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.

3 participants