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

validate trajectory before execution #63

Merged
merged 11 commits into from Sep 16, 2016

Conversation

Projects
None yet
6 participants
@rhaschke
Contributor

rhaschke commented Aug 17, 2016

Extracted trajectory validation from ros-planning/moveit_ros#716 and ros-planning/moveit_ros#728. First point of trajectory is validated to match current robot state just before execution.

  • First commit has code from previous PRs.
  • Second commit addresses review comments already raised there.
  • Third commit moves validation into TrajectoryExecutionManager. This requires some API changes, but ensures validation in any case.

This should be cherry-picked into Indigo/Jade finally.

Show outdated Hide outdated ...clude/moveit/trajectory_execution_manager/trajectory_execution_manager.h
@@ -288,6 +291,7 @@ class TrajectoryExecutionManager
bool execution_duration_monitoring_;
double allowed_execution_duration_scaling_;
double allowed_goal_duration_margin_;
double allowed_start_deviation_;

This comment has been minimized.

@davetcoleman

davetcoleman Aug 17, 2016

Member

add comment here explaining units - for revolute joints this would be tolerance of radians amount correct?

@davetcoleman

davetcoleman Aug 17, 2016

Member

add comment here explaining units - for revolute joints this would be tolerance of radians amount correct?

Show outdated Hide outdated ...anning/trajectory_execution_manager/src/trajectory_execution_manager.cpp
if (!node_handle_.getParam("allowed_goal_duration_margin", allowed_goal_duration_margin_))
allowed_goal_duration_margin_ = DEFAULT_CONTROLLER_GOAL_DURATION_MARGIN;
if (!node_handle_.getParam("allowed_start_deviation", allowed_start_deviation_))

This comment has been minimized.

@davetcoleman

davetcoleman Aug 17, 2016

Member

can you add an example of this setting (and perhaps the other two above) to the setup_assistant templates within this PR?

@davetcoleman

davetcoleman Aug 17, 2016

Member

can you add an example of this setting (and perhaps the other two above) to the setup_assistant templates within this PR?

