-
Notifications
You must be signed in to change notification settings - Fork 345
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
[JTC] Use time of the last command for set_point_before_trajectory_msg in open-loop mode #780
[JTC] Use time of the last command for set_point_before_trajectory_msg in open-loop mode #780
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #780 +/- ##
==========================================
- Coverage 84.66% 84.64% -0.02%
==========================================
Files 123 123
Lines 11419 11425 +6
Branches 961 964 +3
==========================================
+ Hits 9668 9671 +3
Misses 1449 1449
- Partials 302 305 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Looks good for me (even more if the mentioned tests are added)
concerning closed-loop mode: I'm not sure if this is necessary, shouldn't the trajectory generator be aware of the current state? But I guess there might be configurations where this could improve something.
This is important only when trajectory replacement is done. My comment was along: "if strange behavior happens when replacing trajectories - it might be that the state of is too |
I'm asking myself if it is desirable at all to interpolate from measured state instead of the last command with closed-loop config. but that really depends on the algorithm generating the trajectory |
4a85ca0
to
8fc35dc
Compare
This pull request is in conflict. Could you fix it @destogl? |
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.
test_no_jump_when_state_tracking_error_not_updated
fails
This pull request is in conflict. Could you fix it @destogl? |
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.
LGTM, I fixed the test now.
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.
Looks good to me, just need to change open-loop
to be interpolate_from_desired_state
according to #1525 but I do not have the permissions for this
for this PR it is fine with open-loop, if you approve we can merge that immediately. we then can change that in your PR, where we have to wait for a review of another maintainer anyways |
fc20a27
into
ros-controls:master
This PR tackles the issues that happen in open-loop mode where on replacing trajectories the last command is repeated.
The figures below show the results of the output trajectories before and after this functionality. The trajectory that uses the time of the last command as point before trajectory messages gets smoother output.
Functionality right now:

Functionality with this PR:

P.S. The similar thing would be useful for trajectory replacement in closed loop mode, but then we would need to know exactly when the state is read from the hardware. This is just something to think about in the future.
P.P.S. I will add tests too.