-
Notifications
You must be signed in to change notification settings - Fork 1
Remove auto reconnect, ConnectionStatus.Reconnecting, and discarding of device on disconnect
#85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 12 commits
6d159fa
392a377
3d3518b
0721d63
1fff6b2
0e99b38
94a01b2
1d9128e
f686b91
87ea022
0db54e9
c84449f
8bdf2a3
c0bb2bb
713edeb
e141be7
205fdb6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,8 +56,7 @@ export interface Service { | |
|
|
||
| interface ConnectCallbacks { | ||
| onConnecting: () => void; | ||
| onReconnecting: () => void; | ||
| onFail: () => void; | ||
| onDisconnect: () => void; | ||
| onSuccess: () => void; | ||
| } | ||
|
|
||
|
|
@@ -84,12 +83,7 @@ interface ConnectCallbacks { | |
| // > this out after 30 seconds and reload the page | ||
|
|
||
| export class BluetoothDeviceWrapper implements Logging { | ||
| // Used to avoid automatic reconnection during user triggered connect/disconnect | ||
| // or reconnection itself. | ||
| private duringExplicitConnectDisconnect: number = 0; | ||
|
|
||
| connected = false; | ||
| private isReconnect = false; | ||
|
|
||
| // Only updated after the full connection flow completes not during bond handling. | ||
| private serviceIds: Set<string> = new Set(); | ||
|
|
@@ -145,16 +139,10 @@ export class BluetoothDeviceWrapper implements Logging { | |
| async connect(options?: ConnectOptions): Promise<void> { | ||
| const progress = options?.progress ?? (() => {}); | ||
| this.logging.event({ | ||
| type: this.isReconnect ? "Reconnect" : "Connect", | ||
| type: "Connect", | ||
| message: "Bluetooth connect start", | ||
| }); | ||
| if (this.isReconnect) { | ||
| this.callbacks.onReconnecting(); | ||
| } else { | ||
| this.callbacks.onConnecting(); | ||
| } | ||
|
|
||
| this.duringExplicitConnectDisconnect++; | ||
| this.callbacks.onConnecting(); | ||
|
|
||
| try { | ||
| if (Capacitor.isNativePlatform()) { | ||
|
|
@@ -174,18 +162,18 @@ export class BluetoothDeviceWrapper implements Logging { | |
| events.forEach((e) => this.startNotifications(e as TypedServiceEvent)); | ||
|
|
||
| this.logging.event({ | ||
| type: this.isReconnect ? "Reconnect" : "Connect", | ||
| type: "Connect", | ||
| message: "Bluetooth connect success", | ||
| }); | ||
| this.callbacks.onSuccess(); | ||
| } catch (e) { | ||
| this.logging.error("Bluetooth connect error", e); | ||
| this.logging.event({ | ||
| type: this.isReconnect ? "Reconnect" : "Connect", | ||
| type: "Connect", | ||
| message: "Bluetooth connect failed", | ||
| }); | ||
| await this.disconnectInternal(false); | ||
| this.callbacks.onFail(); | ||
| await this.disconnectInternal(); | ||
| this.callbacks.onDisconnect(); | ||
|
|
||
| if (e instanceof DeviceError) { | ||
| throw e; | ||
|
|
@@ -210,10 +198,6 @@ export class BluetoothDeviceWrapper implements Logging { | |
| code: "bluetooth-connection-failed", | ||
| message: e instanceof Error ? e.message : String(e), | ||
| }); | ||
| } finally { | ||
| this.duringExplicitConnectDisconnect--; | ||
| // Reset isReconnect for next time | ||
| this.isReconnect = false; | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -226,54 +210,28 @@ export class BluetoothDeviceWrapper implements Logging { | |
| } | ||
|
|
||
| async disconnect(): Promise<void> { | ||
| return this.disconnectInternal(true); | ||
| return this.disconnectInternal(); | ||
| } | ||
|
|
||
| private async disconnectInternal(userTriggered: boolean): Promise<void> { | ||
| this.logging.log( | ||
| `Bluetooth disconnect ${userTriggered ? "(user triggered)" : "(programmatic)"}`, | ||
| ); | ||
| this.duringExplicitConnectDisconnect++; | ||
| private async disconnectInternal(): Promise<void> { | ||
| this.logging.log("Bluetooth disconnect"); | ||
| try { | ||
| if (this.connected) { | ||
| await BleClient.disconnect(this.device.deviceId); | ||
| } | ||
| } catch (e) { | ||
| this.logging.error("Bluetooth GATT disconnect error (ignored)", e); | ||
| // We might have already lost the connection. | ||
| } finally { | ||
| this.duringExplicitConnectDisconnect--; | ||
| } | ||
| } | ||
|
|
||
| async reconnect(): Promise<void> { | ||
| this.logging.log("Bluetooth reconnect"); | ||
| this.isReconnect = true; | ||
| await this.connect(); | ||
| } | ||
|
|
||
| handleDisconnectEvent = async (): Promise<void> => { | ||
| handleDisconnectEvent = (): void => { | ||
| this.waitingForDisconnectEventCallbacks.forEach((cb) => cb()); | ||
| this.waitingForDisconnectEventCallbacks.length = 0; | ||
|
|
||
| this.connected = false; | ||
| try { | ||
| if (!this.duringExplicitConnectDisconnect) { | ||
| this.logging.log( | ||
| "Bluetooth disconnected... automatically trying reconnect", | ||
| ); | ||
| await this.reconnect(); | ||
| } else { | ||
| this.logging.log( | ||
| "Bluetooth disconnect ignored during explicit disconnect", | ||
| ); | ||
| } | ||
| } catch (e) { | ||
| this.logging.error( | ||
| "Bluetooth connect triggered by disconnect listener failed", | ||
| e, | ||
| ); | ||
| } | ||
| this.logging.log("Bluetooth disconnect"); | ||
|
||
| this.callbacks.onDisconnect(); | ||
| }; | ||
|
|
||
| async getBoardVersion(): Promise<BoardVersion> { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,7 @@ import { | |
| import * as protocol from "./usb-serial-protocol.js"; | ||
| import { MicrobitWebUSBConnection } from "./usb.js"; | ||
|
|
||
| const connectTimeoutDuration: number = 10000; | ||
| const connectTimeoutDuration: number = 10_000; | ||
|
|
||
| class BridgeError extends Error {} | ||
| class RemoteError extends Error {} | ||
|
|
@@ -35,8 +35,7 @@ export interface MicrobitRadioBridgeConnectionOptions { | |
|
|
||
| interface ConnectCallbacks { | ||
| onConnecting: () => void; | ||
| onReconnecting: () => void; | ||
| onRestartConnection: () => void; | ||
| onBeforeConnectionLostDispose: () => void; | ||
| onFail: () => void; | ||
| onSuccess: () => void; | ||
| } | ||
|
|
@@ -96,12 +95,10 @@ class MicrobitRadioBridgeConnectionImpl | |
| } | ||
| } else { | ||
| this.status = ConnectionStatus.DISCONNECTED; | ||
| // Reconnect the serial session if we were previously disconnected or paused. | ||
| // Reconnect the serial session if we were previously paused. | ||
| // PAUSED means the USB connection was temporarily suspended due to tab | ||
| // visibility, and now the tab is visible again so we should reconnect. | ||
| const shouldReconnect = | ||
| currentStatus === ConnectionStatus.DISCONNECTED || | ||
| currentStatus === ConnectionStatus.PAUSED; | ||
| const shouldReconnect = currentStatus === ConnectionStatus.PAUSED; | ||
| if (shouldReconnect && this.serialSessionOpen) { | ||
| this.serialSession?.connect(); | ||
| } | ||
|
|
@@ -174,23 +171,14 @@ class MicrobitRadioBridgeConnectionImpl | |
| this.dispatchTypedEvent.bind(this), | ||
| { | ||
| onConnecting: () => this.setStatus(ConnectionStatus.CONNECTING), | ||
| onReconnecting: () => { | ||
| // Leave serial connection running in case the remote device comes back. | ||
| if (this.status !== ConnectionStatus.RECONNECTING) { | ||
| this.setStatus(ConnectionStatus.RECONNECTING); | ||
| } | ||
| }, | ||
| onRestartConnection: () => { | ||
| // So that serial session does not get repetitively disposed in | ||
| // delegate status listener when delegate is disconnected for restarting connection | ||
| this.ignoreDelegateStatus = true; | ||
| onBeforeConnectionLostDispose: () => { | ||
| this.ignoreDelegateStatus = false; | ||
| this.serialSessionOpen = false; | ||
| }, | ||
| onFail: () => { | ||
| if (this.status !== ConnectionStatus.DISCONNECTED) { | ||
| this.setStatus(ConnectionStatus.DISCONNECTED); | ||
| } | ||
| this.ignoreDelegateStatus = false; | ||
| this.serialSessionOpen = false; | ||
| }, | ||
| onSuccess: () => { | ||
| if (this.status !== ConnectionStatus.CONNECTED) { | ||
|
|
@@ -260,7 +248,6 @@ class RadioBridgeSerialSession { | |
| private onPeriodicMessageReceived: (() => void) | undefined; | ||
| private lastReceivedMessageTimestamp: number | undefined; | ||
| private connectionCheckIntervalId: ReturnType<typeof setInterval> | undefined; | ||
| private isRestartingConnection: boolean = false; | ||
|
|
||
| private serialErrorListener = (event: SerialErrorEvent) => { | ||
| this.logging.error("Serial error", event.error); | ||
|
|
@@ -340,11 +327,7 @@ class RadioBridgeSerialSession { | |
| this.delegate.addEventListener("serialdata", this.serialDataListener); | ||
| this.delegate.addEventListener("serialerror", this.serialErrorListener); | ||
| try { | ||
| if (this.isRestartingConnection) { | ||
| this.callbacks.onReconnecting(); | ||
| } else { | ||
| this.callbacks.onConnecting(); | ||
| } | ||
| this.callbacks.onConnecting(); | ||
| await this.handshake(); | ||
|
|
||
| this.logging.log(`Serial: using remote device id ${this.remoteDeviceId}`); | ||
|
|
@@ -372,7 +355,7 @@ class RadioBridgeSerialSession { | |
| setTimeout(() => { | ||
| this.onPeriodicMessageReceived = undefined; | ||
| reject(new Error("Failed to receive data from remote micro:bit")); | ||
| }, 500); | ||
| }, connectTimeoutDuration); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Increased connection timeout from 500ms to 10_000ms to match bluetooth (https://github.com/microbit-foundation/microbit-connection/blob/remove-auto-reconnect/lib/bluetooth-device-wrapper.ts#L45) |
||
| }); | ||
|
|
||
| const startCmdResponse = await this.sendCmdWaitResponse(startCmd); | ||
|
|
@@ -385,7 +368,6 @@ class RadioBridgeSerialSession { | |
| // TODO: in the first-time connection case we used to move the error/disconnect to the background here, why? timing? | ||
| await periodicMessagePromise; | ||
|
|
||
| this.isRestartingConnection = false; | ||
| await this.startConnectionCheck(); | ||
| this.callbacks.onSuccess(); | ||
| } catch (e) { | ||
|
|
@@ -447,33 +429,16 @@ class RadioBridgeSerialSession { | |
| ) { | ||
| this.logging.event({ | ||
| type: "Serial", | ||
| message: "Serial connection lost...attempt to reconnect", | ||
| message: "Serial connection lost", | ||
| }); | ||
| this.callbacks.onReconnecting(); | ||
| } | ||
| if ( | ||
| this.lastReceivedMessageTimestamp && | ||
| Date.now() - this.lastReceivedMessageTimestamp > | ||
| connectTimeoutDuration | ||
| ) { | ||
| await this.restartConnection(); | ||
| this.callbacks.onBeforeConnectionLostDispose(); | ||
|
||
| await this.dispose(true); | ||
| this.callbacks.onFail(); | ||
microbit-grace marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| }, 1000); | ||
| } | ||
| } | ||
|
|
||
| private async restartConnection() { | ||
| this.isRestartingConnection = true; | ||
| this.logging.event({ | ||
| type: "Serial", | ||
| message: "Serial connection lost...restart connection", | ||
| }); | ||
| this.callbacks.onRestartConnection(); | ||
| await this.dispose(true); | ||
| await this.delegate.connect(); | ||
| await this.connect(); | ||
| } | ||
|
|
||
| private stopConnectionCheck() { | ||
| clearInterval(this.connectionCheckIntervalId); | ||
| this.connectionCheckIntervalId = undefined; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can collapse these now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found that knowing whether the disconnect is occurring programatically vs user triggered is quite useful. Not sure why I removed that in the first place. Have recovered it here -> 713edeb