Show outdated Hide outdated ...anning/trajectory_execution_manager/src/trajectory_execution_manager.cpp
robot_state::RobotStatePtr current_state;
if (!csm_->waitForCurrentState(1.0) || !(current_state = csm_->getCurrentState()))
{
ROS_WARN_NAMED("traj_execution", "Failed to validate trajectory: couldn't receive full current joint state");

This comment has been minimized.

@davetcoleman

davetcoleman Aug 17, 2016

Member

append: "within 1 second"

@davetcoleman

davetcoleman Aug 17, 2016

Member

append: "within 1 second"

Show outdated Hide outdated ...anning/trajectory_execution_manager/src/trajectory_execution_manager.cpp
const std::vector<double> &positions = trajectory.joint_trajectory.points.front().positions;
const std::vector<std::string> &joint_names = trajectory.joint_trajectory.joint_names;
std::size_t n = joint_names.size();

This comment has been minimized.

@davetcoleman

davetcoleman Aug 17, 2016

Member

const

Show outdated Hide outdated ...anning/trajectory_execution_manager/src/trajectory_execution_manager.cpp
// TODO: check multi-DoF joints ?
if (fabs(current_state->getJointPositions(jm)[0] - positions[i]) > allowed_start_deviation_)
{
ROS_ERROR_NAMED("traj_execution", "Invalid Trajectory: start point deviates from current robot state");

This comment has been minimized.

@davetcoleman

davetcoleman Aug 17, 2016

Member

append " more than X amount"

@davetcoleman

davetcoleman Aug 17, 2016

Member

append " more than X amount"

Show outdated Hide outdated ...clude/moveit/trajectory_execution_manager/trajectory_execution_manager.h
@@ -219,6 +220,7 @@ class TrajectoryExecutionManager
void reloadControllerInformation();
bool validate(const moveit_msgs::RobotTrajectory &trajectory) const;

This comment has been minimized.

@davetcoleman

davetcoleman Aug 17, 2016

Member

need to specify in function name or comments what this validates, e.g.

// First point of trajectory is validated to match current robot state just before execution.

@davetcoleman

davetcoleman Aug 17, 2016

Member

need to specify in function name or comments what this validates, e.g.

// First point of trajectory is validated to match current robot state just before execution.

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Aug 17, 2016

Member

Looks like a great improvement, thanks

Member

davetcoleman commented Aug 17, 2016

Looks like a great improvement, thanks

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
Member

davetcoleman commented Aug 18, 2016

+1

@rhaschke

This comment has been minimized.

Show comment
Hide comment
@rhaschke

rhaschke Aug 18, 2016

Contributor

@v4hn Could you have a look too?

Contributor

rhaschke commented Aug 18, 2016

@v4hn Could you have a look too?

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Aug 18, 2016

Member

On Thu, Aug 18, 2016 at 04:02:10AM -0700, Robert Haschke wrote:

@v4hn Could you have a look too?

I will do that over the weekend.
Sorry I'm over the top stressed at the moment.

Member

v4hn commented Aug 18, 2016

On Thu, Aug 18, 2016 at 04:02:10AM -0700, Robert Haschke wrote:

@v4hn Could you have a look too?

I will do that over the weekend.
Sorry I'm over the top stressed at the moment.

@rhaschke

This comment has been minimized.

Show comment
Hide comment
@rhaschke

rhaschke Aug 18, 2016

Contributor

I will do that over the weekend.

Great. Thanks.

Contributor

rhaschke commented Aug 18, 2016

I will do that over the weekend.

Great. Thanks.

Show outdated Hide outdated ...ajectory_execution_manager/cfg/TrajectoryExecutionDynamicReconfigure.cfg
@@ -7,5 +7,6 @@ gen.add("execution_duration_monitoring", bool_t, 1, "Monitor the execution durat
gen.add("allowed_execution_duration_scaling", double_t, 2, "Accept durations that take a little more time than specified", 1.1, 1, 10)
gen.add("allowed_goal_duration_margin", double_t, 3, "Allow more than the expected execution time before triggering a trajectory cancel (applied after scaling)", 0.5, 0.1, 5)
gen.add("execution_velocity_scaling", double_t, 4, "Multiplicative factor for execution speed", 1, 0.1, 10)
gen.add("allowed_start_tolerance", double_t, 5, "Allowed joint-value tolerance for validation of trajectory's start point against current robot state", 1e-3, 0);

This comment has been minimized.

@v4hn

v4hn Aug 22, 2016

Member

1e-3 feels rather small for a hard threshold.
Everyone can adapt this if required, but I would prefer 0.01 as the default in order to prevent false-positives.
We don't want people to trip over it, we want it to support the user.

@v4hn

v4hn Aug 22, 2016

Member

1e-3 feels rather small for a hard threshold.
Everyone can adapt this if required, but I would prefer 0.01 as the default in order to prevent false-positives.
We don't want people to trip over it, we want it to support the user.

This comment has been minimized.

@rhaschke

rhaschke Aug 23, 2016

Contributor

1e-3 is exactly the value you proposed previously: ros-planning/moveit_ros#728 (comment). With a 1m lever this is about 1mm, which is fine, while 1e-2 would be 1cm already.

@rhaschke

rhaschke Aug 23, 2016

Contributor

1e-3 is exactly the value you proposed previously: ros-planning/moveit_ros#728 (comment). With a 1m lever this is about 1mm, which is fine, while 1e-2 would be 1cm already.

This comment has been minimized.

@v4hn

v4hn Aug 23, 2016

Member
@v4hn

v4hn via email Aug 23, 2016

Member

This comment has been minimized.

@v4hn

v4hn Aug 23, 2016

Member

the noise in the reported joint states of the ur5 at rest is around 8e-5.
What about other platforms?

@davetcoleman what do you think about the value of the threshold?

@v4hn

v4hn Aug 23, 2016

Member

the noise in the reported joint states of the ur5 at rest is around 8e-5.
What about other platforms?

@davetcoleman what do you think about the value of the threshold?

This comment has been minimized.

@davetcoleman

davetcoleman Aug 23, 2016

Member

considering 8e-5 is what UR5 has, seems like a good number. let's set it to whatever the noise of baxter is... i doubt any robot could be nosier, lol

@IanTheEngineer

if this is a tricky parameter to tune we should document it for others to tune themselves per robot. i'd recommend a new tutorial named "Tuning Magic Numbers for Real Robots" and list all the parameters in our various yaml and roslaunch files

@davetcoleman

davetcoleman Aug 23, 2016

Member

considering 8e-5 is what UR5 has, seems like a good number. let's set it to whatever the noise of baxter is... i doubt any robot could be nosier, lol

@IanTheEngineer

if this is a tricky parameter to tune we should document it for others to tune themselves per robot. i'd recommend a new tutorial named "Tuning Magic Numbers for Real Robots" and list all the parameters in our various yaml and roslaunch files

This comment has been minimized.

@rhaschke

rhaschke Aug 24, 2016

Contributor

Having 8e-5 on UR5, the suggested 1e-3 seems to be a good compromise. It's safe with regard to a 1m lever only "jumping" 1mm in that case. I agree to @v4hn that the value can be made more restrictive if necessary. I wouldn't increase further (e.g. to 1e-2 as suggested in https://github.com/ros-planning/moveit/pull/63/files#r75665102), because that would move by 1cm.

@rhaschke

rhaschke Aug 24, 2016

Contributor

Having 8e-5 on UR5, the suggested 1e-3 seems to be a good compromise. It's safe with regard to a 1m lever only "jumping" 1mm in that case. I agree to @v4hn that the value can be made more restrictive if necessary. I wouldn't increase further (e.g. to 1e-2 as suggested in https://github.com/ros-planning/moveit/pull/63/files#r75665102), because that would move by 1cm.

This comment has been minimized.

@davetcoleman

davetcoleman Sep 9, 2016

Member

can you add a comment in the code re-capping how we've arrived at this default value?

@davetcoleman

davetcoleman Sep 9, 2016

Member

can you add a comment in the code re-capping how we've arrived at this default value?

This comment has been minimized.

@v4hn

v4hn Sep 13, 2016

Member

Looking at this again, I believe it is unnecessarily complex to have this parameter in dynamic reconfigure.

  • it's one more place where the default value has to be provided
  • if anyone wants to add per-joint thresholds later on, the meaning of this parameter becomes "default if none is specified"
    This meaning does not deserved to be changed dynamically imho
  • Personally I don't see a reason why anyone would ever change that value during runtime. This is what dynamic reconfigure is for.

Opinions?

@v4hn

v4hn Sep 13, 2016

Member

Looking at this again, I believe it is unnecessarily complex to have this parameter in dynamic reconfigure.

  • it's one more place where the default value has to be provided
  • if anyone wants to add per-joint thresholds later on, the meaning of this parameter becomes "default if none is specified"
    This meaning does not deserved to be changed dynamically imho
  • Personally I don't see a reason why anyone would ever change that value during runtime. This is what dynamic reconfigure is for.

Opinions?

This comment has been minimized.

@gavanderhoorn

gavanderhoorn Sep 13, 2016

Member

I think I would lean toward leaving this out of dyn reconfig. Your bullets sum up the arguments I would've used as well.

Beyond this one PR though, I can see making things like this dynamically (re)configurable being useful when requirements change during runtime. Two examples:

  1. I know of people using MoveIt with drones and other more mobile robots. These robots are mobile perhaps even just to add some 'temporary' mobility to an otherwise static manipulator, but still. I would expect -- but cannot back it up with nrs right now -- that noise in joint states would increase during motion. Manipulation during motion would then need to have (a slightly more) relaxed threshold(s). Afterwards, the old setting could be restored.

  2. (slightly more of a stretch, as considering MoveIt as part of the safety features of any robot system is currently not even considerable): in the context of co-robots, changing thresholds / safety related distances and things like maximum velocities and forces based on the proximity of humans becomes more and more common place.

    I don't see MoveIt used for these things (in production systems at least), but dynamically changeable parameters would seem to be a requirement for this kind of functionality.

@gavanderhoorn

gavanderhoorn Sep 13, 2016

Member

I think I would lean toward leaving this out of dyn reconfig. Your bullets sum up the arguments I would've used as well.

Beyond this one PR though, I can see making things like this dynamically (re)configurable being useful when requirements change during runtime. Two examples:

  1. I know of people using MoveIt with drones and other more mobile robots. These robots are mobile perhaps even just to add some 'temporary' mobility to an otherwise static manipulator, but still. I would expect -- but cannot back it up with nrs right now -- that noise in joint states would increase during motion. Manipulation during motion would then need to have (a slightly more) relaxed threshold(s). Afterwards, the old setting could be restored.

  2. (slightly more of a stretch, as considering MoveIt as part of the safety features of any robot system is currently not even considerable): in the context of co-robots, changing thresholds / safety related distances and things like maximum velocities and forces based on the proximity of humans becomes more and more common place.

    I don't see MoveIt used for these things (in production systems at least), but dynamically changeable parameters would seem to be a requirement for this kind of functionality.

This comment has been minimized.

@v4hn

v4hn Sep 13, 2016

Member

On Tue, Sep 13, 2016 at 04:02:28AM -0700, G.A. vd. Hoorn wrote:

I would expect -- but cannot back it up with nrs right now -- that noise in joint states would increase during motion. Manipulation during motion would then need to have (a slightly more) relaxed threshold(s). Afterwards, the old setting could be restored.

Feel free to correct me on this, but this is not state-of-the-art but cutting-edge research and people doing research in this field, they can always disable this additional safety feature if they exceed this threshold and enforce it in their drivers (this is where it actually belongs).

in the context of co-robots, changing thresholds / safety related distances and things like maximum velocities and forces based on the proximity of humans becomes more and more common place.

Yes, I don't believe the parameter under discussion falls into this category though. At least not for now.

@v4hn

v4hn Sep 13, 2016

Member

On Tue, Sep 13, 2016 at 04:02:28AM -0700, G.A. vd. Hoorn wrote:

I would expect -- but cannot back it up with nrs right now -- that noise in joint states would increase during motion. Manipulation during motion would then need to have (a slightly more) relaxed threshold(s). Afterwards, the old setting could be restored.

Feel free to correct me on this, but this is not state-of-the-art but cutting-edge research and people doing research in this field, they can always disable this additional safety feature if they exceed this threshold and enforce it in their drivers (this is where it actually belongs).

in the context of co-robots, changing thresholds / safety related distances and things like maximum velocities and forces based on the proximity of humans becomes more and more common place.

Yes, I don't believe the parameter under discussion falls into this category though. At least not for now.

Show outdated Hide outdated ...anning/trajectory_execution_manager/src/trajectory_execution_manager.cpp
@@ -109,6 +114,7 @@ void TrajectoryExecutionManager::initialize()
run_continuous_execution_thread_ = true;
execution_duration_monitoring_ = true;
execution_velocity_scaling_ = 1.0;
allowed_start_tolerance_ = 1e-3;

This comment has been minimized.

@v4hn

v4hn Aug 22, 2016

Member

see above.

@v4hn

v4hn Aug 22, 2016

Member

see above.

Show outdated Hide outdated ...anning/trajectory_execution_manager/src/trajectory_execution_manager.cpp
const std::vector<std::string> &joint_names = trajectory.joint_trajectory.joint_names;
const std::size_t n = joint_names.size();
if (positions.size() != n)
return false;

This comment has been minimized.

@v4hn

v4hn Aug 22, 2016

Member

Please add ROS_ERROR_NAMED here.
This should never happen and we should not just silently fail.

@v4hn

v4hn Aug 22, 2016

Member

Please add ROS_ERROR_NAMED here.
This should never happen and we should not just silently fail.

This comment has been minimized.

@rhaschke

rhaschke Aug 23, 2016

Contributor

OK.

@rhaschke

rhaschke Aug 23, 2016

Contributor

OK.

Show outdated Hide outdated ...anning/trajectory_execution_manager/src/trajectory_execution_manager.cpp
if (fabs(current_state->getJointPositions(jm)[0] - positions[i]) > allowed_start_tolerance_)
{
ROS_ERROR_NAMED("traj_execution",
"Invalid Trajectory: start point deviates from current robot state more than %g",

This comment has been minimized.

@v4hn

v4hn Aug 22, 2016

Member

Please add the joint name and the expected / current values here to support debugging.

@v4hn

v4hn Aug 22, 2016

Member

Please add the joint name and the expected / current values here to support debugging.

This comment has been minimized.

@rhaschke

rhaschke Aug 23, 2016

Contributor

OK.

@rhaschke

rhaschke Aug 23, 2016

Contributor

OK.

Show outdated Hide outdated ...plates/moveit_config_pkg_template/launch/trajectory_execution.launch.xml
@@ -10,6 +10,8 @@
<param name="trajectory_execution/allowed_execution_duration_scaling" value="1.2"/> <!-- default 1.2 -->
<!-- Allow more than the expected execution time before triggering a trajectory cancel (applied after scaling) -->
<param name="trajectory_execution/allowed_goal_duration_margin" value="0.5"/> <!-- default 0.5 -->
<!-- Allowed joint-value tolerance for validation that trajectory's first point matches current robot state -->
<param name="trajectory_execution/allowed_start_tolerance" value="1.e-3"/> <!-- default 1.e-3 -->

This comment has been minimized.

@v4hn

v4hn Aug 22, 2016

Member

again, I would prefer 0.01, see above.

@v4hn

v4hn Aug 22, 2016

Member

again, I would prefer 0.01, see above.

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Aug 22, 2016

Member

Overall, this looks great.

As mentioned, this breaks API and I'm not sure there is a way around it.
For Indigo, I would prefer to change all MoveIt-internal references to the proposed two-parameter constructor of TrajectoryExecutionManager, but still provide the old constructor.
Thus, validation would be optional if the user creates an instance of that class on their own, but we can add a ROS_ERROR that tells them to switch to the new version instead.

Member

v4hn commented Aug 22, 2016

Overall, this looks great.

As mentioned, this breaks API and I'm not sure there is a way around it.
For Indigo, I would prefer to change all MoveIt-internal references to the proposed two-parameter constructor of TrajectoryExecutionManager, but still provide the old constructor.
Thus, validation would be optional if the user creates an instance of that class on their own, but we can add a ROS_ERROR that tells them to switch to the new version instead.

@v4hn v4hn referenced this pull request Aug 22, 2016

Closed

Initial Indigo release from ros-planning/moveit repo #100

15 of 15 tasks complete
@rhaschke

This comment has been minimized.

Show comment
Hide comment
@rhaschke

rhaschke Aug 23, 2016

Contributor

As mentioned, this breaks API and I'm not sure there is a way around it.
For Indigo, I would prefer to change all MoveIt-internal references to the proposed two-parameter constructor of TrajectoryExecutionManager, but still provide the old constructor.
Thus, validation would be optional if the user creates an instance of that class on their own, but we can add a ROS_ERROR that tells them to switch to the new version instead.

Good idea. I will also add a deprecation warning during compile time then.
@v4hn If you merge this PR, I can take care of cherry-pick to Jade (without changes, right?) and I will prepare a new PR for Indigo, with the deprecation adaptations.

Contributor

rhaschke commented Aug 23, 2016

As mentioned, this breaks API and I'm not sure there is a way around it.
For Indigo, I would prefer to change all MoveIt-internal references to the proposed two-parameter constructor of TrajectoryExecutionManager, but still provide the old constructor.
Thus, validation would be optional if the user creates an instance of that class on their own, but we can add a ROS_ERROR that tells them to switch to the new version instead.

Good idea. I will also add a deprecation warning during compile time then.
@v4hn If you merge this PR, I can take care of cherry-pick to Jade (without changes, right?) and I will prepare a new PR for Indigo, with the deprecation adaptations.

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Aug 23, 2016

Member

On Tue, Aug 23, 2016 at 01:58:45AM -0700, Robert Haschke wrote:

Good idea. I will also add a deprecation warning during compile time then.

+1

@v4hn If you merge this PR, I can take care of cherry-pick to Jade (without changes, right?) and I will prepare a new PR for Indigo, with the deprecation adaptations.

+1 as soon as the threshold got somewhat tested. (We'll try to test the ur5 here today)

Member

v4hn commented Aug 23, 2016

On Tue, Aug 23, 2016 at 01:58:45AM -0700, Robert Haschke wrote:

Good idea. I will also add a deprecation warning during compile time then.

+1

@v4hn If you merge this PR, I can take care of cherry-pick to Jade (without changes, right?) and I will prepare a new PR for Indigo, with the deprecation adaptations.

+1 as soon as the threshold got somewhat tested. (We'll try to test the ur5 here today)

@rhaschke

This comment has been minimized.

Show comment
Hide comment
@rhaschke

rhaschke Aug 24, 2016

Contributor

As mentioned, this breaks API and I'm not sure there is a way around it.

With the changes suggested in #63 (comment), API compatibility can be maintained, but ABI will break anyway, because we need to add a new class member. Alternatively, we could fallback to the version before 9f77f91, which is ABI and API compatible because it adds the validateTrajectory method at the level of MoveGroupContext, which gets called only by the trajectory execution service then.

The plan/move capability enforces setting the start state of the robot, such that it shouldn't happen that the start state deviates there. However, other capability plugins would need to call the validation method themselves if necessary.

I think, this could be a compromise to maintain ABI compatibility and having the trajectory validation in most cases.
On the other hand, as the synchronization-fix will break ABI as well, we could decide to drop it here too.
@v4hn @davetcoleman Opinions?

UPDATE:
The proposed option, would require to call the validation method in the new ExecuteAction capability.

Contributor

rhaschke commented Aug 24, 2016

As mentioned, this breaks API and I'm not sure there is a way around it.

With the changes suggested in #63 (comment), API compatibility can be maintained, but ABI will break anyway, because we need to add a new class member. Alternatively, we could fallback to the version before 9f77f91, which is ABI and API compatible because it adds the validateTrajectory method at the level of MoveGroupContext, which gets called only by the trajectory execution service then.

The plan/move capability enforces setting the start state of the robot, such that it shouldn't happen that the start state deviates there. However, other capability plugins would need to call the validation method themselves if necessary.

I think, this could be a compromise to maintain ABI compatibility and having the trajectory validation in most cases.
On the other hand, as the synchronization-fix will break ABI as well, we could decide to drop it here too.
@v4hn @davetcoleman Opinions?

UPDATE:
The proposed option, would require to call the validation method in the new ExecuteAction capability.

Show outdated Hide outdated ...anning/trajectory_execution_manager/src/trajectory_execution_manager.cpp
return false;
}
// TODO: check multi-DoF joints ?
if (fabs(current_state->getJointPositions(jm)[0] - positions[i]) > allowed_start_tolerance_)

This comment has been minimized.

@v4hn

v4hn Aug 24, 2016

Member

Would it be a good idea to skip this check if the allowed tolerance is set to 0.0?
This would disallow setting it to 0.0 if you think you have a perfectly unnoisy robot,
but it would give a well-defined and simple way to disable the safety check from the launch file.

I suppose I would want to do that if I'm sure my controller does this in a better way...

@v4hn

v4hn Aug 24, 2016

Member

Would it be a good idea to skip this check if the allowed tolerance is set to 0.0?
This would disallow setting it to 0.0 if you think you have a perfectly unnoisy robot,
but it would give a well-defined and simple way to disable the safety check from the launch file.

I suppose I would want to do that if I'm sure my controller does this in a better way...

This comment has been minimized.

@davetcoleman

davetcoleman Aug 24, 2016

Member

+1 to skipping the for loop if the tolerance is smaller than epsilon

@davetcoleman

davetcoleman Aug 24, 2016

Member

+1 to skipping the for loop if the tolerance is smaller than epsilon

This comment has been minimized.

@rhaschke

rhaschke Aug 24, 2016

Contributor

I will do that.

@rhaschke

rhaschke Aug 24, 2016

Contributor

I will do that.

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Aug 24, 2016

Member

As mentioned, this breaks API and I'm not sure there is a way around it.

You misunderstood. I'm fine with breaking the ABI once in indigo to fix safety hazards.
Assuming you add the old constructor back in indigo (as I mentioned in #63 (comment) ,
my only concern left is the default threshold value. It's (1) a default value that
(2) most people will never touch, so it should allow normal workflow with most setups (1) and still do the job (2).
Pretty hard trade-off, really.

So I was waiting for some more measurements from other robot setups.

Member

v4hn commented Aug 24, 2016

As mentioned, this breaks API and I'm not sure there is a way around it.

You misunderstood. I'm fine with breaking the ABI once in indigo to fix safety hazards.
Assuming you add the old constructor back in indigo (as I mentioned in #63 (comment) ,
my only concern left is the default threshold value. It's (1) a default value that
(2) most people will never touch, so it should allow normal workflow with most setups (1) and still do the job (2).
Pretty hard trade-off, really.

So I was waiting for some more measurements from other robot setups.

@gavanderhoorn

This comment has been minimized.

Show comment
Hide comment
@gavanderhoorn

gavanderhoorn Aug 24, 2016

Member

I haven't read the code completely, but have we thought about the impact of this on things like dynamic trajectory replacement?

Are we expecting users to disable the traj.mon. in case they want to do something like that?

Member

gavanderhoorn commented Aug 24, 2016

I haven't read the code completely, but have we thought about the impact of this on things like dynamic trajectory replacement?

Are we expecting users to disable the traj.mon. in case they want to do something like that?

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Aug 24, 2016

Member

By now, I heard the argument twice that this threshold should be specified per joint instead of having one value. In both cases it was suggested to make this an optional new field in the robot_moveit_config/config/joint_limits.yaml.

The most important use-case I see for this are prismatic joints. We might not want to use the same threshold for validation of rotational and prismatic joints, but we can't differentiate them at this point (and I still think this is the correct place to add the checks).

Up to now my reaction to this was: if the simple value proofs inappropriate, we can always put a per-joint mechanism on top of it and make the ros-param the default for unspecified limits.
Nevertheless I wanted to add this here.

Member

v4hn commented Aug 24, 2016

By now, I heard the argument twice that this threshold should be specified per joint instead of having one value. In both cases it was suggested to make this an optional new field in the robot_moveit_config/config/joint_limits.yaml.

The most important use-case I see for this are prismatic joints. We might not want to use the same threshold for validation of rotational and prismatic joints, but we can't differentiate them at this point (and I still think this is the correct place to add the checks).

Up to now my reaction to this was: if the simple value proofs inappropriate, we can always put a per-joint mechanism on top of it and make the ros-param the default for unspecified limits.
Nevertheless I wanted to add this here.

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Aug 24, 2016

Member

@gavanderhoorn Thanks!

@rhaschke why did you add the validation to the configure method? This is called for every pushed trajectory, but if you want to push multiple trajectories, this will fail.
I just had a look into the execution manager code and I believe this should be called in executeThread.

Member

v4hn commented Aug 24, 2016

@gavanderhoorn Thanks!

@rhaschke why did you add the validation to the configure method? This is called for every pushed trajectory, but if you want to push multiple trajectories, this will fail.
I just had a look into the execution manager code and I believe this should be called in executeThread.

@gavanderhoorn

This comment has been minimized.

Show comment
Hide comment
@gavanderhoorn

gavanderhoorn Aug 24, 2016

Member

@v4hn wrote:

@gavanderhoorn Thanks!

hm? For what? :)

Member

gavanderhoorn commented Aug 24, 2016

@v4hn wrote:

@gavanderhoorn Thanks!

hm? For what? :)

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Aug 24, 2016

Member

On Wed, Aug 24, 2016 at 07:59:24AM -0700, G.A. vd. Hoorn wrote:

hm? For what? :)

Looking at this :-)
And especially pointing out possible bugs :D

Member

v4hn commented Aug 24, 2016

On Wed, Aug 24, 2016 at 07:59:24AM -0700, G.A. vd. Hoorn wrote:

hm? For what? :)

Looking at this :-)
And especially pointing out possible bugs :D

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Aug 24, 2016

Member

I think its fine if we break ABI in indigo all at once for several difference safety-related checks
For now I think having just one value instead of per-joint is a good starting point

Member

davetcoleman commented Aug 24, 2016

I think its fine if we break ABI in indigo all at once for several difference safety-related checks
For now I think having just one value instead of per-joint is a good starting point

@rhaschke

This comment has been minimized.

Show comment
Hide comment
@rhaschke

rhaschke Aug 24, 2016

Contributor

@rhaschke why did you add the validation to the configure method? This is called for every pushed trajectory, but if you want to push multiple trajectories, this will fail.
I just had a look into the execution manager code and I believe this should be called in executeThread.

Indeed. I missed that point. However, I wouldn't go for executeThread, but try to fail already in push / pushAndExecute: Instead of looking at the most recent RobotState there, I could use the one from the latest trajectory point in trajectories_.

Also there is not only executeThread, but also continuousExecutionThread, which seems to be feeded by pushAndExecute, which is not used externally. What is the difference? Why there are two different trajectory queues and filling / execution methods?

Contributor

rhaschke commented Aug 24, 2016

@rhaschke why did you add the validation to the configure method? This is called for every pushed trajectory, but if you want to push multiple trajectories, this will fail.
I just had a look into the execution manager code and I believe this should be called in executeThread.

Indeed. I missed that point. However, I wouldn't go for executeThread, but try to fail already in push / pushAndExecute: Instead of looking at the most recent RobotState there, I could use the one from the latest trajectory point in trajectories_.

Also there is not only executeThread, but also continuousExecutionThread, which seems to be feeded by pushAndExecute, which is not used externally. What is the difference? Why there are two different trajectory queues and filling / execution methods?

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Aug 25, 2016

Member

On Wed, Aug 24, 2016 at 04:19:50PM -0700, Robert Haschke wrote:

However, I wouldn't go for executeThread, but try to fail already in push / pushAndExecute: Instead of looking at the most recent RobotState there, I could use the one from the latest trajectory point in trajectories_.

The problem I see with this, is that the first added trajectory already has to be valid when it is pushed and not just when it is (later on) executed.
I'm not sure whether this is a problem or not.

Also there is not only executeThread, but also continuousExecutionThread, which seems to be feeded by pushAndExecute, which is not used externally. What is the difference? Why there are two different trajectory queues and filling / execution methods?

One of them provides callbacks and the other one does not. One of them supports appending new trajectories on the fly (via pushAndExecute), the other one requires batch executions.
So I suppose someone originally implemented one version and later on decided that they needed the other one for their use-case. :)

Member

v4hn commented Aug 25, 2016

On Wed, Aug 24, 2016 at 04:19:50PM -0700, Robert Haschke wrote:

However, I wouldn't go for executeThread, but try to fail already in push / pushAndExecute: Instead of looking at the most recent RobotState there, I could use the one from the latest trajectory point in trajectories_.

The problem I see with this, is that the first added trajectory already has to be valid when it is pushed and not just when it is (later on) executed.
I'm not sure whether this is a problem or not.

Also there is not only executeThread, but also continuousExecutionThread, which seems to be feeded by pushAndExecute, which is not used externally. What is the difference? Why there are two different trajectory queues and filling / execution methods?

One of them provides callbacks and the other one does not. One of them supports appending new trajectories on the fly (via pushAndExecute), the other one requires batch executions.
So I suppose someone originally implemented one version and later on decided that they needed the other one for their use-case. :)

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Aug 25, 2016

Member

Now that I'm reading my own comment...

Validation of trajectories in push when they are not the first one is intrinsically impossible.
The trajectories before this one might specify different joints and there is no guarantee the respective controllers will only change the specified joints (this is not the case for the controlling joint in the GripperAction).
So we could validate the first one in executeThread and (with a modification to e.g. pop_front the currently executing trajectory only after execution in continuousExecutionThread) also in pushAndExecute.

Alternatively we could validate all trajectories right before execution in executePart and continuousExecutionThread and compare them to the current state.
I'm not sure whether this would introduce a noticable delay, and it probably requires a working waitForCurrentState because otherwise this would fail with slow network connections.
I would definitely prefer checking only the first trajectory. Imagine you send trajectories that do not slow down but combine with non-zero velocities.
In this situation, checking the individual parts against the current state would definitely be a problem.
I believe it is safe enough to check only the first trajectory before starting execution.

Member

v4hn commented Aug 25, 2016

Now that I'm reading my own comment...

Validation of trajectories in push when they are not the first one is intrinsically impossible.
The trajectories before this one might specify different joints and there is no guarantee the respective controllers will only change the specified joints (this is not the case for the controlling joint in the GripperAction).
So we could validate the first one in executeThread and (with a modification to e.g. pop_front the currently executing trajectory only after execution in continuousExecutionThread) also in pushAndExecute.

Alternatively we could validate all trajectories right before execution in executePart and continuousExecutionThread and compare them to the current state.
I'm not sure whether this would introduce a noticable delay, and it probably requires a working waitForCurrentState because otherwise this would fail with slow network connections.
I would definitely prefer checking only the first trajectory. Imagine you send trajectories that do not slow down but combine with non-zero velocities.
In this situation, checking the individual parts against the current state would definitely be a problem.
I believe it is safe enough to check only the first trajectory before starting execution.

@rhaschke

This comment has been minimized.

Show comment
Hide comment
@rhaschke

rhaschke Aug 26, 2016

Contributor

So we could validate ... with a modification to e.g. pop_front the currently executing trajectory only after execution in continuousExecutionThread in pushAndExecute.

@v4hn Could you explain this again? Validating the currently executing trajectory after execution doesn't make sense, does it?

Contributor

rhaschke commented Aug 26, 2016

So we could validate ... with a modification to e.g. pop_front the currently executing trajectory only after execution in continuousExecutionThread in pushAndExecute.

@v4hn Could you explain this again? Validating the currently executing trajectory after execution doesn't make sense, does it?

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Aug 26, 2016

Member

Just ignore the stuff in parenthesis until you look into the details in code. :)
It's just a proposal to allow to verify a new trajectory when none is currently executing.
Afaics, there's currently no way to check for whether a trajectory is executing right now with continuousExecutionThread...

Member

v4hn commented Aug 26, 2016

Just ignore the stuff in parenthesis until you look into the details in code. :)
It's just a proposal to allow to verify a new trajectory when none is currently executing.
Afaics, there's currently no way to check for whether a trajectory is executing right now with continuousExecutionThread...

