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

Convert rcl_params_t to ParameterMap #485

Merged
merged 3 commits into from
Jun 5, 2018
Merged

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented May 31, 2018

PR complete, waiting for #481 to be merged first, then this needs to be targeted at master. Ready to review

This allows the initial parameter values for a node to be passed in via it's constructor. These parameters are then immediately set on the node.

edit: invoked CI wrong, will wait to run until #481 is in.

blocks #477
requires #481

@sloretz sloretz added the in progress Actively being worked on (Kanban column) label May 31, 2018
@sloretz sloretz added this to the bouncy milestone May 31, 2018
@sloretz sloretz self-assigned this May 31, 2018
@sloretz sloretz changed the base branch from master to split_parametervariant May 31, 2018 19:43
@sloretz sloretz mentioned this pull request May 31, 2018
10 tasks
{

/// Indicate `rcl_params_t` is or invalid.
class InvalidParametersException : public std::exception
Copy link
Contributor Author

Choose a reason for hiding this comment

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

exception type should be moved to rclcpp/exceptions.hpp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved plus inherit from runtime_error in b415df7

@sloretz sloretz changed the base branch from split_parametervariant to master June 1, 2018 20:26
Adds rclcpp::parameter_map_from(const rcl_params_t * const)
Adds rclcpp::parameter_value_from(const rcl_variant_t * const)
Adds dependency on rcl_yaml_param_parser
@sloretz
Copy link
Contributor Author

sloretz commented Jun 1, 2018

CI

@sloretz sloretz added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jun 1, 2018
@sloretz sloretz requested a review from wjwwood June 5, 2018 01:02
}

/// make sure there is a leading slash on the fully qualified node name
std::string node_name("/");
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep this here for now 👍

It may be better (in the future) to enforce this at a lower level.
It could be done in the parser:

  • reject node_namespace + node_name not starting with a leading slash
  • add the leading slash to any provided node_namespace + node_name that doesnt have one

If the structure always assumes fully qualified name, I guess we could enforce this it in the structure itself by providing structure functions to assign element and reject or add leading slash to the provided node names. This would allow other users of the structure to not have to duplicate that logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Roger. I wasn't sure if I should make a bug on rcl_yaml_param_parser about it returning names without a leading slash, or if there will one day be a use case for that (like pushing nodes up a namespace in ros2 launch?). Thanks for the reviews!

Copy link
Member

@mikaelarguedas mikaelarguedas 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, thanks @sloretz

@sloretz sloretz merged commit 8f793fd into master Jun 5, 2018
@sloretz sloretz deleted the params_from_yaml_parser branch June 5, 2018 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants