search

ros-industrial / motoman

ROS-Industrial Motoman support (http://wiki.ros.org/motoman)
146 stars 193 forks source link

driver: difference between global AS and per-group AS #226

Open gavanderhoorn opened 6 years ago

gavanderhoorn commented 6 years ago

There is quite a difference between the regular FollowJointTrajectory ActionServer started without any namespacing, and the ones created for each of the motion groups in motoman_driver.

The goal callback for the 'global' one:

https://github.com/ros-industrial/motoman/blob/42e30c47405209e2597202167274e0b00d287d47/motoman_driver/src/industrial_robot_client/joint_trajectory_action.cpp#L192-L305

The callback for the per-group one:

https://github.com/ros-industrial/motoman/blob/42e30c47405209e2597202167274e0b00d287d47/motoman_driver/src/industrial_robot_client/joint_trajectory_action.cpp#L315-L429

Notice the absence of a lot of error checking and handling code, checking for empty goals, joint presence and order checks and other things in the global callback. Initially I assumed this was because the global one was a straight copy of the one in industrial_core (and the rest of the code being the extension for multi-group support), but that doesn't seem to be the case.

This divergence (together with some other missing logic and handling of incoming messages and goals) seems to be the underlying cause for a nr of outstanding issues (among which #84, #103, #111, #188, #189 and #216).

@thiagodefreitas: seeing as you offered in #53: could you see whether you remember why this code is the way it is?

gavanderhoorn commented 6 years ago

Note that what is discussed in #219 (in_motion not accurately reflecting robot state) also influences some of the mentioned issues, but that is only tangentially related.

gavanderhoorn commented 6 years ago

Ping @thiagodefreitas.

gavanderhoorn commented 6 years ago

After more inspection of the current JTA implementation and the history of the various related files and commits, it seems that the differences between both goal callbacks were an oversight when the multi-group support was introduced in ba3ca9a6. Multi-group support for the JTA was first added in a separate file (joint_trajectory_action2.cpp), but even there the regular goal cb wasn't a copy of the one in industrial_robot_client (see this version which is from around the same time).

The goal callback associated with the global action server (ie: for the whole robot or one with a single motion group configured) is apparently functional, but the code is not very robust and there are multiple control flows possible that lead to (eventual) failure.

Suggestion: reuse (a significant portion of) the code from the per-group goal callback or the code in industrial_robot_client.

Unfortunately I won't have time to work on this now, so I'm leaving this comment for either my future self, or another future reader.

Danfoa commented 5 years ago

@gavanderhoorn I try to fix (and think I did) the problems mentioned in this issue on the second commit of the PR #259.

There were several errors (or at least they seemed like errors) on the handling of multi-group goals, and I try to solve them in a way consistent to the current development style. It would be very appreciated if you provide feedback on it.