Skip to content

fix: pitchBendを符号付き16bitで読み取る - #70

Merged
9c5s merged 1 commit into
mainfrom
fix/pitch-bend-signed
Apr 11, 2026
Merged

9c5s merged 1 commit into
mainfrom
fix/pitch-bend-signed

Conversation

@9c5s

@9c5s 9c5s commented Apr 11, 2026

Copy link
Copy Markdown
Owner

Summary

  • MetricsパケットのpitchBendフィールドをreadUInt16LEからreadInt16LEに変更
  • 負のピッチ値 (速度低下方向) が正しく負の数値として返されるようにする

問題

pitchBendが符号なし16bitで読み取られていたため、負のピッチ値が不正な大きな正の値として返されていた。

例: ピッチ-10.00%のとき、期待値-1000に対して64536が返されていた。

speed pitch (旧) pitch (修正後)
0 555.36% -100.00%
90 645.36% -10.00%
100 0.00% 0.00%
110 10.00% 10.00%

Test plan

  • 既存テストの期待値を符号付きに修正
  • 正値・負値・境界値 (0, ±600, 32767, -32768) のパラメタライズドテストで検証
  • 278テスト全PASS

readUInt16LEをreadInt16LEに変更し、負のピッチ値が
65536加算された不正な値として返される問題を修正する
@changeset-bot

changeset-bot Bot commented Apr 11, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 275296e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@9c5s/node-tcnet Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coderabbitai

coderabbitai Bot commented Apr 11, 2026 •

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: e67a8308-8843-49ae-aa80-f5051c79dac3

📥 Commits

Reviewing files that changed from the base of the PR and between 981d903 and 275296e.

📒 Files selected for processing (3)
  • .changeset/fix-pitch-bend-signed.md
  • src/network.ts
  • tests/packet-parsers.test.ts
