Add private buffer & tf listener to PSM #654

JafarAbdi · 2021-08-31T13:10:10Z

Description

Address #617
Fix: #646

Checklist

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

schornakj · 2021-08-31T13:34:48Z

This fixes the issues I was seeing related to #646. Awesome!

codecov · 2021-08-31T14:27:46Z

Codecov Report

Merging #654 (fee9fb0) into main (7292b9c) will decrease coverage by 0.02%.
The diff coverage is 81.82%.

@@            Coverage Diff             @@
##             main     #654      +/-   ##
==========================================
- Coverage   54.22%   54.21%   -0.01%     
==========================================
  Files         192      192              
  Lines       20227    20224       -3     
==========================================
- Hits        10966    10962       -4     
- Misses       9261     9262       +1

Impacted Files	Coverage Δ
...it/planning_scene_monitor/planning_scene_monitor.h	`33.34% <ø> (ø)`
...anning_scene_monitor/src/current_state_monitor.cpp	`74.41% <ø> (+0.23%)`	⬆️
...nning_scene_monitor/src/planning_scene_monitor.cpp	`45.31% <80.00%> (-0.18%)`	⬇️
moveit_ros/moveit_servo/src/servo_node.cpp	`78.19% <100.00%> (-0.76%)`	⬇️

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 7292b9c...fee9fb0. Read the comment docs.

v4hn

Having the transforms updated either through the PSM tf listener or through a CSM listener is quite ugly.

Why did you not just add a mechanism to start the tf listener in the CSM when it's used from the PSM - whether or not there are MultiDOF joints around?

v4hn · 2021-08-31T20:24:29Z

moveit_ros/planning/planning_scene_monitor/src/current_state_monitor.cpp

