search

ros-industrial / industrial_core

ROS-Industrial core communication packages (http://wiki.ros.org/industrial_core)
156 stars 181 forks source link

Issue#114 - Action Server withinGoalConstraints check removal #115

Closed Jmeyer1292 closed 9 years ago

Jmeyer1292 commented 9 years ago

This PR removes the goal constraints check from the goal-acceptance function of the industrial action server. Because of a previous check to size of the trajectory, this effectively removes only the check on whether the first and last points are the same.

The robot continues to check for goal completion in the callback, but that should behave correctly. In my own testing, this pull request has fixed the issues I was experiencing.

shaun-edwards commented 9 years ago

@JeremyZoss, @gavanderhoorn, any thoughts?

I suggested this change to @Jmeyer1292. The check is a hold over from the original PR2 code, which was pick and place focused. As we move to more process based paths, this issue could occur more often.

gavanderhoorn commented 9 years ago

Change seems to make sense. I'd say that any action server that performs a similar check ("is last point of trajectory within goal constraints?") will encounter the same issue with trajectories that end where they start.

Should we take a look at JointTrajectoryAction::controllerStateCB too? Seems to follow the same logic (here)? Or do we rely on RobotStatus.msg::in_motion to catch that?

Should we backport this to Hydro as well? Seems like a rather critical bug to me. The fix would be behaviour changing though.

JeremyZoss commented 9 years ago

+1. This just pushes the burden for "check if we're already at goal" back on the application code, which is probably a better fit anyway.

@gavanderhoorn : I think the controllerStateCB logic needs to stay as-is, for now. If the robot driver doesn't support in_motion, this is our only way to detect end-of-trajectory. We could add something to ind_rbt_client to support trajectory status-feedback, but this would be difficult for trajectories that are downloaded all-at-once to the robot. It'd work fine for streaming trajectories, as the ind_rbt_client (mostly) knows when the trajectory is done.

shaun-edwards commented 9 years ago

For now I'm going to push this into Indigo only. If this change causes an issue and we push it into Hydro, I'm afraid we would miss the last sync and end up with a permanently "broken" hydro debian.