📜 Recent review details
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: 9c5s
Repo: 9c5s/node-tcnet PR: 2
File: src/tcnet.ts:249-255
Timestamp: 2026-03-21T05:30:31.035Z
Learning: In `src/network.ts` of the `node-tcnet` repository: `TCNetDataPacket.read()` (class defined at line 321, read() at line 330) converts the wire-format 1-based layer to 0-based by doing `this.layer = this.buffer.readUInt8(25) - 1`. In contrast, `TCNetRequestPacket.read()` (class defined at line 225, read() at line 231) does NOT subtract 1: `this.layer = this.buffer.readUInt8(25)`. Therefore, `dataPacket.layer` from a received `TCNetDataPacket` is already 0-based and correctly matches the 0-based key stored in `requestData()`. Flagging a missing `-1` in `TCNetDataPacket.read()` is a false positive.
Learnt from: 9c5s
Repo: 9c5s/node-tcnet PR: 64
File: src/tcnet.ts:31-35
Timestamp: 2026-04-09T16:28:34.323Z
Learning: In node-tcnet, the changeset bump type is kept as `patch` even for breaking changes (removal of `autoReauth`/`reauthInterval`, `reauthenticated`/`reauthFailed` events, `AuthState` `"refreshing"`, addition of `TCNetLogger.warn`) because the library is pre-release (0.x semver), and the maintainer (9c5s) considers strict semver minor/major bumps unnecessary at this stage.
Learnt from: 9c5s
Repo: 9c5s/node-tcnet PR: 64
File: src/tcnet.ts:961-966
Timestamp: 2026-04-09T17:09:02.148Z
Learning: In `src/tcnet.ts` (node-tcnet, TypeScript), the TOCTOU race condition in `performBridgeOsDetection` (where an in-flight ping could overwrite `bridgeIsWindows` with a stale result after a Bridge switch) is resolved by capturing `bridgeIp = this.server?.address` at detection start and checking `this.server?.address === bridgeIp` immediately before writing to `this.bridgeIsWindows`. The maintainer (9c5s) deliberately rejected clearing `bridgeOsDetectionPromise` in `resetAuthSession` (doesn't stop internal execution) and a generation-counter approach (triggers on same-Bridge resets too) in favor of this simpler address-comparison guard.
Learnt from: 9c5s
Repo: 9c5s/node-tcnet PR: 41
File: src/utils.ts:94-94
Timestamp: 2026-03-24T19:15:40.455Z
Learning: In the `node-tcnet` repository (`src/utils.ts`), the project targets ES2022 + NodeNext and does not need to support Node.js 18.0.0. The temporary numeric `family` field (4/6) in `os.networkInterfaces()` introduced in Node.js 18.0.0 and fixed in 18.1.0 does not need to be handled. Suggesting numeric `family` handling for defensive programming is unnecessary for this project.
📚 Learning: 2026-04-09T16:28:27.476Z
Learnt from: 9c5s
Repo: 9c5s/node-tcnet PR: 64
File: src/tcnet.ts:31-35
Timestamp: 2026-04-09T16:28:27.476Z
Learning: In this pre-release (0.x) node-tcnet repo, when writing Changesets, keep the changeset bump type as `patch` even for changes that would normally be considered breaking (e.g., removing `autoReauth`/`reauthInterval`, changing/ removing `reauthenticated`/`reauthFailed` events, altering `AuthState` to include/handle `"refreshing"`, or adding `TCNetLogger.warn`). The project’s maintainer policy treats strict semver minor/major bumps as unnecessary during 0.x development.

Applied to files:

  • .changeset/fix-pitch-bend-signed.md
📚 Learning: 2026-03-21T05:30:31.035Z
Learnt from: 9c5s
Repo: 9c5s/node-tcnet PR: 2
File: src/tcnet.ts:249-255
Timestamp: 2026-03-21T05:30:31.035Z
Learning: In `src/network.ts` of the `node-tcnet` repository: `TCNetDataPacket.read()` (class defined at line 321, read() at line 330) converts the wire-format 1-based layer to 0-based by doing `this.layer = this.buffer.readUInt8(25) - 1`. In contrast, `TCNetRequestPacket.read()` (class defined at line 225, read() at line 231) does NOT subtract 1: `this.layer = this.buffer.readUInt8(25)`. Therefore, `dataPacket.layer` from a received `TCNetDataPacket` is already 0-based and correctly matches the 0-based key stored in `requestData()`. Flagging a missing `-1` in `TCNetDataPacket.read()` is a false positive.

Applied to files:

  • .changeset/fix-pitch-bend-signed.md
  • src/network.ts
  • tests/packet-parsers.test.ts
📚 Learning: 2026-03-21T11:31:23.742Z
Learnt from: 9c5s
Repo: 9c5s/node-tcnet PR: 8
File: README.MD:31-33
Timestamp: 2026-03-21T11:31:23.742Z
Learning: In `src/network.ts` of the `node-tcnet` repository: `TCNetDataPacketMetadata` (line 389) has an `info` property (not `data`). `data` property belongs to the sibling class `TCNetDataPacketMetrics` (line 352). Flagging `meta.info` in README/docs examples as wrong and suggesting `meta.data` is a false positive.

Applied to files:

  • src/network.ts
  • tests/packet-parsers.test.ts
📚 Learning: 2026-04-09T16:28:34.323Z
Learnt from: 9c5s
Repo: 9c5s/node-tcnet PR: 64
File: src/tcnet.ts:31-35
Timestamp: 2026-04-09T16:28:34.323Z
Learning: In node-tcnet, the changeset bump type is kept as `patch` even for breaking changes (removal of `autoReauth`/`reauthInterval`, `reauthenticated`/`reauthFailed` events, `AuthState` `"refreshing"`, addition of `TCNetLogger.warn`) because the library is pre-release (0.x semver), and the maintainer (9c5s) considers strict semver minor/major bumps unnecessary at this stage.

Applied to files:

  • src/network.ts
  • tests/packet-parsers.test.ts
📚 Learning: 2026-03-27T08:24:18.571Z
Learnt from: 9c5s
Repo: 9c5s/node-tcnet PR: 50
File: src/network.ts:1010-1024
Timestamp: 2026-03-27T08:24:18.571Z
Learning: In the `node-tcnet` repository (`src/network.ts`), `Buffer.allocUnsafe()` is never used — only `Buffer.alloc()` is used throughout the codebase. Reserved fields in packet `write()` methods do not need explicit zero-initialization because `Buffer.alloc()` always zero-initializes. Suggesting defensive zero-writes for reserved packet offsets is unnecessary (YAGNI) for this codebase.

Applied to files:

  • src/network.ts
  • tests/packet-parsers.test.ts
📚 Learning: 2026-03-24T19:15:40.455Z
Learnt from: 9c5s
Repo: 9c5s/node-tcnet PR: 41
File: src/utils.ts:94-94
Timestamp: 2026-03-24T19:15:40.455Z
Learning: In the `node-tcnet` repository (`src/utils.ts`), the project targets ES2022 + NodeNext and does not need to support Node.js 18.0.0. The temporary numeric `family` field (4/6) in `os.networkInterfaces()` introduced in Node.js 18.0.0 and fixed in 18.1.0 does not need to be handled. Suggesting numeric `family` handling for defensive programming is unnecessary for this project.

Applied to files:

  • src/network.ts
  • tests/packet-parsers.test.ts
📚 Learning: 2026-03-21T05:02:27.762Z
Learnt from: 9c5s
Repo: 9c5s/node-tcnet PR: 2
File: src/tcnet.ts:397-400
Timestamp: 2026-03-21T05:02:27.762Z
Learning: In the `node-tcnet` repository (`src/network.ts`, `src/tcnet.ts`), `TCNetDataPacket.write()` has no callers. `TCNetDataPacket` objects are only ever deserialized on receive paths (`read()`), never serialized and sent. The only outbound packet carrying a layer field is `TCNetRequestPacket`, constructed inside `requestData()` in `src/tcnet.ts`. Suggesting to move layer conversion logic into `TCNetDataPacket.write()` is inappropriate because that method is dead code.

Applied to files:

  • src/network.ts
  • tests/packet-parsers.test.ts
📚 Learning: 2026-04-09T23:09:19.524Z
Learnt from: 9c5s
Repo: 9c5s/node-tcnet PR: 65
File: src/tcnet.ts:1158-1167
Timestamp: 2026-04-09T23:09:19.524Z
Learning: In `src/tcnet.ts` (node-tcnet, TypeScript), the race condition where a stale `sendAuthSequence` from an old auth generation could call `resetAuthSession` and destroy a newly-started pending auth state is resolved by: (1) adding `expectedToken: number` parameter to `sendAuthSequence` and checking `this.sessionToken !== expectedToken` at every async boundary (entry, after cmd=0 send, after 50ms wait, after prepareAuthPayload) — silently returning without calling `resetAuthSession` when stale; (2) capturing `tokenForThisAttempt` in `handleInitialAuthRequest` and only calling `resetAuthSession` in the catch block when `this.sessionToken === tokenForThisAttempt`; (3) keeping the `tokenBeforePing` guard inside `prepareAuthPayload` as a secondary defense. The maintainer (9c5s) intentionally uses this two-layer guard design.

Applied to files:

  • tests/packet-parsers.test.ts
📚 Learning: 2026-04-09T16:28:34.323Z
Learnt from: 9c5s
Repo: 9c5s/node-tcnet PR: 64
File: src/tcnet.ts:31-35
Timestamp: 2026-04-09T16:28:34.323Z
Learning: In `src/tcnet.ts` (node-tcnet, TypeScript), `TCNetLogger.warn` is intentionally a required method (not optional). The maintainer (9c5s) made it required so that TypeScript catches missing logger implementations at compile time; making it optional would silently drop warn-level messages (e.g., unexpected token changes during reauth). The library is pre-release, so this breaking change is acceptable and is documented in the PR and changeset.

Applied to files:

  • tests/packet-parsers.test.ts
📚 Learning: 2026-04-09T17:09:02.148Z
Learnt from: 9c5s
Repo: 9c5s/node-tcnet PR: 64
File: src/tcnet.ts:961-966
Timestamp: 2026-04-09T17:09:02.148Z
Learning: In `src/tcnet.ts` (node-tcnet, TypeScript), the TOCTOU race condition in `performBridgeOsDetection` (where an in-flight ping could overwrite `bridgeIsWindows` with a stale result after a Bridge switch) is resolved by capturing `bridgeIp = this.server?.address` at detection start and checking `this.server?.address === bridgeIp` immediately before writing to `this.bridgeIsWindows`. The maintainer (9c5s) deliberately rejected clearing `bridgeOsDetectionPromise` in `resetAuthSession` (doesn't stop internal execution) and a generation-counter approach (triggers on same-Bridge resets too) in favor of this simpler address-comparison guard.

Applied to files:

  • tests/packet-parsers.test.ts
🔇 Additional comments (4)
.changeset/fix-pitch-bend-signed.md (1)

1-7: Changesetの記述は適切です。

修正内容(readUInt16LE から readInt16LE への変更)とユーザー影響が簡潔に整理されており、今回の実装・テスト変更と一致しています。

Based on learnings: pre-release(0.x)ではChangesetのbump種別をpatchで運用する方針で、この変更はその方針に準拠しています。

src/network.ts (1)

577-577: Line 577 の符号付き読み取り変更は正しいです。

pitchBend を readInt16LE で読むことで、負のピッチ値が期待通り負数として復元されます。修正対象に対して最小で的確な変更です。

tests/packet-parsers.test.ts (2)

547-568: 符号付き仕様へのテスト更新は妥当です。

writeInt16LE と期待値(600)への更新により、TCNetDataPacketMetrics.read() の新仕様を正しく検証できています。


583-600: pitchBend の境界値テストが十分に網羅されています。

0, ±600, 32767, -32768 を it.each で確認しており、符号付き16bitの主要ケースを適切にカバーしています。


Summary by CodeRabbit

バグ修正

  • ピッチベンド値の読み込み処理を改善しました。負のピッチベンド値が正しく負の数値として返されるようになりました。これによりオーディオ処理におけるピッチシフトがより正確に動作するようになります。

Walkthrough

@9c5s/node-tcnetパッケージのピッチベンド値の読み込み方法を変更しました。readUInt16LEからreadInt16LEに切り替えて、符号付き16ビット整数として解釈するようにし、負の値が正しく負の数として返されるようにしています。

Changes

Cohort / File(s) Summary
ピッチベンド符号付き解釈修正
src/network.ts, tests/packet-parsers.test.ts
TCNetDataPacketMetricsのピッチベンド解析をunsignedからsignedへ変更。readUInt16LEをreadInt16LEに置き換え、テストケースも符号付き値の範囲(-32768~32767)に更新。
変更セットドキュメント
.changeset/fix-pitch-bend-signed.md
パッチバージョン変更用の変更セットエントリを追加。ピッチベンド値の符号付き解釈への変更を文書化。

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

  • #7:同じオフセット(116)のピッチベンド解析に関連し、符号付き解釈への変更に対応した修正。

Possibly related PRs

  • #69:同じsrc/network.tsのピッチベンド解析を修正しますが、符号付き/符号なし解釈の方向が対立しています。
  • #23:TCNetDataPacketMetricsのピッチベンド解析の符号付き/符号なし解釈に関連しています。
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed プルリクエストのタイトルは、pitchBendフィールドを符号なし16bitから符号付き16bitへ変更する主要な変更内容を簡潔に説明しており、変更セット全体を正確に反映している。
Description check ✅ Passed プルリクエストの説明は、変更内容、発生していた問題、具体例、テスト計画を詳細に記載しており、変更セット全体と密接に関連している。
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/pitch-bend-signed

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request corrects the handling of the pitchBend field in TCNetDataPacketMetrics by reading it as a signed 16-bit integer instead of an unsigned one. Corresponding updates were made to the test suite to verify correct parsing of both positive and negative pitch values. I have no feedback to provide.

@9c5s
9c5s merged commit baff2de into main Apr 11, 2026
2 checks passed
@9c5s
9c5s deleted the fix/pitch-bend-signed branch April 11, 2026 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant