driver: suspicuous magic nr (3) in JointTrajectoryAction::withinGoalConstraints(..)

[gavanderhoorn]

I'm not sure I completely understand the flow of control in JointTrajectoryAction, but it would appear JointTrajectoryAction::withinGoalConstraints(const control_msgs::FollowJointTrajectoryFeedbackConstPtr &msg, const motoman_msgs::DynamicJointTrajectory & traj) (here) compares the current state of a group's trajectory execution with the positions vector of the 4th group, irrespective of the group for which it has been passed joint states or a trajectory (here).

Excerpt:

...
else
{
    int last_point = traj.points.size() - 1;

    const motoman_msgs::DynamicJointsGroup &pt = traj.points[last_point].groups[3];

    int group_number = pt.group_number;

    if (industrial_robot_client::utils::isWithinRange(
          last_trajectory_state_map_[group_number]->joint_names,
          last_trajectory_state_map_[group_number]->actual.positions, traj.joint_names,
          traj.points[last_point].groups[3].positions, goal_threshold_))
    {
      rtn = true;
    }
...

Note the magic nr 3 two times in this.

At leas the magic nr would need to be explained, but I'm rather curious how this would work on systems with > 1 and < 4 groups.

[gavanderhoorn]

@thiagodefreitas: anything you could share?

[gavanderhoorn]

Thanks @jettan and @mbharatheesha for reporting.

[gavanderhoorn]

@ted-miller: is there a special case for SDA robots where only checking the 4th group to determine completion makes sense?

[ted-miller]

@gavanderhoorn
No.

On an SDA, the 3rd and 4th groups are duplicates of eachother. They both represent the waist axis (15th axis) which is a shared base-axis for each of the 7 axis arms. The position of group [2] and [3] should always match.

I don't understand if/how this applies to your situation... but that is the only thing "special" about an SDA that I can think of.

[gavanderhoorn]

Right, thanks for clarification.

I'm having a hard time understanding why this was implemented this way, and it also seems rather specific to setups with 4 groups.

I'm hoping @thiagodefreitas can shed some light on this, but I'm not sure he even monitors his github account anymore.

[gavanderhoorn]

Reclassifying this as a bug for now, based on input from @ted-miller and my understanding of the code.

[gavanderhoorn]

With #227 merged (which fixed in_motion to really reflect controller motion execution status), checking only a single group is now less of an actual problem (it will only go low when all groups are within the defined tolerance). However, unconditionally checking the fourth group (whether there is one configured or not) does remain an issue.

#236 proposes to remove this particular overload however as it doesn't appear to be actually used anywhere in the JTA.

If/when accepted, this issue can probably be closed.

[gavanderhoorn]

Closing this as #236 was merged.