Refactor servo packet handling to reuse buffers#348
Refactor servo packet handling to reuse buffers#348meganetaaan wants to merge 1 commit intodev/v1.0from
Conversation
WalkthroughIntroduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant PacketHandler
participant PayloadBuffer
participant Callback
Note over PacketHandler,PayloadBuffer: New Flow: Buffer-based Payloads
Caller->>PacketHandler: onPacketReceived(frame)
PacketHandler->>PayloadBuffer: copyFrom(frame, length)
PayloadBuffer-->>PacketHandler: Uint8Array view
PacketHandler->>Callback: callback(buffer, length)
Callback->>Caller: process binary payload
rect rgb(240, 248, 255)
Note over PacketHandler,Callback: Replaces old flow: numeric array callbacks
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
firmware/stackchan/drivers/dynamixel.ts (1)
487-498:readOffsetAnglesign and payload decoding are inconsistent with other readsTwo problems here:
values[0] & 0x8000can never be non-zero becausevalues[0]is a single byte (0–255); the sign bit is in the combined 16‑bit value, not in a standalone 8‑bit value.- Like
readModelNumber, this method appears to treatvalues[0]/values[1]as data, but elsewherevalues[1]is the status byte and data starts atvalues[2].A safer implementation that follows the same layout as the other reads would be:
async readOffsetAngle(): Promise<number> { const values = await this.#sendCommand(INSTRUCTION.READ, ADDRESS.HOMING_OFFSET, 2) - if (values == null || values.length < 2) { - throw new Error('failed to read offset angle') - } - const isCcw = Boolean(values[0] & 0x8000) - let offset = ((values[1] & 0x7fff) << 8) | values[0] + if (values == null || values.length < 4) { + throw new Error('failed to read offset angle') + } + if (values[1] !== 0) { + throw new Error(`servo returned error code: ${values[1]}`) + } + const raw = values[2] | (values[3] << 8) + const isCcw = Boolean(raw & 0x8000) + let offset = raw & 0x7fff if (isCcw) { offset *= -1 } return offset }This both fixes the impossible 0x8000 mask on a byte and aligns index usage with the known status/data layout.
firmware/stackchan/drivers/scservo.ts (1)
249-257:readOffsetAngledoesn’t handle timeouts and uses an impossible bit maskTwo issues here:
#sendCommandnow resolves toUint8Array | undefined, butreadOffsetAngleassumes a non‑undefined array. On timeout,valueswill beundefinedandvalues[0]will throw.- As in the Dynamixel case,
values[0] & 0x8000can never be non‑zero becausevalues[0]is a byte; the sign bit lives in the combined 16‑bit value.A safer implementation that matches how you encode the offset (
(Number(isCcw) << 15) | (a & 0x7fff)and send viale) would be:async readOffsetAngle(): Promise<number> { const values = await this.#sendCommand(COMMAND.READ, ADDRESS.OFFSET, 2) - const isCcw = Boolean(values[0] & 0x8000) - let offset = ((values[0] & 0x7fff) << 8) | values[1] + if (values == null || values.length < 2) { + throw new Error('failed to read offset angle') + } + const raw = (values[0] << 8) | values[1] + const isCcw = Boolean(raw & 0x8000) + let offset = raw & 0x7fff if (isCcw) { offset *= -1 } return offset }This adds the missing timeout/length guard and correctly interprets the sign bit from the combined 16‑bit value.
🧹 Nitpick comments (5)
firmware/package.json (1)
15-16: Make test script more portable and confirm Node test runner supportThe test flow itself looks good (clean → compile with
tsconfig.test.json→node --testondist-tests/tests/unit). Two minor concerns:
rm -rf dist-testsis POSIX-specific and will break on Windows shells. If cross‑platform support matters, consider using a Node-based cleanup (node -e "import('node:fs/promises').then(fs => fs.rm('dist-tests',{recursive:true,force:true}))") or a tool likerimraf.- Ensure your CI/dev environment runs on a Node version that supports
node --testwith ESM (and theNodeNextmodule kind you’ve configured).firmware/tsconfig.test.json (1)
1-16: Test tsconfig wiring looks consistent (small optional cleanup)The test config matches the new test pipeline: it compiles
payload-buffer.tsplustests/unit/**/*.tsintodist-testswith Node/ESM settings and Node types, which fits thenode --test dist-tests/tests/unitinvocation.If you want to trim noise, you can drop the explicit
"exclude": []since it’s redundant when"include"is already scoped.firmware/stackchan/drivers/payload-buffer.ts (1)
1-20: Consider usingUint8Array#setfor clearer and possibly faster copiesThe current implementation is correct and matches the tests. If you’d like to simplify and lean on built-in typed-array ops, you could implement
copyFromlike this:- copyFrom(source: Uint8Array, length: number, offset = 0): Uint8Array { - this.ensureCapacity(length) - for (let i = 0; i < length; i++) { - this.#buffer[i] = source[offset + i] - } - return this.#buffer.subarray(0, length) - } + copyFrom(source: Uint8Array, length: number, offset = 0): Uint8Array { + this.ensureCapacity(length) + this.#buffer.set(source.subarray(offset, offset + length), 0) + return this.#buffer.subarray(0, length) + }Behavior is the same as long as callers pass valid
offset/length, but the intent is a bit clearer and avoids the manual loop.firmware/stackchan/drivers/rs30x.ts (1)
67-79: Align checksum helper documentation and naming with new signature
checksumnow operates over aUint8Arrayslice defined by[start, end):function checksum(buffer: Uint8Array, start: number, end: number): number { let sum = 0 for (let i = start; i < end; i++) { sum ^= buffer[i] } return sum }and is called as:
const cs = checksum(rxBuf, 2, this.#idx - 1) & 0xff this.#txBuf[idx] = checksum(this.#txBuf, 2, idx)The existing comment still refers to a “packet array except checksum”. For future readers, consider updating the JSDoc to describe the
(buffer, start, endExclusive)parameters explicitly and maybe renameend→endExclusiveto make the half-open range clear.Also applies to: 232-233
firmware/stackchan/drivers/scservo.ts (1)
5-6: PacketHandler + checksum refactor looks correct
- The new
(buffer: Uint8Array, length: number)callback shape and#payloadBufferintegration avoid per-message allocations while still decoupling from the RX buffer.checksum(buffer, length)is used consistently:
- On RX:
checksum(rxBuf, this.#idx - 1)sums bytes[2 .. this.#idx - 2]and compares against the last byte (rxBuf[this.#idx - 1]).- On TX:
checksum(this.#txBuf, idx)sums[2 .. idx - 1]and writes the one‑byte checksum atidx.- The send path with
Promise<Uint8Array | undefined>and timeout handling mirrors the Dynamixel pattern and is sound.Only nit: the commented trace line still refers to
arrinstead ofbuffer, but that’s cosmetic.Also applies to: 63-145, 152-160, 208-234
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
firmware/.gitignore(1 hunks)firmware/package.json(1 hunks)firmware/stackchan/drivers/dynamixel.ts(13 hunks)firmware/stackchan/drivers/payload-buffer.ts(1 hunks)firmware/stackchan/drivers/rs30x.ts(8 hunks)firmware/stackchan/drivers/scservo.ts(8 hunks)firmware/tests/unit/payload-buffer.test.ts(1 hunks)firmware/tsconfig.test.json(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-06-19T01:00:25.501Z
Learnt from: meganetaaan
Repo: stack-chan/stack-chan PR: 330
File: firmware/tests/services/mcp-client-service/main.ts:72-77
Timestamp: 2025-06-19T01:00:25.501Z
Learning: In the stack-chan project, scripts under the tests directory are not intended for automated testing but serve as manual testing/development tools. They don't implement explicit cleanup, and this will be considered when implementing proper automated testing in the future.
Applied to files:
firmware/.gitignorefirmware/package.json
📚 Learning: 2025-06-19T01:00:25.789Z
Learnt from: meganetaaan
Repo: stack-chan/stack-chan PR: 330
File: firmware/tests/services/mcp-client-service/main.ts:72-77
Timestamp: 2025-06-19T01:00:25.789Z
Learning: In the stack-chan project, scripts under the tests/ directory are not intended for automated testing but rather for manual testing and demonstration purposes. These scripts do not perform explicit cleanup operations, which is intentional for their current use case. Cleanup considerations will be addressed collectively when moving towards automated testing.
Applied to files:
firmware/.gitignorefirmware/package.json
📚 Learning: 2024-10-05T16:15:44.408Z
Learnt from: meganetaaan
Repo: stack-chan/stack-chan PR: 298
File: firmware/stackchan/drivers/dynamixel.ts:451-451
Timestamp: 2024-10-05T16:15:44.408Z
Learning: `dynamixel.ts`の`readFirmwareVersion()`メソッドでは、`values[1]`がステータスコードを表し、`values[1] === 0`がコマンド成功を意味する。
Applied to files:
firmware/stackchan/drivers/dynamixel.ts
📚 Learning: 2025-06-04T16:04:50.265Z
Learnt from: meganetaaan
Repo: stack-chan/stack-chan PR: 323
File: firmware/stackchan/services/mcp-server/mcp-server.ts:222-253
Timestamp: 2025-06-04T16:04:50.265Z
Learning: In the MCP server implementation (firmware/stackchan/services/mcp-server/mcp-server.ts), the user prefers to keep the message handler code minimal and simple, with validation logic integrated at the Tool definition level rather than in the message handler itself.
Applied to files:
firmware/stackchan/drivers/scservo.ts
🧬 Code graph analysis (5)
firmware/stackchan/drivers/payload-buffer.ts (1)
firmware/stackchan/ble/beacon-packet.ts (4)
BeaconDataPacket(16-102)payload(99-101)major(81-83)constructor(26-35)
firmware/tests/unit/payload-buffer.test.ts (1)
firmware/stackchan/drivers/payload-buffer.ts (1)
PayloadBuffer(1-21)
firmware/stackchan/drivers/dynamixel.ts (2)
firmware/stackchan/drivers/payload-buffer.ts (1)
PayloadBuffer(1-21)firmware/stackchan/drivers/scservo.ts (2)
command(208-234)id(204-206)
firmware/stackchan/drivers/rs30x.ts (1)
firmware/stackchan/drivers/payload-buffer.ts (1)
PayloadBuffer(1-21)
firmware/stackchan/drivers/scservo.ts (1)
firmware/stackchan/drivers/payload-buffer.ts (1)
PayloadBuffer(1-21)
⏰ 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: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (6)
firmware/.gitignore (1)
5-6: Ignore dist and dist-tests artifacts under firmwareAdding
/dist/and/dist-tests/here is consistent with the new build/test outputs and helps keep generated files out of version control. No issues spotted.firmware/tests/unit/payload-buffer.test.ts (1)
1-32: Good coverage of PayloadBuffer reuse and growth semanticsThe tests accurately exercise
copyFromunder both “enough capacity” and “needs to grow once” scenarios, including buffer identity checks and offset handling. They align well with the intended behavior ofPayloadBuffer.firmware/stackchan/drivers/rs30x.ts (1)
89-91: PayloadBuffer integration in RS30X/PacketHandler looks sound (verify external usages of response buffers)The refactor to
(buffer: Uint8Array, length: number)callbacks plusPayloadBufferreuse in bothPacketHandlerandRS30Xlooks consistent:
- RX path computes
payloadLength = this.#idx - 8and copies fromrxBufoffset 7, which correctly skips header/metadata and excludes the checksum.PacketHandleruses a singlePayloadBuffer(32)and reuses it across packets; RS30X then copies into its own#responseBuffer, so callers ofRS30Xsee a buffer that’s independent of the RX buffer but still reused per instance.- The promise queue + timeout handling in
#sendCommandremains ordered and matches the new callback signature.Given that responses now come from a reusable buffer (
#responseBuffer), any external code that stores the returnedUint8Arraybeyond the lifetime of a single command will see its contents change on subsequent commands. Within this file,readStatus()consumes the response synchronously and doesn’t leak it, so it’s safe. Please double-check other call sites to ensure they don’t assume responses are immutable per call.Also applies to: 126-138, 156-159, 165-166, 177-195, 223-251, 308-317
firmware/stackchan/drivers/dynamixel.ts (2)
5-6: PayloadBuffer wiring and promise/timeout flow look solid
- PacketHandler’s switch to
(buffer: Uint8Array, length: number)callbacks with an internal#payloadBufferand Dynamixel’s own#responseBufferkeeps allocations down while avoiding reuse of the shared RX buffer.- The
#promisesqueue,Timer.set/Timer.clear, and#onCommandReadresolution logic correctly handle timeout vs. response and avoid double-resolving or leaking entries.No issues from a correctness or reliability perspective; the refactor is a net improvement.
Also applies to: 90-182, 210-253, 261-315
467-481: Read-side guards and error handling are consistent and reasonable
readFirmwareVersion,readPresentCurrent,readPresentVelocity, andreadPresentPositionall now:
- Guard against
undefinedresponses from#sendCommand.- Enforce a minimum payload length before indexing.
- Use
values[1]as the status/error byte and return structuredMayberesults.This matches the established convention for Dynamixel status packets and makes the read APIs more robust against truncated or missing responses.
Also applies to: 504-524, 531-549, 555-573
firmware/stackchan/drivers/scservo.ts (1)
351-364:readStatusguard + payload usage look good
- The new
values == null || values.length < 15guard correctly protects against timeouts and truncated responses from#sendCommand.- With PacketHandler slicing payload from index 5, treating
values[0]/[1]as the low/high bytes of the position word is consistent with the encoding used elsewhere in this driver.No additional issues here.
| async readModelNumber(): Promise<number> { | ||
| const values = await this.#sendCommand(INSTRUCTION.READ, ADDRESS.MODEL_NUMBER, 2) | ||
| if (values == null || values.length < 2) { | ||
| throw new Error('failed to read model number') | ||
| } | ||
| return el(values[0], values[1]) | ||
| } |
There was a problem hiding this comment.
readModelNumber is likely reading the wrong bytes
Given readFirmwareVersion and the present-value readers treat values[1] as the status code and values[2+] as actual data, using values[0]/values[1] as the model-number bytes will include the instruction and status instead of the payload. This will produce an incorrect model number whenever the call succeeds.
Consider tightening the length check and decoding from the data bytes after the status, e.g.:
async readModelNumber(): Promise<number> {
const values = await this.#sendCommand(INSTRUCTION.READ, ADDRESS.MODEL_NUMBER, 2)
- if (values == null || values.length < 2) {
- throw new Error('failed to read model number')
- }
- return el(values[0], values[1])
+ if (values == null || values.length < 4) {
+ throw new Error('failed to read model number')
+ }
+ if (values[1] !== 0) {
+ throw new Error(`servo returned error code: ${values[1]}`)
+ }
+ const low = values[2]
+ const high = values[3]
+ return (high << 8) | low
}This matches the “values[1] is status, payload starts at index 2” convention used elsewhere.
📝 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.
| async readModelNumber(): Promise<number> { | |
| const values = await this.#sendCommand(INSTRUCTION.READ, ADDRESS.MODEL_NUMBER, 2) | |
| if (values == null || values.length < 2) { | |
| throw new Error('failed to read model number') | |
| } | |
| return el(values[0], values[1]) | |
| } | |
| async readModelNumber(): Promise<number> { | |
| const values = await this.#sendCommand(INSTRUCTION.READ, ADDRESS.MODEL_NUMBER, 2) | |
| if (values == null || values.length < 4) { | |
| throw new Error('failed to read model number') | |
| } | |
| if (values[1] !== 0) { | |
| throw new Error(`servo returned error code: ${values[1]}`) | |
| } | |
| const low = values[2] | |
| const high = values[3] | |
| return (high << 8) | low | |
| } |
🤖 Prompt for AI Agents
In firmware/stackchan/drivers/dynamixel.ts around lines 455 to 461,
readModelNumber currently decodes bytes at values[0]/values[1] which include the
instruction/status bytes; tighten the length check to require at least 4 bytes
(instruction + status + 2 payload bytes) and decode the model number from the
payload at values[2] and values[3] (e.g. use el(values[2], values[3])); keep the
existing error throw when the response is too short.
|
#352 で管理します |
1 participant
Summary
Testing
Codex Task
Summary by CodeRabbit
New Features
Tests
Chores