📦 (mobile) [DSDK-610]: Setup sample app + rn ble transport #666
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
jdabbech-ledger
force-pushed
the
feat/dsdk-610-rn-mobile-ble
branch
from
642d44d to
3c32096
Compare
jdabbech-ledger
force-pushed
the
feat/dsdk-610-rn-mobile-ble
branch
from
3c32096 to
eb57fca
Compare
jdabbech-ledger
changed the title
jdabbech-ledger
force-pushed
the
feat/dsdk-610-rn-mobile-ble
branch
from
2413323 to
2e4aaba
Compare
jdabbech-ledger
force-pushed
the
feat/dsdk-610-rn-mobile-ble
branch
from
2e4aaba to
46c4748
Compare
jdabbech-ledger
force-pushed
the
feat/dsdk-610-rn-mobile-ble
branch
from
29e8b53 to
2537264
Compare
jdabbech-ledger
force-pushed
the
feat/dsdk-610-rn-mobile-ble
branch
from
2537264 to
5b7543c
Compare
jdabbech-ledger
force-pushed
the
feat/dsdk-610-rn-mobile-ble
branch
from
5b7543c to
5790b8c
Compare
jdabbech-ledger
force-pushed
the
feat/dsdk-610-rn-mobile-ble
branch
from
642fe2c to
a1eb875
Compare
jdabbech-ledger
force-pushed
the
feat/dsdk-610-rn-mobile-ble
branch
from
70fb8f1 to
bda6d37
Compare
jdabbech-ledger
force-pushed
the
feat/dsdk-610-rn-mobile-ble
branch
from
bda6d37 to
696eb28
Compare
| @@ -0,0 +1,79 @@ | |||
| This is a new [**React Native**](https://reactnative.dev) project, bootstrapped using [`@react-native-community/cli`](https://github.com/react-native-community/cli). | |||
There was a problem hiding this comment.
[COULD] update the readme
There was a problem hiding this comment.
I'm not sure that it is necessary, otherwise we should probably take time to update web sample app readme as well
| "react-native": "0.76.6", | ||
| "react-native-ble-plx": "^3.4.0", | ||
| "react-native-gesture-handler": "^2.22.0", | ||
| "react-native-pager-view": "^6.7.0", |
There was a problem hiding this comment.
[ASK] not sure we are using this package, check all deps to remove if not used ?
There was a problem hiding this comment.
It's a package required by @react-navigation/material-top-tabs https://reactnavigation.org/docs/material-top-tab-navigator/
apps/mobile/src/hooks/useApduForm.ts
Outdated
|
|
||
| const getHexString = useCallback((raw: Uint8Array): string => { | ||
| return raw | ||
| .reduce((acc, curr) => acc + " " + curr.toString(16).padStart(2, "0"), "") |
There was a problem hiding this comment.
[COULD] we have some utils doing this on dmk: bufferToHexaString
There was a problem hiding this comment.
This hook isn't used, will delete it in mobile
There was a problem hiding this comment.
[SHOULD] rename the file to DeviceSessionsProvider
There was a problem hiding this comment.
Not sure about it, in web sample app provider naming is in PascalCase as they are folders with index file.
In mobile sample app we can't import this way actually (probably need to rework bundle things, didn't took time to do so).
IMO it's clearer with this kind of naming, as the file doesn't have a default export (and the cache computing is faster on my IDE without index file nor default export ^^)
jdabbech-ledger
force-pushed
the
feat/dsdk-610-rn-mobile-ble
branch
from
c980479 to
9b1a744
Compare
jdabbech-ledger
force-pushed
the
feat/dsdk-610-rn-mobile-ble
branch
from
9b1a744 to
9010ec3
Compare
jdabbech-ledger
force-pushed
the
feat/dsdk-610-rn-mobile-ble
branch
from
e23ac94 to
528d45c
Compare
| readonly deviceModel: DeviceModel; | ||
| readonly transport: TransportIdentifier; | ||
| readonly rssi?: number | null; |
There was a problem hiding this comment.
In the future we should try to find a better solution to expose this kind of transport specific data...
Maybe a function getDiscoveredDeviceMetadata(discoveredDevice) that can be implemented by each transport?
| closeConnection(connectedDevice: TransportConnectedDevice) { | ||
| const transport = this.getTransport(connectedDevice.transport); | ||
| transport.map((t) => | ||
| t.disconnect({ | ||
| connectedDevice, | ||
| }), | ||
| ); | ||
| } |
| this._isDeviceReady.next(false); | ||
| const requestMtuApdu = Uint8Array.from([0x08, 0x00, 0x00, 0x00, 0x00]); | ||
| await this.write(Base64.fromUint8Array(requestMtuApdu)); | ||
| let sub: Subscription; | ||
| await new Promise<void>((resolve) => { | ||
| sub = this._isDeviceReady.subscribe((isReady) => { | ||
| if (isReady) { | ||
| resolve(); | ||
| sub.unsubscribe(); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[ASK] Have we tested without this ? From my understanding, react-native-ble-plx does the MTU negociation and then stores the result in device.mtu.
Are there known cases where there's a difference between the value of device.mtu and the value obtained by sending this frame?
If no (we should test on multiple devices & at least android and iOS), then we can skip this whole part and remove the isDeviceReady logic, as the MTU will already be known from scratch.
There was a problem hiding this comment.
Yes I tested without this (flex/stax only) and I'm pretty sure that it's not necessary, but it could have side effect that I haven't seen yet and I prefered to let it.
I'll create a task about it
| }, | ||
| ); | ||
| this._isDeviceReady.next(false); | ||
| const requestMtuApdu = Uint8Array.from([0x08, 0x00, 0x00, 0x00, 0x00]); |
There was a problem hiding this comment.
[COMMENT] this is not an APDU, it's an entire BLE frame.
| import React, { useEffect, useRef } from "react"; | ||
| import { createContext, type PropsWithChildren, useContext } from "react"; |
There was a problem hiding this comment.
[SHOULD] be one import from "react"
📝 Description
apps/folder❓ Context
✅ Checklist
Pull Requests must pass CI checks and undergo code review. Set the PR as Draft if it is not yet ready for review.
🧐 Checklist for the PR Reviewers