search

ros-industrial / abb_libegm

A C++ library for interfacing with ABB robot controllers supporting Externally Guided Motion (689-1)
BSD 3-Clause "New" or "Revised" License
93 stars 53 forks source link

Verify outbound values #83

Closed jontje closed 4 years ago

jontje commented 4 years ago

Fix #69.

This PR adds support functions for verifying that no NaN or infinity value(s) are passed to the robot controller.

If a user passes a command containing NaN or infinity value(s) to abb_libegm, then this will be detected and the command will be (silently) ignored. No message will be sent to the robot controller, which will trigger a 41822: No data from the UdpUc device error message in the robot controller.

To improve the behaviour, then abb_libegm should perhaps throw an error instead of ignoring the command silently.

gavanderhoorn commented 4 years ago

So do we really want to silently ignore invalid values?

As the intended behaviour is for the controller to error-out and stop the EGM session anyway, what about throwing an exception in the library?

Ideally we'd notify the user of the problem in some way. printf(..) or similar are undesirable, but seeing as producing a NaN or Inf is an exceptional situation, throwing an exception seems like an appropriate thing to do.

jontje commented 4 years ago

So do we really want to silently ignore invalid values?

For now it is better than nothing.

As the intended behaviour is for the controller to error-out and stop the EGM session anyway, what about throwing an exception in the library?

That is what I mentioned at the end of my summary 😉

Ideally we'd notify the user of the problem in some way. printf(..) or similar are undesirable, but seeing as producing a NaN or Inf is an exceptional situation, throwing an exception seems like an appropriate thing to do.

Agreed, however, I am a bit out-of-practice with exception handling (especially in multi-threaded applications). I also have other more pressing tasks in the near future, but I can add an issue for it.

gavanderhoorn commented 4 years ago

Could we then at least printf(..) something? I'd really like to avoid silently not doing anything, without any notification to the user.

Normally printf(..) would be really bad, but seeing as the controller will terminate the session anyway, it seems like it wouldn't matter much.

jontje commented 4 years ago

Could we then at least printf(..) something? I'd really like to avoid silently not doing anything, without any notification to the user.

Normally printf(..) would be really bad, but seeing as the controller will terminate the session anyway, it seems like it wouldn't matter much.

Hm... I guess we could do that as a temporary solution.

But I would prefer to do it in a separate PR, after at least this branch jontje/abb_libegm/add_external_axes_support has been merged. This is because that can also silently fail, if there is a mismatch between user configurations and the actual EGM messages received.

gavanderhoorn commented 4 years ago

I still believe it would be better not to silently fail (and I expect support requests about this in the future), but I also don't want to hold this up too long.

So :+1:

jontje commented 4 years ago

Thanks for the review @gavanderhoorn