@rhaschke

This comment has been minimized.

Show comment
Hide comment
@rhaschke

rhaschke Aug 27, 2016

Contributor

I finally moved the validate() call to execute(), which is called to start a queued set of trajectories.
Only the first trajectory is validated. I think it doesn't make sense to validate continuous execution, because there is no notion of a first trajectory in that case.

Doing this, I noticed that TrajectoryExecutionManager has lots of potential race conditions. The code seems to assume that access to the boolean variable execution_complete_ is atomic, which is in general not the case. Only write access to that variable is protected by execution_state_mutex_, but usually not read access. Simultaneously this variable seems to protect access to the trajectories_ queue. I tried to fix this, but this turned into a nightmare. I guess somebody else, who is more familiar with the TrajectoryExecutionManager code should tackle this. Maybe it already suffices to use std::atomic<bool>. As this would break ABI and the code didn't exhibited obvious race conditions in the past, I skipped that, considering the issue as not so urgent.

I also have a unittest prepared, which I can add after #184 has been merged.

Contributor

rhaschke commented Aug 27, 2016

I finally moved the validate() call to execute(), which is called to start a queued set of trajectories.
Only the first trajectory is validated. I think it doesn't make sense to validate continuous execution, because there is no notion of a first trajectory in that case.

Doing this, I noticed that TrajectoryExecutionManager has lots of potential race conditions. The code seems to assume that access to the boolean variable execution_complete_ is atomic, which is in general not the case. Only write access to that variable is protected by execution_state_mutex_, but usually not read access. Simultaneously this variable seems to protect access to the trajectories_ queue. I tried to fix this, but this turned into a nightmare. I guess somebody else, who is more familiar with the TrajectoryExecutionManager code should tackle this. Maybe it already suffices to use std::atomic<bool>. As this would break ABI and the code didn't exhibited obvious race conditions in the past, I skipped that, considering the issue as not so urgent.

