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 group states #1781

Merged
merged 2 commits into from
Dec 3, 2019
Merged

Fix group states #1781

merged 2 commits into from
Dec 3, 2019

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Nov 26, 2019

As pointed out in #1780 (comment),

  1. The robot model loader doesn't complain about malformed group states (having missing joints).
  2. FakeControllers used those uninitialized joint values for publishing.

Both issues are fixed with this PR. This should fix the flaky moveit-cpp unit test.

@rhaschke rhaschke requested review from henningkayser and JafarAbdi and removed request for jonbinney November 26, 2019 12:14
@rhaschke rhaschke mentioned this pull request Nov 26, 2019
6 tasks
@@ -349,6 +354,8 @@ void RobotModel::buildGroupStates(const srdf::Model& srdf_model)
"but that joint is not part of group '%s'",
group_state.name_.c_str(), jt->first.c_str(), jmg->getName().c_str());
}
if (!remaining_joints.empty())
Copy link
Member

@henningkayser henningkayser Nov 26, 2019

Choose a reason for hiding this comment

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

Additionally to throwing a warning, default values should be 0 or the state might be invalid. Alternatively, we could also make this an error and don't add the state at all. Or do you think we should allow incomplete states in order to support values for unspecified "subgroups"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The group state is essentially a map: joint name -> value. Hence, it is fine to leave some values "undefined" (not listed in the map) as long as setToDefaultState starts from a fully-defined state.
This should be the case anyway as the there might be joints outside the JMG, which will definitely not be modified by setToDefaultState. The fake controller manager takes care of this (now).

@rhaschke
Copy link
Contributor Author

@henningkayser, @JafarAbdi: This fix doesn't resolve the failing unit test. Same for #1780.

@JafarAbdi
Copy link
Contributor

@rhaschke @henningkayser I debugged this exception

terminate called after throwing an instance of 'boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<boost::lock_error> >'
  what():  boost: mutex lock failed in pthread_mutex_lock: Invalid argument

I think the reason is a dangling reference, the exception happens in this line

In the MoveItCpp constructor, we pass tf_buffer_ to the tf_listener which takes a reference and store it as reference, now when the MoveItCpp destructor is called it will reset tf_buffer_ which will cause the pointee to be destructed while there's another thread that uses the object when we default the destructor the tf_listener_ will be destructed before tf_buffer_ and the exception won't occur

This's the stack call for a reference

Screenshot from 2019-12-02 16-59-41

@rhaschke
Copy link
Contributor Author

rhaschke commented Dec 2, 2019

Just fix the order of resetting variables in the custom destructor:

  1. tf_listener_
  2. tf_buffer_

@codecov-io
Copy link

codecov-io commented Dec 2, 2019

Codecov Report

Merging #1781 into master will increase coverage by 0.72%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1781      +/-   ##
=========================================
+ Coverage   47.58%   48.3%   +0.72%     
=========================================
  Files         290     294       +4     
  Lines       22950   23232     +282     
=========================================
+ Hits        10920   11223     +303     
+ Misses      12030   12009      -21
Impacted Files Coverage Δ
...s/planning_interface/moveit_cpp/src/moveit_cpp.cpp 62.29% <ø> (ø)
moveit_core/robot_model/src/robot_model.cpp 77.2% <100%> (+0.97%) ⬆️
...ler_manager/src/moveit_fake_controller_manager.cpp 63.44% <100%> (+23.22%) ⬆️
...ng_interface/moveit_cpp/src/planning_component.cpp 48.43% <100%> (ø)
...cpp/include/moveit/moveit_cpp/planning_component.h 100% <0%> (ø)
.../moveit_cpp/include/moveit/moveit_cpp/moveit_cpp.h 100% <0%> (ø)
moveit_core/robot_state/src/conversions.cpp 30.94% <0%> (+0.44%) ⬆️
moveit_core/robot_model/src/joint_model_group.cpp 59.39% <0%> (+0.6%) ⬆️
... and 13 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 5fab740...601f550. Read the comment docs.

rhaschke and others added 2 commits December 3, 2019 12:06
- issue a warning when building the robot model
- use a RobotState initialized to joint defaults for fake controllers
- update robot state before planning
- release tf_listener_ and tf_buffer_ in correct order
- publish virtual joint
@rhaschke rhaschke merged commit 1a308e4 into moveit:master Dec 3, 2019
@rhaschke rhaschke deleted the fix-group-states branch December 3, 2019 11:38
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.

None yet

4 participants