Skip to content
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

Fix moveit_py rclcpp::init() #2223

Merged
merged 8 commits into from Jun 20, 2023
Merged

Conversation

henningkayser
Copy link
Member

@henningkayser henningkayser commented Jun 2, 2023

Fixes #2220. Rclcpp has been initialized without args which was problematic for some use cases like clock simulation. Parameters like use_sim_time:=true need to be passed to rclcpp, also NodeOptions access the global rcl state on construction.

@peterdavidfagan maybe we should just pass all the launch arguments from moveit_py over to rclcpp through the MoveItPy constructor. That would also take care of #2219

Rclcpp has been initialized without args which was problematic
for some use cases like clock simulation. Parameters like
use_sim_time:=true need to be passed to rclcpp, also
NodeOptions access the global rcl state on construction.
@codecov
Copy link

codecov bot commented Jun 2, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (e589048) 50.51% compared to head (4b5dbdc) 50.50%.

❗ Current head 4b5dbdc differs from pull request most recent head 268d46d. Consider uploading reports for the commit 268d46d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2223      +/-   ##
==========================================
- Coverage   50.51%   50.50%   -0.00%     
==========================================
  Files         386      386              
  Lines       31731    31731              
==========================================
- Hits        16026    16023       -3     
- Misses      15705    15708       +3     

see 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@henningkayser
Copy link
Member Author

of course clang-tidy is not happy...

so, this does indeed fix simulation, but it somehow changes something with the topic internals.
The Python tutorial works completely, but moveit_py has started to publish planning scene updates to /monitored_planning_scene even though the topic parameter is set to /moveit_cpp/monitored_planning_scene (I could even verify this with ros2 param get). No idea why topic and parameter diverge here. All other communication channels (ros2_control, path publisher to RViz, etc) just work. Changing the monitor topic to /monitored_planning_scene in RViz also works without issues. I'm simply confused why this PR would break a single topic that way. What am I missing?

@JafarAbdi JafarAbdi enabled auto-merge (squash) June 20, 2023 19:57
@JafarAbdi JafarAbdi merged commit 636b988 into moveit:main Jun 20, 2023
6 of 7 checks passed
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.

MoveItPy does not support use_sim_time:=True
2 participants