Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds retargeted finger-joint JointState publishing and wrist Transform broadcasting to the teleop ROS2 publisher node; introduces world/right/left wrist frame parameters, TriHand retargeters for left/right hands, finger_joints topic and publishers, and exposes finger joint outputs in the node pipeline. Changes
Sequence DiagramsequenceDiagram
participant Node as TeleopRos2PublisherNode
participant ControllerSource as ControllersSource
participant RetargeterR as TriHandMotionControllerRetargeter (right)
participant RetargeterL as TriHandMotionControllerRetargeter (left)
participant Combiner as OutputCombiner
participant TF as TransformBroadcaster
participant FJPub as FingerJoints Publisher
ControllerSource->>Node: controller inputs (left/right)
Node->>RetargeterR: forward right controller input
Node->>RetargeterL: forward left controller input
RetargeterR-->>Node: right hand finger joints + wrist pose
RetargeterL-->>Node: left hand finger joints + wrist pose
Node->>Combiner: expose finger_joints_left / finger_joints_right
Combiner-->>Node: combined pipeline outputs
Node->>FJPub: publish JointState (names=_FINGER_JOINT_NAMES, positions=left+right, frame=world_frame)
Node->>TF: build wrist TransformStamped (world_frame -> wrist frames)
TF-->>Node: broadcast wrist TFs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
kajananchinniahNV
force-pushed
the
kchinniah/add-retargeting-of-finger-joints
branch
from
c185931 to
2fd7ffe
Compare
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/teleop_ros2/python/teleop_ros2_publisher.py`:
- Around line 485-497: Validate and guard the retargeter outputs before building
and publishing finger_joints_msg: check result contains "finger_joints_left" and
"finger_joints_right", coerce them to numpy arrays and verify their
shapes/lengths match the expected counts implied by _FINGER_JOINT_NAMES (or half
each if names are split), and handle mismatches by logging a warning and either
skipping publish or filling with safe defaults (e.g. zeros) so that
finger_joints_msg.position always has the correct length; update the code around
finger_joints_msg, left_joints/right_joints, and the publish call to perform
these checks and safe fallback behavior.
In `@examples/teleop_ros2/README.md`:
- Line 74: The README's parameter list omitted the declared parameter
finger_joints_topic; update the documented "Available parameters" string to
include `finger_joints_topic` alongside `hand_topic`, `twist_topic`, etc., so
the README matches the node's actual parameters (the node named
/teleop_ros2_publisher and its parameter `finger_joints_topic`).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e4ae86d1-1adb-4578-af50-ce75854cb856
📒 Files selected for processing (2)
examples/teleop_ros2/README.mdexamples/teleop_ros2/python/teleop_ros2_publisher.py
kajananchinniahNV
force-pushed
the
kchinniah/add-retargeting-of-finger-joints
branch
from
2fd7ffe to
126837c
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/teleop_ros2/python/teleop_ros2_publisher.py`:
- Around line 586-599: The wrist TFs are being built from the AIM pose instead
of the controller grip pose; change the wrist frame construction so
_build_wrist_tfs uses the controllers' ControllerInputIndex.GRIP_POSITION and
ControllerInputIndex.GRIP_ORIENTATION (or pass explicit grip pose objects)
rather than the aim pose produced by _build_ee_msg_from_controllers, updating
the call site that currently passes ee_msg and the boolean checks for
right_ctrl.is_none/left_ctrl.is_none to supply the grip poses (or change
_build_wrist_tfs to read grip fields), and keep the TF names
right_wrist_frame/left_wrist_frame as-is (or rename if you intentionally want
aim-based frames).
- Around line 397-406: Add a deprecated alias parameter "frame_id" that maps to
the existing "world_frame" so legacy --ros-args -p frame_id:=... continues to
work: declare a second parameter using declare_parameter("frame_id", "world",
ParameterDescriptor(description="DEPRECATED: use 'world_frame' instead",
additional_description_or_deprecation_flag_if_available)) and, wherever the node
reads the frame name (e.g., code that calls get_parameter or accesses
"world_frame"), prefer an explicit precedence rule that checks "frame_id" first
and falls back to "world_frame" (or default "world") so both declarations are
accepted; apply the same change for the other occurrence block referenced (lines
449-475).
In `@examples/teleop_ros2/README.md`:
- Around line 36-38: Add documentation for the new publisher by updating the
"Published Topics" list to include the topic name xr_teleop/finger_joints with
message type sensor_msgs/JointState and a short description that it publishes
retargeted finger joint angles for the robot; mention which modes publish it
(controller_teleop and hand_teleop) and note that it contains joint names and
position arrays corresponding to the robot finger joints so users know to
subscribe for finger-joint retargeting data (reference the topic name
xr_teleop/finger_joints and message type sensor_msgs/JointState when editing
README.md).
- Around line 94-95: The README table row for the `controller_teleop` mode is
missing the `finger_joints` output; update the `controller_teleop` row to list
`finger_joints` alongside the existing outputs (e.g., `ee_poses`, `root_twist`,
`root_pose`, `tf`) so the table correctly reflects that `controller_teleop`
publishes the new `finger_joints` topic.
- Line 112: Add a line showing how to echo the finger joint JointState so users
can validate retargeting: include an example command to inspect the
xr_teleop/finger_joints topic (sensor_msgs/msg/JointState) in the README near
the existing `/tf` example, e.g., mirror the `/tf` example by adding a `ros2
topic echo /xr_teleop/finger_joints sensor_msgs/msg/JointState` entry so readers
can view finger joint messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 15f37740-eed7-40be-935f-8c00458eca1b
📒 Files selected for processing (2)
examples/teleop_ros2/README.mdexamples/teleop_ros2/python/teleop_ros2_publisher.py
kajananchinniahNV
force-pushed
the
kchinniah/add-retargeting-of-finger-joints
branch
from
126837c to
5d6c7d9
Compare
life1ess
reviewed
Mar 9, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/teleop_ros2/python/teleop_ros2_publisher.py`:
- Around line 623-637: In the controller_teleop branch (where self._mode ==
"controller_teleop"), change the publishing logic so finger joints are published
when either left_joints or right_joints is available (use OR, not AND) and build
the position array from whichever side(s) exist; specifically, update the
condition that checks left_joints and right_joints, use list(left_joints) /
list(right_joints) (or list(TensorGroup)) when converting to numpy to honor
sequence protocol, and concatenate the available arrays (treat missing side as
an empty sequence) before setting finger_joints_msg.position and calling
self._pub_finger_joints.publish; keep references to the same symbols:
left_joints, right_joints, finger_joints_msg, _pub_finger_joints, and
_FINGER_JOINT_NAMES.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c2f9b6f1-5000-4487-8421-1d6aa505ce49
📒 Files selected for processing (1)
examples/teleop_ros2/python/teleop_ros2_publisher.py
life1ess
approved these changes
Mar 10, 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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Documentation