Yaskawa-Global / motoros2

ROS 2 (rcl, rclc & micro-ROS) node for MotoPlus-compatible Yaskawa Motoman robot controllers
95 stars 16 forks source link

Warn about failure to retrieve multi-robot calib data #54

Closed gavanderhoorn closed 6 months ago

gavanderhoorn commented 1 year ago

The current implementation silently ignores failure to call mpGetRobotCalibrationData(..):

https://github.com/Yaskawa-Global/motoros2/blob/8c1a524e8cad8e45e865454bb01678cabc416e38/src/ControllerStatusIO.c#L116-L135

MotoROS2 won't crash if the call fails, but it will result in TF transforms being incorrect, which will surprise users of multi-robot systems (especially if they have performed the robot-to-robot calibration).

A new failure code should be defined and reported on the pendant.

gavanderhoorn commented 1 year ago

If TF broadcasting is not enabled, we might prefer to ignore the failure instead of asserting on it.

gavanderhoorn commented 1 year ago

@ted-miller: my M+ SDK API manual doesn't mention mpGetRobotCalibrationData(..).

Does it return an error for single-group systems?

What about multi-robot systems for which the calibration hasn't been performed? Would that result in what is described in Incorrect transform tree origin with multi-robot setups?

If so: can we make this a fatal error?

If not, should we perhaps raise an alarm, but just make it informational?

ted-miller commented 1 year ago

my M+ SDK API manual doesn't mention mpGetRobotCalibrationData(..)

Neither does mine.

Does it return an error for single-group systems?

I would expect that it does.

What about multi-robot systems for which the calibration hasn't been performed? Would that result in what is described in Incorrect transform tree origin with multi-robot setups?

Yes. That's how I got that image which was used in the RIC presentation.

If so: can we make this a fatal error?

I'm definitely against making it fatal. (And I'm just now realizing the title of this issue...)

If not, should we perhaps raise an alarm, but just make it informational?

I don't know. Frankly, I could argue against this entire issue...

If the robot's are all working directly together, then it is very likely that the calibration won't be performed.

For instance, in a configuration like this, the robots might be calibrated to the positioner (but possibly not), but it's unlikely that they would be calibrated to each other. image

Although the lack of calibration would certainly interfere with visualization, I'm assuming that someone could manually add an offset in the vis environment.

gavanderhoorn commented 1 year ago

Hm.

But as a ROS user, I would be puzzled as to why my TF tree, which I configured MotoROS2 to broadcast for me, doesn't look right. And I would not know why that is, unless I happen to have read this in the known issues/ROS API sections.

There would be no notice or warning from MotoROS2, even though the absence of such calibration data would prevent it from broadcasting a correct TF tree.

We could gate the assert on whether publish_tf is true, and/or add another config item which lets users override the assert (ie: "I know there is no calibration data, but publish TF anyway").

To me, publishing TF without that robot<->robot calibration having been performed would not make sense, as anything using those transforms would find two (or more) colliding structures, which (fi) motion planners would not be happy about.

ted-miller commented 1 year ago

We could gate the assert on whether publish_tf is true, and/or add another config item which lets users override the assert (ie: "I know there is no calibration data, but publish TF anyway").

I could go for that. (But I still think it should be non-fatal.)

gavanderhoorn commented 1 year ago

@ted-miller: could you clarify whether there is any relationship between how many motion groups there are, and how many calibration files there "should" be?

Could it be the case all groups are calibrated against all others, resulting in a maximum of (M x N) / 2 files, or does it work in some other way? From your https://github.com/Yaskawa-Global/motoros2/issues/54#issuecomment-1595217178, I guess there could be 0, even for systems with 8 (or more) groups?

Additionally: if file i didn't load/exist (ie: Ros_mpGetRobotCalibrationData(i, ..) returned != OK), can i + 1 still load? Are the indices sequential, or could it for instance happen that an index is for some reason no longer used (perhaps a calibration was deleted, and the files with a higher index don't get renumbered)?

And I don't quite understand this comment:

If the robot's are all working directly together, then it is very likely that the calibration won't be performed.

In general I would expect someone installing MotoROS2 to want to have TF correct (if TF broadcasting is enabled). If they haven't calibrated already, I'd consider calibrating part of the setup process for MotoROS2.

But as discussed above, we could make raising alarms about this conditional upon whether publish_tf == true and ignore_missing_robot_calibration == true.

ted-miller commented 1 year ago

There isn't a quantifiable number for how many calibration files there "should be". There are cases where there are zero calibrations, but you can have up to 32.

Additionally, there are cases where a robot could have multiple calibrations. For instance, here's an example where the only difference is which one is the "master". image

Additionally: if file i didn't load/exist (ie: Ros_mpGetRobotCalibrationData(i, ..) returned != OK), can i + 1 still load?

Yes. They are not necessarily sequential.

image

If the robot's are all working directly together, then it is very likely that the calibration won't be performed.

Should say "If the robot's are NOT all working directly together, then it is very likely that the calibration won't be performed."

So a welder might be calibrated to a positioner for welding a tube. But then a handling robot that places the finished part onto a conveyor wouldn't be calibrated to anything. (That's a real example.)

we could make raising alarms about this conditional upon whether publish_tf == true and ignore_missing_robot_calibration == true.

I'm ok with alarming unless they ignore_missing_robot_calibration is set in the config file. I just don't want it to be fatal since the robot is fully capable of operation.

gavanderhoorn commented 1 year ago

Given the complexity, we may want to postpone implementing something to a later milestone.

I suggest we move it to 0.1.2.

gavanderhoorn commented 11 months ago

To clarify: we'd be OK with posting a non-fatal/non-asserting alarm in case:

?

ted-miller commented 11 months ago

Yeah, I guess.

gavanderhoorn commented 11 months ago

See #169.

gavanderhoorn commented 10 months ago

I don't want to hold up the release of 0.1.2 any longer, and I don't have enough time to work on finishing #169 right now.

I've changed the milestone here to untargeted for now.