-
Notifications
You must be signed in to change notification settings - Fork 324
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
Partial joints #68
Partial joints #68
Conversation
Codecov Report
@@ Coverage Diff @@
## master #68 +/- ##
==========================================
+ Coverage 33.12% 35.78% +2.66%
==========================================
Files 12 14 +2
Lines 1123 1358 +235
Branches 740 879 +139
==========================================
+ Hits 372 486 +114
+ Misses 103 91 -12
- Partials 648 781 +133
Continue to review full report at Codecov.
|
@ddengster Could I get your take on these changes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR and sorry for the late reply. In general the only bug to fix is the joint->get_position()
call.
const auto & joint_state = registered_joint_state_handles_[index]; | ||
for (auto & it : trajectory_msg->points) { | ||
// Assume hold position with 0 velocity and acceleration for missing joints | ||
it.positions.push_back(joint_state->get_position()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is called on a separate thread from the main JTC updating loop, you'll get some kind of race condition if you call joint_state->get_position()
here.
traj_external_point_ptr_->update(msg);
is where we sync up, we could put some kind of coordination there or delay it until the first parsing of the message (this is how I did immediate playbacks of trajectory messages where messages are sent with time 0). Perhaps we could use a boolean per joint to indicate whether to retain the last known joint position.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for noticing this, it is indeed a problem.
Furthermore, On the topic callback we update the trajectory without acquiring the trajectory_mtx_
which is another race condition.
Probably when we receive a trajectory (whether from topic or action) on an "auxiliary thread" we should do some basic checks to ensure that we can accept it, and store it somewhere protected by a mutex.
Then on the next update
call on the "main thread", we should process it, fill the partial goals and do whatever needs to be done.
Something along these lines I believe would fix these issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed a fix for these issues on this same branch. 188c0e4
Fixes #31
Fixes #37