Skip to content

Planning Scene Monitor Node Executor#230

Merged
henningkayser merged 3 commits intomoveit:masterfrom
jrgnicho:improvement-planning-scene-monitor-asynch-executor
Aug 18, 2020
Merged

Planning Scene Monitor Node Executor#230
henningkayser merged 3 commits intomoveit:masterfrom
jrgnicho:improvement-planning-scene-monitor-asynch-executor

Conversation

@jrgnicho
Copy link
Copy Markdown
Contributor

@jrgnicho jrgnicho commented Jul 8, 2020

Description

Executes internal timer callback using a Single Threaded Executor, this is an attempt to mimic the ROS1 behavior that used an Asynch Spinner and an internal Callback Queue to manage the timer callback invocation. Need some guidance on how best to test this.

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 8, 2020 •
edited
Loading

Codecov Report

Merging #230 into master will decrease coverage by 0.18%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
- Coverage   46.63%   46.45%   -0.19%     
==========================================
  Files         122      123       +1     
  Lines       12740    12781      +41     
==========================================
- Hits         5941     5937       -4     
- Misses       6799     6844      +45
Impacted Files Coverage Δ
...e/src/parameterization/model_based_state_space.cpp 65.97% <0.00%> (-2.78%) ⬇️
moveit_core/planning_scene/src/planning_scene.cpp 33.68% <0.00%> (-0.10%) ⬇️
moveit_core/profiler/src/profiler.cpp 64.70% <0.00%> (ø)
moveit_core/robot_model/src/robot_model.cpp 76.38% <0.00%> (ø)
...veit_core/robot_model/src/revolute_joint_model.cpp 79.52% <0.00%> (ø)
...lanners/ompl/ompl_interface/src/ompl_interface.cpp 0.00% <0.00%> (ø)
.../transforms/include/moveit/transforms/transforms.h 100.00% <0.00%> (ø)
...lision_distance_field/src/collision_env_hybrid.cpp 0.00% <0.00%> (ø)
...t_state/include/moveit/robot_state/attached_body.h 87.50% <0.00%> (ø)
...raint_samplers/src/default_constraint_samplers.cpp 73.42% <0.00%> (ø)
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 247b667...ed89d08. Read the comment docs.

@jrgnicho jrgnicho force-pushed the improvement-planning-scene-monitor-asynch-executor branch from 1961c05 to 6b9573e Compare July 11, 2020 03:45
@jrgnicho jrgnicho changed the title [WIP] Planning Scene Monitor Node Executor Planning Scene Monitor Node Executor Jul 11, 2020
@jrgnicho
Copy link
Copy Markdown
Contributor Author

jrgnicho commented Jul 11, 2020

I tested this change by running the run_moveit_cpp demo
This is ready for review

@jrgnicho jrgnicho force-pushed the improvement-planning-scene-monitor-asynch-executor branch 2 times, most recently from 9135542 to 9c6d59d Compare July 22, 2020 17:33
@jrgnicho
Copy link
Copy Markdown
Contributor Author

jrgnicho commented Jul 22, 2020

@henningkayser the CI just passed

@henningkayser
Copy link
Copy Markdown
Member

henningkayser commented Jul 23, 2020

@jrgnicho looks good! But shouldn't we then use the private node for running the other callbacks as well? The original implementation had an AsyncSpinner spinning a callback queue assigned to the node handle (which effectively runs the callbacks in a separate thread). Here we initialize the executor in a separate thread and use this for spinning a private node. So for actually running the callbacks in that thread I think we have to initialize publishers/subscribers/services with the private node instead of node_. Related to this, there is currently being worked on supporting callback groups (ros2/rclcpp#519) which might at some point allow a more direct replacement of the old behavior.

@henningkayser henningkayser mentioned this pull request Aug 11, 2020
Merged
1 task
@jrgnicho jrgnicho force-pushed the improvement-planning-scene-monitor-asynch-executor branch from 0d8cfb1 to 43e41c0 Compare August 12, 2020 14:26
@jrgnicho
Copy link
Copy Markdown
Contributor Author

jrgnicho commented Aug 12, 2020

@henningkayser I've made the requested changes

@henningkayser henningkayser force-pushed the improvement-planning-scene-monitor-asynch-executor branch from 43e41c0 to ed89d08 Compare August 18, 2020 14:01
henningkayser
henningkayser approved these changes Aug 18, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

I fixed name and namespace of the private node (get_fully_qualified_name() was giving the full namespace which would result in an invalid node name). Now everything seems to work just fine.

@henningkayser henningkayser merged commit 76d5d4b into moveit:master Aug 18, 2020
@AdamPettinger AdamPettinger mentioned this pull request Aug 20, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@henningkayser henningkayser henningkayser approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jrgnicho @henningkayser