ROS: Standardize hand and EE pose topics with NamedPoseArray#755
ROS: Standardize hand and EE pose topics with NamedPoseArray#755sgrizan-nv wants to merge 8 commits into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds the Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TeleopRos2Node
participant MessageBuilders
participant NamedPoseTopics
participant WristTFBroadcaster
TeleopRos2Node->>MessageBuilders: Build NamedPoseArray
MessageBuilders-->>TeleopRos2Node: Return named poses and validity flags
TeleopRos2Node->>NamedPoseTopics: Publish xr_teleop/ee_pose or xr_teleop/hand
TeleopRos2Node->>WristTFBroadcaster: Broadcast wrist transforms
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
sgrizan-nv
force-pushed
the
sgrizan/branch2
branch
from
75ee52a to
7ab32a5
Compare
sgrizan-nv
changed the title
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
| @@ -0,0 +1,8 @@ | |||
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. | |||
There was a problem hiding this comment.
Nit: we should consider renaming this
Some options are:
- PoseStampedArray (although this might not be exactly right)
- LinkState (although not confident in this one but this message is analogous to: https://docs.ros.org/en/noetic/api/sensor_msgs/html/msg/JointState.html)
Not sure what else, but we should look at what other ROS Msgs are available and trying to have a similar name to that basically
There was a problem hiding this comment.
Yeah, I'm not happy with the current name either, but it doesn't feel like either PoseStampedArray or LinkState describe this message better
There was a problem hiding this comment.
I'm curious what Lionel thinks a better name is since I agree that those two message names aren't ideal either
| <name>teleop_ros2_interfaces</name> | ||
| <version>0.1.0</version> | ||
| <description>Custom ROS 2 interfaces for the Isaac Teleop ROS 2 example.</description> | ||
| <maintainer email="isaacteleop-maintainers@nvidia.com">Isaac Teleop Maintainers</maintainer> |
There was a problem hiding this comment.
Does the email actually work?
There was a problem hiding this comment.
Good catch, we don't have an alias. Changed it to my email address
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/teleop_ros2/teleop_ros2_interfaces/package.xml`:
- Around line 13-21: Update the package.xml dependency declarations: replace the
separate build_depend and exec_depend entries for geometry_msgs and std_msgs
with <depend> entries, and move rosidl_default_generators from build_depend to
buildtool_depend alongside ament_cmake. Preserve rosidl_default_runtime as an
exec_depend.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a2f80567-1198-4360-a56c-08f64aef76a9
📒 Files selected for processing (11)
examples/teleop_ros2/AGENTS.mdexamples/teleop_ros2/CMakeLists.txtexamples/teleop_ros2/Dockerfileexamples/teleop_ros2/README.mdexamples/teleop_ros2/python/constants.pyexamples/teleop_ros2/python/integration_tests/teleop_ros2_topic_verifier.pyexamples/teleop_ros2/python/messages.pyexamples/teleop_ros2/python/teleop_ros2_node.pyexamples/teleop_ros2/teleop_ros2_interfaces/CMakeLists.txtexamples/teleop_ros2/teleop_ros2_interfaces/msg/NamedPoseArray.msgexamples/teleop_ros2/teleop_ros2_interfaces/package.xml
| SPDX-License-Identifier: Apache-2.0 | ||
| --> | ||
| <package format="3"> | ||
| <name>teleop_ros2_interfaces</name> |
There was a problem hiding this comment.
Would we want to rename this package into something else? The current name is fine, but another name could be:
- isaac_teleop_ros2_interfaces
| def _compose_ee_msg( | ||
| left_pose: Pose | None, | ||
| right_pose: Pose | None, | ||
| now, | ||
| frame_id: str, |
There was a problem hiding this comment.
Nit: could we make the interface of this into:
def _compose_ee_msg(pose, name, frame_id)
instead and then just call it twice:
_compose_ee_msg(left_pose, "left", now, frame_id)
_compose_ee_msg(right_pose, "right", now, frame_id)
There was a problem hiding this comment.
_compose_ee_msg produces one NamedPoseArray. Calling it twice will produce two NamedPoseArrays
There was a problem hiding this comment.
Clarified what I meant here: #755 (comment)
We can discuss it offline if it's still not clear; it's mostly just an idea for restructuring the code not changing the final output
| def build_ee_msg_from_controllers( | ||
| left_ctrl: OptionalTensorGroup, | ||
| right_ctrl: OptionalTensorGroup, |
There was a problem hiding this comment.
Could we keep the call here the same as before and just call it twice?
There was a problem hiding this comment.
build_ee_msg_from_controllers and build_ee_msg_from_hands package left and right poses into the same NamedPoseArray, just as it was with PoseArray. Do you suggest splitting both EEs and hand poses into two topics (left and right)?
There was a problem hiding this comment.
I think keeping them in the same namedposearray makes sense; although we can have the hand poses in a separate topic (similar to how it was before)
The proposal is just changing it in code so it looks structually like:
named_pose_msg = NamedPoseMessage()
left_pose = build_left_msg()
right_pose = build_right_msg()
named_pose_msg.append([left_pose, right_pose])
Admittedly since we are doing the message as an structure of arrays it might not fit as nicely but I'd prefer trying to make things generic until we can't - since I feel like it'd support non-bimanual cases in the future easier
There was a problem hiding this comment.
The current approach is to isolate all message building logic in messages.py, so that downstream functions just send it over without modifying. Any common logic between different builders is extracted into private functions within messages.py
There was a problem hiding this comment.
Would accepting an array instead make sense here or do you think we should just resolve this for now until we explicitly want to support non-bimanual cases? That case is the main reason I'm worried
I think we can resolve this for now however
|
@coderabbitai review |
✅ Action performedReview finished.
|
kajananchinniahNV
left a comment
kajananchinniahNV
left a comment
There was a problem hiding this comment.
LGTM beyond the minor nitpicks!
2 participants
Description
Fixes #645
Type of change
Testing
Manual & integration testing
Checklist
SKIP=check-copyright-year pre-commit run --all-filesgit commit -s) per the DCOSummary by CodeRabbit
New Features
NamedPoseArraymessage support (names alongside poses and per-entry validity).xr_teleop/ee_poseusingNamedPoseArray.xr_teleop/handusingNamedPoseArray.teleop_ros2_interfacesROS 2 package and installation of its interfaces forexamples/teleop_ros2.Bug Fixes
Documentation