Publish industrial_msgs::RobotStatus #5

simonschmeisser · 2017-11-15T14:47:56Z

This adds a publisher for industrial_msgs::RobotStatus

the mapping is not quite unique and might need to be discussed some more. Especially protective_stopped is currently mapped to in_error. There are probably other error states that should lead to in_error as well and be assigned an unique error_code. @gavanderhoorn Do we have a generic list of such error_codes somewhere or are these "vendor specific" and if so where would I place an enum/message description?

the mapping is not quite simple and might need to be discussed some more

gavanderhoorn · 2017-11-15T14:49:14Z

Is this a forward-port of ros-industrial-attic#143?

simonschmeisser · 2017-11-15T14:50:46Z

more of a rewrite as the code changed significantly, but yes I meant to reference that PR

gavanderhoorn · 2017-11-15T14:50:58Z

@simonschmeisser wrote:

Do we have a generic list of such error_codes somewhere or are these "vendor specific" and if so where would I place an enum/message description?

The message docs say this:

Error code: Vendor specific error code (non zero indicates error)

For UR we could maybe see whether we could use some of the status codes that are sometimes printed in the log (on the TP) for certain events. But I wouldn't know where to find those.

gavanderhoorn · 2017-11-15T14:52:34Z

@simonschmeisser wrote:

more of a rewrite as the code changed significantly, but yes I meant to reference that PR

ok. Well that PR includes some more logic for when in_error should be set. Did you not include that on purpose?

simonschmeisser · 2017-11-15T14:59:09Z

I couldn't really follow that logic, so yes, I did not copy that on purpose.

Zagitta · 2017-11-15T15:05:17Z

src/ros/mb_publisher.cpp

+  msg.e_stopped.val = data.emergency_stopped;
+  msg.in_motion.val = data.program_running;
+  msg.in_error.val = data.protective_stopped;
+  msg.motion_possible.val = (robot_mode_V3_X::RUNNING == data.robot_mode) ? industrial_msgs::TriState::ON : industrial_msgs::TriState::OFF;


Nitpicking but data.robot_mode == robot_mode_V3_X::RUNNING (reverse ordering) is probably preferred as this is the style used elsewhere.

ah, yes, sorry, I was experimenting if I could get this away:

ur_modern_driver/src/ros/mb_publisher.cpp:41: Warnung: enumeral mismatch in conditional expression: ‘industrial_msgs::TriState_<std::allocator<void> >::<anonymous enum>’ vs ‘industrial_msgs::TriState_<std::allocator<void> >::<anonymous enum>’ [-Wenum-compare] msg.motion_possible.val = (data.robot_mode == robot_mode_V3_X::RUNNING) ? industrial_msgs::TriState::ON : industrial_msgs::TriState::OFF; ^

Zagitta · 2017-11-15T15:07:41Z

Thank you very much for the PR @simonschmeisser, looks very good to me, my only comment is the above nitpick and I'd be more than happy to merge your contribution 😄

gavanderhoorn · 2017-11-15T15:12:44Z

src/ros/mb_publisher.cpp

+
+  msg.drives_powered.val = data.robot_power_on;
+  msg.e_stopped.val = data.emergency_stopped;
+  msg.in_motion.val = data.program_running;


I don't believe in_motion is the same as program_running, is it?

Not sure whether there is a way to determine the robot is in motion without a program, or whether that is even possible actually.

Can secondary programs contain motion commands? And do they then update program_running?

it is if but not iff, ie there will always be a program running when it moves but having a program running does not signify that it will actually move. At least that's my understanding

Ok. But then this expression doesn't currently accurately encode the state of the robot, right?

It could be executing a program without any motion?

industrial_msgs::TriState::Unknown then?

we don't use this field except for decorative purposes, so I don't care from a consumer point of view

we don't use this field except for decorative purposes, so I don't care from a consumer point of view

That's great, but if we add this (RobotStatus) it should accurately encode the status of the robot.

If we can't derive something from what the robot provides us, then yes, I believe using TriState::Unknown for now would be better.

gavanderhoorn · 2017-11-15T15:13:42Z

src/ros/mb_publisher.cpp

+  msg.drives_powered.val = data.robot_power_on;
+  msg.e_stopped.val = data.emergency_stopped;
+  msg.in_motion.val = data.program_running;
+  msg.in_error.val = data.protective_stopped;


Is protective_stopped high for all errors that can occur?

there does not seem to be another way to be informed about errors looking at the format in robot_mode.h

What about emergency_stopped? That would definitely be an 'in_error' situation, but that's not captured here at the moment. ros-industrial-attic#143 makes in_error the union of protective_stopped and emergency_stopped.

ah okay, I thought of e-stopped more like a state and the error would appear separately once you try moving despite it being in e-stopped mode. But I can change it to said union. This would again ask for a place to define the "vendor specific" error codes and then setting error_code to differentiate protective_stopped

"Vendor specific" means that the error codes are defined by the vendor. Not by us, for every vendor.

This field is typically populated by copying the error code that the robot controller gives you for different errors. On Fanucs fi you would be copying the code returned by the ERR_DATA(..) Karel function (which is typically a 5 or 6 digit number).

Those codes are defined by Fanuc. We don't make them up ourselves.

UR has similar codes I believe (just take a look at the log on the TP), but I'm not sure we can retrieve those from somewhere.

I thought of e-stopped more like a state and the error would appear separately once you try moving despite it being in e-stopped mode.

That could definitely be a way to implement it, but we would need a way to reliably determine which of the two it is. I don't have a scripting manual with me right now, but if there is a get_last_error_code() function that we can use, then we can definitely do what you propose.

You're correct. e_stopped is the state, and in_error should follow from whether the robot controller defines this as an error.

Let's keep what you have for now.

Sorry for the noise.

re: vendor specific codes: looking at a UR we have here I keep seeing C100A5 in the log whenever the robot is Robot changed mode: EMERGENCY STOPPED.

Those are the codes that would go into error_code.

gavanderhoorn · 2017-11-15T15:28:35Z

src/ros/mb_publisher.cpp

+void MBPublisher::publishRobotStatus(SharedRobotModeData& data)
+{
+  industrial_msgs::RobotStatus msg;
+  msg.drives_powered.val = data.robot_power_on;


Can robot_power_on ever be true if real_robot_enabled is false? Perhaps in ur-sim?

testing with ursim I get the following output in Poly:
0000d05h41m35.184s RobotInterface C102A0: Real Robot not connected - Simulating Robot

meanwhile the robot mode contains the following data:

when I turn off the robot:
robot_power_on: 0 physical robot: 0 real_robot: 1 mode: 3 (POWER_OFF)

when I turn on the robot but do not yet release the brakes (ON but not START):
robot_power_on: 1 physical robot: 0 real_robot: 1 mode: 5 (IDLE)

when I release the brakes:
robot_power_on: 1 physical robot: 0 real_robot: 1 mode: 7 (RUNNING)

so I'm not sure what real_robot_enabled is supposed to tell us, maybe there is a way to switch to moving a simulated robot on the actual controller?

so I'm not sure what real_robot_enabled is supposed to tell us, maybe there is a way to switch to moving a simulated robot on the actual controller?

There is a two-option selection bullet list at the bottom right or left (don't remember) that contains those two options. I've never used it, but IIRC you can use it to switch to a virtual robot.

~~The difference between IDLE and RUNNING may indicate that RUNNING would be better indicator of drives_powered?~~

Edit: hm, no. That makes no sense.

ok, found that (for the reference: it is in 'Program Robot' -> 'Move' or 'I/O' but I can only change it to simulated in IO), real_robot_enabled then does turn to false. But this doesn't map to any of the industrial_msgs::RobotStatus fields does it?

# Drive power status: True if drives are powered. Motion commands will # automatically enable the drives if required. Drive power is not requred # for possible motion industrial_msgs/TriState drives_powered

So yes, maybe drives powered should be obtained from Mode == RUNNING as in that halfway on mode (breakes still on) it does not automatically start moving. But then drives_powered would be the same as motion_possible. And as far as I understand motion_possible is authorative?

I think drives_powered should reflect the status of the drives (ie: robot_power_on.

RUNNING could then be used in the predicate for motion_possible (ie: mode != RUNNING -> motion_possible = false.

gavanderhoorn · 2017-11-15T16:42:10Z

I think if you can add some comments (fi, why in_motion is Unknown at the moment), this would be ok to merge.

this allows to extract motion_possible and mode as well added comments for everything non obvious or disputable

simonschmeisser · 2017-11-15T17:59:03Z

Switching to using either version 1 or 3 of the data package instead of using the default type allows publishing some more info. I also commented everything and added const correctness.

simonschmeisser · 2017-11-23T13:17:05Z

@Zagitta @gavanderhoorn anything else you would like me to modify?

Zagitta · 2017-11-23T18:41:25Z

It looks good to me but since I'm not really involved in anything related to UR anymore I'll let @gavanderhoorn have the final say 😃

Zagitta · 2017-12-08T13:11:00Z

include/ur_modern_driver/ros/mb_publisher.h


 public:
  MBPublisher() : io_pub_(nh_.advertise<ur_msgs::IOStates>("ur_driver/io_states", 1))
+                , status_pub_(nh_.advertise<industrial_msgs::RobotStatus>("robot_status", 1))


Should this perhaps be published at "ur_driver/robot_status"?

We could, but that would make it non-compliant with other ros-i robot drivers.

A remap can fix that though.

Alright yes that makes perfect sense, let's keep it as it is here then.

Well .. let's actually not.

All topics for the ur_driver are below a ur_driver namespace already, so having this one outside it breaks that.

If/when we need to make this driver ros-i compliant, we can either blanketly remap everything or update the topics.

So I'll add the prefix for consistencies sake?

I believe that makes the most sense for now, yes.

Otherwise users would find all topics of the driver under the ur_driver namespace, but a lone (and seemingly unrelated) /robot_status topic in the global one.

gavanderhoorn · 2017-12-08T13:15:13Z

I would be ok with merging this. My only question would be whether the duplication between publishRobotStatus(const RobotModeData_V1_X&) and publishRobotStatus(const RobotModeData_V3_0__1&) is needed.

@simonschmeisser: would it make sense to introduce a separate function that sets up and publishes the message that gets called from those two callbacks?

simonschmeisser · 2017-12-08T13:39:17Z

@gavanderhoorn yes, I can add a generic publishRobotStatus(industrial_msgs::RobotStatus) that gets called after setting the version-specific fields. Will do so later

gavanderhoorn · 2017-12-08T13:40:59Z

If the version-specific fields are just regular primitive types, you could potentially create a publishRobotStatus(int robot_mode, .. etc) that gets called.

Zagitta · 2017-12-08T14:07:38Z

I suggest following the approach here to keep things consistent 😃

simonschmeisser · 2017-12-20T13:16:34Z

@gavanderhoorn @Zagitta are you fine with the current state or should I change anything else?

Zagitta · 2018-01-02T19:17:42Z

Thank you very much for your contribution 🎉

Publish industrial_msgs::RobotStatus

528e707

the mapping is not quite simple and might need to be discussed some more

Zagitta reviewed Nov 15, 2017

View reviewed changes

gavanderhoorn reviewed Nov 15, 2017

View reviewed changes

don't infer in_motion from programm_running, style fix

f119853

use RobotModeData_V1_X instead of SharedRobotModeData for Version < 3

3c7b2a0

this allows to extract motion_possible and mode as well added comments for everything non obvious or disputable

Zagitta reviewed Dec 8, 2017

View reviewed changes

reduce code duplication, consistent namespacing of topics

a2c620f

Zagitta merged commit 24eef75 into Zagitta:master Jan 2, 2018

simonschmeisser deleted the robot_status branch January 3, 2018 11:28

gavanderhoorn mentioned this pull request Mar 18, 2020

Publish industrial_msgs/RobotStatus messages UniversalRobots/Universal_Robots_ROS_Driver#32

Closed

axelschroth mentioned this pull request Mar 24, 2020

Robot status topic via controller UniversalRobots/Universal_Robots_ROS_Driver#141

Merged

Publish industrial_msgs::RobotStatus #5

Publish industrial_msgs::RobotStatus #5

Conversation

simonschmeisser commented Nov 15, 2017

gavanderhoorn commented Nov 15, 2017

simonschmeisser commented Nov 15, 2017

gavanderhoorn commented Nov 15, 2017 • edited Loading

gavanderhoorn commented Nov 15, 2017

simonschmeisser commented Nov 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zagitta commented Nov 15, 2017

gavanderhoorn Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavanderhoorn Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonschmeisser Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavanderhoorn commented Nov 15, 2017

simonschmeisser commented Nov 15, 2017

simonschmeisser commented Nov 23, 2017

Zagitta commented Nov 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavanderhoorn commented Dec 8, 2017

simonschmeisser commented Dec 8, 2017

gavanderhoorn commented Dec 8, 2017

Zagitta commented Dec 8, 2017

simonschmeisser commented Dec 20, 2017

Zagitta commented Jan 2, 2018

gavanderhoorn commented Nov 15, 2017 •

edited

Loading

gavanderhoorn Nov 15, 2017 •

edited

Loading

gavanderhoorn Nov 15, 2017 •

edited

Loading

simonschmeisser Nov 15, 2017 •

edited

Loading