-      {
-        RCLCPP_WARN(LOGGER, "The tf2_ros::Buffer is attached to tf2_ros::TransformListener and the internal tf "
-                            "subscribers inside CurrentStateMonitor, you may want to remove the transform listener to "
-                            "avoid duplicate addition to the same transforms");


This is still a very valid warning when CSM is used outside PSM.
There was probably a reason why this was added in the first place and I would be grateful for the feedback in such a case.

I agree, we should keep the warning

I added this warning in the last PR to warn about the previous behavior of using an external tf listener, but since we have an internal one now it will always print this warning which may confuse users

Hmm, so in both cases (PSM+CSM, CSM standalone) we use a dedicated thread for the tf_buffer, right? I guess there is no straight-forward way to check for external listeners anymore. It's fine to remove this warning then, IMO.
Thoughts for a follow-up: Since we don't want external listeners to run at the same time (only PSM is allowed), how about we make the current CSM constructors friend constructors for PSM and only provide public versions without tf_buffer?

moveit_ros/planning/planning_scene_monitor/src/planning_scene_monitor.cpp

henningkayser

I think this is a good step forward, especially removing the buffer from the PSM constructor.

Having the transforms updated either through the PSM tf listener or through a CSM listener is quite ugly.

I totally agree. Ideally, we would be initializing a single listener and then simply registering the CSM callback dynamically. If buffer and listener are maintained by the PSM, this would be straight forward. If we want to be able to run CSM standalone, the buffer and listener need default values in the CSM constructor.

Why did you not just add a mechanism to start the tf listener in the CSM when it's used from the PSM - whether or not there are MultiDOF joints around?

Not sure if I follow you there. Are you basically describing what I wrote above?

moveit_ros/planning/planning_scene_monitor/src/planning_scene_monitor.cpp

henningkayser

+1 after addressing the comments

moveit_ros/planning_interface/common_planning_interface_objects/src/common_objects.cpp

henningkayser · 2021-09-28T21:24:02Z

moveit_ros/planning/planning_scene_monitor/src/current_state_monitor.cpp

-      {
-        RCLCPP_WARN(LOGGER, "The tf2_ros::Buffer is attached to tf2_ros::TransformListener and the internal tf "
-                            "subscribers inside CurrentStateMonitor, you may want to remove the transform listener to "
-                            "avoid duplicate addition to the same transforms");


I agree, we should keep the warning

schornakj

I tested the latest version of this PR in the context of #646 and it also fixes the problems I'd reported there.

moveit_ros/planning/moveit_cpp/src/moveit_cpp.cpp

tylerjw

minor comment to help users transition to the new psm constructor

...anning/planning_scene_monitor/include/moveit/planning_scene_monitor/planning_scene_monitor.h

henningkayser

I think this has turned out pretty great! lgtm

schornakj · 2021-10-06T01:17:18Z

~~One new thing I do see when testing with this branch is new error log messages from the move_group node:~~

[move_group-6] [ERROR] [1633475204.932107474] [tf2_buffer]: Do not call canTransform or lookupTransform with a timeout unless you are using another thread for populating data. Without a dedicated thread it will always timeout.  If you have a separate thread servicing tf messages, call setUsingDedicatedThread(true) on your Buffer instance.

~~This doesn't appear to have a functional effect, though.~~

Update: this error log is from an old version of the PR, so there's nothing that needs to be fixed here.

moveit_ros/planning/planning_scene_monitor/src/planning_scene_monitor.cpp

schornakj

Explicit re-approval: I tested with the latest version of this PR and it works as expected.

henningkayser · 2021-10-06T19:37:19Z

Explicit re-approval: I tested with the latest version of this PR and it works as expected.

Thanks for looking into this again

JafarAbdi requested a review from henningkayser August 31, 2021 13:10

v4hn reviewed Aug 31, 2021

View reviewed changes

henningkayser reviewed Aug 31, 2021

View reviewed changes

moveit_ros/planning/planning_scene_monitor/src/planning_scene_monitor.cpp Outdated Show resolved Hide resolved

henningkayser added this to In Progress in Galactic/Rolling - 2.3.0 - September 10 Sep 7, 2021

henningkayser added the blocks release This PR should be merged before the next release label Sep 14, 2021

henningkayser moved this from In Progress to Review in progress in Galactic/Rolling - 2.3.0 - September 10 Sep 21, 2021

JafarAbdi force-pushed the pr-private_buffer branch from 24a9f22 to 7bad500 Compare September 28, 2021 17:32

henningkayser reviewed Sep 28, 2021

View reviewed changes

henningkayser approved these changes Sep 29, 2021

View reviewed changes

schornakj approved these changes Sep 29, 2021

View reviewed changes

henningkayser reviewed Sep 30, 2021

View reviewed changes

moveit_ros/planning/moveit_cpp/src/moveit_cpp.cpp Outdated Show resolved Hide resolved

tylerjw reviewed Oct 3, 2021

View reviewed changes

...anning/planning_scene_monitor/include/moveit/planning_scene_monitor/planning_scene_monitor.h Show resolved Hide resolved

JafarAbdi added 4 commits October 5, 2021 14:15

Add private buffer & tf listener to PSM

858c07c

Remove coupled deleter

0729ea8

Decouple PSM from CSM

6cbe2c4

Deprecate old constructors

c6443a4

JafarAbdi force-pushed the pr-private_buffer branch from 28f1911 to c6443a4 Compare October 5, 2021 11:54

henningkayser approved these changes Oct 5, 2021

View reviewed changes

henningkayser reviewed Oct 6, 2021

View reviewed changes

moveit_ros/planning/planning_scene_monitor/src/planning_scene_monitor.cpp Show resolved Hide resolved

schornakj approved these changes Oct 6, 2021

View reviewed changes

Merge branch 'main' into pr-private_buffer

fee9fb0

henningkayser merged commit 656180d into moveit:main Oct 7, 2021

Galactic/Rolling - 2.3.0 - September 10 automation moved this from Review in progress to Done Oct 7, 2021

JafarAbdi mentioned this pull request Oct 12, 2021

Refactor PSM & CSM to have private tf_buffer & tf_listener #617

Closed

This was referenced Oct 20, 2021

PSM updateFrameTransforms() does not work with externally-published tf frames moveit/moveit#2921

Open

Servo: Enable usage of transforms published during runtime moveit/moveit#2894

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add private buffer & tf listener to PSM #654

Add private buffer & tf listener to PSM #654

JafarAbdi commented Aug 31, 2021

schornakj commented Aug 31, 2021

codecov bot commented Aug 31, 2021 •

edited

Loading

v4hn left a comment

v4hn Aug 31, 2021

henningkayser Sep 28, 2021

JafarAbdi Sep 29, 2021

henningkayser Sep 29, 2021

henningkayser left a comment •

edited

Loading

henningkayser left a comment

henningkayser Sep 28, 2021

schornakj left a comment

tylerjw left a comment

henningkayser left a comment

schornakj commented Oct 6, 2021 •

edited

Loading

schornakj left a comment

henningkayser commented Oct 6, 2021

Add private buffer & tf listener to PSM #654

Add private buffer & tf listener to PSM #654

Conversation

JafarAbdi commented Aug 31, 2021

Description

Checklist

schornakj commented Aug 31, 2021

codecov bot commented Aug 31, 2021 • edited Loading

Codecov Report

v4hn left a comment

Choose a reason for hiding this comment

v4hn Aug 31, 2021

Choose a reason for hiding this comment

henningkayser Sep 28, 2021

Choose a reason for hiding this comment

JafarAbdi Sep 29, 2021

Choose a reason for hiding this comment

henningkayser Sep 29, 2021

Choose a reason for hiding this comment

henningkayser left a comment • edited Loading

Choose a reason for hiding this comment

henningkayser left a comment

Choose a reason for hiding this comment

henningkayser Sep 28, 2021

Choose a reason for hiding this comment

schornakj left a comment

Choose a reason for hiding this comment

tylerjw left a comment

Choose a reason for hiding this comment

henningkayser left a comment

Choose a reason for hiding this comment

schornakj commented Oct 6, 2021 • edited Loading

schornakj left a comment

Choose a reason for hiding this comment

henningkayser commented Oct 6, 2021

codecov bot commented Aug 31, 2021 •

edited

Loading

henningkayser left a comment •

edited

Loading

schornakj commented Oct 6, 2021 •

edited

Loading