Skip to content

Remove ROS 2 topic parameters in favor of standard remapping#343

Merged
sgrizan-nv merged 2 commits intomainfrom
sgrizan/branch2
Apr 2, 2026
Merged

Remove ROS 2 topic parameters in favor of standard remapping#343
sgrizan-nv merged 2 commits intomainfrom
sgrizan/branch2

Conversation

@sgrizan-nv
Copy link
Copy Markdown
Collaborator

@sgrizan-nv sgrizan-nv commented Apr 1, 2026

Remove redundant topic parameters from the ROS 2 publisher node and its documentation, relying on standard ROS 2 topic remapping

Summary by CodeRabbit

  • Documentation

    • Renamed and expanded "Overriding parameters" to cover ROS 2 parameter overrides and topic remapping, updated command example, trimmed listed configurable parameters to non-topic settings, and added a list of remappable topics plus instructions to inspect active remaps.
  • Changes

    • Teleop publisher no longer exposes topic-name parameters at runtime; it uses fixed default topic names that can be redirected via ROS 2 remapping.

@sgrizan-nv sgrizan-nv self-assigned this Apr 1, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 1, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 665672a1-29b8-4d22-8845-1dff2d2cbc7e

📥 Commits

Reviewing files that changed from the base of the PR and between a0fb72c and db70def.

📒 Files selected for processing (2)
  • examples/teleop_ros2/README.md
  • examples/teleop_ros2/python/teleop_ros2_publisher.py

📝 Walkthrough

Walkthrough

The PR removes runtime ROS 2 parameters for teleop topic names in the teleop_ros2_publisher node, hardcodes those topic names, and updates README to remove topic parameters, add remapping guidance, and show example --ros-args -r ... usage.

Changes

Cohort / File(s) Summary
Publisher implementation
examples/teleop_ros2/python/teleop_ros2_publisher.py
Removed declarations and reads of topic-name parameters and their private members; publishers now use fixed hard-coded topics (xr_teleop/hand, xr_teleop/ee_poses, xr_teleop/root_twist, xr_teleop/root_pose, xr_teleop/controller_data, xr_teleop/full_body, xr_teleop/finger_joints). Docstring updated to state topics are remappable via ROS 2 remapping.
Documentation
examples/teleop_ros2/README.md
Renamed/expanded "Overriding parameters" to "Overriding parameters and remapping topics"; examples show --ros-args -p ... and --ros-args -r ... usage; removed topic-related parameters from "Available parameters" and added an "Available topics for remapping" list and ros2 node info /teleop_ros2_publisher guidance.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰
I nudged the params into their nest,
Fixed the paths, now snug and blessed.
Remap a route, give commands a wink—
The publisher hops, faster than you'd think. 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title accurately summarizes the main change: removing ROS 2 topic parameters in favor of standard remapping, which is the core objective across both modified files.

✏️ 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 sgrizan/branch2

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

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
examples/teleop_ros2/README.md (1)

75-88: ⚠️ Potential issue | 🟡 Minor

Document topic remapping explicitly in this section.

After removing topic parameters, users need a concrete replacement workflow here (e.g., --ros-args -r old:=new). Line 88 updates the parameter list, but the section does not yet show the new remapping path.

📘 Proposed README addition
 ### Overriding parameters
 
 It's possible to set ROS 2 parameters from the command line when running the container. Append `--ros-args -p param_name:=value` after the image name:
@@
 Available parameters: `rate_hz`, `mode`, `world_frame`, `right_wrist_frame`, `left_wrist_frame`. Use `ros2 param list /teleop_ros2_publisher` and `ros2 param describe /teleop_ros2_publisher <param>` (with the node running) for the full set.
+
+Topic names are no longer ROS parameters. Use standard ROS 2 remapping instead, for example:
+
+```bash
+teleop_ros2_ref --ros-args \
+  -r xr_teleop/hand:=my_robot/hand \
+  -r xr_teleop/ee_poses:=my_robot/ee_poses
+```
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@examples/teleop_ros2/README.md` around lines 75 - 88, The README's
"Overriding parameters" section mentions runtime parameter overrides but no
longer documents how to remap topics after topic parameters were removed; update
the paragraph for the container run example (referencing the teleop_ros2_ref
invocation) to include a concrete topic-remapping example using ROS 2 remap
arguments (e.g., use --ros-args -r old_topic:=new_topic) and show remapping of
xr_teleop/hand and xr_teleop/ee_poses to user topics (use the -r flag twice),
and also note users can inspect active remaps with ros2 node info
/teleop_ros2_publisher or similar if needed.
🤖 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 451-467: Update the stale wording in the module/class docstrings
and inline comments in teleop_ros2_publisher.py (any text referring to
"configurable via parameters" for topics like self._pub_hand, self._pub_ee_pose,
self._pub_root_twist, self._pub_root_pose, self._pub_controller,
self._pub_full_body, self._pub_finger_joints) to say "remappable via ROS 2
remapping" instead; search the file for phrases mentioning "configurable via
parameters" or "configurable via parameter" and replace them with "remappable
via ROS 2 remapping" or equivalent concise wording so the documentation reflects
the removed parameter-based topic configuration.

---

Outside diff comments:
In `@examples/teleop_ros2/README.md`:
- Around line 75-88: The README's "Overriding parameters" section mentions
runtime parameter overrides but no longer documents how to remap topics after
topic parameters were removed; update the paragraph for the container run
example (referencing the teleop_ros2_ref invocation) to include a concrete
topic-remapping example using ROS 2 remap arguments (e.g., use --ros-args -r
old_topic:=new_topic) and show remapping of xr_teleop/hand and
xr_teleop/ee_poses to user topics (use the -r flag twice), and also note users
can inspect active remaps with ros2 node info /teleop_ros2_publisher or similar
if needed.
🪄 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: ASSERTIVE

Plan: Pro

Run ID: 4e4cc1ed-3cc4-4ce9-b64c-e8befa898c2c

📥 Commits

Reviewing files that changed from the base of the PR and between 8a61d96 and 7f9927c.

📒 Files selected for processing (2)
  • examples/teleop_ros2/README.md
  • examples/teleop_ros2/python/teleop_ros2_publisher.py

@sgrizan-nv sgrizan-nv enabled auto-merge (squash) April 2, 2026 20:46
@sgrizan-nv sgrizan-nv merged commit 5d3a342 into main Apr 2, 2026
31 checks passed
@sgrizan-nv sgrizan-nv deleted the sgrizan/branch2 branch April 2, 2026 21:16
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.

2 participants