Uh oh!
There was an error while loading. Please reload this page.
Add E-Stim Systems 2B support - #64
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds EStim2b protocol/device/factory/provider, introduces a SerialPortFactory and GenericDeviceProviderFactory, refactors serial provider abstractions and provider factories, adjusts device attribute APIs and device/provider wiring, expands logging typings, updates DI registrations, tweaks ESLint/package script, and updates tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User / Discovery
participant Provider as EStim2bSerial<br/>DeviceProvider
participant Transport as Serial<br/>Transport
participant Protocol as EStim2b<br/>Protocol
participant Factory as Estim2b<br/>DeviceFactory
participant Device as EStim2b<br/>Device
User->>Provider: connectSerialDevice(port, portInfo)
activate Provider
Provider->>Transport: wrap port (Readline + Synchronous wrapper)
Provider->>Protocol: new EStim2bProtocol(transport)
Provider->>Protocol: requestStatus()
activate Protocol
Protocol->>Transport: send(status request)
Transport-->>Protocol: raw status
Protocol-->>Provider: parsed EStim2bStatus
deactivate Protocol
Provider->>Factory: create(protocol, initialStatus, providerName)
activate Factory
Factory->>Device: new EStim2bDevice(..., attributes)
Device-->>Factory: constructed device
deactivate Factory
Provider->>Device: initDeviceStatusUpdater()
Provider-->>User: emit deviceConnected
deactivate Provider
User->>Device: setAttribute(mode/level/...), value
activate Device
Device->>Protocol: setMode/setPower/...
Protocol->>Transport: send(command)
Transport-->>Protocol: response status
Protocol-->>Device: updated status
Device->>Device: updateAttributeValues(status)
deactivate Device
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
heavyrubberslave
marked this pull request as ready for review
January 18, 2026 16:31
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/device/protocol/zc95/zc95Device.ts (1)
84-112: Add range validation for power-channel updates.Line 98 accepts any number and forwards it to the device without checking the attribute’s valid range, which can push out‑of‑range values and break state consistency. Suggest validating via the attribute’s
isValidValuebefore sending.🐛 Suggested fix
- if (this.isPowerChannelAttribute(attributeName) && typeof value === 'number') {- await this.setAttributePowerChannel(attributeName, value);- return value;- }+ if (this.isPowerChannelAttribute(attributeName) && typeof value === 'number') {+ const attr = this.attributes[attributeName];+ if (!attr || !attr.isValidValue(value)) {+ throw new Error(+ `Could not set value ${JSON.stringify(value)} (type: ${typeof value}) for attribute '${attributeName}'`+ );+ }+ await this.setAttributePowerChannel(attributeName, value);+ return value;+ }src/device/transport/serialPortObserver.ts (1)
66-72: ConfirmconnectToDevicereturns a strict boolean before relying on truthiness.Line 70 now accepts any truthy value; if any provider returns an object/string by mistake, discovery will stop early. If the contract is boolean, consider keeping the strict check (or enforce the return type at the interface).
💡 Suggested fix
- if (result) {+ if (result === true) { break; }Please verify the return types in the interface and implementations:
#!/bin/bash# Locate connectToDevice definitions and usages rg -n -C3 --type=ts '\bconnectToDevice\s*\('
🤖 Fix all issues with AI agents
In `@package.json`:
- Line 17: The dependency modern-errors is pinned to a vulnerable version
(7.1.4); update the version string to at least 7.4.1 in package.json (replace
"modern-errors": "7.1.4" with "modern-errors": ">=7.4.1" or the exact "7.4.1"),
then regenerate the lockfile by running your package manager install (npm
install or yarn install) to update package-lock.json / yarn.lock and run the
test suite; ensure no other code relies on breaking changes in modern-errors and
adjust imports/usages only if tests or linting fail.
In `@src/device/protocol/estim2b/estim2bDevice.ts`:
- Around line 205-227: The IntRangeDeviceAttribute max values in
createPulsePwmAttribute and createPulseFrequencyAttribute exceed the protocol
limit (currently Int.from(100)); update both calls to use Int.from(99) as the
upper bound so the attribute ranges match the protocol setters, i.e., change the
IntRangeDeviceAttribute.createInitialized max argument for 'pulsePwm' and
'pulseFrequency' from 100 to 99.
In `@src/device/protocol/estim2b/estim2bDeviceFactory.ts`:
- Around line 76-118: pulseFrequency and pulsePwm are incorrectly initialized
from channelALevel/channelBLevel and use a max of 100 while protocol setters cap
at 99; update the IntRangeDeviceAttribute.createInitialized calls for
pulseFrequency and pulsePwm to use the correct initialStatus fields (e.g.,
initialStatus.pulseFrequency and initialStatus.pulsePwm) and make the upper
bound Int.from(99) to match the protocol setters (or, if 100 is intended, update
the protocol setter bounds instead) so the initial values and UI ranges are
consistent with the device protocol.
In `@src/device/protocol/estim2b/estim2bSerialDeviceProvider.ts`:
- Around line 54-67: The code mistakenly uses the method reference
device.getDeviceId instead of calling it, causing Map key collisions and wrong
log output; update every usage in estim2bSerialDeviceProvider (e.g.,
this.connectedDevices.set(device.getDeviceId, device),
this.connectedDevices.delete(device.getDeviceId), logger.debug(`Assigned device
id: ${device.getDeviceId} ...`), logger.info('Lost serial device: ' +
device.getDeviceId), and the connected devices size logs) to call the method
(device.getDeviceId()) so the actual ID string is used as the Map key and in
logs/events.
🧹 Nitpick comments (6)
src/types.d.ts (1)
14-17: Allow readonly tuples in event maps to avoidneverlisteners.If event maps are inferred via
as const,T[K]becomes a readonly tuple and failsunknown[], makingListenerresolve tonever. Consider accepting readonly arrays to keep inference usable.♻️ Suggested tweak
-type Listener<K, T> = T extends DefaultEventMap ? (...args: any[]) => void : (- K extends keyof T ? (- T[K] extends unknown[] ? (...args: T[K]) => void : never- )- : never- );+type Listener<K, T> = T extends DefaultEventMap ? (...args: any[]) => void : (+ K extends keyof T ? (+ T[K] extends readonly unknown[] ? (...args: T[K]) => void : never+ )+ : never+ );src/device/device.ts (1)
54-56: Consider keepingattributesreadonly unless full reassignment is required.readonlystill allows value mutations (e.g.,this.attributes.foo.value = …) while preventing wholesale map replacement. If reassignment isn’t needed, retainingreadonlypreserves invariants.♻️ Optional revision (if reassignment isn’t required)
- protected attributes: TAttributes;+ protected readonly attributes: TAttributes;src/logging/Logger.ts (1)
9-20:ErrorContext | unknowncollapses tounknown; the new type adds no type safety.In TypeScript,
unknownis the top type—it absorbs any union. If the goal is to enforce stricter typing for error contexts, removeunknownfrom the union (or add overloads for specific cases).♻️ Suggested tightening
- error(msg: string, context?: ErrorContext | unknown): void;+ error(msg: string, context?: ErrorContext): void;src/device/protocol/estim2b/estim2bProtocol.ts (1)
135-154: Harden response parsing (trim + NaN checks).Consider trimming the response and validating numeric parts to fail fast on malformed frames.
♻️ Suggested hardening
- private static parseResponse(response: string): EStim2bStatus {- const parts = response.split(':');+ private static parseResponse(response: string): EStim2bStatus {+ const parts = response.trim().split(':'); if (9 !== parts.length) { throw new Error( `Could not parse status message of 2B device: expected 9 parts, got ${parts.length} (${response})` ); } + const nums = parts.slice(0, 7).map((p) => Number.parseInt(p, 10));+ if (nums.some((n) => Number.isNaN(n))) {+ throw new Error(`Could not parse numeric fields in status message: ${response}`);+ }+ return { - batteryLevel: parseInt(parts[0], 10),- channelALevel: parseInt(parts[1], 10)/2,- channelBLevel: parseInt(parts[2], 10)/2,- pulseFrequency: parseInt(parts[3], 10)/2,- pulsePwm: parseInt(parts[4], 10)/2,- currentMode: parseInt(parts[5], 10),- powerMode: parts[6],- channelsJoined: parseInt(parts[7], 10) === 1,- firmwareVersion: parts[8]+ batteryLevel: nums[0],+ channelALevel: nums[1]/2,+ channelBLevel: nums[2]/2,+ pulseFrequency: nums[3]/2,+ pulsePwm: nums[4]/2,+ currentMode: nums[5],+ powerMode: parts[6],+ channelsJoined: nums[6] === 1,+ firmwareVersion: parts[8].trim() }; }src/device/protocol/zc95/zc95SerialDeviceProvider.ts (1)
32-75: Well-structured device connection lifecycle management.The
connectSerialDeviceimplementation properly:
- Resets the device before querying version details
- Validates the response before proceeding
- Sets up the device status updater
- Cleans up resources (interval, map entry) in the
closehandlerOne minor nit on line 33:
- const serialLogger = this.logger.child({ name: Zc95Serial.name })+ const serialLogger = this.logger.child({ name: Zc95Serial.name });src/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.ts (1)
112-130: Consider adding a timeout for the Arduino ready byte wait.If the Arduino device never sends the ready byte (e.g., firmware issue, device stuck in reset loop), the promise returned by
preparePortwill hang indefinitely. This could cause connection attempts to stall without clear error feedback.♻️ Proposed fix with timeout
protected preparePort(port: SerialPort, portInfo: PortInfo): Promise<void> { return new Promise<void>(resolve => { + const READY_TIMEOUT_MS = 5000;+ if (portInfo.vendorId !== SlvCtrlPlusSerialDeviceProvider.arduinoVendorId) { // It's NOT an Arduino resolve(); return; } const readyParser = port.pipe(new ReadyParser({ delimiter: [SlvCtrlPlusSerialDeviceProvider.moduleReadyByte] })); + const timeout = setTimeout(() => {+ port.unpipe(readyParser);+ readyParser.destroy();+ this.logger.warn('Arduino ready byte timeout - proceeding anyway');+ resolve();+ }, READY_TIMEOUT_MS);+ readyParser.once('ready', () => { + clearTimeout(timeout); port.unpipe(readyParser); readyParser.destroy(); resolve(); }); }); }
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.ts`:
- Around line 112-130: preparePort currently waits indefinitely for the
ReadyParser to emit the moduleReadyByte which can hang connectToDevice; add a
timeout (e.g. 3s) inside SlvCtrlPlusSerialDeviceProvider.preparePort that, when
elapsed, cleans up readyParser (unpipe + destroy), removes the 'ready' listener,
clears the timer, and resolves the promise so the higher-level connectToDevice
can continue; also ensure the timer is cleared if readyParser.once('ready', ...)
fires and that the non-Arduino early-return behavior remains unchanged.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/serial/SynchronousSerialPort.ts`:
- Around line 80-81: The return line in SynchronousSerialPort incorrectly
asserts the awaited value as Promise<string>; since await unwraps the promise
the result is a string. Update the return from the push call on this.queue (call
site: this.queue.push(wrappedPromise, options)) to remove the "as
Promise<string>" cast (or change it to assert a string if necessary) so the
async function returns the correct string type without wrapping a Promise twice.
🧹 Nitpick comments (1)
src/serial/SynchronousSerialPort.ts (1)
51-56: Redundant null check.
removeListenersis always defined (initialized to an empty function on line 41), so theundefined !== removeListenerscheck is always true.Proposed simplification
const dataHandler = (receivedData: string): void => { - if (undefined !== removeListeners) {- removeListeners();- }+ removeListeners(); resolve(receivedData); };
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.ts`:
- Around line 84-96: parseDeviceInfo currently assumes
introductionResult.split(';') yields parts[1]; if the input lacks the ';'
delimiter parts[1] is undefined and parts[1].split(',') throws. Update
parseDeviceInfo to guard after splitting by ';' — verify parts.length > 1 (and
that parts[0] === 'introduce') before accessing parts[1]; only then split
parts[1] into deviceInfoParts and continue validation (deviceInfoParts.length
=== 3). Ensure the function returns undefined for malformed inputs instead of
throwing.
- Around line 112-137: The Promise constructor in preparePort has an extra
opening parenthesis causing a syntax error; update new Promise<void>(((resolve,
reject) => { ... }) to use a single parenthesis around the executor (new
Promise<void>((resolve, reject) => { ... })) and ensure the matching closing
parentheses/braces remain balanced in
SlvCtrlPlusSerialDeviceProvider.preparePort so the function compiles.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.ts`:
- Around line 112-137: preparePort can reject (e.g., on the ready-byte timeout)
which currently leads to unhandled rejections and leaked open ports; wrap the
call to SlvCtrlPlusSerialDeviceProvider.preparePort(...) in a try-catch
(wherever you call it) and on catch ensure the SerialPort instance (port) is
explicitly closed/destroyed (e.g., port.close() or port.destroy()) before
rethrowing or returning an error; alternatively, modify the caller to await
preparePort inside a promise chain that on rejection always closes the port—this
ensures any timeout rejection from readyParser/preparePort does not leave the
port open.
♻️ Duplicate comments (1)
src/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.ts (1)
84-105: Guard against missing;in introduction payload.parts[1]can be undefined when the delimiter is absent, causing a throw before returningundefined.🔧 Suggested hardening
protected parseDeviceInfo(introductionResult: string): DeviceInfo | undefined { const parts = introductionResult.split(';'); - if ('introduce' !== parts[0]) {+ if ('introduce' !== parts[0] || parts.length < 2) { return undefined; } const deviceInfoParts = parts[1].split(',');
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/device/provider/serialDeviceProvider.ts`:
- Line 37: There's a trailing whitespace in the serialDeviceProvider module —
remove the stray space character at the end of the offending line in
src/device/provider/serialDeviceProvider.ts so the file contains no trailing
spaces; edit the SerialDeviceProvider (or the top-level export in that file) and
save the file, then run ESLint/format to confirm the trailing whitespace is
gone.
- Around line 35-42: The preparePort call is happening before the serial port is
opened, so the ReadyParser (used in preparePort for Arduino vendor ID '2341')
never receives data when ports are created with autoOpen: false; move the
port.open(...) promise to execute before preparePort(port, portInfo) to ensure
the port is open when preparePort attaches the ReadyParser, then call result =
await this.connectSerialDevice(port, portInfo) as before.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Mar 4, 2026
Closed
Closed
This was referenced Jul 15, 2026
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 freeto 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
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.