DynamixelドライバーのESP-IDF5対応#342
Conversation
WalkthroughIntroduces a 4096-step offset in PControl to adjust goal/present positions, changes default Dynamixel serial pin/port values and baud handling, and replaces PWM-based pan/tilt configuration with a Dynamixel driver and new ID/pin mappings in the manifest. Changes
Sequence Diagram(s)sequenceDiagram
participant Init as PControl.init
participant Servo as Dynamixel
participant HW as Motor HW
Note over Init,Servo: PControl initialization flow (new)
Init->>Servo: readPresentPosition()
alt read OK and value > 4096
Servo-->>Init: present = value
Init->>Init: set _offset = 4096
else read OK and value <= 4096
Servo-->>Init: present = value
Init->>Init: _offset = 0
else read FAIL
Servo-->>Init: error
Init-->>Init: log trace, _offset = 0
end
sequenceDiagram
participant App as Controller
participant P as PControl
participant D as Dynamixel
participant M as Motor HW
Note over App,P: Setting a new goal (changed)
App->>P: setGoal(goalPosition)
P->>P: goalWithOffset = goalPosition + _offset
P->>D: writeGoalPosition(goalWithOffset)
D->>M: sendGoal()
M-->>D: ack
D-->>P: write result
P->>D: readPresentPosition()
D-->>P: rawPresent
P->>P: present = rawPresent - _offset
P->>P: compute current = f(goalPosition, present, gain, saturation)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
firmware/stackchan/drivers/dynamixel-driver.ts(4 hunks)firmware/stackchan/drivers/dynamixel.ts(2 hunks)firmware/stackchan/manifest.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
firmware/stackchan/manifest.json (2)
firmware/stackchan/drivers/rs30x-driver.ts (1)
RS30XDriver(10-56)firmware/stackchan/drivers/rs30x.ts (1)
RS30X(172-310)
firmware/stackchan/drivers/dynamixel.ts (2)
firmware/stackchan/drivers/rs30x.ts (3)
PacketHandler(86-165)constructor(92-155)PacketHandler(93-145)firmware/stackchan/drivers/scservo.ts (1)
PacketHandler(61-139)
firmware/stackchan/drivers/dynamixel-driver.ts (1)
firmware/stackchan/drivers/sg90-driver.ts (1)
PWMServoDriver(17-97)
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (6)
firmware/stackchan/drivers/dynamixel-driver.ts (3)
17-17: LGTM: Offset field initialization.The offset mechanism is properly initialized to handle servos with extended position ranges.
Also applies to: 27-27
45-45: LGTM: Offset applied to goal position.The offset is correctly added to the goal position when writing to the servo, maintaining symmetry with the read operation.
54-56: LGTM: Offset correctly applied in control calculations.The present position is properly adjusted by subtracting the offset, and the normalized position is used in the control calculation. This maintains consistency between goal and present positions in the same coordinate space.
firmware/stackchan/drivers/dynamixel.ts (1)
232-235: LGTM: Serial configuration updated for ESP-IDF 5.The PacketHandler initialization now uses the correct default pins (receive: 6, transmit: 7) and port (1), aligning with the manifest.json configuration for CoreS3. These changes are consistent across both initialization sites (setBaud and constructor).
firmware/stackchan/manifest.json (2)
113-115: LGTM: CoreS3 driver configuration migrated to Dynamixel.The configuration correctly switches from PWM-based servo control to Dynamixel protocol with appropriate servo IDs. This aligns with the
DynamixelDriverPropstype defined indynamixel-driver.tsand matches the RT version configuration mentioned in the PR objectives.
121-122: LGTM: Serial pins updated for CoreS3 Dynamixel communication.The serial pin configuration (transmit: 7, receive: 6) is correctly updated and consistent with the defaults in
dynamixel.ts. This ensures proper communication with Dynamixel servos on the CoreS3 hardware under ESP-IDF 5.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
firmware/stackchan/drivers/dynamixel-driver.ts (1)
32-37: Error handling for failed offset detection already flagged.The initialization continues even if the position read fails, potentially leaving
_offsetat 0 for servos that require it. This concern has been thoroughly analyzed in the previous review, which recommends mirroring the error-handling pattern fromupdate()and aborting initialization on read failure.
🧹 Nitpick comments (1)
firmware/stackchan/drivers/dynamixel-driver.ts (1)
56-58: Consider simplifying the local variable usage.The
positionvariable on line 57 is only used once on line 58. While the current code is readable, you could simplify slightly by usingthis.presentPositiondirectly in the calculation:this.presentPosition = result.value - this._offset -const position = this.presentPosition -const current = Math.min(Math.abs(this.goalPosition - position) * this.gain, this.saturation) +const current = Math.min(Math.abs(this.goalPosition - this.presentPosition) * this.gain, this.saturation)However, the current approach is also perfectly acceptable and may improve readability by keeping the calculation line shorter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
firmware/stackchan/drivers/dynamixel-driver.ts(4 hunks)firmware/stackchan/drivers/dynamixel.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- firmware/stackchan/drivers/dynamixel.ts
🧰 Additional context used
🧬 Code graph analysis (1)
firmware/stackchan/drivers/dynamixel-driver.ts (1)
firmware/stackchan/drivers/sg90-driver.ts (1)
PWMServoDriver(17-97)
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (2)
firmware/stackchan/drivers/dynamixel-driver.ts (2)
17-17: LGTM: Clean addition of offset tracking field.The
_offsetfield is properly declared and initialized to a sensible default of 0 before detection occurs duringinit().Also applies to: 27-27
47-47: LGTM: Offset correctly applied to goal position.Adding
_offsetwhen setting the goal position correctly maps the logical [0, 4096) range to the servo's actual position range for multi-turn servos.
|
@meganetaaan さん、ご確認おねがいします |
meganetaaan
left a comment
meganetaaan
left a comment
There was a problem hiding this comment.
CoreS3（RT Version）で動作することを確認しました。
素の状態で書き込むとmanifest_local.jsonのdriverの設定で上書きされてしまうのを対処しないとですね。別途issueを立てます。
ありがとうございます！
2 participants
ESP-IDF5系以降でDynamixelドライバーが動作しなくなる問題があるため、RT版からの更新内容の取り込みとなります。
合わせてCoreS3のサーボの設定をRT版に合わせています。
一方で、#307 の問題は未解決のため、CoreS3で動作させるためにはPSRAMを無効にする必要があります。
Summary by CodeRabbit
Bug Fixes
Chores