I also have a unittest prepared, which I can add after #184 has been merged.

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Sep 9, 2016

Member

On Thu, Sep 08, 2016 at 05:41:56PM -0700, Dave Coleman wrote:

+1 besides my request for a comment

I still want to see this verified on a baxter robot before merging.
The last thing we want is to release MoveIt with broken out-of-the-box Baxter support.

Member

v4hn commented Sep 9, 2016

On Thu, Sep 08, 2016 at 05:41:56PM -0700, Dave Coleman wrote:

+1 besides my request for a comment

I still want to see this verified on a baxter robot before merging.
The last thing we want is to release MoveIt with broken out-of-the-box Baxter support.

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Sep 9, 2016

Member

Do we know who will test this? I just pinged @IanTheEngineer

Member

davetcoleman commented Sep 9, 2016

Do we know who will test this? I just pinged @IanTheEngineer

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Sep 9, 2016

Member

On Thu, Sep 08, 2016 at 07:21:43PM -0700, Dave Coleman wrote:

Do we know who will test this? I just pinged @IanTheEngineer

No one volunteered yet...
I just visited a lab with a baxter today, but I didn't get around to test anything.
However, after looking at it for a while, I'm not convinced anymore that the current
threshold value is valid there.

So just to summarize: We need the maximum distance in a single joint between
any two received joint states where the robot was not intentionally moved. (just let the robot run for 30 seconds or so without movement and extract the data)
Any other measurements are welcome too.

