feat: enhance BLE service discovery with p-retry and fresh scan fallback #532
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughBumps many package versions from 1.1.4-alpha.4 to 1.1.5, updates the firmware submodule pointer, and adds retry/fallback and forced-reconnect logic plus logging and caching for BLE connection and discovery in the Electron transport. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant NobleHandler
participant Peripheral
participant BLEScanner
App->>NobleHandler: connectDevice(deviceId)
alt peripheral cached & connected
NobleHandler->>NobleHandler: discoverServicesAndCharacteristicsWithRetry(peripheral, deviceId)
alt discovery succeeds
NobleHandler->>App: return characteristics
else discovery retries exhausted
NobleHandler->>NobleHandler: connectAndDiscoverWithFreshScan(deviceId)
alt fresh scan succeeds
NobleHandler->>App: return characteristics
else fresh scan fails
NobleHandler->>App: throw connection error
end
end
else not connected or stale
NobleHandler->>Peripheral: connect()
NobleHandler->>NobleHandler: discoverServicesAndCharacteristicsWithRetry(peripheral, deviceId)
alt discovery succeeds
NobleHandler->>App: return characteristics
else discovery retries exhausted
NobleHandler->>NobleHandler: forceReconnectPeripheral(peripheral, deviceId)
NobleHandler->>NobleHandler: connectAndDiscoverWithFreshScan(deviceId)
NobleHandler->>App: result or error
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (15)
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
packages/connect-examples/electron-example/package.json(1 hunks)packages/connect-examples/expo-example/package.json(2 hunks)packages/connect-examples/expo-playground/package.json(2 hunks)packages/core/package.json(2 hunks)packages/hd-ble-sdk/package.json(2 hunks)packages/hd-common-connect-sdk/package.json(2 hunks)packages/hd-transport-electron/package.json(2 hunks)packages/hd-transport-electron/src/noble-ble-handler.ts(5 hunks)packages/hd-transport-emulator/package.json(2 hunks)packages/hd-transport-http/package.json(2 hunks)packages/hd-transport-lowlevel/package.json(2 hunks)packages/hd-transport-react-native/package.json(2 hunks)packages/hd-transport-web-device/package.json(2 hunks)packages/hd-transport/package.json(1 hunks)packages/hd-web-sdk/package.json(2 hunks)packages/shared/package.json(1 hunks)submodules/firmware(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/hd-transport-electron/src/noble-ble-handler.ts (1)
packages/hd-transport-electron/src/types/noble-extended.ts (1)
CharacteristicPair(16-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: build (20.x)
- GitHub Check: lint (20.x)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (26)
packages/connect-examples/electron-example/package.json (1)
5-5: Version bump looks good.The change is straightforward and keeps this example in line with the rest of the workspace.
submodules/firmware (1)
1-1: Confirm firmware–host compatibility before mergingThe bump in submodules/firmware to 0ae92cc777ed276b8b2f75a155fd8f13605973b5 couldn’t be auto-verified due to missing refs in the shallow clone. Please manually verify:
- Fetch full history in submodules/firmware (e.g.
git -C submodules/firmware fetch --unshallow).- Inspect the commit range:
git -C submodules/firmware log --oneline 4fb782f04dc465e5b9114922600234b50326b17b..0ae92cc777ed276b8b2f75a155fd8f13605973b5- Scan for protocol or breaking-change markers:
git -C submodules/firmware log -n50 --pretty=format:'%h %s' | grep -iE 'BREAK|PROTO'- Run BLE scans with the new p-retry logic in hd-transport-electron. Ensure no silent failures.
packages/hd-transport/package.json (1)
3-3: Version bump to 1.1.3 is fine.
Matches the global release tag; nothing else changed.packages/shared/package.json (1)
3-3: Shared utils now at 1.1.3 – looks good.
No further action needed.packages/core/package.json (2)
3-3: Core package bumped to 1.1.3.
Clean and consistent.
28-29: Internal deps fully updatedA ripgrep scan for
"@onekeyfe/...": "^1.1.2"returned no matches. All internal packages are now on ^1.1.3.packages/hd-transport-lowlevel/package.json (2)
3-3: Low-level transport at 1.1.3 – okay.
22-23: Dependency versions aligned.All internal links now use ^1.1.3. Run the repo-wide scan to be safe.
packages/hd-transport-http/package.json (1)
3-3: HTTP transport version bump is correct.packages/hd-transport-react-native/package.json (1)
3-3: Versions stay in sync—good.You bumped the package and its internal deps to
1.1.3. This keeps the monorepo consistent and avoids peer-mismatch warnings. Nice sweep.Also applies to: 22-24
packages/hd-transport-web-device/package.json (1)
3-3: Version bump looks fine, but remember the lockfile.Make sure you commit the updated
pnpm-lock.yaml/package-lock.jsonso CI installs the right versions everywhere.Also applies to: 23-24
packages/hd-ble-sdk/package.json (1)
3-3: Aligned BLE SDK deps—LGTM.All direct deps point to
^1.1.3, matching the others. No red flags.Also applies to: 23-25
packages/hd-transport-emulator/package.json (1)
3-3: Emulator stays current—good catch.The emulator now rides the same version train. 👍
Also applies to: 27-28
packages/hd-transport-electron/package.json (2)
3-3: Version bump noted.Nothing else to add here.
29-31: [email protected] — MIT license & ~13 KB unpacked
- License: MIT
- Unpacked size: 13 KB
This adds minimal bundle weight and uses an MIT license that your compliance tooling should already cover. No further action needed.
packages/connect-examples/expo-example/package.json (1)
3-3: Version bump looks consistent.The package version and internal dependencies are properly aligned at 1.1.3.
Also applies to: 22-25
packages/connect-examples/expo-playground/package.json (1)
3-3: Version alignment is correct.Package and dependency versions are consistently updated to 1.1.3.
Also applies to: 20-22
packages/hd-common-connect-sdk/package.json (1)
3-3: Dependencies properly updated.All internal package dependencies align with the 1.1.3 version bump.
Also applies to: 23-28
packages/hd-web-sdk/package.json (1)
3-3: Version consistency maintained.Package version and internal dependencies properly aligned at 1.1.3.
Also applies to: 24-27
packages/hd-transport-electron/src/noble-ble-handler.ts (7)
22-22: Good addition of retry library.The p-retry import adds robust retry capabilities for BLE operations.
567-567: Clear function documentation.The comment clarifies this is the core single-attempt function.
647-686: Well-structured force reconnect logic.The function properly handles disconnection, state cleanup, and reconnection with appropriate delays. The step-by-step approach is clear and methodical.
689-741: Smart fallback strategy implementation.The function tries existing peripheral first, then falls back to fresh scan. Error handling is comprehensive and logging provides good visibility.
744-806: Robust retry mechanism with p-retry.The retry configuration is well-tuned:
- 5 total attempts with exponential backoff
- Force reconnect on 3rd attempt to clear state issues
- Comprehensive logging for debugging
- Reasonable timeout progression (1s → 1.5s → 2.25s → 3s)
914-914: Proper integration of enhanced discovery.The calls to
connectAndDiscoverWithFreshScanreplace direct service discovery, providing the full retry and fallback mechanism.Also applies to: 950-950
919-923: Enhanced error messaging.The updated error messages clearly indicate failure "after all attempts", improving debugging clarity.
Also applies to: 957-961
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (2)
packages/hd-transport-electron/src/noble-ble-handler.ts (2)
79-79: Comment mismatch: 3000ms is 3 seconds, not 15.Fix the comment or the value.
-const CONNECTION_TIMEOUT = 3000; // 15 seconds for device connection +const CONNECTION_TIMEOUT = 3000; // 3 seconds for device connection
26-26: Typo in comment.“bundlinpissues” → “bundling issues”.
-// Noble will be dynamically imported to avoid bundlinpissues +// Noble will be dynamically imported to avoid bundling issues
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
packages/hd-transport-electron/src/noble-ble-handler.ts(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint (20.x)
- GitHub Check: build (20.x)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/hd-transport-electron/src/noble-ble-handler.ts (1)
254-256: Good move: set persistent state listener before init.This captures early state changes and avoids missing the first transition.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (4)
packages/hd-transport-electron/src/noble-ble-handler.ts (4)
388-388: Broken inline “\n” text + undefined deviceCache.This entire caching block is commented out by “//” and contains literal “\n”. Also, deviceCache is not defined. Fix the formatting and add a cache map, or remove the dead code.
Here’s a concrete fix.
Add near other globals (after line 50):
+interface DeviceCacheEntry { + peripheral: Peripheral; + lastSeen: number; + connectionSuccess: boolean; +} +const deviceCache = new Map<string, DeviceCacheEntry>();Replace the malformed line with real code:
- // First check if we have a recent cached peripheral\n const cachedDevice = deviceCache.get(targetDeviceId);\n if (cachedDevice && (Date.now() - cachedDevice.lastSeen) < 30000) { // 30 seconds cache\n logger?.info('[NobleBLE] Using cached device for fast connection:', targetDeviceId);\n \n // Use cached device if it was successful before\n if (cachedDevice.connectionSuccess) {\n discoveredDevices.set(targetDeviceId, cachedDevice.peripheral);\n return cachedDevice.peripheral;\n }\n }\n\n logger?.info('[NobleBLE] Starting targeted scan for device:', targetDeviceId); + // First check if we have a recent cached peripheral (30s cache) + const cachedDevice = deviceCache.get(targetDeviceId); + if (cachedDevice && Date.now() - cachedDevice.lastSeen < 30_000) { + logger?.info('[NobleBLE] Using cached device for fast connection:', targetDeviceId); + if (cachedDevice.connectionSuccess) { + discoveredDevices.set(targetDeviceId, cachedDevice.peripheral); + return cachedDevice.peripheral; + } + } + + logger?.info('[NobleBLE] Starting targeted scan for device:', targetDeviceId);
390-399: Leak: remove discover listener on timeout path too.You stop scanning, but you don’t remove the onDiscover handler if the timeout fires.
const timeout = setTimeout(() => { if (noble) { noble.stopScanning(); + noble.removeListener('discover', onDiscover); } logger?.info('[NobleBLE] Targeted scan timeout for device:', targetDeviceId); resolve(null); }, FAST_SCAN_TIMEOUT);
957-964: Avoid spurious UI disconnect on discovery failure.You call disconnect(), which will fire the ‘disconnect’ listener and emit BLE_DEVICE_DISCONNECTED to the renderer. Consider temporarily removing the listener or skipping the UI event for this internal failure.
- // Disconnect on failure - connectedPeripheral.disconnect(); + // Optional: avoid noisy UI disconnect on internal failure + // peripheral.removeAllListeners('disconnect'); + // connectedPeripheral.disconnect();
79-79: Align CONNECTION_TIMEOUT with commentThe timeout is set to 3000 ms (3 seconds), but the comment says 15 seconds. Increase it to 15000 ms for a 15-second BLE connection window.
- File: packages/hd-transport-electron/src/noble-ble-handler.ts (line 79)
-const CONNECTION_TIMEOUT = 3000; // 15 seconds for device connection +const CONNECTION_TIMEOUT = 15000; // 15 seconds for device connection
♻️ Duplicate comments (8)
packages/hd-transport-http/package.json (1)
3-3: Deps bumped; ensure lockfile refreshed.After merging, re-install to regenerate the lockfile so CI picks up new ranges.
Also applies to: 27-28
packages/hd-transport-electron/src/noble-ble-handler.ts (7)
22-22: p-retry v6 is ESM-only; confirm interop or switch to safe require.This default import can throw at runtime if the build emits CJS without esModuleInterop. Either enable ESM interop/module in tsconfig or use a CJS-safe require.
Suggested quick fix:
- import pRetry from 'p-retry'; + // eslint-disable-next-line @typescript-eslint/no-var-requires + const pRetry = require('p-retry').default as typeof import('p-retry').default;
646-686: Force reconnect will emit a user-visible disconnect; reattach listener after.Temporarily suppress the ‘disconnect’ listener during forced resets and reattach it post-reconnect; pass webContents to allow re-registration.
-async function forceReconnectPeripheral(peripheral: Peripheral, deviceId: string): Promise<void> { +async function forceReconnectPeripheral( + peripheral: Peripheral, + deviceId: string, + webContents?: WebContents +): Promise<void> { logger?.info('[NobleBLE] Forcing connection reset for device:', deviceId); + const hadDisconnectListeners = + typeof (peripheral as any).listenerCount === 'function' + ? (peripheral as any).listenerCount('disconnect') > 0 + : true; + if (hadDisconnectListeners) { + peripheral.removeAllListeners('disconnect'); + } // Step 1: Force disconnect if connected if (peripheral.state === 'connected') { await new Promise<void>(resolve => { peripheral.disconnect(() => { logger?.info('[NobleBLE] Force disconnect completed'); resolve(); }); }); // Wait for complete disconnection await wait(1000); } // Step 2: Clear device state connectedDevices.delete(deviceId); deviceCharacteristics.delete(deviceId); devicePacketStates.delete(deviceId); subscribedDevices.delete(deviceId); subscriptionOperations.delete(deviceId); // Step 3: Re-establish connection await new Promise<void>((resolve, reject) => { peripheral.connect((error: string) => { if (error) { logger?.error('[NobleBLE] Force reconnect failed:', error); reject(new Error(`Force reconnect failed: ${error}`)); } else { logger?.info('[NobleBLE] Force reconnect successful'); connectedDevices.set(deviceId, peripheral); + if (hadDisconnectListeners && webContents) { + setupDisconnectListener(peripheral, deviceId, webContents); + } resolve(); } }); }); // Wait for connection to stabilize await wait(500); }
688-742: Fresh-scan path: attach disconnect listener and use retry wrapper.After connecting the fresh peripheral, attach the disconnect listener. Also, reuse the retry-enabled discovery for consistency.
-async function connectAndDiscoverWithFreshScan(deviceId: string): Promise<CharacteristicPair> { +async function connectAndDiscoverWithFreshScan( + deviceId: string, + webContents: WebContents +): Promise<CharacteristicPair> { logger?.info('[NobleBLE] Attempting connection with fresh peripheral scan as fallback'); const currentPeripheral = discoveredDevices.get(deviceId); if (currentPeripheral) { try { - return await discoverServicesAndCharacteristicsWithRetry(currentPeripheral, deviceId); + return await discoverServicesAndCharacteristicsWithRetry(currentPeripheral, deviceId, webContents); } catch (error) { logger?.error( '[NobleBLE] Service discovery failed with existing peripheral, attempting fresh scan...' ); } } // Last resort: Fresh scan logger?.info('[NobleBLE] Performing fresh scan to get new peripheral object for device:', deviceId); try { const freshPeripheral = await performTargetedScan(deviceId); if (!freshPeripheral) { throw new Error(`Device ${deviceId} not found in fresh scan`); } discoveredDevices.set(deviceId, freshPeripheral); await new Promise<void>((resolve, reject) => { freshPeripheral.connect((error: string) => { if (error) { reject(new Error(`Fresh peripheral connection failed: ${error}`)); } else { connectedDevices.set(deviceId, freshPeripheral); + setupDisconnectListener(freshPeripheral, deviceId, webContents); resolve(); } }); }); logger?.info('[NobleBLE] Attempting service discovery with fresh peripheral'); await wait(1000); - return await discoverServicesAndCharacteristics(freshPeripheral); + return await discoverServicesAndCharacteristicsWithRetry(freshPeripheral, deviceId, webContents); } catch (error) { logger?.error('[NobleBLE] Fresh scan and connection failed:', error); throw error; } }
743-806: Thread webContents through retry and forceReconnect.So you can reattach listeners after forced reconnects.
-async function discoverServicesAndCharacteristicsWithRetry( - peripheral: Peripheral, - deviceId: string -): Promise<CharacteristicPair> { +async function discoverServicesAndCharacteristicsWithRetry( + peripheral: Peripheral, + deviceId: string, + webContents?: WebContents +): Promise<CharacteristicPair> { return pRetry( async attemptNumber => { // ... if (attemptNumber === 3) { logger?.info('[NobleBLE] Attempting force reconnect to clear connection state...'); try { - await forceReconnectPeripheral(peripheral, deviceId); + await forceReconnectPeripheral(peripheral, deviceId, webContents); } catch (error) { logger?.error('[NobleBLE] Force reconnect failed:', error); throw error; } } // ... }, { retries: 4, factor: 1.5, minTimeout: 1000, maxTimeout: 3000, - onFailedAttempt: error => { + onFailedAttempt: error => { logger?.error(`[NobleBLE] Service discovery attempt ${error.attemptNumber} failed:`, { message: error.message, retriesLeft: error.retriesLeft, - nextRetryIn: `${Math.min(1000 * 1.5 ** error.attemptNumber, 3000)}ms`, + approximateNextRetryIn: `${Math.min(1000 * 1.5 ** error.attemptNumber, 3000)}ms`, }); }, } ); }
792-804: Minor: “nextRetryIn” can mislead.p-retry’s internal backoff may differ. Rename to “approximateNextRetryIn” or drop it.
912-924: Pass webContents to fresh-scan discovery.Needed to attach disconnect listener on the discovered peripheral.
- const characteristics = await connectAndDiscoverWithFreshScan(deviceId); + const characteristics = await connectAndDiscoverWithFreshScan(deviceId, webContents);
949-961: Same here: pass webContents through the post-connect path.Ensures disconnect handler is attached on the newly connected peripheral.
- connectAndDiscoverWithFreshScan(deviceId) + connectAndDiscoverWithFreshScan(deviceId, webContents) .then(characteristics => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (16)
packages/connect-examples/electron-example/package.json(1 hunks)packages/connect-examples/expo-example/package.json(2 hunks)packages/connect-examples/expo-playground/package.json(2 hunks)packages/core/package.json(2 hunks)packages/hd-ble-sdk/package.json(2 hunks)packages/hd-common-connect-sdk/package.json(2 hunks)packages/hd-transport-electron/package.json(2 hunks)packages/hd-transport-electron/src/noble-ble-handler.ts(7 hunks)packages/hd-transport-emulator/package.json(2 hunks)packages/hd-transport-http/package.json(2 hunks)packages/hd-transport-lowlevel/package.json(2 hunks)packages/hd-transport-react-native/package.json(2 hunks)packages/hd-transport-web-device/package.json(2 hunks)packages/hd-transport/package.json(1 hunks)packages/hd-web-sdk/package.json(2 hunks)packages/shared/package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: release-web
- GitHub Check: package-publish
- GitHub Check: lint (20.x)
- GitHub Check: build (20.x)
🔇 Additional comments (13)
packages/hd-transport/package.json (1)
3-3: Version bump looks good.No other changes. Safe to publish.
packages/shared/package.json (1)
3-3: LGTM on version bump.No API or dependency changes here.
packages/core/package.json (1)
3-3: Core bump and internal deps aligned.The ranges to ^1.1.4-alpha.1 are consistent across the repo.
Also applies to: 28-29
packages/hd-transport-electron/package.json (1)
3-3: Electron transport bump + p-retry add look fine.We’ll address ESM import concerns in the TS file.
Also applies to: 29-30
packages/hd-transport-emulator/package.json (1)
3-3: LGTM.Version and internal deps updated consistently.
Also applies to: 27-28
packages/hd-transport-react-native/package.json (1)
3-3: Looks good.No functional changes beyond version alignment.
Also applies to: 22-23
packages/hd-transport-electron/src/noble-ble-handler.ts (1)
254-256: Good move: persistent state listener before init wait.This ensures we don’t miss early state changes.
packages/connect-examples/expo-playground/package.json (1)
3-3: Version and deps aligned — good.All internal deps target ^1.1.4-alpha.1. No issues spotted.
Also applies to: 20-22
packages/hd-transport-web-device/package.json (1)
3-3: Alignment looks solid.Version and internal deps match the alpha series. Good to go.
Also applies to: 23-24
packages/connect-examples/expo-example/package.json (1)
3-3: Version bump and dependency sync are correct.Examples point to the new alpha versions. No action needed.
Also applies to: 22-25
packages/hd-common-connect-sdk/package.json (1)
3-3: Common connect SDK deps updated cohesively.All transports and core/shared now target ^1.1.4-alpha.1. Looks consistent.
Also applies to: 23-28
packages/hd-web-sdk/package.json (1)
3-3: hd-web-sdk aligned with transports/core/shared — nice.No incompatibilities apparent from package metadata.
Also applies to: 24-27
packages/hd-transport-lowlevel/package.json (1)
3-3: Low-level transport bump is consistent.Dependency ranges match the 1.1.4-alpha.1 release line.
Also applies to: 22-23
…/desktop-ble-transport # Conflicts: # packages/connect-examples/electron-example/package.json # packages/connect-examples/expo-example/package.json # packages/connect-examples/expo-playground/package.json # packages/core/package.json # packages/hd-ble-sdk/package.json # packages/hd-common-connect-sdk/package.json # packages/hd-transport-electron/package.json # packages/hd-transport-emulator/package.json # packages/hd-transport-http/package.json # packages/hd-transport-lowlevel/package.json # packages/hd-transport-react-native/package.json # packages/hd-transport-web-device/package.json # packages/hd-transport/package.json # packages/hd-web-sdk/package.json # packages/shared/package.json # yarn.lock
Summary by CodeRabbit
New Features
Chores