Resolve PSI lock-up in RViz display #1951
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rhaschke
approved these changes
Mar 12, 2020
| { | ||
| if (!planning_scene_service_.exists() || !apply_planning_scene_service_.exists()) | ||
| { | ||
| throw std::runtime_error("ROS Services not available"); |
There was a problem hiding this comment.
Suggested change
| throw std::runtime_error("ROS Services not available"); | |
| throw std::runtime_error("ROS services not available"); |
It is still impossible to create a PSI if no move_group is running, but this allows users to detect the failure to create an instance. Might be useful for people who want to run the same node with and without a `move_group` node.
v4hn
force-pushed
the
pr-master-psi-lockups
branch
from
1024a30
to
1916d2c
Compare
The display maintains a PSM listening to move_group's PSM. Using an additional PSI (connecting to the move_group via services) is bad design and the use of it - to poll via ros-services - is even worse. Implemented the same functionality through the local PSM. Note that the second argument of updateDetectedObjectsList was never used.
v4hn
force-pushed
the
pr-master-psi-lockups
branch
from
1916d2c
to
acaebbf
Compare
|
Do you want to merge this as a merge commit, keeping the two rather independent commits? |
That was the idea, yes. Still waiting for CI though after clang-tidy was still complaining. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Supersedes #1950 .
I implemented an alternative way to construct a PSI, enabling users to detect issues with the ROS services instead of locking up the thread waiting for them.
It does not harm to have this option around.
Looking at the RViz display code again, I decided against making use of this feature here though.
The display already maintains a PSM, so I rewrote the logic to use this instead and altogether got rid of the PSI.