In the long run, I believe the currently proposed solution is far from good and there are better ways to resolve this in theory.
We could either have one "noise-level" value for each joint (any two joint states whose difference in each joint is below that level could be considered the same)
or check if the arm can officially go from the current pose to the first trajectory pose in some very short time-span, given the current velocity/acceleration limits.

Member

v4hn commented Sep 9, 2016

On Thu, Sep 08, 2016 at 07:21:43PM -0700, Dave Coleman wrote:

Do we know who will test this? I just pinged @IanTheEngineer

No one volunteered yet...
I just visited a lab with a baxter today, but I didn't get around to test anything.
However, after looking at it for a while, I'm not convinced anymore that the current
threshold value is valid there.

So just to summarize: We need the maximum distance in a single joint between
any two received joint states where the robot was not intentionally moved. (just let the robot run for 30 seconds or so without movement and extract the data)
Any other measurements are welcome too.

In the long run, I believe the currently proposed solution is far from good and there are better ways to resolve this in theory.
We could either have one "noise-level" value for each joint (any two joint states whose difference in each joint is below that level could be considered the same)
or check if the arm can officially go from the current pose to the first trajectory pose in some very short time-span, given the current velocity/acceleration limits.

@IanTheEngineer

This comment has been minimized.

Show comment
Hide comment
@IanTheEngineer

