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

Added error handling for dynamic_reconfigure exceptions #10

Merged
merged 2 commits into from
Jul 12, 2017
Merged

Added error handling for dynamic_reconfigure exceptions #10

merged 2 commits into from
Jul 12, 2017

Conversation

arne48
Copy link
Member

@arne48 arne48 commented Jun 22, 2017

When attempting to update the configuration of a node via dynamic_reconfigure two exceptions could be raised and weren't catched.
dynamic_reconfigure.DynamicReconfigureParameterException: don't know parameter: ....
and
rospy.service.ServiceException: service [/node .../set_parameters] unavailable
The later one is gets raised because dynamic_reconfigure is not catching it itself and reraising its own exception. I will also issue a PR for the reraising at the dynamic_reconfigure project.
Besides the catches for the occurring exceptions I also already added the catch for the exception I expected will be raised once the PR might have been merged in dynamic_reconfigure.

@dirk-thomas
Copy link
Contributor

@cottsay FYI. Please take a look at the patch when you find some time.

@cottsay
Copy link
Member

cottsay commented Jul 6, 2017

Ack, I'll make a note so I don't forget. Thanks.

@cottsay
Copy link
Member

cottsay commented Jul 6, 2017

@dirk-thomas, can you make me a contributor for this repo? Doesn't look like I can change the assignment or accept the PR or anything. Thanks!

@dirk-thomas
Copy link
Contributor

@cottsay Done.

@cottsay cottsay self-assigned this Jul 6, 2017
@cottsay cottsay self-requested a review July 6, 2017 21:21
Copy link
Member

@cottsay cottsay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I think some time we should consider if some of these exceptions constitute the ROS "ERROR" level of logging, but that is outside the scope of this change.

@dirk-thomas
Copy link
Contributor

@cottsay As the maintainer you are now in charge of (squash) merging PRs and also decide when to make new releases with bloom for all the available ROS distros in order to make the fixes available to users not building from source.

@cottsay cottsay changed the title Added error handling for update_configuration function Added error handling for dynamic_reconfigure exceptions Jul 12, 2017
@cottsay cottsay merged commit cd14fc3 into ros-visualization:master Jul 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants