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

[jade] validate trajectory before execution #225

Merged
merged 3 commits into from Sep 17, 2016

Conversation

Projects
None yet
3 participants
@davetcoleman
Member

davetcoleman commented Sep 16, 2016

Discussion continued from #63 (comment)

@rhaschke feel free to make changes to this shared branch as necessary

rhaschke and others added some commits Sep 16, 2016

validate trajectory before execution (#63)
* 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
@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Sep 16, 2016

Member

As discussed in #63 (comment) ,
the idea was to remove the old constructor in kinetic (for safety reasons) but still provide it in indigo.
Because jade has been officially released in the meanwhile, we should also provide the old constructor in jade. I would propose to deprecate the old constructor in jade.

Member

v4hn commented Sep 16, 2016

As discussed in #63 (comment) ,
the idea was to remove the old constructor in kinetic (for safety reasons) but still provide it in indigo.
Because jade has been officially released in the meanwhile, we should also provide the old constructor in jade. I would propose to deprecate the old constructor in jade.

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Sep 16, 2016

Member

I agree with @v4hn

Member

davetcoleman commented Sep 16, 2016

I agree with @v4hn

@rhaschke

This comment has been minimized.

Show comment
Hide comment
@rhaschke

rhaschke Sep 16, 2016

Contributor

OK, I will prepare this.

Contributor

rhaschke commented Sep 16, 2016

OK, I will prepare this.

@rhaschke

This comment has been minimized.

Show comment
Hide comment
@rhaschke

rhaschke Sep 16, 2016

Contributor

Why not deprecate the old constructor in Indigo too? This is a security feature and we should enforce/motivate people to switch asap.

Contributor

rhaschke commented Sep 16, 2016

Why not deprecate the old constructor in Indigo too? This is a security feature and we should enforce/motivate people to switch asap.

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Sep 16, 2016

Member

If the deprecation is simple a warning during compile time, +1 to adding it to indigo too

Member

davetcoleman commented Sep 16, 2016

If the deprecation is simple a warning during compile time, +1 to adding it to indigo too

@davetcoleman

Looks good after comments

Show outdated Hide outdated ...anning/trajectory_execution_manager/src/trajectory_execution_manager.cpp
@@ -872,7 +885,7 @@ bool TrajectoryExecutionManager::distributeTrajectory(const moveit_msgs::RobotTr
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

This comment has been minimized.

@davetcoleman

davetcoleman Sep 16, 2016

Member

improve comment for new condition

@davetcoleman

davetcoleman Sep 16, 2016

Member

improve comment for new condition

Show outdated Hide outdated ...anning/trajectory_execution_manager/src/trajectory_execution_manager.cpp
@@ -171,6 +181,9 @@ void TrajectoryExecutionManager::initialize()
reconfigure_impl_ = new DynamicReconfigureImpl(this);
if (!csm_)
ROS_ERROR_NAMED("traj_execution","Trajectory validation before execution is disabled, because no CurrentStateMonitor was provided.");

This comment has been minimized.

@davetcoleman

davetcoleman Sep 16, 2016

Member

This sentence was hard to read - remove the "before execution" part

Also, I think this should just be a warning

@davetcoleman

davetcoleman Sep 16, 2016

Member

This sentence was hard to read - remove the "before execution" part

Also, I think this should just be a warning

retain API compatibility
* re-added replaced constructors (but deprecated)
* issue error msg when CSM is invalid
* skip validation
@rhaschke

This comment has been minimized.

Show comment
Hide comment
@rhaschke

rhaschke Sep 16, 2016

Contributor

@davetcoleman I addressed your comments. According to #225 (comment) this could be cherry-picked to Indigo 1:1.

Contributor

rhaschke commented Sep 16, 2016

@davetcoleman I addressed your comments. According to #225 (comment) this could be cherry-picked to Indigo 1:1.

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
Member

davetcoleman commented Sep 16, 2016

+1

@rhaschke

This comment has been minimized.

Show comment
Hide comment
@rhaschke

rhaschke Sep 16, 2016

Contributor

@davetcoleman Any reason to not merge yet?

Contributor

rhaschke commented Sep 16, 2016

@davetcoleman Any reason to not merge yet?

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Sep 17, 2016

Member

feel free to merge (my pr) and cherry-pick to indigo

Member

davetcoleman commented Sep 17, 2016

feel free to merge (my pr) and cherry-pick to indigo

@rhaschke rhaschke merged commit 8c7e39c into jade-devel Sep 17, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@rhaschke rhaschke deleted the pr-jade-validate-trajectory branch Sep 17, 2016

@rhaschke

This comment has been minimized.

Show comment
Hide comment
@rhaschke

rhaschke Sep 17, 2016

Contributor

Cherry-picked into Indigo.

Contributor

rhaschke commented Sep 17, 2016

Cherry-picked into Indigo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment