-
-
Notifications
You must be signed in to change notification settings - Fork 248
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
BT transport #11855
base: develop
Are you sure you want to change the base?
BT transport #11855
Conversation
b189c0e
to
8b7b77a
Compare
450bb2a
to
4f69c75
Compare
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
|
||
export class BleApi extends AbstractApi { | ||
chunkSize = 244; | ||
devices: BleDevice[] = []; |
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.
unused, remove
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.
Looks great, can't wait to test it
} | ||
|
||
private async enumerate() { | ||
const connectedDevices = await bleManagerInstance().connectedDevices(devicesUUIDs); |
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.
Can not be this.bleManager
accessed instead? Since the _bleManager
should be initialized only once, it should be safe.
const connectedDevices = await bleManagerInstance().connectedDevices(devicesUUIDs); | |
const connectedDevices = this.bleManager.connectedDevices(devicesUUIDs); |
this.bleManager.startDeviceScan(devicesUUIDs, scanOptions, (error, scannedDevice) => { | ||
if (error) { | ||
debugLog('Scan error'); | ||
console.error(error); |
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.
Should not this be a debugLog?
// await awaitsBleOn(bleManagerInstance()); | ||
|
||
// Returns a list of known devices by their identifiers | ||
const devices = await bleManagerInstance().devices([deviceOrId]); |
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.
const devices = await bleManagerInstance().devices([deviceOrId]); | |
const devices = await this.bleManager.devices([deviceOrId]); |
// Returns a list of the peripherals currently connected to the system | ||
// which have discovered services, connected to system doesn't mean | ||
// connected to our app, we check that below. | ||
const connectedDevices = await bleManagerInstance().connectedDevices(devicesUUIDs); |
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.
const connectedDevices = await bleManagerInstance().connectedDevices(devicesUUIDs); | |
const connectedDevices = await this.bleManager.connectedDevices(devicesUUIDs); |
|
||
// Nb ConnectionOptions dropped since it's not used internally by ble-plx. | ||
try { | ||
device = await bleManagerInstance().connectToDevice(deviceOrId, connectOptions); |
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.
this.bleManager
let writeCharacteristic: Characteristic | undefined; | ||
let notifyCharacteristic: Characteristic | undefined; | ||
for (const characteristic of characteristics) { | ||
if (characteristic.isWritableWithoutResponse) { |
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.
are characteristic UUIDs static or dynamic? I'm wondering if this matching should not be done based on characteristic UUID. But it would be possible only if all the Trezors would have the write/notify characteristic with the same UUID. Does it work like that or is the UUID assigned dynamicaly?
Of course no need to change it in this PR since the prototype is still in development.
return this.appConnectedDevices.map(d => d.bleDevice); | ||
}; | ||
|
||
public findDevice = (deviceId: string) => { |
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.
nit for better clarity:
public findDevice = (deviceId: string) => { | |
public findConnectedDevice = (deviceId: string) => { |
@@ -76,6 +76,11 @@ export const selectDiscoverySupportedNetworks = memoizeWithArgs( | |||
(state: DeviceRootState, areTestnetsEnabled: boolean) => | |||
pipe( | |||
selectDeviceSupportedNetworks(state), | |||
symbols => { |
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.
leftover that should be removed?
@@ -17,6 +18,10 @@ export const prepareDiscoveryMiddleware = createMiddlewareWithExtraDeps( | |||
dispatch(discoveryActions.removeDiscovery(action.payload.state)); | |||
} | |||
|
|||
if (action.type.startsWith(DISCOVERY_MODULE_PREFIX)) { |
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.
debug leftover
@@ -474,6 +474,8 @@ export const createDescriptorPreloadedDiscoveryThunk = createThunk( | |||
return; | |||
} | |||
|
|||
console.log('createDescriptorPreloadedDiscoveryThunk', deviceState, supportedNetworks); |
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.
remove this
|
||
export const NUS_SERVICE_UUID = '6e400001-b5a3-f393-e0a9-e50e24dcca9e'; | ||
// const NUS_CHARACTERISTIC_NOTIFY = '6e400003-b5a3-f393-e0a9-e50e24dcca9e'; | ||
// const NUS_CHARACTERISTIC_TX = '6e400002-b5a3-f393-e0a9-e50e24dcca9e'; |
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.
unused?
}; | ||
} | ||
|
||
public async enumerate() { |
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.
enumerateDevices ?
|
||
if (typeof deviceOrId === 'string') { | ||
debugLog(`Trying to open device: ${deviceOrId}`); | ||
// await awaitsBleOn(bleManagerInstance()); |
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.
to be deleted?
let characteristics: Characteristic[] = | ||
await device.characteristicsForService(NUS_SERVICE_UUID); | ||
|
||
// debugLog('Characteristics: ', JSON.stringify(characteristics)); |
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.
to delete?
} | ||
|
||
if (bluetoothAdapterState === AdapterState.Unsupported) { | ||
return <AlertBox title={'Bluetooth Unsupported on this device'} variant="error" />; |
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.
please, remove curly braces
return ( | ||
<VStack spacing="small"> | ||
<AlertBox | ||
title={'Bluetooth is turned off. Please turn of Bluetooth to continue.'} |
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.
curly braces
import { AlertBox, Box, Button, HStack, Loader, Text, VStack } from '@suite-native/atoms'; | ||
import { Translation } from '@suite-native/intl'; | ||
import { prepareNativeStyle, useNativeStyles } from '@trezor/styles'; | ||
import { BLEScannedDevice, nativeBleManager } from '@trezor/transport-native-ble'; |
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.
checks failing due to unused imports
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.
looks OK, couple of notes.
// TODO: implement proper device type | ||
return devices.map(d => ({ path: d.id, type: DEVICE_TYPE.TypeT2 })); |
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.
yes, we probably don't know the type yet? and also we maybe don't know the unique values that are use to match the type yet?
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.
it might be useful to add return value DescriptorApiLevel
to this function. There were some additions to this type, you probably will need to stuff some dummy values (for example product
field). they are not that much important for higher layers. But who knows, maybe you have some value like this also available.
|
||
const DEBUG_LOGS = true; | ||
|
||
const debugLog = (...args: any[]) => { |
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.
now it should be possible to pass logger instance to BleApi from outside. So feel free to use it if you find it useful.
return this.success(undefined); | ||
} | ||
|
||
public async openInternal(_path: string, _first: boolean) { |
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.
openInternal is private so it doesn't need to be here imho
bleDevice: Device; | ||
} | ||
|
||
// TODO(possibly?): would be nice to have state machine to handle all possible states and transitions |
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.
also it would be nice to have this somehow unified with desktop bluetooth implementation. lets agree on some common grounds and ideally use some shared types?
// This is for debug purposes it should be removed once BT goes to production | ||
export const log = (message: string) => { | ||
logs.push(message); | ||
}; |
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.
one attempt to unify logs is this https://github.com/trezor/trezor-suite/blob/develop/packages/utils/src/logs.ts#L12
// TODO: descriptors are somewhere mutated which causes problems on mobile BLE | ||
descriptors: descriptors.map(d => ({ ...d })), |
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.
this should be now solved. the mutation happend in background.ts.
cf42223
to
d2199b1
Compare
b1224e8
to
8146dd4
Compare
859b656
to
34043ec
Compare
34043ec
to
5d9f983
Compare
Description
POC of BT transport layer in Suite Mobile. Implemented using
react-native-ble-plx
lib which is used by all crypto apps that I explored. It's also nice for 3rd parties because they won't need to add extra native dependency and it will also help us avoid some conflicts (hopefully) that could occur when multiple libraries for BT access are used.Also please keep in mind this is very first version, even there is lot edge cases handled, some stuff is missing for example erasing bonds (compared to @szymonlesisz PR). I also did not encountered any packet duplication (but I guess it can happen), but I think it's fine to omit it now and wait for new protocol which will solve it anyway.
Video: https://satoshilabs.slack.com/files/U02RLDSFCPP/F071BCP430S/screen-20240430-154355.mp4
Summary of changes
@trezor/transport-native-ble
nativeBleManager.ts
which is acting like desktop api in @szymonlesisz branch for desktop. It must be single instance and we must be able to import this instance in app UI to handle stuff like connecting, scanning etc.2.
bleApi.ts
- it's basically BT version ofusbApi
@suite-native/app
EXPO_PUBLIC_BLUETOOTH_ENABLED
env variable - this needs to betrue
before build to include some native code for requesting permission for BT (libraries will be included in all types build anyway, but at least it won't show extra permission in stores). By default this will be enabled for Debug and Develop builds so anyone can turn ON BT for testing.@suite-native/bluetooth
@suite-native/bluetooth
- This is special kind of static feature flag because it must be persisted because app must be restarted when it's changed. Once we will support multi-transport restart woudn't be neccessary.Related Issue
Resolve #11751
Screenshots: