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

Delete failing parameter undeclare in JointGroupPositionController #222

Conversation

schornakj
Copy link
Contributor

This is one way to fix #221 and allow loading the JointGroupPositionController plugin on Rolling.

If I understand correctly, the JointGroupPositionController just sets the value of interface_name_ to hardware_interface::HW_IF_POSITION instead of reading it from a parameter, so it shouldn't make a functional difference whether the parameter is explicitly undeclared or simply not read.

Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

I think that solution goes into the right direction. Maybe would be even better to set the parameter to hardware_interface::HW_IF_POSITION there. This would make it more explicit for someone reading it. And since we don't have dynamic reconfigure, its change during runtime would not change anything.

@schornakj What do you think?

@bmagyar
Copy link
Member

bmagyar commented Aug 13, 2021

also, in general, this should then apply to all forward command controller derivatives, right?

@schornakj
Copy link
Contributor Author

also, in general, this should then apply to all forward command controller derivatives, right?

Good point, there are other plugins with the same issue. I can fix those here as well.

Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

One small comment since we are already editing this part of the code.

@schornakj thanks for your PR and for addressing our comments very fast.

// Explicitly set the interface parameter declared by the forward_command_controller
// to match the value set in the JointGroupEffortController constructor.
get_node()->set_parameter(rclcpp::Parameter("interface_name", hardware_interface::HW_IF_EFFORT));

} catch (const std::exception & e) {
Copy link
Member

Choose a reason for hiding this comment

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

I would use this opportunity also to make this catch more specific. According to docstring set_parameter method throws rclcpp::exceptions::ParameterNotDeclaredException. Should be make this more explicit? @schornakj, @bmagyar what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'd actually continue catching std::exception. If a different exception is thrown other than ParameterNotDeclaredException (which is not likely) we still want to catch it and cleanly exit the initialization function by returning an error code.

@schornakj
Copy link
Contributor Author

@destogl @bmagyar I applied the formatting changes required by ament_cpplint, so this is ready for CI again.

Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

thanks a lot for this!

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.

JointGroupPositionController fails to initialize on Rolling due to rclcpp "undeclare_parameter" API change
3 participants