IanTheEngineer Sep 9, 2016

Member

Hi all. I've been a bit slammed recently, and haven't had a chance to determine Baxter's standing joint "noise" level. I agree that this proposed solution is far from good if we need to verify the chosen threshold doesn't break robots already using MoveIt.

I expect joint level noise to be configuration dependent, ie, a fully extended arm will have more noise than folded. I made a 30 second bagfile recording the Baxter's /robot/joint_states topic of my (very well used) Baxter in the following pose:
img_2908
If someone has time to parse the bagfile for max and min joint_state positions before I get the chance, please go for it. One gotcha is that I have a separate joint state publisher for the end effector than the rest of the robot.
baxter_joint_states.bag

Member

IanTheEngineer commented Sep 9, 2016

Hi all. I've been a bit slammed recently, and haven't had a chance to determine Baxter's standing joint "noise" level. I agree that this proposed solution is far from good if we need to verify the chosen threshold doesn't break robots already using MoveIt.

I expect joint level noise to be configuration dependent, ie, a fully extended arm will have more noise than folded. I made a 30 second bagfile recording the Baxter's /robot/joint_states topic of my (very well used) Baxter in the following pose:
img_2908
If someone has time to parse the bagfile for max and min joint_state positions before I get the chance, please go for it. One gotcha is that I have a separate joint state publisher for the end effector than the rest of the robot.
baxter_joint_states.bag

