Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verify outbound values #83

Merged
merged 2 commits into from
Mar 12, 2020

Conversation

jontje
Copy link
Contributor

@jontje jontje commented Feb 19, 2020

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
Copy link
Member

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
Copy link
Contributor Author

jontje commented Mar 2, 2020

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
Copy link
Member

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
Copy link
Contributor Author

jontje commented Mar 9, 2020

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
Copy link
Member

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 👍

@gavanderhoorn gavanderhoorn merged commit ea8d239 into ros-industrial:master Mar 12, 2020
@jontje jontje deleted the verify_outbound_values branch March 13, 2020 06:45
@jontje
Copy link
Contributor Author

jontje commented Mar 13, 2020

Thanks for the review @gavanderhoorn

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Improve verification of outbound EGM commands
2 participants