search

ros-industrial / industrial_core

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

JointTrajectoryDownloader/Streamer provides no feedback #131

Open simonschmeisser opened 8 years ago

simonschmeisser commented 8 years ago

If there is an error during upload of the trajectory, there is no way for JointTrajectoryDownloader to communicate this error. This happens for me ie when the controller is set to "Manual mode" instead of "automatic". The mitsubishi controller will allow for upload but returns an error once I try to start execution of the trajectory. This does not lead to the controller being in an error state, which could be detected by RobotStateInterface and handled by pr #83. So these two are complementary.

I would personally implement this using (1) a service call instead of a one way topic, but that would probably break compatibility in drivers. So instead (2) a separate return topic could be used with the Downloader/Streamer publishing errors. JointTrajectoryAction would then listen for these and react in a way similar to pr #83.

gavanderhoorn commented 8 years ago

Just providing my insight here:

If there is an error during upload of the trajectory, there is no way for JointTrajectoryDownloader to communicate this error.

Well, with msgs like JointTrajPt(Full) being a SERVICE_REQUEST, the idea is that server implementations ACK or NACK the action of enqueuing points. In your situation (upload ok, exec not) that doesn't help, but it does allow the server to report either success or failure during the streaming/uploading process. Support for this is currently incomplete in the generic clients (#118), but the protocol allows for it.

This happens for me ie when the controller is set to "Manual mode" instead of "automatic". The mitsubishi controller will allow for upload but returns an error once I try to start execution of the trajectory. This does not lead to the controller being in an error state, which could be detected by RobotStateInterface and handled by pr #83. So these two are complementary.

STATUS includes a field motion_possible, which is supposed to be set to TRUE only if the controller is able to execute trajectories (ie: move). If for Mitsubishi controllers being able to move includes things like not being in manual mode, then it should be FALSE in those cases.

I've drivers for controllers where being in manual mode is fine, as long as the operator is pressing some keys on the teach pendant. The condition (in the driver) for setting motion_possible to TRUE includes the state of those keys, hence it will only be TRUE if motion is actually possible. In addition: attempting motion execution while the operator is not holding those keys on some controllers results in an error, which should then also be reported using the in_error and error_code fields.

Again, support for this in the generic clients is incomplete, and #83 rectifies some of that.

Nothing currently checks the status of the motion_possible field though, which I think is crucial to remedying your issue.

I would personally implement this using (1) a service call instead of a one way topic, but that would probably break compatibility in drivers. So instead (2) a separate return topic could be used with the Downloader/Streamer publishing errors.

If I understand your description correctly, I'd say we already have a service call for this (at least with JointTrajPt(Full)), and a topic (RobotStatus). It's just the integration of feedback across those two existing mechanisms that seems incomplete.

simonschmeisser commented 8 years ago

So if we take the case of #118, when the return value would be NACK and it would be checked, you would then publish "ERROR" on the RobotStatus topic?

I agree about the #83 part and will come up with a new PR that follows your comments here and there soon.

gavanderhoorn commented 8 years ago

So if we take the case of #118, when the return value would be NACK and it would be checked, you would then publish "ERROR" on the RobotStatus topic?

No. NACK-ing the queueing of a trajectory pt should result in the Goal being aborted immediately with the proper error value/msg attached to it. Right now we just have ACK and NACK, but that could of course be extended.

The status topic should just reflect the current state of the robot. If NACKing a pt does not lead to a change in that state, then I don't think there is a reason to change any of the msg fields.

simonschmeisser commented 8 years ago

I agree with you that status should not be (ab)used for that and on what should happen. However I can't see via which mechanism/topic/service you would trigger that? I don't see a communication channel so to say from JointTrajectoryStreamer/Downloader back to JointTrajectoryAction?

gavanderhoorn commented 8 years ago

I don't see a communication channel so to say from JointTrajectoryStreamer/Downloader back to JointTrajectoryAction?

yeah, that is the actual issue here. I think we identified that before.

One way to tackle this is by merging the action server(s) into the respective bridging components. That would give them access to state and req/resp pairs whenever needed. The original design deliberately didn't do this. Perhaps @shaun-edwards or @JeremyZoss can give some more detail?

Another approach would indeed be introducing a topic that closes the loop between the JointTrajectoryAction and the Streamer/Downloader nodes.