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

Exposed the joint trajectory controller verbose setting as a parameter. #273

Merged
merged 1 commit into from
Aug 10, 2017

Conversation

bponsler
Copy link
Contributor

Expose the joint trajectory controller verbose setting as a parameter

@bmagyar
Copy link
Member

bmagyar commented May 10, 2017

This is something I think should be documented well. The verbose parameter there is for advanced use as it breaks real-time safety by using ROS logging services. I would not "open it to the public" as a parameter. In it's current state there has to be a strong need to go debugging the controller and the one seeking it will find it.

What do you think?

What do you think, gentlemen? @graiola @ipa-mdl

@bponsler
Copy link
Contributor Author

That seems reasonable, totally fine if you want to pass on this.

@bmagyar
Copy link
Member

bmagyar commented May 11, 2017

Maybe you could still change this PR so that instead of opening it as a param, you add a comment in the code to explain that the verbose flag possibly breaks realtime-safety.

@bponsler
Copy link
Contributor Author

I'm fine with adding a comment, but I would suggest also displaying a runtime warning to the user whenever the verbose flag is enabled to help prevent someone from accidentally leaving the flag enabled.

Thoughts?

@bmagyar
Copy link
Member

bmagyar commented Aug 10, 2017

There is a failing test on travis due a timeout but this code it only affecting a print. Merging...

@bmagyar bmagyar merged commit 2c6c412 into ros-controls:kinetic-devel Aug 10, 2017
@bponsler bponsler deleted the kinetic-expose-verbose branch August 10, 2017 17:14
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.

None yet

2 participants