Include disconnect status in NotConnectedException#1216
Draft
twyatt wants to merge 1 commit into
Draft
Conversation
Adds a 'status' property ('State.Disconnected.Status?') to
'NotConnectedException', allowing clients to examine the underlying
cause of a connection failure (e.g. Android GATT status) directly from
the exception, rather than having to separately monitor the
'State.Disconnected' state.
The status is populated wherever it is available:
- Android: 'onConnectionStateChange' GATT status when closing the
response channel, and 'State.Disconnected.status' when a disconnect
is detected or an I/O operation is attempted without a connection.
- Apple: 'State.Disconnected.status' when a disconnect is detected or
an I/O operation is attempted without a connection; status is
propagated when the peripheral delegate is closed.
- Common: 'State.Disconnected.status' when suspending until a
connection state is reached.
Closes #1154
Co-authored-by: Travis Wyatt <travis.i.wyatt@gmail.com>
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.
Closes #1154
Summary
Adds a
status: State.Disconnected.Status?property toNotConnectedException, so clients can examine the underlying cause of a connection failure (e.g. Android GATT status) directly from the exception, rather than having to separately monitorState.Disconnected(which can be racy, or masked by a downstream symptom such as aGattStatusExceptionfrom a failedrequestMtu, as described in #1154).statusisnullwhen unavailable (e.g. disconnect was requested, or the underlying platform did not provide a status).Where
statusis populatedCallback.onConnectionStateChange: the GATT status (translated via the existingdisconnectedConnectionStatusmapping) is included in theNotConnectedExceptionused to close the response channel — this is the location referenced in Include disconnectedConnectionStatus in NotConnectedException #1154.Connection: "Disconnect detected" disposal includesState.Disconnected.status.BluetoothDeviceAndroidPeripheral.connectionOrThrow: includes the status of the currentDisconnectedstate (if applicable).Connection: "Disconnect detected" disposal includesState.Disconnected.status(translated fromCBErrorvia the existingNSError.toStatus()mapping).PeripheralDelegate.close: propagates the status from the causingNotConnectedException(if any) when closing the response channel.CBPeripheralCoreBluetoothPeripheral.connectionOrThrow: includes the status of the currentDisconnectedstate (if applicable).Peripheral.suspendUntilOrThrow: includesState.Disconnected.status(e.g. connection dropped while establishing a connection).JavaScript/Web is unchanged, as
State.Disconnected.statusis alwaysnullon that platform. JVM (btleplug) is unchanged, as no disconnect status is available from the underlying callback.API / binary compatibility
Since the new constructor parameter has a default value, this change is source compatible but not binary compatible (the two-arg
<init>(String, Throwable)JVM constructor is replaced by a three-arg constructor). Theapi/jvmandapi/androiddumps have been updated accordingly.Note
./gradlew :kable-core:apiDumponly regenerated theapi/jvmdump on my machine (no BCV dump task appears to be registered for theandroidtarget under AGP 9.3 / BCV 0.18.1 — possibly related:api/android/kable-core.apiwas left untouched by #1197 and still lags the JVM dump), so theapi/androiddump portion of this change was updated manually to match.Testing
CallbackTests(Robolectric,androidHostTest) verifying thatonConnectionStateChangecloses the response channel with aNotConnectedExceptioncarrying:TimeoutforGATT_CONN_TIMEOUT),Unknown(5)status for statuses without a dedicated mapping (e.g.GATT_INSUFFICIENT_AUTHENTICATION, the scenario from Include disconnectedConnectionStatus in NotConnectedException #1154), andnullforGATT_SUCCESS../gradlew :kable-core:apiCheck :kable-core:lintKotlin :kable-core:jvmTest :kable-core:testAndroidHostTestpasses locally (Linux; Apple targets compile-verified by CI only, as they are excluded from non-macOS builds).