Refactoring: Merge observer into device provider#93
Draft
heavyrubberslave wants to merge 17 commits into
Draft
Conversation
Centralizes the previously duplicated createKnownDevice() logic (found in SlvCtrlPlusDeviceFactory, SlvCtrlPlusSerialDeviceProvider, AiroticDeviceProvider and ButtplugIoWebsocketDeviceProvider) into a single shared KnownDeviceResolver. Keeps identity resolution in the Factory for protocols where the device type is only known after a successful handshake (SlvCtrlPlus), and in the Provider for protocols where identity is known upfront from external metadata (Buttplug, Airotic). Also reverts the abstract DeviceProvider base class back to not requiring a Settings dependency - injecting KnownDeviceResolver explicitly where needed is more robust than threading a base constructor param through every provider subclass. Fixes the build, which was broken by an in-progress, uncommitted refactor.
BleDeviceProvider now owns BLE discovery (noble scanning/hotplug) directly instead of relying on a separate BleObserver + DeviceManager's announce/acquire/release/claim arbitration queue. Since there's only ever one BLE radio to scan with, a shared broker to arbitrate between independently scanning observers/providers is unnecessary complexity - the provider is both the sole observer and the sole consumer of what it observes. AiroticDeviceProvider's handshake/construction logic is extracted into a new AiroticDeviceFactory implementing the new BleProtocolFactory interface. BleDeviceProvider tries every registered factory (in registration order) for each newly discovered peripheral until one connects, or none do - replacing the old event-driven mutex queue with a plain loop. Factory registration is settings-driven: which DeviceSource entries are present in settings.json (by protocol name, e.g. 'airotic') determines which factories get registered into the shared provider, preserving today's per-protocol enable/disable behavior without requiring a settings migration. bleObserver.spec.ts is replaced by bleDeviceProvider.spec.ts covering the merged discovery + factory-trial + dedupe behavior.
SerialDeviceProvider now owns serial port discovery (USB list + hotplug events) directly, instead of relying on a separate SerialPortObserver + DeviceManager's announce/acquire/release/claim arbitration queue. SlvCtrlPlusSerialDeviceProvider, Zc95SerialDeviceProvider and EStim2bSerialDeviceProvider are removed - their port-open/handshake logic moves into SlvCtrlPlusDeviceFactory, Zc95DeviceFactory and EStim2bDeviceFactory respectively, each implementing the new SerialProtocolFactory interface (getPortOpenOptions, optional preparePort, tryConnect). For each newly discovered port, SerialDeviceProvider tries every registered factory in registration order - reopening the port fresh with each factory's own port settings (baud rate etc.) between attempts - until one connects or none do. This replaces the old event-driven mutex queue with a plain loop, while preserving the existing 'try each newly-seen port exactly once, retry if it disappears and reappears' semantics from the old observer. Factory registration is settings-driven, same as the BLE merge: which DeviceSource entries are present in settings.json (by protocol name, e.g. 'slvCtrlPlusSerial') determines which factories get registered, preserving today's per-protocol enable/disable behavior without a settings migration. serialPortObserver.spec.ts is replaced by serialDeviceProvider.spec.ts. Integration tests updated to reference the merged provider service key and the relocated protocolName constants.
Now that SerialDeviceProvider and BleDeviceProvider both own their discovery and factory-trial loop directly, DeviceManager's deviceDetected event and its announce/acquire/release/claim/revoke arbitration queue have no remaining consumers - that machinery existed only to arbitrate between multiple independently-subscribed providers racing over a shared detected device, which no longer happens now that there's exactly one provider per transport. DeviceManager goes back to being just the connected-device registry plus refresh scheduling and connected/disconnected/refreshed/notification events. Also removes two now-dead leftovers exposed by this cleanup: - GenericDeviceProviderFactory, which had no remaining callers now that SlvCtrlPlus/Zc95/EStim2b/Airotic no longer go through DeviceProviderFactory/DeviceProviderManager (only Buttplug and Virtual do, via their own hand-written factories). - deviceProviderEvent.ts, an empty, unreferenced file.
Object.defineProperty instead of direct assignment for Peripheral.state (readonly in @stoprocent/noble's types), and explicit mock<T>() type params where TS otherwise infers unknown.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SerialDeviceProvider had its own start() method instead of overriding the DeviceProvider base class's init(), unlike BleDeviceProvider which correctly overrides init(). Renaming to align naming and properly fulfil the base class's public lifecycle contract.
resolveOrCreate() previously called settings.addKnownDevice() immediately when creating a new KnownDevice, before the caller had actually finished constructing the Device. If device construction failed afterwards (e.g. attribute fetch throws after identity resolution succeeds), a phantom KnownDevice entry was already persisted to settings.json even though no Device was ever actually connected. This was a regression introduced while extracting KnownDeviceResolver - the original, duplicated per-factory code always persisted at the very end, after the Device was fully constructed. Fixed by having resolveOrCreate() take a buildDevice callback: for a new identity, the KnownDevice is only persisted after buildDevice resolves successfully. This makes the correct ordering structural rather than relying on every caller remembering to persist at the right point. Updates SlvCtrlPlusDeviceFactory, AiroticDeviceFactory and ButtplugIoWebsocketDeviceProvider to pass their device-construction logic as the callback. Adds knownDeviceResolver.spec.ts covering the persist-on-success guarantee for both new and already-known devices.
…ly built" This reverts commit 6211804.
The previous callback-based fix (resolveOrCreate(..., buildDevice)) made the resolver responsible for orchestrating device construction, which isn't its job - it conflated 'resolve an identity' with 'decide when construction succeeded'. Splits the API into two plain methods instead: - resolve(deviceId, type, provider, name?): looks up an existing identity or builds a new, not-yet-persisted one. Never has side effects. - persist(knownDevice): explicitly saves it. Safe to call unconditionally after success, even for an already-known device (harmless no-op re-registration). Callers go back to a plain linear flow - resolve, do their protocol-specific work, construct the Device, then persist - which is exactly what the original (pre-refactor, duplicated) per-factory code did. The class is renamed to KnownDeviceRegistry since 'Resolver' undersold what it actually does (look up AND register), and now that persistence is an explicit, separate call, there's no ambiguity about it 'resolving' the whole thing on its own. Callers stay in control of their own control flow instead of the registry owning it via an injected callback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/device/protocol/zc95/zc95DeviceFactory.ts (1)
143-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winZC95 identity/name isn't stable across reconnects. Unlike the other migrated factories,
create()callsthis.nameGenerator.generateName()directly instead of resolving viaKnownDeviceRegistry, so every reconnect produces a new random name (and persistence is intentionally disabled per the issue-151 comment). If a stable name/id across reconnects is desired, resolve throughknownDeviceRegistry.resolve(deviceId, ...)(returning the existing identity when known) even while persistence stays gated. Please confirm the current behavior is intended.Note that
this.settingsis now unused (only referenced in the commented-out line), so it and its constructor parameter can be dropped once the registry decision is settled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/device/protocol/zc95/zc95DeviceFactory.ts` around lines 143 - 146, The Zc95DeviceFactory.create path is generating a fresh name on every reconnect by calling this.nameGenerator.generateName() directly instead of reusing the existing identity through KnownDeviceRegistry. Update create() to resolve the device via knownDeviceRegistry.resolve(deviceId, ...) so reconnects keep a stable name/id when the device is already known, while preserving the existing persistence gating; also clean up the now-unused this.settings reference and constructor parameter if they are no longer needed.tests/unit/device/provider/serialDeviceProvider.spec.ts (2)
79-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead
openErrorparameter — port-open-failure path isn't actually tested.
createFactory'sopenErrorparameter is accepted but only silenced withvoid openError;; it's never passed toFakeSerialPortor otherwise used bymockSerialPortFactory.create. No test currently exercises a failing portopen(). Either wire this through toFakeSerialPortconstruction or drop the unused parameter.Also applies to: 96-103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/device/provider/serialDeviceProvider.spec.ts` around lines 79 - 86, The createFactory helper has an unused openError parameter, so the failing port-open path is not covered. Update createFactory and the related mockSerialPortFactory.create/FakeSerialPort setup so openError is actually forwarded into the mocked port open behavior, then add or adjust the SerialDeviceProvider tests that rely on this failure path; if you don’t need that scenario, remove the openError parameter from createFactory and the affected test cases.
206-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't verify the port is actually closed.
Despite the title, this test only checks
addDevicewasn't called; it never asserts oncreatedPorts[0](e.g.isOpen/closecall) to confirm the port is actually closed when no factory recognizes the device.createdPortsis tracked for this purpose but unused throughout the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/device/provider/serialDeviceProvider.spec.ts` around lines 206 - 218, The test named “closes the port again when no factory recognizes the device” only checks that addDevice is not called, so it does not verify the port was actually closed. Update the SerialDeviceProvider spec to assert against createdPorts[0] after discoverSerialDevices() completes, using the tracked MockPort instance to confirm close() was called or the port is no longer open. Keep the focus on the no-recognition path in createProvider, createFactory, and discoverSerialDevices.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/device/protocol/estim2b/estim2bDeviceFactory.ts`:
- Around line 20-38: Estim2bDeviceFactory is still generating a new device name
on each connection instead of reusing a persisted KnownDevice. Update the create
flow in Estim2bDeviceFactory to resolve the device through KnownDeviceRegistry,
similar to SlvCtrlPlusDeviceFactory, and pass the stable KnownDevice into the
device creation path instead of calling this.nameGenerator.generateName()
directly. Make sure the factory uses the existing unique identifiers such as
create(), KnownDeviceRegistry, and nameGenerator so reconnects preserve identity
and config.
In `@src/device/provider/bleDeviceProvider.ts`:
- Around line 26-28: The connectedDevices set in BleDeviceProvider is only
cleaned up in stop(), so devices that disconnect mid-runtime remain referenced
and the set can grow without bound. Update the connection/disconnect flow in
BleDeviceProvider, especially the connect handling and any device
close/disconnect callbacks around the affected methods, to remove each BleDevice
from connectedDevices as soon as it disconnects or fails to reconnect. Make sure
the prune logic is wired into the same lifecycle path that adds devices to
connectedDevices so rediscovered devices do not accumulate duplicate stale
entries.
- Around line 64-78: The shutdown loop in BLEDeviceProvider.stop() stops on the
first failing device.close(), which can leave other devices open and prevent
connectedDevices.clear() from running. Update stop() to handle each connected
device independently, similar to DeviceManager.reset(), by catching and logging
errors around each close() call and continuing through the rest of
connectedDevices before clearing the set.
- Around line 44-62: The BLE initialization flow can call observe() twice, which
allows startScanningAsync to race when init() invokes observe() and the
noble.on('stateChange') poweredOn path re-enters it. Fix this by making
BleDeviceProvider.observe() mark isScanning = true before any await inside the
scanning startup path, or by removing one of the two entry points from init() so
only a single path can start scanning. Keep the guard consistent around
observe(), init(), and the stateChange handler to prevent duplicate scan starts.
In `@src/device/provider/serialDeviceProvider.ts`:
- Around line 102-112: `SerialDeviceProvider` is marking ports as managed before
`attemptConnect`, so a port that yields no device stays in `managedPortIds` and
won’t be retried on later scans. Update the connect flow in the
`SerialDeviceProvider` scan logic so that when `attemptConnect` fails to
identify a device (or returns no device), the corresponding port id is removed
from `managedPortIds` before exiting the catch path. Keep the existing
`logError` handling, but ensure the retry state is cleared in the same block
that handles failed connection attempts.
- Around line 56-81: The debounced USB rescan in SerialDeviceProvider.init() can
be lost when discoveryInFlight is already true, so a hotplug event during an
ongoing scan never triggers a follow-up scan. Update the
onUsbEventRef/setTimeout flow to remember that a rescan is pending when
discoverSerialDevices() is busy, and re-run the scan immediately after the
current discoverSerialDevices() completes. Use the existing discoveryInFlight,
rescanTimer, and discoverSerialDevices() logic to re-arm the deferred scan
instead of returning early.
---
Nitpick comments:
In `@src/device/protocol/zc95/zc95DeviceFactory.ts`:
- Around line 143-146: The Zc95DeviceFactory.create path is generating a fresh
name on every reconnect by calling this.nameGenerator.generateName() directly
instead of reusing the existing identity through KnownDeviceRegistry. Update
create() to resolve the device via knownDeviceRegistry.resolve(deviceId, ...) so
reconnects keep a stable name/id when the device is already known, while
preserving the existing persistence gating; also clean up the now-unused
this.settings reference and constructor parameter if they are no longer needed.
In `@tests/unit/device/provider/serialDeviceProvider.spec.ts`:
- Around line 79-86: The createFactory helper has an unused openError parameter,
so the failing port-open path is not covered. Update createFactory and the
related mockSerialPortFactory.create/FakeSerialPort setup so openError is
actually forwarded into the mocked port open behavior, then add or adjust the
SerialDeviceProvider tests that rely on this failure path; if you don’t need
that scenario, remove the openError parameter from createFactory and the
affected test cases.
- Around line 206-218: The test named “closes the port again when no factory
recognizes the device” only checks that addDevice is not called, so it does not
verify the port was actually closed. Update the SerialDeviceProvider spec to
assert against createdPorts[0] after discoverSerialDevices() completes, using
the tracked MockPort instance to confirm close() was called or the port is no
longer open. Keep the focus on the no-recognition path in createProvider,
createFactory, and discoverSerialDevices.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48da89b2-66d2-436d-baa3-3f3eacc5b79b
📒 Files selected for processing (37)
src/app.tssrc/device/deviceManager.tssrc/device/knownDeviceRegistry.tssrc/device/protocol/airotic/airoticDeviceFactory.tssrc/device/protocol/buttplugIo/buttplugIoDeviceFactory.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProviderFactory.tssrc/device/protocol/estim2b/estim2bDeviceFactory.tssrc/device/protocol/estim2b/estim2bSerialDeviceProvider.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusDeviceFactory.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.tssrc/device/protocol/zc95/zc95DeviceFactory.tssrc/device/protocol/zc95/zc95SerialDeviceProvider.tssrc/device/provider/bleDeviceProvider.tssrc/device/provider/bleProtocolFactory.tssrc/device/provider/deviceProviderEvent.tssrc/device/provider/genericDeviceProviderFactory.tssrc/device/provider/serialDeviceProvider.tssrc/device/provider/serialProtocolFactory.tssrc/device/transport/bleDeviceTransport.tssrc/device/transport/bleObserver.tssrc/device/transport/serialPortObserver.tssrc/serviceMap.tssrc/serviceProvider/deviceServiceProvider.tssrc/settings/knownDevice.tssrc/settings/settingsManager.tstests/integration/devices/airoticDevice.spec.tstests/integration/devices/estim2bDevice.spec.tstests/integration/devices/slvCtrlSerialDevice.spec.tstests/integration/devices/zc95Device.spec.tstests/integration/helpers/mockSerialPortFactory.tstests/unit/device/deviceManager.spec.tstests/unit/device/knownDeviceRegistry.spec.tstests/unit/device/provider/bleDeviceProvider.spec.tstests/unit/device/provider/serialDeviceProvider.spec.tstests/unit/device/transport/bleObserver.spec.tstests/unit/device/transport/serialPortObserver.spec.ts
💤 Files with no reviewable changes (8)
- tests/unit/device/transport/bleObserver.spec.ts
- src/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.ts
- tests/unit/device/transport/serialPortObserver.spec.ts
- src/device/provider/genericDeviceProviderFactory.ts
- src/device/transport/bleObserver.ts
- src/device/protocol/zc95/zc95SerialDeviceProvider.ts
- src/device/transport/serialPortObserver.ts
- src/device/protocol/estim2b/estim2bSerialDeviceProvider.ts
Comment on lines
+20
to
+38
| import SynchronousSerialPort from '../../../serial/synchronousSerialPort.js'; | ||
| import SerialDeviceTransportFactory from '../../transport/serialDeviceTransportFactory.js'; | ||
| import { getErrorFromDecodeResult } from '../deviceProtocol.js'; | ||
| import SerialProtocolFactory, { SerialDeviceInfo, SerialDeviceProviderPortOpenOptions } from '../../provider/serialProtocolFactory.js'; | ||
|
|
||
| export default class Estim2bDeviceFactory | ||
| export default class Estim2bDeviceFactory implements SerialProtocolFactory<Estim2bDevice> | ||
| { | ||
| public static readonly protocolName = 'estim2bSerial'; | ||
|
|
||
| public readonly protocolName = Estim2bDeviceFactory.protocolName; | ||
|
|
||
| private readonly dateFactory: DateFactory; | ||
|
|
||
| private readonly settings: Settings; | ||
|
|
||
| private readonly nameGenerator: DeviceNameGenerator; | ||
|
|
||
| private readonly transportFactory: SerialDeviceTransportFactory; | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare SlvCtrlPlusDeviceFactory's KnownDeviceRegistry usage against Estim2bDeviceFactory.
ast-grep run --pattern 'class SlvCtrlPlusDeviceFactory {
$$$
}' --lang typescript src/device/protocol/slvCtrlPlus/slvCtrlPlusDeviceFactory.ts
rg -n -A5 -B5 'knownDeviceRegistry' src/device/protocol/slvCtrlPlus/slvCtrlPlusDeviceFactory.ts
rg -n -A5 -B5 'device.factory.estim2b|device.serial.factory.slvCtrlPlus' src/serviceProvider/deviceServiceProvider.tsRepository: SlvCtrlPlus/slvctrlplus-server
Length of output: 4949
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Estim2bDeviceFactory outline =="
ast-grep outline src/device/protocol/estim2b/estim2bDeviceFactory.ts --view expanded || true
echo
echo "== Estim2bDeviceFactory relevant lines =="
sed -n '1,220p' src/device/protocol/estim2b/estim2bDeviceFactory.ts | cat -n
echo
echo "== Search for KnownDeviceRegistry usage in Estim2b protocol =="
rg -n "KnownDeviceRegistry|knownDeviceRegistry|generateName\\(|persist\\(" src/device/protocol/estim2b src/serviceProvider/deviceServiceProvider.ts
echo
echo "== Compare with other factories =="
rg -n -A4 -B4 "class .*DeviceFactory|knownDeviceRegistry|generateName\\(" src/device/protocol/{slvCtrlPlus,estim2b,zc95,buttplugIo}/*.tsRepository: SlvCtrlPlus/slvctrlplus-server
Length of output: 31680
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/device/protocol/estim2b/estim2bDeviceFactory.ts | cat -nRepository: SlvCtrlPlus/slvctrlplus-server
Length of output: 8630
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba src/device/protocol/estim2b/estim2bDeviceFactory.ts | sed -n '1,220p'Repository: SlvCtrlPlus/slvctrlplus-server
Length of output: 208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Estim2bDeviceFactory =="
nl -ba src/device/protocol/estim2b/estim2bDeviceFactory.ts | sed -n '1,220p'
echo
echo "== Nearby factory patterns =="
for f in src/device/protocol/slvCtrlPlus/slvCtrlPlusDeviceFactory.ts src/device/protocol/zc95/zc95DeviceFactory.ts src/device/protocol/buttplugIo/buttplugIoDeviceFactory.ts; do
echo "--- $f ---"
rg -n -A4 -B4 "knownDeviceRegistry|generateName\\(|resolve\\(|persist\\(" "$f" || true
doneRepository: SlvCtrlPlus/slvctrlplus-server
Length of output: 235
Estim2bDeviceFactory still creates a fresh name on every connect
create() still calls this.nameGenerator.generateName() and never resolves/persists a KnownDevice, so Estim2b devices won’t keep a stable identity/config across reconnects like SlvCtrlPlusDeviceFactory does. Hook this factory into KnownDeviceRegistry the same way as the sibling serial factory.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/device/protocol/estim2b/estim2bDeviceFactory.ts` around lines 20 - 38,
Estim2bDeviceFactory is still generating a new device name on each connection
instead of reusing a persisted KnownDevice. Update the create flow in
Estim2bDeviceFactory to resolve the device through KnownDeviceRegistry, similar
to SlvCtrlPlusDeviceFactory, and pass the stable KnownDevice into the device
creation path instead of calling this.nameGenerator.generateName() directly.
Make sure the factory uses the existing unique identifiers such as create(),
KnownDeviceRegistry, and nameGenerator so reconnects preserve identity and
config.
Comment on lines
+26
to
+28
| private readonly factories: BleProtocolFactory<any>[] = []; | ||
|
|
||
| private readonly connectedDevices: Set<BleDevice<any, any, any>> = new Set(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
connectedDevices set grows unbounded — never pruned on device disconnect.
Devices are added to connectedDevices on successful connect but only ever removed in stop(). A device that disconnects (or fails to reconnect and closes itself) mid-runtime stays referenced in this set indefinitely, and re-discovered devices keep adding new entries. For a long-running scanner this is an unbounded memory leak.
🔒 Proposed fix: prune on disconnect
this.connectedDevices.add(device);
this.deviceManager.addDevice(device);
+ device.on(DeviceEvent.deviceDisconnected, () => this.connectedDevices.delete(device));
return;Also applies to: 116-134
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/device/provider/bleDeviceProvider.ts` around lines 26 - 28, The
connectedDevices set in BleDeviceProvider is only cleaned up in stop(), so
devices that disconnect mid-runtime remain referenced and the set can grow
without bound. Update the connection/disconnect flow in BleDeviceProvider,
especially the connect handling and any device close/disconnect callbacks around
the affected methods, to remove each BleDevice from connectedDevices as soon as
it disconnects or fails to reconnect. Make sure the prune logic is wired into
the same lifecycle path that adds devices to connectedDevices so rediscovered
devices do not accumulate duplicate stale entries.
Comment on lines
+44
to
+62
| public override async init(): Promise<void> { | ||
| noble.on('discover', asyncHandler( | ||
| this.onDiscover.bind(this), | ||
| (err: unknown) => logError(this.logger, 'Error in discover handler', err) | ||
| )); | ||
|
|
||
| noble.on('stateChange', asyncHandler( | ||
| async (state) => { | ||
| if (state === 'poweredOn') { | ||
| await this.observe(); | ||
| } | ||
| }, | ||
| (err: unknown) => logError(this.logger, 'Error in stateChange handler', err) | ||
| )); | ||
|
|
||
| noble.on('scanStop', () => { this.logger.info('Noble scanning stopped'); }); | ||
|
|
||
| await this.observe(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## File list\n'
git ls-files 'src/device/provider/bleDeviceProvider.ts' 'src/**' | sed -n '1,120p'
printf '\n## Relevant source snippets\n'
for f in src/device/provider/bleDeviceProvider.ts; do
echo "\n### $f"
ast-grep outline "$f" --view expanded || true
done
printf '\n## observe()/init() context\n'
sed -n '1,180p' src/device/provider/bleDeviceProvider.tsRepository: SlvCtrlPlus/slvctrlplus-server
Length of output: 12187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Search for waitForPoweredOnAsync usage/definition\n'
rg -n "waitForPoweredOnAsync|startScanningAsync|stateChange|poweredOn" src . -g '!**/node_modules/**' -g '!**/dist/**' || true
printf '\n## Package references\n'
rg -n '"noble"|noble' package.json pnpm-lock.yaml yarn.lock package-lock.json . -g '!**/node_modules/**' || trueRepository: SlvCtrlPlus/slvctrlplus-server
Length of output: 7981
🌐 Web query:
noble waitForPoweredOnAsync stateChange poweredOn behavior docs implementation
💡 Result:
The waitForPoweredOnAsync method in the noble library (specifically the @stoprocent/noble fork) is designed to simplify asynchronous workflows by waiting for the Bluetooth adapter to reach the poweredOn state before proceeding with operations like scanning or connecting [1][2][3]. Behavior and Implementation Details: - Functionality: It returns a Promise that resolves once the adapter state becomes poweredOn [1][4]. If the adapter is already in the poweredOn state when the method is called, it resolves immediately [5]. - Parameters: The method accepts an optional timeout parameter (in milliseconds) to prevent the operation from hanging indefinitely if the adapter fails to power on [1][6][4]. - State Management: Historically, noble used the stateChange event to manually monitor the adapter's state [7][8][9]. waitForPoweredOnAsync encapsulates this logic internally, removing the need for boilerplate noble.on('stateChange', ...) listeners for simple initialization flows [1][2]. - Reliability: Recent updates to the library have addressed issues where the method might time out incorrectly shortly after initialization or hang if called when the state was already active [10][5]. Usage Example: async function initializeNoble { const noble = require('@stoprocent/noble'); try { // Wait for the adapter to be ready await noble.waitForPoweredOnAsync; // Safe to proceed with scanning await noble.startScanningAsync; } catch (error) { console.error('Failed to initialize or power on:', error); } } The stateChange event remains available as part of the public API for applications that require reactive behavior, such as automatically stopping scans if the adapter is powered off or becomes unavailable during runtime [2][3][11].
Citations:
- 1: https://github.com/stoprocent/noble?tab=readme-ov-file
- 2: https://registry.npmjs.org/@stoprocent/noble
- 3: https://unpkg.com/@stoprocent/noble@2.5.5/README.md
- 4: https://cdn.jsdelivr.net/npm/@stoprocent/noble@2.3.18/index.d.ts
- 5: stoprocent/noble@v1.18.2...v2.3.6
- 6: https://www.npmjs.com/package/@stoprocent/noble
- 7: https://github.com/noble/noble/wiki/Getting-started
- 8: https://www.npmjs.com/package/noble
- 9: https://github.com/sandeepmistry/noble/
- 10: stoprocent/noble@132ec55
- 11: https://www.npmjs.com/package/@abandonware/noble
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,170p' tests/unit/device/provider/bleDeviceProvider.spec.tsRepository: SlvCtrlPlus/slvctrlplus-server
Length of output: 7320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('tests/unit/device/provider/bleDeviceProvider.spec.ts')
for i, line in enumerate(p.read_text().splitlines(), 1):
if 70 <= i <= 130:
print(f"{i:4}: {line}")
PYRepository: SlvCtrlPlus/slvctrlplus-server
Length of output: 2963
TOCTOU race can trigger startScanningAsync twice.
waitForPoweredOnAsync() waits for a later poweredOn transition unless the adapter is already on, so the direct observe() call can still be pending when the stateChange handler re-enters observe(). Set isScanning = true before awaiting, or remove one of the two entry points.
Proposed fix
private async observe(): Promise<void> {
if (this.isScanning) {
return;
}
+ this.isScanning = true;
+
try {
// Wait for Adapter poweredOn state
await noble.waitForPoweredOnAsync();
- this.isScanning = true;
await noble.startScanningAsync([BleDeviceProvider.UART_SERVICE_UUID], true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/device/provider/bleDeviceProvider.ts` around lines 44 - 62, The BLE
initialization flow can call observe() twice, which allows startScanningAsync to
race when init() invokes observe() and the noble.on('stateChange') poweredOn
path re-enters it. Fix this by making BleDeviceProvider.observe() mark
isScanning = true before any await inside the scanning startup path, or by
removing one of the two entry points from init() so only a single path can start
scanning. Keep the guard consistent around observe(), init(), and the
stateChange handler to prevent duplicate scan starts.
Comment on lines
+64
to
+78
| public override async stop(): Promise<void> { | ||
| noble.removeAllListeners(); | ||
|
|
||
| if (this.isScanning) { | ||
| await noble.stopScanningAsync(); | ||
| this.isScanning = false; | ||
| } | ||
|
|
||
| this.logger.debug(`Requesting to acquire device: ${deviceInfo.id}`); | ||
| noble.stop(); | ||
|
|
||
| const acquireResult = await this.deviceManager.acquireDetectedDevice(deviceInfo.id); | ||
| for (const device of this.connectedDevices) { | ||
| await device.close(); | ||
| } | ||
| this.connectedDevices.clear(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
A single failing device.close() aborts the whole shutdown loop.
Unlike DeviceManager.reset(), which catches and logs per-device close errors and continues, this loop has no error handling — one throwing close() call prevents remaining devices from being closed and skips connectedDevices.clear().
🔒 Proposed fix
for (const device of this.connectedDevices) {
- await device.close();
+ try {
+ await device.close();
+ } catch (e: unknown) {
+ logError(this.logger, `Error closing BLE device ${device.deviceId}`, e);
+ }
}
this.connectedDevices.clear();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public override async stop(): Promise<void> { | |
| noble.removeAllListeners(); | |
| if (this.isScanning) { | |
| await noble.stopScanningAsync(); | |
| this.isScanning = false; | |
| } | |
| this.logger.debug(`Requesting to acquire device: ${deviceInfo.id}`); | |
| noble.stop(); | |
| const acquireResult = await this.deviceManager.acquireDetectedDevice(deviceInfo.id); | |
| for (const device of this.connectedDevices) { | |
| await device.close(); | |
| } | |
| this.connectedDevices.clear(); | |
| } | |
| public override async stop(): Promise<void> { | |
| noble.removeAllListeners(); | |
| if (this.isScanning) { | |
| await noble.stopScanningAsync(); | |
| this.isScanning = false; | |
| } | |
| noble.stop(); | |
| for (const device of this.connectedDevices) { | |
| try { | |
| await device.close(); | |
| } catch (e: unknown) { | |
| logError(this.logger, `Error closing BLE device ${device.deviceId}`, e); | |
| } | |
| } | |
| this.connectedDevices.clear(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/device/provider/bleDeviceProvider.ts` around lines 64 - 78, The shutdown
loop in BLEDeviceProvider.stop() stops on the first failing device.close(),
which can leave other devices open and prevent connectedDevices.clear() from
running. Update stop() to handle each connected device independently, similar to
DeviceManager.reset(), by catching and logging errors around each close() call
and continuing through the rest of connectedDevices before clearing the set.
Comment on lines
+56
to
+81
| public override async init(): Promise<void> { | ||
| await this.discoverSerialDevices(); | ||
|
|
||
| this.onUsbEventRef = (): void => { | ||
| this.logger.debug('USB event detected, scanning for serial devices in 1s...'); | ||
|
|
||
| if (this.rescanTimer !== undefined) { | ||
| clearTimeout(this.rescanTimer); | ||
| } | ||
|
|
||
| this.rescanTimer = setTimeout(() => { | ||
| if (this.discoveryInFlight) { | ||
| return; | ||
| } | ||
| this.discoveryInFlight = true; | ||
| this.discoverSerialDevices() | ||
| .catch(e => logError(this.logger, 'Error while scanning for new serial devices', e)) | ||
| .finally(() => { | ||
| this.discoveryInFlight = false; | ||
| }); | ||
| }, 1000); | ||
| }; | ||
|
|
||
| usb.addEventListener('connect', this.onUsbEventRef); | ||
| usb.addEventListener('disconnect', this.onUsbEventRef); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file structure first.
ast-grep outline src/device/provider/serialDeviceProvider.ts --view expanded
# Read the relevant section with line numbers.
cat -n src/device/provider/serialDeviceProvider.ts | sed -n '1,220p'
# Find related discovery/USB event state and any other rescan triggers.
rg -n "discoveryInFlight|rescanTimer|onUsbEventRef|discoverSerialDevices|addEventListener\('connect'|'disconnect'" src/device/provider -nRepository: SlvCtrlPlus/slvctrlplus-server
Length of output: 13827
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find every caller of discoverSerialDevices and any other rescan triggers.
rg -n "discoverSerialDevices\(" src
rg -n "usb\.addEventListener\('connect'|usb\.addEventListener\('disconnect'|rescanTimer|discoveryInFlight" src
# Inspect the rest of the provider for lifecycle or retry logic after line 220.
cat -n src/device/provider/serialDeviceProvider.ts | sed -n '220,320p'Repository: SlvCtrlPlus/slvctrlplus-server
Length of output: 2800
Debounced rescan can be dropped while a discovery is already running. src/device/provider/serialDeviceProvider.ts:66-75 If the timer fires with discoveryInFlight set, it exits without queueing another scan, so a hotplug event that lands during a long discovery can be missed until some later USB event. Re-arm a pending rescan after the current discoverSerialDevices() completes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/device/provider/serialDeviceProvider.ts` around lines 56 - 81, The
debounced USB rescan in SerialDeviceProvider.init() can be lost when
discoveryInFlight is already true, so a hotplug event during an ongoing scan
never triggers a follow-up scan. Update the onUsbEventRef/setTimeout flow to
remember that a rescan is pending when discoverSerialDevices() is busy, and
re-run the scan immediately after the current discoverSerialDevices() completes.
Use the existing discoveryInFlight, rescanTimer, and discoverSerialDevices()
logic to re-arm the deferred scan instead of returning early.
Comment on lines
+102
to
+112
| if (!this.managedPortIds.has(portInfo.serialNumber)) { | ||
| this.managedPortIds.add(portInfo.serialNumber); | ||
| this.logger.debug(`Managed devices: ${this.managedPortIds.size}`); | ||
|
|
||
| const deviceInfo: SerialDeviceInfo = { id: DeviceId.create(portInfo.serialNumber), portInfo }; | ||
|
|
||
| connectAttempts.push( | ||
| this.attemptConnect(deviceInfo) | ||
| .catch((err: unknown) => logError(this.logger, `Error while connecting to serial device '${portInfo.path}'`, err)) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how managedPortIds / attemptConnect interact to confirm retry behavior on failure.
fd serialDeviceProvider.ts --exec sed -n '80,180p' {}Repository: SlvCtrlPlus/slvctrlplus-server
Length of output: 4166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find every use of managedPortIds and the retry/rescan wiring in this provider.
rg -n "managedPortIds|discoverSerialDevices|attemptConnect|rescanTimer|onUsbEventRef" src/device/provider/serialDeviceProvider.ts
# Read the full provider in focused slices to see whether any failure path clears the ID.
sed -n '1,220p' src/device/provider/serialDeviceProvider.tsRepository: SlvCtrlPlus/slvctrlplus-server
Length of output: 10547
Retry failed serial ports on the next scan managedPortIds is still added before attemptConnect, and the only removal path is when the port disappears from SerialPort.list(). A port that returns no device stays marked managed and won’t be retried until it is unplugged/replugged. If transient identification failures are expected, clear the id when attemptConnect yields no device.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/device/provider/serialDeviceProvider.ts` around lines 102 - 112,
`SerialDeviceProvider` is marking ports as managed before `attemptConnect`, so a
port that yields no device stays in `managedPortIds` and won’t be retried on
later scans. Update the connect flow in the `SerialDeviceProvider` scan logic so
that when `attemptConnect` fails to identify a device (or returns no device),
the corresponding port id is removed from `managedPortIds` before exiting the
catch path. Keep the existing `logError` handling, but ensure the retry state is
cleared in the same block that handles failed connection attempts.
Estim2bDeviceFactory injected Settings and DeviceNameGenerator directly but never actually persisted a KnownDevice - it just called nameGenerator.generateName() unconditionally on every connect, giving each reconnect a brand new random name instead of a stable one. Unlike Zc95DeviceFactory, which deliberately skips known-device persistence due to a documented upstream firmware issue with unstable USB serial numbers (CrashOverride85/zc95#151), there's no such reason here - this looks like it was just copy-pasted scaffolding that never got wired up. Fixed to resolve/ persist through KnownDeviceRegistry like SlvCtrlPlusDeviceFactory and AiroticDeviceFactory already do, so EStim2b devices keep a stable name across reconnects.
ButtplugIoDeviceFactory.create(buttplugDevice, knownDevice) already had the right shape: takes an already-resolved KnownDevice as a parameter and has no dependency on KnownDeviceRegistry at all - identity resolution/persistence is the calling provider's job, not the factory construction method's. SlvCtrlPlusDeviceFactory and Estim2bDeviceFactory were inconsistent with this: their public create() methods reached into KnownDeviceRegistry themselves (resolve at the top, persist at the bottom), mixing 'orchestrate the connection attempt' with 'construct the device object' in one method. Restructured both (and AiroticDeviceFactory, extracting a matching create() for symmetry) so that: - tryConnect() is the only place that touches KnownDeviceRegistry - it resolves the identity once the protocol-specific type is known (which, for SlvCtrlPlus, only happens after the 'introduce' handshake completes - hence resolve() couldn't move out to the generic SerialDeviceProvider, which has no protocol-specific knowledge), then persists after create() returns successfully. - create() becomes a pure, synchronous builder that takes the already- resolved KnownDevice as a parameter and has no registry dependency. No behavior change - same resolve-before/persist-after-success guarantee, just cleaner responsibility split matching the Buttplug.io factory/provider pair that already did this correctly.
Settings is wrapped with on-change (see SettingsManager.load()), so any mutating call anywhere in its object graph - including Map.set() on the nested knownDevices map - triggers a full settings.json disk write and a settings-changed WebSocket broadcast to every connected client. persist() was calling settings.addKnownDevice() unconditionally, including for an already-known, completely unchanged identity. That meant every single device connect/reconnect wrote to disk and broadcast a change event, even for a device that's been known for months and never changes - this exact bug exists on main today (SlvCtrlPlusDeviceFactory, AiroticDeviceProvider and ButtplugIoDeviceFactory all unconditionally call settings.addKnownDevice() at the end of their create() methods, regardless of whether the known device lookup returned an existing, unchanged entry). Fixed by skipping the settings.addKnownDevice() call when the given KnownDevice is already the exact stored instance for its id. This is safe because KnownDevice is fully immutable (readonly fields, no setters) and resolve() always returns the same instance for an already-known device, so reference equality is a reliable signal that nothing actually changed.
Reverts commits 2d05929..e9a47b3 (the entire 'merge observer into device provider' effort and the KnownDeviceRegistry work built on top of it). The restructuring didn't lead anywhere useful: splitting device providers into one instance per DeviceSource reintroduced uncoordinated, redundant hardware scanning across different protocols on the same transport - a problem the original shared SerialPortObserver/BleObserver design didn't have. Back to main's original architecture. Device-provider config schema validation (added during this work) will be re-applied on top of it separately, since that part is worth keeping.
Re-applies device-provider config validation on top of main's
original architecture (after reverting the observer-merge/factory
restructuring in the previous commit):
- DeviceProvider<TConfig> is generic again; config is the FIRST
constructor parameter across every provider, since that lets
GenericDeviceProviderFactory capture the remaining ("dependency")
constructor arguments as a plain rest-inferred tuple.
- DeviceProviderFactory<DP> carries a configSchema (TSchema tied to
ConfigOf<DP> via TypeBox's phantom static property), validated by
DeviceProviderManager before a provider is constructed.
- GenericDeviceProviderFactory<DP, TDependencyArgs> now uniformly
handles all 6 providers (previously ButtplugIoWebsocketDeviceProvider
and VirtualDeviceProvider needed their own dedicated factory classes,
since the old generic factory ignored config entirely) - both
dedicated factories are deleted.
- DeviceProviderManager hydrates missing config fields with their
schema's TypeBox "default" (Value.Default) before validating, so
e.g. VirtualDeviceProvider no longer needs its own scanIntervalMs
fallback logic. Uses a JSON round-trip (not structuredClone) to
clone DeviceSource.config first, since Settings is wrapped in an
on-change Proxy that structuredClone can't handle.
- Same config-schema-on-factory pattern applied to
GenericVirtualDeviceFactory/VirtualDeviceLogicFactory, replacing the
old LogicFactoryAndConfigTuple bidirectional-assignability type and
GenericVirtualDeviceLogicFactory's private-constructor-plus-static-
from() indirection with a plain public constructor.
- New NoDeviceProviderConfig (distinct from the existing NoDeviceConfig,
which is for device configs, not provider configs) for the 4
providers that take no configuration.
Full gate green: typecheck, lint, 355/355 tests.
SlvCtrlPlusDeviceFactory, AiroticDeviceProvider, and ButtplugIoDeviceFactory were unconditionally calling settings.addKnownDevice() on every successful device connect, even for an already-known, unchanged device. Since Settings is wrapped with on-change to auto-save to disk, this triggered a full settings.json write plus a settings-changed WebSocket broadcast on every single reconnect, not just for genuinely new devices. Restores KnownDeviceRegistry (resolve()/persist()), previously removed as part of a larger architectural revert earlier in this branch, since it's a self-contained fix unrelated to that restructuring: - resolve() is pure (no side effects) - looks up an existing identity or builds a new, not-yet-persisted one. - persist() is only called once a Device has actually been built successfully, not right after resolve(). If device construction throws in between, the newly-resolved-but-unpersisted KnownDevice is simply discarded instead of leaking a phantom entry into settings.json. - persist() is a no-op (reference-equality check) for an already-known, unchanged identity, which is what actually prevents the spurious writes/broadcasts on reconnect. Zc95 already has known-device persistence disabled intentionally (unstable USB serial numbers across firmware resets), and EStim2b doesn't persist known devices at all on this architecture - neither is affected by this bug. Full gate green: typecheck, lint, 364/364 tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes