From 0ceb52ddd7470400ffbcb752fa9a8d06c846c11b Mon Sep 17 00:00:00 2001 From: jdabbech-ledger Date: Thu, 3 Oct 2024 14:32:22 +0200 Subject: [PATCH] :art: (core): BleConnection use sendapdu promise instead of rxjs subject --- .../ble/transport/BleDeviceConnection.test.ts | 4 +- .../ble/transport/BleDeviceConnection.ts | 106 ++++++++++-------- .../ble/transport/WebBleTransport.test.ts | 2 +- .../ble/transport/WebBleTransport.ts | 18 ++- 4 files changed, 73 insertions(+), 57 deletions(-) diff --git a/packages/core/src/internal/transport/ble/transport/BleDeviceConnection.test.ts b/packages/core/src/internal/transport/ble/transport/BleDeviceConnection.test.ts index 75b9c76ea..3aeb2f621 100644 --- a/packages/core/src/internal/transport/ble/transport/BleDeviceConnection.test.ts +++ b/packages/core/src/internal/transport/ble/transport/BleDeviceConnection.test.ts @@ -44,9 +44,7 @@ describe("BleDeviceConnection", () => { // @ts-expect-error private function call to mock web ble response connection.onNotifyCharacteristicValueChanged({ target: { - value: { - buffer, - }, + value: new DataView(buffer.buffer), }, } as DataViewEvent); } diff --git a/packages/core/src/internal/transport/ble/transport/BleDeviceConnection.ts b/packages/core/src/internal/transport/ble/transport/BleDeviceConnection.ts index 2d2d5443b..d55ba3dc8 100644 --- a/packages/core/src/internal/transport/ble/transport/BleDeviceConnection.ts +++ b/packages/core/src/internal/transport/ble/transport/BleDeviceConnection.ts @@ -1,5 +1,4 @@ import { Either, Left, Maybe, Nothing, Right } from "purify-ts"; -import { Subject } from "rxjs"; import { CommandUtils } from "@api/command/utils/CommandUtils"; import { ApduResponse } from "@api/device-session/ApduResponse"; @@ -39,11 +38,13 @@ export class BleDeviceConnection implements DeviceConnection { ) => ApduSenderService; private readonly _apduReceiver: ApduReceiverService; private _isDeviceReady: boolean; - private _sendApduSubject: Subject; - private _settleReconnectionPromise: Maybe<{ + private _sendApduPromiseResolver: Maybe<{ + resolve(value: Either): void; + }>; + private _settleReconnectionPromiseResolvers: Maybe<{ resolve(): void; reject(err: SdkError): void; - }> = Maybe.zero(); + }>; constructor( { @@ -65,9 +66,16 @@ export class BleDeviceConnection implements DeviceConnection { this.onNotifyCharacteristicValueChanged, ); this._isDeviceReady = false; - this._sendApduSubject = new Subject(); + this._sendApduPromiseResolver = Maybe.zero(); + this._settleReconnectionPromiseResolvers = Maybe.zero(); } + /** + * NotifyCharacteristic setter + * Register a listener on characteristic value change + * @param notifyCharacteristic + * @private + */ private set notifyCharacteristic( notifyCharacteristic: BluetoothRemoteGATTCharacteristic, ) { @@ -78,6 +86,11 @@ export class BleDeviceConnection implements DeviceConnection { ); } + /** + * WriteCharacteristic setter + * @param writeCharacteristic + * @private + */ private set writeCharacteristic( writeCharacteristic: BluetoothRemoteGATTCharacteristic, ) { @@ -95,9 +108,9 @@ export class BleDeviceConnection implements DeviceConnection { const [frameSize] = mtuResponse.slice(5); if (frameSize) { this._apduSender = Maybe.of(this._apduSenderFactory({ frameSize })); - this._settleReconnectionPromise.ifJust((promise) => { + this._settleReconnectionPromiseResolvers.ifJust((promise) => { promise.resolve(); - this._settleReconnectionPromise = Maybe.zero(); + this._settleReconnectionPromiseResolvers = Maybe.zero(); }); this._isDeviceReady = true; } @@ -132,47 +145,50 @@ export class BleDeviceConnection implements DeviceConnection { * APDU 0x0800000000 is used to get this mtu size */ public async setup() { - const apdu = Uint8Array.from([0x08, 0x00, 0x00, 0x00, 0x00]); + const requestMtuApdu = Uint8Array.from([0x08, 0x00, 0x00, 0x00, 0x00]); await this._notifyCharacteristic.startNotifications(); - await this._writeCharacteristic.writeValueWithResponse(apdu); + await this._writeCharacteristic.writeValueWithResponse(requestMtuApdu); } /** * Receive APDU response - * Complete sendApdu subject once the framer receives all the frames of the response + * Resolve sendApdu promise once the framer receives all the frames of the response * @param data */ receiveApdu(data: ArrayBuffer) { const response = this._apduReceiver.handleFrame(new Uint8Array(data)); - response.caseOf({ - Right: (maybeApduResponse) => { + response + .map((maybeApduResponse) => { maybeApduResponse.map((apduResponse) => { - this._sendApduSubject.next(apduResponse); this._logger.debug("Received APDU Response", { data: { response: apduResponse }, }); - this._sendApduSubject.complete(); + this._sendApduPromiseResolver.map(({ resolve }) => + resolve(Right(apduResponse)), + ); }); - }, - Left: (error) => this._sendApduSubject.error(error), - }); + }) + .mapLeft((error) => { + this._sendApduPromiseResolver.map(({ resolve }) => + resolve(Left(error)), + ); + }); } /** * Send apdu if the mtu had been set * * Get all frames for a given APDU - * Subscribe to a Subject that would be complete once the response had been received + * Save a promise that would be completed once the response had been received * @param apdu + * @param triggersDisconnection */ async sendApdu( apdu: Uint8Array, triggersDisconnection?: boolean, ): Promise> { - this._sendApduSubject = new Subject(); - if (!this._isDeviceReady) { return Promise.resolve( Left(new DeviceNotInitializedError("Unknown MTU")), @@ -181,22 +197,8 @@ export class BleDeviceConnection implements DeviceConnection { // Create a promise that would be resolved once the response had been received const resultPromise = new Promise>( (resolve) => { - this._sendApduSubject.subscribe({ - next: async (response) => { - if ( - triggersDisconnection && - CommandUtils.isSuccessResponse(response) - ) { - const reconnectionRes = await this.setupWaitForReconnection(); - reconnectionRes.caseOf({ - Left: (err) => resolve(Left(err)), - Right: () => resolve(Right(response)), - }); - } else { - resolve(Right(response)); - } - }, - error: (err) => resolve(Left(err)), + this._sendApduPromiseResolver = Maybe.of({ + resolve, }); }, ); @@ -216,7 +218,22 @@ export class BleDeviceConnection implements DeviceConnection { this._logger.error("Error sending frame", { data: { error } }); } } - return resultPromise; + const response = await resultPromise; + this._sendApduPromiseResolver = Maybe.zero(); + return response.caseOf({ + Right: async (apduResponse) => { + if ( + triggersDisconnection && + CommandUtils.isSuccessResponse(apduResponse) + ) { + const reconnectionRes = await this.setupWaitForReconnection(); + return reconnectionRes.map(() => apduResponse); + } else { + return Right(apduResponse); + } + }, + Left: async (error) => Promise.resolve(Left(error)), + }); } /** @@ -227,16 +244,9 @@ export class BleDeviceConnection implements DeviceConnection { */ private isDataViewEvent(event: Event): event is DataViewEvent { return ( - typeof event.target === "object" && event.target !== null && "value" in event.target && - typeof event.target.value === "object" && - event.target.value !== null && - "buffer" in event.target.value && - typeof event.target.value.buffer === "object" && - event.target.value.buffer !== null && - "byteLength" in event.target.value.buffer && - typeof event.target.value.buffer.byteLength === "number" + event.target.value instanceof DataView ); } @@ -247,7 +257,7 @@ export class BleDeviceConnection implements DeviceConnection { */ private setupWaitForReconnection(): Promise> { return new Promise>((resolve) => { - this._settleReconnectionPromise = Maybe.of({ + this._settleReconnectionPromiseResolvers = Maybe.of({ resolve: () => resolve(Right(undefined)), reject: (error: SdkError) => resolve(Left(error)), }); @@ -278,9 +288,9 @@ export class BleDeviceConnection implements DeviceConnection { */ public async disconnect() { // if a reconnection promise is pending, reject it - this._settleReconnectionPromise.ifJust((promise) => { + this._settleReconnectionPromiseResolvers.ifJust((promise) => { promise.reject(new ReconnectionFailedError()); - this._settleReconnectionPromise = Maybe.zero(); + this._settleReconnectionPromiseResolvers = Maybe.zero(); }); this._notifyCharacteristic.removeEventListener( "characteristicvaluechanged", diff --git a/packages/core/src/internal/transport/ble/transport/WebBleTransport.test.ts b/packages/core/src/internal/transport/ble/transport/WebBleTransport.test.ts index c6e190589..1ba4ded59 100644 --- a/packages/core/src/internal/transport/ble/transport/WebBleTransport.test.ts +++ b/packages/core/src/internal/transport/ble/transport/WebBleTransport.test.ts @@ -158,7 +158,7 @@ describe("WebBleTransport", () => { }); it("should emit an error if the user did not grant us access to a device (clicking on cancel on the browser popup for ex)", (done) => { - mockedRequestDevice.mockResolvedValueOnce({}); + mockedRequestDevice.mockResolvedValueOnce({ forget: jest.fn() }); discoverDevice( (discoveredDevice) => { diff --git a/packages/core/src/internal/transport/ble/transport/WebBleTransport.ts b/packages/core/src/internal/transport/ble/transport/WebBleTransport.ts index 4d7ddf7aa..7c08ed007 100644 --- a/packages/core/src/internal/transport/ble/transport/WebBleTransport.ts +++ b/packages/core/src/internal/transport/ble/transport/WebBleTransport.ts @@ -81,7 +81,7 @@ export class WebBleTransport implements Transport { return Left(new BleTransportNotSupportedError("WebBle not supported")); } - isSupported() { + isSupported(): boolean { try { const result = !!navigator?.bluetooth; return result; @@ -254,13 +254,13 @@ export class WebBleTransport implements Transport { }, Left: (error) => { - bleDevice.gatt?.disconnect(); + bleDevice.forget(); throw error; }, }); }, Left: (error) => { - bleDevice.gatt?.disconnect(); + bleDevice.forget(); throw error; }, }); @@ -302,6 +302,7 @@ export class WebBleTransport implements Transport { }); return Left(new UnknownDeviceError(`Unknown device ${deviceId}`)); } + // if device already connected, remove device id from internal state and remove error if (this._connectedDevices.includes(internalDevice.bleDevice)) { this._internalDevicesById.delete(deviceId); return Left(new DeviceAlreadyConnectedError("Device already connected")); @@ -342,6 +343,7 @@ export class WebBleTransport implements Transport { this._connectedDevices.push(internalDevice.bleDevice); return Right(connectedDevice); } catch (error) { + await internalDevice.bleDevice.forget(); this._internalDevicesById.delete(deviceId); this._logger.error("Error while getting characteristics", { data: { error }, @@ -424,12 +426,18 @@ export class WebBleTransport implements Transport { ); maybeDeviceConnection.map((dConnection) => dConnection.disconnect()); // disconnect device gatt server - bleDevice.gatt?.disconnect(); + if (bleDevice.gatt?.connected) { + bleDevice.gatt.disconnect(); + } // clean up objects this._internalDevicesById.delete(device.id); this._deviceConnectionById.delete(device.id); this._disconnectionHandlersById.delete(device.id); - delete this._connectedDevices[this._connectedDevices.indexOf(bleDevice)]; + if (this._connectedDevices.includes(bleDevice)) { + delete this._connectedDevices[ + this._connectedDevices.indexOf(bleDevice) + ]; + } }); return Right(void 0);