Skip to content
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

Feature/add validate traj exec param #883

Conversation

Projects
None yet
2 participants
@srsidd
Copy link
Contributor

commented May 4, 2018

Description

This addresses #840.

Checklist

  • Required: Code is auto formatted using clang-format
  • Extended the tutorials / documentation, if necessary reference
  • Include a screenshot if changing a GUI
  • Optional: Created tests, which fail without this PR reference
  • Optional: Decide if this should be cherry-picked to other current ROS branches (Indigo, Jade, Kinetic)
@rhaschke
Copy link
Collaborator

left a comment

I'm not at all convinced that this addition is useful. However, if accepted by other maintainers, I request some changes as indicated in inline comments.

To emphasize the necessity of this PR, you should provide a unittest illustrating your usecase.

if (allowed_start_tolerance_ == 0) // skip validation on this magic number
// skip validation
if (not validate_trajectory_complete_) {
ROS_DEBUG_NAMED(name_, "Not validating if trajectory execution is complete");

This comment has been minimized.

Copy link
@rhaschke

rhaschke May 4, 2018

Collaborator

This validation concerns deviations of start start from current state. It's not related to completion!
This change should be reverted!

@@ -348,6 +351,7 @@ class TrajectoryExecutionManager
double allowed_goal_duration_margin_;
double allowed_start_tolerance_; // joint tolerance for validate(): radians for revolute joints
double execution_velocity_scaling_;
bool validate_trajectory_complete_;

This comment has been minimized.

Copy link
@rhaschke

rhaschke May 4, 2018

Collaborator

In #840 we discussed to have a double parameter, e.g. allowed_goal_tolerance_, similar to allowed_start_tolerance_. They should be initialized from each other if only one is specified.
However, as this parameter is dynamically reconfigurable, that's probably not feasible: they are always explicitly set by DynamicReconfigure.
If we keep a bool here, I suggest wait_for_completion_

@@ -71,6 +71,7 @@ class TrajectoryExecutionManager::DynamicReconfigureImpl
owner_->setAllowedGoalDurationMargin(config.allowed_goal_duration_margin);
owner_->setExecutionVelocityScaling(config.execution_velocity_scaling);
owner_->setAllowedStartTolerance(config.allowed_start_tolerance);
owner_->enableValidateTrajectoryExecution(config.validate_trajectory_complete);

This comment has been minimized.

Copy link
@rhaschke

rhaschke May 4, 2018

Collaborator

Use name setWaitForCompletion()?

@@ -1511,8 +1520,11 @@ bool TrajectoryExecutionManager::executePart(std::size_t part_index)

bool TrajectoryExecutionManager::waitForRobotToStop(const TrajectoryExecutionContext& context, double wait_time)
{
if (allowed_start_tolerance_ == 0) // skip validation on this magic number
// skip validation

This comment has been minimized.

Copy link
@rhaschke

rhaschke May 4, 2018

Collaborator

Here the check should read
if (allowed_start_tolerance_ == 0 || !wait_for_completion_)

This comment has been minimized.

Copy link
@srsidd

srsidd Jul 20, 2018

Author Contributor

Fixed.

Siddharth Srivatsa added some commits May 4, 2018

@srsidd srsidd force-pushed the srsidd:feature/add-validate-traj-exec-param branch from 3a7256c to 025c847 Jul 18, 2018

Siddharth Srivatsa and others added some commits Jul 20, 2018

@rhaschke rhaschke merged commit db6f760 into ros-planning:kinetic-devel Aug 23, 2018

1 check passed

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

rhaschke added a commit that referenced this pull request Aug 23, 2018

added 'wait_for_trajectory_completion' parameter (#883)
Introduce new dynamic-reconfigure parameter wait_for_trajectory_completion to disable waiting for convergence independently from start-state checking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.