Skip to content

Fix moveit_py rclcpp::init()#2223

Merged
JafarAbdi merged 8 commits intomoveit:mainfrom
henningkayser:pr/fix_moveit_py_sim
Jun 20, 2023
Merged

Fix moveit_py rclcpp::init()#2223
JafarAbdi merged 8 commits intomoveit:mainfrom
henningkayser:pr/fix_moveit_py_sim

Conversation

@henningkayser
Copy link
Copy Markdown
Member

@henningkayser henningkayser commented Jun 2, 2023 •
edited
Loading

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

@henningkayser
Fix moveit_py rclcpp::init()
e4d22b4 
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.
@henningkayser henningkayser mentioned this pull request Jun 2, 2023
Closed
@codecov
Copy link
Copy Markdown

codecov bot commented Jun 2, 2023 •
edited
Loading

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
Copy Markdown
Member Author

henningkayser commented Jun 2, 2023

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 reviewed Jun 13, 2023
View reviewed changes
JafarAbdi
JafarAbdi reviewed Jun 13, 2023
View reviewed changes
JafarAbdi
JafarAbdi reviewed Jun 13, 2023
View reviewed changes
@JafarAbdi JafarAbdi enabled auto-merge (squash) June 20, 2023 19:57
@JafarAbdi JafarAbdi merged commit 636b988 into moveit:main Jun 20, 2023
@JafarAbdi JafarAbdi mentioned this pull request Jun 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JafarAbdi JafarAbdi JafarAbdi approved these changes

@peterdavidfagan peterdavidfagan Awaiting requested review from peterdavidfagan

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

MoveItPy does not support use_sim_time:=True

2 participants

@henningkayser @JafarAbdi