-
Notifications
You must be signed in to change notification settings - Fork 82
feat(examples/teleop_ros2): Add head-relative EE pose frame to teleop_ros2 example #992
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ | |
| from constants import BODY_JOINT_NAMES, HAND_POSE_JOINT_INDICES, HAND_POSE_NAMES | ||
| from geometry import ( | ||
| apply_manus_controller_to_hand_pose, | ||
| apply_relative_pose, | ||
| apply_transform_to_pose, | ||
| make_transform, | ||
| to_pose, | ||
|
|
@@ -235,6 +236,33 @@ def _as_float(ctrl, index): | |
| ) | ||
|
|
||
|
|
||
| def rebase_ee_poses_relative_to_head( | ||
| ee_poses_msg: NamedPoseArray, | ||
| head_msg: PoseStamped | None, | ||
| left_wrist_frame: str, | ||
| right_wrist_frame: str, | ||
| ) -> tuple[NamedPoseArray, list[TransformStamped]]: | ||
| relative_poses: list[Pose | None] = [] | ||
| for pose, is_valid in zip(ee_poses_msg.pose, ee_poses_msg.is_valid): | ||
| if is_valid: | ||
| relative_poses.append(apply_relative_pose(head_msg.pose, pose)) | ||
| else: | ||
| relative_poses.append(None) | ||
|
|
||
| rebased_msg = _compose_ee_msg( | ||
| relative_poses[0], | ||
| relative_poses[1], | ||
| ee_poses_msg.header.stamp, | ||
| "head", | ||
| ) | ||
|
Comment on lines
+252
to
+257
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Use the configured Line 256 hard-codes Add a Proposed fix def rebase_ee_poses_relative_to_head(
ee_poses_msg: NamedPoseArray,
head_msg: PoseStamped | None,
+ head_frame: str,
left_wrist_frame: str,
right_wrist_frame: str,
) -> tuple[NamedPoseArray, list[TransformStamped]]:
@@
- "head",
+ head_frame, ee_poses_msg, wrist_tfs = rebase_ee_poses_relative_to_head(
ee_poses_msg,
head_msg,
+ self._params.head_frame,
self._params.left_wrist_frame,
self._params.right_wrist_frame,
)🤖 Prompt for AI Agents
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 |
||
|
|
||
| return rebased_msg, _wrist_tfs_from_ee_msg( | ||
| rebased_msg, | ||
| left_wrist_frame, | ||
| right_wrist_frame, | ||
| ) | ||
|
|
||
|
|
||
| def build_ee_output_from_controllers( | ||
| left_ctrl: OptionalTensorGroup, | ||
| right_ctrl: OptionalTensorGroup, | ||
|
|
@@ -278,6 +306,7 @@ def build_ee_output_from_hands( | |
| right_wrist_frame: str, | ||
| transform_rot: Rotation | None = None, | ||
| transform_trans: Sequence[float] | None = None, | ||
| reference_pose: Pose | None = None, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| ) -> tuple[NamedPoseArray, list[TransformStamped]]: | ||
| """Build the hand-derived EE message and its valid wrist TFs.""" | ||
| left_pose = _compute_ee_pose_from_hand( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,7 @@ | |
| - world_frame -> right_wrist_frame | ||
| - world_frame -> left_wrist_frame | ||
| - world_frame -> head_frame | ||
| - (With ee_poses_frame=head, world_frame -> head_frame -> left/right_wrist_frame) | ||
| """ | ||
|
|
||
| import time | ||
|
|
@@ -62,6 +63,7 @@ | |
| build_hand_msg, | ||
| build_head_output, | ||
| build_root_command_output, | ||
| rebase_ee_poses_relative_to_head, | ||
| ) | ||
| from teleop_profiles import ( | ||
| PublishType, | ||
|
|
@@ -111,7 +113,9 @@ def _create_publishers(self) -> None: | |
| ) | ||
| self._pub_head = self.create_publisher(PoseStamped, "xr_teleop/head_pose", 10) | ||
|
|
||
| def _publish_ee_poses_from_controllers(self, result: SessionResult, now) -> None: | ||
| def _publish_ee_poses_from_controllers( | ||
| self, result: SessionResult, now, head_msg: PoseStamped | None | ||
| ) -> None: | ||
| ee_poses_msg, wrist_tfs = build_ee_output_from_controllers( | ||
| result["controller_left"], | ||
| result["controller_right"], | ||
|
|
@@ -123,6 +127,20 @@ def _publish_ee_poses_from_controllers(self, result: SessionResult, now) -> None | |
| self._params.transform_translation, | ||
| self._params.controller_uses_hands_source, | ||
| ) | ||
| if self._params.ee_poses_frame.value == "head": | ||
| if head_msg is None: | ||
| self.get_logger().warn( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Falling back from head-relative output to world-frame output creates a discontinuity in both the pose coordinates and TF parent. A consumer that assumes a stable frame (or directly treats these values as robot targets) could interpret this as a large instantaneous motion. It'd be better to fail closed when
|
||
| "ee_poses_frame is 'head' but no head pose is available; " | ||
| "publishing ee_poses_frame in default frame 'world'.", | ||
| throttle_duration_sec=5.0, | ||
| ) | ||
| else: | ||
| ee_poses_msg, wrist_tfs = rebase_ee_poses_relative_to_head( | ||
| ee_poses_msg, | ||
| head_msg, | ||
| self._params.left_wrist_frame, | ||
| self._params.right_wrist_frame, | ||
| ) | ||
| self._pub_ee_poses.publish(ee_poses_msg) | ||
| if wrist_tfs: | ||
| self._tf_broadcaster.sendTransform(wrist_tfs) | ||
|
|
@@ -161,7 +179,9 @@ def _publish_hand_poses(self, result: SessionResult, now) -> None: | |
| ) | ||
| self._pub_hand.publish(hand_msg) | ||
|
|
||
| def _publish_ee_poses_from_hands(self, result: SessionResult, now) -> None: | ||
| def _publish_ee_poses_from_hands( | ||
| self, result: SessionResult, now, head_msg: PoseStamped | None | ||
| ) -> None: | ||
| ee_poses_msg, wrist_tfs = build_ee_output_from_hands( | ||
| result["hand_left"], | ||
| result["hand_right"], | ||
|
|
@@ -172,11 +192,25 @@ def _publish_ee_poses_from_hands(self, result: SessionResult, now) -> None: | |
| self._params.transform_rotation, | ||
| self._params.transform_translation, | ||
| ) | ||
| if self._params.ee_poses_frame.value == "head": | ||
| if head_msg is None: | ||
| self.get_logger().warn( | ||
| "ee_poses_frame is 'head' but no head pose is available;" | ||
| "publishing ee_poses_frame in default frame 'world'.", | ||
| throttle_duration_sec=5.0, | ||
| ) | ||
| else: | ||
| ee_poses_msg, wrist_tfs = rebase_ee_poses_relative_to_head( | ||
| ee_poses_msg, | ||
| head_msg, | ||
| self._params.left_wrist_frame, | ||
| self._params.right_wrist_frame, | ||
| ) | ||
| self._pub_ee_poses.publish(ee_poses_msg) | ||
| if wrist_tfs: | ||
| self._tf_broadcaster.sendTransform(wrist_tfs) | ||
|
|
||
| def _publish_head(self, result: SessionResult, now) -> None: | ||
| def _publish_head(self, result: SessionResult, now) -> PoseStamped | None: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please keep Instead, let the EE output builders consume the raw head input. Keep their controller- and hand-specific pose extraction separate, but move the common head-relative validation, rebasing, frame selection, fail-closed behavior, and wrist-TF construction into a shared EE finalization helper. This avoids duplicating the new behavior across both builders while preserving the existing architecture: each publisher calls one output-specific builder and only publishes or broadcasts its result. |
||
| maybe_head_output = build_head_output( | ||
| result["head"], | ||
| now, | ||
|
|
@@ -192,6 +226,8 @@ def _publish_head(self, result: SessionResult, now) -> None: | |
| self._pub_head.publish(head_msg) | ||
| self._tf_broadcaster.sendTransform(head_tf) | ||
|
|
||
| return head_msg | ||
|
|
||
| def _publish_root_command(self, result: SessionResult, now) -> None: | ||
| maybe_root_output = build_root_command_output( | ||
| result["root_command"], | ||
|
|
@@ -235,16 +271,22 @@ def _run_session_loop(self, launcher: CloudXRLauncher | None = None) -> int: | |
|
|
||
| now = self.get_clock().now().to_msg() | ||
|
|
||
| if PublishType.HEAD in self._profile_spec.publish_types: | ||
| head_messages = self._publish_head(result, now) | ||
| if ( | ||
| PublishType.EE_FROM_HANDS | ||
| in self._profile_spec.publish_types | ||
| ): | ||
| self._publish_ee_poses_from_hands(result, now) | ||
| self._publish_ee_poses_from_hands( | ||
| result, now, head_messages | ||
| ) | ||
| if ( | ||
| PublishType.EE_FROM_CONTROLLERS | ||
| in self._profile_spec.publish_types | ||
| ): | ||
| self._publish_ee_poses_from_controllers(result, now) | ||
| self._publish_ee_poses_from_controllers( | ||
| result, now, head_messages | ||
| ) | ||
| if PublishType.HAND_POSES in self._profile_spec.publish_types: | ||
| self._publish_hand_poses(result, now) | ||
| if PublishType.ROOT_COMMAND in self._profile_spec.publish_types: | ||
|
|
@@ -254,8 +296,6 @@ def _run_session_loop(self, launcher: CloudXRLauncher | None = None) -> int: | |
| in self._profile_spec.publish_types | ||
| ): | ||
| self._publish_finger_joints(result, now) | ||
| if PublishType.HEAD in self._profile_spec.publish_types: | ||
| self._publish_head(result, now) | ||
| if ( | ||
| PublishType.CONTROLLER_PAYLOAD | ||
| in self._profile_spec.publish_types | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This helper is in the appropriate module, but it receives an EE message after the output has already been fully constructed. In head mode,
build_ee_output_from_*()has already generated world-frame wrist transforms; those transforms are then discarded while this helper rebuilds the message and generates another set.Please have the EE builder determine the final reference frame and rebase the source poses before calling
_compose_ee_msg()and_wrist_tfs_from_ee_msg(), so the final message and transforms are constructed only once.