-
Notifications
You must be signed in to change notification settings - Fork 525
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
Remove deprecated parameter hold_trajectory_duration #386
Remove deprecated parameter hold_trajectory_duration #386
Conversation
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 this.
Even though I'm approving now the CI may still fail if we have tests directly aimed at this. If any, those will need to be removed.
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 change also removes the parameter we'd like to keep :)
@@ -229,6 +229,7 @@ bool JointTrajectoryController<SegmentImpl, HardwareInterface>::init(HardwareInt | |||
|
|||
// Stop trajectory duration | |||
stop_trajectory_duration_ = 0.0; | |||
controller_nh_.getParam("stop_trajectory_duration", stop_trajectory_duration_); |
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.
For semantic reasons I'd prefer
stop_trajectory_duration_ = controller_nh_.param("stop_trajectory_duration", 0.0);
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.
There is a copy&paste bug in the new commit. I rolled back to the original version again.
In addition the other parameters are retrieved the same way, so let's keep it like this for now.
Tests pass again, |
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.
Other than the point @ipa-mdl raised I'm happy with the PR. Please feel free to merge once done
f8a907a
to
c7ef6e6
Compare
No description provided.