[jade] validate trajectory before execution#225
Conversation
* validate trajectory before execution * addressed review comments * moved validateTrajectory to TrajectoryExecutionManager * addressed @davetcoleman's comments * make allowed_start_tolerance dynamically configurable * addressed @v4hn's comments * moved validate to executeThread * allow_start_tolerance == 0 disables trajectory validation * moved validate to execute() * add validation test * increased default value for allowed_start_tolerance to 0.01
|
As discussed in #63 (comment) , |
|
I agree with @v4hn |
|
OK, I will prepare this. |
|
Why not deprecate the old constructor in Indigo too? This is a security feature and we should enforce/motivate people to switch asap. |
|
If the deprecation is simple a warning during compile time, +1 to adding it to indigo too |
davetcoleman
left a comment
There was a problem hiding this comment.
Looks good after comments
| bool TrajectoryExecutionManager::validate(const TrajectoryExecutionContext &context) const | ||
| { | ||
| if (allowed_start_tolerance_ == 0) // skip validation on this magic number | ||
| if (!csm_ || allowed_start_tolerance_ == 0) // skip validation on this magic number |
There was a problem hiding this comment.
improve comment for new condition
| reconfigure_impl_ = new DynamicReconfigureImpl(this); | ||
|
|
||
| if (!csm_) | ||
| ROS_ERROR_NAMED("traj_execution","Trajectory validation before execution is disabled, because no CurrentStateMonitor was provided."); |
There was a problem hiding this comment.
This sentence was hard to read - remove the "before execution" part
Also, I think this should just be a warning
* re-added replaced constructors (but deprecated) * issue error msg when CSM is invalid * skip validation
5daede8 to
8c7e39c
Compare
|
@davetcoleman I addressed your comments. According to #225 (comment) this could be cherry-picked to Indigo 1:1. |
|
+1 |
|
@davetcoleman Any reason to not merge yet? |
|
feel free to merge (my pr) and cherry-pick to indigo |
|
Cherry-picked into Indigo. |
Discussion continued from #63 (comment)
@rhaschke feel free to make changes to this shared branch as necessary