Sort-of rebase of 83: traj action aborts on error #271

gavanderhoorn · 2021-06-26T16:02:49Z

Sort-of, as it's actually a bit more than what was included in #83.

The proposed changes make the joint_trajectory_action aware of the state of the (OEM) motion server program (as far as it publishes that in the form of RobotStatus messages). In two cases is this information taken into account:

while accepting new goals
while executing already accepted goals

In both cases, the following is monitored / checked:

is an e-stop active?
is there some other error active?
does the motion server report motion_possible is true?

If any of these are not as expected, the goal is rejected, or, if a goal is currently being processed (ie: motion is execution), the goal is aborted and trajectory execution cancelled (as that should now work, with #260 merged (which still requires a cooperating driver of course)).

In all cases, the action result would be INVALID_GOAL, as unfortunately the FollowJointTrajectory action message does not define any really more suitable error codes.

Also in all cases: the reason for aborting or rejecting a goal is described in a human readable message.

When rejecting goals:

[ERROR] [...]: Rejecting goal: controller reported motion not possible (no further information)
[ERROR] [...]: Rejecting goal: controller reported (active) error (OEM code: 11003)
[ERROR] [...]: Rejecting goal: controller reported e-stop

When aborting goals:

[ERROR] [...]: Aborting goal: controller reported motion not possible (no further information)
[ERROR] [...]: Aborting goal: controller reported (active) error (OEM code: 11003)
[ERROR] [...]: Aborting goal: controller reported e-stop

(this is output from fanuc_driver: 11003 is Deadman switch released, which will indeed prevent any motion in manual mode)

While this is certainly not perfect (it doesn't address #265 nor #118), together with #263 and #260 and an (OEM) relay which behaves according to the spec, the UX of industrial_robot_client is improved quite a bit.

Lastly: as previous versions of the JTA did not check anything, misbehaving drivers would still be able to execute motions, even if they didn't properly populate all fields of the RobotStatus messages. In order to still allow usage of such systems, two new parameters were added which can be used to revert the JTA back to its previous behaviour (ie: don't check anything, always forward goals to drivers).

In the current implementation, the default is to disable these new checks. Something to discuss is whether we should make the default to enable the new checks, and users with misbehaving drivers should instead be required to explicitly enable the bw-compatible behaviour.

gavanderhoorn · 2021-06-26T16:03:48Z

Note: 55034f5 is basically what #83 proposed, but it also actually signals to the relay it should stop sending points.

Subsequent commits rewrite this into what the current implementation does.

gavanderhoorn · 2021-06-26T16:09:11Z

I've requested @Levi-Armstrong and/or @JeremyZoss review this, as they've been involved with the development of industrial_core in the past.

@simonschmeisser: if you're still using this, and have some time, I'd appreciate your opinion as well.

Note: I do not intent to keep developing all of this. I just wanted to make sure I can get #83 in before releasing a new version of industrial_core into Melodic and Noetic. But since #83 actually only did half of what it was supposed to, I opted to flesh it out a bit more. And with #260 in place, it was actually not that difficult.

Note 2: it's still possible for trajectory relays to keep moving after an e-stop or error state is cleared: there is no way for the ROS side here (so the IRC node) to enforce anything. The streamer will now in all cases send a "traj abort" message, but if the trajectory and state relays (running on the OEM controller) do not cooperate, there isn't much we can do.

fanuc_driver fi will continue to execute the last sent point after resuming the relay program. Then it will stop. fanuc_driver_exp is slightly better, in that it purges its internal motion buffer and actually SKIPs the current segment. Other drivers may behave similarly (abb_driver fi).

gavanderhoorn · 2021-06-26T16:12:35Z

This probably also addresses #131 partly, as this should not be possible any more (the JTA should reject the goal due to the controller not being in AUTO mode, or at least not reporting that motion_possible is true):

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.

Of course it will depend on the (OEM) state program to properly report controller state (ie: motion_possible should accurately reflect whether motion is possible or not).

The action server will now take (OEM) motion server state into account when accepting or executing goals. If the motion server reports (via the state server) it cannot currently execute motion, either because of a controller error or e-stop, or because a required program is not running on the (OEM) controller, new goals are rejected and currently executing goals are cancelled. Cancelled trajectories should not resume executing once the (OEM) controller error state is cleared. This will however depend on a cooperating motion server (ie: it must manage its queues properly and watch for and act on errors on the OEM controller). Cancellations and rejections are accompanied by appropriate human error messages, giving the reason for why a goal was cancelled or rejected. Unfortunately this is mostly only human readable as the FollowJointTrajectory action doesn't currently define appropriately expressive result codes.

To keep the behaviour of the action server bw-compatible with the previous behaviour, add two ROS parameters which allow to: - ignore motion server state completely - treat UNKNOWN motion server state as ON or OFF (where appropriate) By default motion server state is neither ignored nor are UNKNOWNs acceptable values (and so UNKNOWNs will result in rejected goals and aborted trajectory executions).

Instead of manually comparing against TriState enum values.

It doesn't need to be *this* verbose.

But keep previous behaviour.

So we can reuse stopping the relay.

gavanderhoorn · 2021-07-01T05:41:19Z

Friendly ping.

I don't believe this needs extensive testing.

The main question is whether we want to change the default behaviour (from not checking to checking).

industrial_robot_client/include/industrial_robot_client/joint_trajectory_action.h

simonschmeisser · 2021-07-01T09:00:47Z

industrial_robot_client/launch/robot_interface_download.launch

@@ -30,6 +30,9 @@
  <node pkg="industrial_robot_client" type="motion_download_interface" name="motion_download_interface"/>

  <!-- joint_trajectory_action: provides actionlib interface for high-level robot control -->
-  <node pkg="industrial_robot_client" type="joint_trajectory_action" name="joint_trajectory_action"/>
+  <node pkg="industrial_robot_client" type="joint_trajectory_action" name="joint_trajectory_action">
+    <param name="ignore_motion_server_error" type="bool" value="true" />


Suggested change

<param name="ignore_motion_server_error" type="bool" value="true" />

<param name="ignore_motion_server_error" type="bool" value="false" />

I really appreciate this being tunable but I think we should be strict by default as it can introduce surprising delayed movements if you later enable movements (maybe ... maybe not ... ?)

So this is indeed the main point of discussion here.

Doing this by default has the potential to break ppl's setups.

We could merge this PR as-is and not enable it on Melodic, then branch for Noetic and enable it there by default.

I hate branching for these little things, but I don't really see another option.

If we leave it disabled, how would you enable it manually? copy paste the launch file?

I totally agree on avoiding branch-mess where possible

I see now that I forgot to export the params using args. I'll add that. That would allow overriding the defaults.

Seems I've forgotten to fix the args missing. I'll need to address that before merging this.

industrial_robot_client/launch/robot_interface_streaming.launch

industrial_robot_client/src/joint_trajectory_action.cpp

Co-authored-by: Simon Schmeisser <[email protected]>

gavanderhoorn · 2021-07-01T09:43:53Z

@simonschmeisser: thanks for the review.

Would you see an opportunity to test this on any real hw?

I'd be interested to know whether this mitigates what you reported in #131 for instance.

simonschmeisser · 2021-07-01T09:47:29Z

I'll try to fit in testing that on our Mitsubishi in the evening

gavanderhoorn · 2021-07-01T12:34:15Z

I actually don't have access to an downloading driver / robot, so I'd be interested to know whether the proposed changes actually work there.

If they do, we'll need to update the .launch file for download drivers as well.

simonschmeisser · 2021-07-01T12:40:37Z

Thanks for pointing that out, I actually only have access to a downloading robot/driver

gavanderhoorn · 2021-07-01T12:42:33Z

Thanks for pointing that out, I actually only have access to a downloading robot/driver

I expect the changes to accepting goals to work for both. I believe that should help with #131.

Aborting active goals will work, but whether the OEM server program also cooperates I can't say -- and will depend on its implementation of course.

Follow the style of IRC.

isOn(..), isOff(..) and isUnknown(..) are a bit too generic to not isolate them a bit.

Instead of as part of the ctor.

simonschmeisser · 2021-07-09T14:55:13Z

We would need to set the error_text string field of the FollowJointTrajectory msg for it to be printed there

I'm not sure why we have the empty controller name in all of our controller files ... what do you usually put there? Something like mitsubishi driver?

gavanderhoorn · 2021-07-09T14:59:23Z

We would need to set the error_text string field of the FollowJointTrajectory msg for it to be printed there

According to the api doc (and the sources), anything you pass to setRejected(..) gets assigned to the error_text. At least that's how I interpreted that.

I'm not sure why we have the empty controller name in all of our controller files ... what do you usually put there? Something like mitsubishi driver?

It's most likely empty because the template in the tutorial lots of people refer to has it set to "".

If you set it to something non-empty, MoveIt will start looking in a different place for your action topics. That's not necessarily a problem though, you'd just have to update your driver to offer the action server there (or remap).

simonschmeisser · 2021-07-09T15:02:06Z

According to the api doc (and the sources), anything you pass to setRejected(..) gets assigned to the error_text. At least that's how I interpreted that.

But we have two text variables for the error here. The generic one you are setting and the redundant one in http://docs.ros.org/en/api/control_msgs/html/action/FollowJointTrajectory.html

gavanderhoorn · 2021-07-09T15:03:19Z

O, crap, you're right.

I'd somehow confused those.

Perhaps MoveIt should check both and display them.

industrial_robot_client/src/joint_trajectory_action.cpp

gavanderhoorn · 2021-07-13T18:19:34Z

So with the technical side out of the way, the question becomes: do we want to merge this into melodic-devel and change the default behaviour?

@simonschmeisser @Levi-Armstrong @anyone-with-an-opinion-really?

gavanderhoorn · 2021-07-18T12:54:04Z

With 0eeb23f and 0fa8bba, MoveIt now reports something like:

[ WARN] [1626612647.088687582]: Controller '' failed with error INVALID_GOAL: Rejecting goal: controller reported motion not possible (no further information)
[ WARN] [1626612647.088809223]: Controller handle '' reports status FAILED
[ INFO] [1626612647.088865293]: Completed trajectory execution with status FAILED ...
[ INFO] [1626612647.088960767]: Execution completed: FAILED

The double controller there can be a bit confusing.

I was thinking of maybe adding "OEM" or "robot" there, so we would get:

[ WARN] [1626612647.088687582]: Controller '' failed with error INVALID_GOAL: Rejecting goal: OEM controller reported motion not possible (no further information)

But this would of course not be accurate if the server side would not run on an OEM controller.

simonschmeisser · 2021-07-19T15:18:20Z

I'm not sure how many people understand what you mean by OEM so I'd rather go for robot controller

industrial_robot_client/src/joint_trajectory_action.cpp

simonschmeisser

There don't seem to be that many opinionated people around any more ...

I vote for merging this as is to melodic-devel as I see the slight change in behavior as a bug fix basically

So their values can be changed by users.

gavanderhoorn · 2022-06-02T14:07:02Z

Ok, to move this forward, I'm going to create a "post noetic" branch.

On that branch, everything discussed here will default to false (ie: ignore_motion_server_error and consider_status_unknowns_ok).

On both Melodic and Noetic, they'll default to true (to maintain the current behaviour).

People still using industrial_robot_client and likely to keep use it could build from source. The binary releases will just play-nice.

simonschmeisser · 2023-01-29T20:54:45Z

Friendly ping, could be part of ros-o

gavanderhoorn requested review from JeremyZoss and Levi-Armstrong June 26, 2021 16:04

gavanderhoorn mentioned this pull request Jun 26, 2021

Trajectory action should abort on robot error. #83

Closed

gavanderhoorn mentioned this pull request Jun 28, 2021

Noetic release #256

Closed

gavanderhoorn added 9 commits June 28, 2021 16:18

jta: abort goal on e-stop or error

55034f5

traj_action: use TriState convenience functions

3a7f82a

Instead of manually comparing against TriState enum values.

traj_action: silence within-goal-constraints check

ad85a38

It doesn't need to be *this* verbose.

traj_action: formatting

c71746c

traj_action: move is_*(..) to utils

16082f8

robot_client: configure new params

e1d78ce

But keep previous behaviour.

traj_action: split cancelling goal and stopping relay

22e3a09

So we can reuse stopping the relay.

gavanderhoorn force-pushed the rebase_83_abort_on_error branch from 8c77870 to 22e3a09 Compare June 28, 2021 14:19

simonschmeisser reviewed Jul 1, 2021

View reviewed changes

gavanderhoorn and others added 2 commits July 1, 2021 11:36

Make param read DRY (ignore srvr error)

d488591

Co-authored-by: Simon Schmeisser <[email protected]>

Make param read DRY (unknowns OK)

d254877

Co-authored-by: Simon Schmeisser <[email protected]>

gavanderhoorn added 4 commits July 1, 2021 14:51

traj_action: use camelCase, not snake_case

fad1597

Follow the style of IRC.

traj_action: put TriState helpers in ns

805b15c

isOn(..), isOff(..) and isUnknown(..) are a bit too generic to not isolate them a bit.

traj_action: init new members where declared

9d4a5f3

Instead of as part of the ctor.

traj_action: clarify ctor overrides param values

fc00588

gavanderhoorn dismissed simonschmeisser’s stale review via ef1bb32 July 9, 2021 14:52

We only need TriState, not the full RobotStatus

ef1bb32

gavanderhoorn commented Jul 9, 2021

View reviewed changes

industrial_robot_client/src/joint_trajectory_action.cpp Show resolved Hide resolved

gavanderhoorn commented Jul 9, 2021

View reviewed changes

industrial_robot_client/src/joint_trajectory_action.cpp Show resolved Hide resolved

gavanderhoorn added 2 commits July 9, 2021 17:11

Send abort_msg back to client

0fa8bba

Send reject_msg back to client

0eeb23f

simonschmeisser previously approved these changes Jul 9, 2021

View reviewed changes

gavanderhoorn mentioned this pull request Aug 22, 2021

val3 controller only get limited trajectory into buffer. (unable to fully excute the trjaectory) ros-industrial/staubli_val3_driver#42

Closed

gavanderhoorn commented Sep 30, 2021

View reviewed changes

industrial_robot_client/src/joint_trajectory_action.cpp Outdated Show resolved Hide resolved

gavanderhoorn commented Sep 30, 2021

View reviewed changes

industrial_robot_client/src/joint_trajectory_action.cpp Outdated Show resolved Hide resolved

gavanderhoorn commented Sep 30, 2021

View reviewed changes

industrial_robot_client/src/joint_trajectory_action.cpp Outdated Show resolved Hide resolved

gavanderhoorn dismissed simonschmeisser’s stale review via 98b6c7b September 30, 2021 09:23

gavanderhoorn added 3 commits September 30, 2021 11:23

Be clear about which controller reported a problem

98b6c7b

Be clear about which controller reported a problem

c64243a

Be clear about which controller reported a problem

2b5b079

simonschmeisser previously approved these changes Sep 30, 2021

View reviewed changes

traj_action: also expose params as args in download launch file

8db3016

So their values can be changed by users.

gavanderhoorn dismissed simonschmeisser’s stale review via 8db3016 October 5, 2021 12:54

simonschmeisser approved these changes Nov 8, 2021

View reviewed changes

This was referenced Mar 29, 2022

Robot not enabled not causing joint_trajectory_action to fail ros-industrial/motoman#486

Closed

Alternative implementation of multi-group trajectories and update motoman error handling ros-industrial/motoman#488

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort-of rebase of 83: traj action aborts on error #271

Sort-of rebase of 83: traj action aborts on error #271

gavanderhoorn commented Jun 26, 2021

gavanderhoorn commented Jun 26, 2021 •

edited

Loading

gavanderhoorn commented Jun 26, 2021 •

edited

Loading

gavanderhoorn commented Jun 26, 2021

gavanderhoorn commented Jul 1, 2021

simonschmeisser Jul 1, 2021

gavanderhoorn Jul 1, 2021

simonschmeisser Jul 1, 2021

gavanderhoorn Jul 1, 2021

gavanderhoorn Sep 30, 2021

gavanderhoorn commented Jul 1, 2021

simonschmeisser commented Jul 1, 2021

gavanderhoorn commented Jul 1, 2021

simonschmeisser commented Jul 1, 2021

gavanderhoorn commented Jul 1, 2021

simonschmeisser commented Jul 9, 2021

gavanderhoorn commented Jul 9, 2021

simonschmeisser commented Jul 9, 2021

gavanderhoorn commented Jul 9, 2021 •

edited

Loading

gavanderhoorn commented Jul 13, 2021

gavanderhoorn commented Jul 18, 2021

simonschmeisser commented Jul 19, 2021

simonschmeisser left a comment

gavanderhoorn commented Jun 2, 2022

simonschmeisser commented Jan 29, 2023

	<param name="ignore_motion_server_error" type="bool" value="true" />
	<param name="ignore_motion_server_error" type="bool" value="false" />

Sort-of rebase of 83: traj action aborts on error #271

Are you sure you want to change the base?

Sort-of rebase of 83: traj action aborts on error #271

Conversation

gavanderhoorn commented Jun 26, 2021

gavanderhoorn commented Jun 26, 2021 • edited Loading

gavanderhoorn commented Jun 26, 2021 • edited Loading

gavanderhoorn commented Jun 26, 2021

gavanderhoorn commented Jul 1, 2021

simonschmeisser Jul 1, 2021

Choose a reason for hiding this comment

gavanderhoorn Jul 1, 2021

Choose a reason for hiding this comment

simonschmeisser Jul 1, 2021

Choose a reason for hiding this comment

gavanderhoorn Jul 1, 2021

Choose a reason for hiding this comment

gavanderhoorn Sep 30, 2021

Choose a reason for hiding this comment

gavanderhoorn commented Jul 1, 2021

simonschmeisser commented Jul 1, 2021

gavanderhoorn commented Jul 1, 2021

simonschmeisser commented Jul 1, 2021

gavanderhoorn commented Jul 1, 2021

simonschmeisser commented Jul 9, 2021

gavanderhoorn commented Jul 9, 2021

simonschmeisser commented Jul 9, 2021

gavanderhoorn commented Jul 9, 2021 • edited Loading

gavanderhoorn commented Jul 13, 2021

gavanderhoorn commented Jul 18, 2021

simonschmeisser commented Jul 19, 2021

simonschmeisser left a comment

Choose a reason for hiding this comment

gavanderhoorn commented Jun 2, 2022

simonschmeisser commented Jan 29, 2023

gavanderhoorn commented Jun 26, 2021 •

edited

Loading

gavanderhoorn commented Jun 26, 2021 •

edited

Loading

gavanderhoorn commented Jul 9, 2021 •

edited

Loading