@rhaschke

This comment has been minimized.

Show comment
Hide comment
@rhaschke

rhaschke Sep 9, 2016

Contributor

I do see the limitations of a single threshold for all joints as well.
However, as @v4hn suggested to start with a single threshold in #63 (comment), I didn't take more actions for now.
I think, extending this feature to per-joint thresholds requires a more in-depth discussion where to place those parameters. As this is a safety-relevant PR, which was urgently asked for, I think, we should leave it like this and open a new PR to extend it. What do you think?

Contributor

rhaschke commented Sep 9, 2016

I do see the limitations of a single threshold for all joints as well.
However, as @v4hn suggested to start with a single threshold in #63 (comment), I didn't take more actions for now.
I think, extending this feature to per-joint thresholds requires a more in-depth discussion where to place those parameters. As this is a safety-relevant PR, which was urgently asked for, I think, we should leave it like this and open a new PR to extend it. What do you think?

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Sep 9, 2016

Member

+1 to sticking with this for now and getting this long-overdue PR merged in

Member

davetcoleman commented Sep 9, 2016

+1 to sticking with this for now and getting this long-overdue PR merged in

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Sep 10, 2016

Member

On Fri, Sep 09, 2016 at 02:42:57PM -0700, Dave Coleman wrote:

+1 to sticking with this for now and getting this long-overdue PR merged in

Once someone looked into Ian's bag and verified the threshold, that's ok for me.

Member

v4hn commented Sep 10, 2016

On Fri, Sep 09, 2016 at 02:42:57PM -0700, Dave Coleman wrote:

+1 to sticking with this for now and getting this long-overdue PR merged in

Once someone looked into Ian's bag and verified the threshold, that's ok for me.

@davetcoleman davetcoleman referenced this pull request Sep 12, 2016

Closed

Kinetic Release #18

19 of 19 tasks complete
@guihomework

This comment has been minimized.

Show comment
Hide comment
@guihomework

guihomework Sep 12, 2016

@rhaschke here is the processed baxter bag file. Sorry for the dirty/quick python/octave code but I suppose this is sufficient
max variation is ~4e-3
baxternoiseprocessed_cropped

@rhaschke here is the processed baxter bag file. Sorry for the dirty/quick python/octave code but I suppose this is sufficient
max variation is ~4e-3
baxternoiseprocessed_cropped

@guihomework

This comment has been minimized.

Show comment
Hide comment
@guihomework

guihomework Sep 12, 2016

@rhaschke, I was curious how much noise the shadow hand produces over 30s
I got worst of noise (min to max) 0.020557 on THJ3 but looking at the curves it looks like the joint changed pulling tendon during the 30s. If I remove this joint that might need retuning, I have 0.0085251 as the maximum noise over 23 joints then. So this is double the value Baxter has.
If we want to be fair, the wrist joints are the one most likely to be used in motion planning, and they have a noise of 0.0014720

I also looked at our PA-10 extended the most I could, with the shadow hand at the end.
max noise is 1.4567e-04

Did not look lwr robot but could do if required. I don't think it can be worse.

hope this helps

