-
Notifications
You must be signed in to change notification settings - Fork 430
Fill point_before_trajectory with same information as trajectory #2043
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
Fill point_before_trajectory with same information as trajectory #2043
Conversation
When executing a trajectory with a start time in the future, the trajectory interpolation will create a setpoint at the robot's current configuration based on the state interface information. However, when the state interface for example doesn't have acceleration information, but the trajectory does contain position, velocity and acceleration, the segment from the current state to the first trajectory point will not use quintic spline interpolation, though the rest of the trajectory will. This commit fills the current state with velocity and acceleration information if it is not given in the state interfaces but it is defined in the trajectory's first point.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2043 +/- ##
=======================================
Coverage 84.93% 84.94%
=======================================
Files 148 148
Lines 14389 14425 +36
Branches 1233 1235 +2
=======================================
+ Hits 12221 12253 +32
- Misses 1740 1742 +2
- Partials 428 430 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
christophfroehlich
left a comment
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 don't understand what the abi checker is complaining about, either. I think backporting it all the way back to humble might make sense. As I wrote earlier, an opt-in flag could be added do not surprisingly change behavior for users. Not that I would expecmany users would notice. Though trajectory starts will be less jerky which might be noticable in some cases. |
|
Ok, I'm fine with that. Would you mind adding something to the release notes then? |
|
Sure thing. I'll try to do it tonight or tomorrow. But just to be sure: What would be the plan to backport this? Should we add the opt-in flag or just backport as is? In the first case I would note that as a change to Lyrical that this is now the default behavior. |
I personally think this is a very valid behaviour and can be backported |
saikishor
left a comment
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. Just release notes as @christophfroehlich mentioned
0eca296
|
Added a release notes entry. |
79f917e
into
ros-controls:master
(cherry picked from commit 79f917e)
(cherry picked from commit 79f917e)
(cherry picked from commit 79f917e) # Conflicts: # doc/release_notes.rst

When executing a trajectory with a start time in the future, the trajectory interpolation will create a setpoint at the robot's current configuration based on the state interface information.
However, when the state interface for example doesn't have acceleration information, but the trajectory does contain position, velocity and acceleration, the segment from the current state to the first trajectory point will not use quintic spline interpolation, though the rest of the trajectory will.
This commit fills the current state with velocity and acceleration information if it is not given in the state interfaces but it is defined in the trajectory's first point.
This fixes #2015
Velocity profile with first point in the future / at other position without this PR applied:

Velocity profile with first point in the future / at other position with this PR applied:

I know that this is strictly speaking behavior changing, but I would nevertheless consider this a bugfix. This could, of course get an opt-in parameter on released versions to avoid the behavior change if desired.