@rhaschke, I was curious how much noise the shadow hand produces over 30s
I got worst of noise (min to max) 0.020557 on THJ3 but looking at the curves it looks like the joint changed pulling tendon during the 30s. If I remove this joint that might need retuning, I have 0.0085251 as the maximum noise over 23 joints then. So this is double the value Baxter has.
If we want to be fair, the wrist joints are the one most likely to be used in motion planning, and they have a noise of 0.0014720

I also looked at our PA-10 extended the most I could, with the shadow hand at the end.
max noise is 1.4567e-04

Did not look lwr robot but could do if required. I don't think it can be worse.

hope this helps

@rhaschke

This comment has been minimized.

Show comment
Hide comment
@rhaschke

rhaschke Sep 13, 2016

Contributor

According to the noise analyses on different robots, I agree to raise the threshold to 0.01, which corresponds then to 1cm offset with a 1m lever arm. This should be still fine for basic safety.
To bring this value to the attention of the user, I suggest to add an appropriate configuration dialog in setup assistant. What do you think?

Contributor

rhaschke commented Sep 13, 2016

According to the noise analyses on different robots, I agree to raise the threshold to 0.01, which corresponds then to 1cm offset with a 1m lever arm. This should be still fine for basic safety.
To bring this value to the attention of the user, I suggest to add an appropriate configuration dialog in setup assistant. What do you think?

@gavanderhoorn

This comment has been minimized.

Show comment
Hide comment
@gavanderhoorn

gavanderhoorn Sep 13, 2016

Member

@guihomework wrote:

I also looked at our PA-10 extended the most I could, with the shadow hand at the end.

slightly off-topic: with PA-10, you mean a Mitsubishi PA-10?

Member

gavanderhoorn commented Sep 13, 2016

@guihomework wrote:

I also looked at our PA-10 extended the most I could, with the shadow hand at the end.

slightly off-topic: with PA-10, you mean a Mitsubishi PA-10?

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Sep 13, 2016

Member

According to the noise analyses on different robots, I agree to raise the threshold to 0.01, which corresponds then to 1cm offset with a 1m lever arm. This should be still fine for basic safety.

+1

I would not add a configuration dialog to the assistant until we change this mechanism to one-value-per-joint, because the semantics of this one value are not very intuitive and the mechanism should "just work" by default.

If you want to have a "magic numbers" widget in the assistant, that sounds pretty cool actually, but I would opt to have support for parsing the files in the loaded robot config before that and this is quite some work. Without this, we would just end up with storing all the parameters in the files and in .setup_assistant.

@guihomework Thanks a lot for the test results!

Member

v4hn commented Sep 13, 2016

According to the noise analyses on different robots, I agree to raise the threshold to 0.01, which corresponds then to 1cm offset with a 1m lever arm. This should be still fine for basic safety.

+1

I would not add a configuration dialog to the assistant until we change this mechanism to one-value-per-joint, because the semantics of this one value are not very intuitive and the mechanism should "just work" by default.

If you want to have a "magic numbers" widget in the assistant, that sounds pretty cool actually, but I would opt to have support for parsing the files in the loaded robot config before that and this is quite some work. Without this, we would just end up with storing all the parameters in the files and in .setup_assistant.

@guihomework Thanks a lot for the test results!

@guihomework

This comment has been minimized.

Show comment
Hide comment
@guihomework

guihomework Sep 13, 2016

@gavanderhoorn , indeed this is a Mitsubishi PA-10 7c with a too heavy payload (shakes when moving fast) but is stable when maintaining position.

@gavanderhoorn , indeed this is a Mitsubishi PA-10 7c with a too heavy payload (shakes when moving fast) but is stable when maintaining position.

@gavanderhoorn

This comment has been minimized.

Show comment
Hide comment
@gavanderhoorn

gavanderhoorn Sep 13, 2016

Member

@guihomework: interesting. I might email you off-list.

Member

gavanderhoorn commented Sep 13, 2016

@guihomework: interesting. I might email you off-list.

@davetcoleman davetcoleman merged commit ce65970 into ros-planning:kinetic-devel Sep 16, 2016

1 check passed

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

davetcoleman added a commit that referenced this pull request 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

davetcoleman added a commit to davetcoleman/moveit that referenced this pull request 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
@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Sep 16, 2016

Member

great work @rhaschke !

cherry-picked to jade, pull request for indigo

Member

davetcoleman commented Sep 16, 2016

great work @rhaschke !

cherry-picked to jade, pull request for indigo

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Sep 16, 2016

Member

I just realized this is not API-compatible - my downstream packages are failing in Jade because of this line

Previously in this thread @rhaschke suggested this could be directly cherry-picked into Jade, which I just did, but now that Jade is "officially" released I think we should discuss this further. And there is a C++11 auto bug that I've fixed locally but has broken the build. I'm going to revert the jade cherry-pick for now.

Member

davetcoleman commented Sep 16, 2016

I just realized this is not API-compatible - my downstream packages are failing in Jade because of this line

Previously in this thread @rhaschke suggested this could be directly cherry-picked into Jade, which I just did, but now that Jade is "officially" released I think we should discuss this further. And there is a C++11 auto bug that I've fixed locally but has broken the build. I'm going to revert the jade cherry-pick for now.

davetcoleman added a commit that referenced this pull request Sep 16, 2016

davetcoleman added a commit that referenced this pull request 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
@rhaschke

This comment has been minimized.

Show comment
Hide comment
@rhaschke

rhaschke Sep 16, 2016

Contributor

@davetcoleman This should be API-, but not ABI-compatible and we agreed to accept the ABI breakage for more safety. Which compilation issues do you observe in your downstream projects?

Contributor

rhaschke commented Sep 16, 2016

@davetcoleman This should be API-, but not ABI-compatible and we agreed to accept the ABI breakage for more safety. Which compilation issues do you observe in your downstream projects?

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Sep 16, 2016

Member

it's not API compatible, because you didn't add the old constructor back in the kinetic request.
We agreed to add it only in indigo @rhaschke

Member

v4hn commented Sep 16, 2016

it's not API compatible, because you didn't add the old constructor back in the kinetic request.
We agreed to add it only in indigo @rhaschke

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Sep 16, 2016

Member

It broke some of my downstream released packages in Jade because of the change in the constructor, so I think we want it in Jade also

Member

davetcoleman commented Sep 16, 2016

It broke some of my downstream released packages in Jade because of the change in the constructor, so I think we want it in Jade also

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