-
Notifications
You must be signed in to change notification settings - Fork 430
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
Initialize params via yaml from command line #488
Conversation
2cb890e
to
f067f5f
Compare
Optimistically moving to in review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine in general, but there are a few things that need to be addressed, like memory leaking.
throw std::runtime_error("Need valid node handle in NodeParameters"); | ||
} | ||
const rcl_node_options_t * options = rcl_node_get_options(node); | ||
if (NULL == options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend using nullptr
in C++.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops bfb497c
auto get_yaml_paths = [&yaml_paths, &options](const rcl_arguments_t * args) { | ||
int num_yaml_files = rcl_arguments_get_param_files_count(args); | ||
if (num_yaml_files > 0) { | ||
char ** param_files; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is never deallocated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 934fa77
combined_name = node_namespace + node_name; | ||
} else { | ||
combined_name = node_namespace + '/' + node_name; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably be a function for this in rcl
, I wouldn't do it now, but an issue would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ticketed in ros2/rcl#255
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
|
||
std::map<std::string, rclcpp::Parameter> parameters; | ||
|
||
// TODO(sloretz) rcl too parse yaml when circular dependency is solved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"use rcl to parse..."?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammar fix in 25716b1
throw std::runtime_error("Failed to parse parameters " + yaml_path); | ||
} | ||
|
||
rclcpp::ParameterMap initial_map = rclcpp::parameter_map_from(yaml_params); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this fini
the yaml_params
? If not then it needs to be, if so then... eew lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not. Added call to ...fini()
in d765738
for (const std::string & yaml_path : yaml_paths) { | ||
rcl_params_t * yaml_params = rcl_yaml_node_struct_init(options->allocator); | ||
if (NULL == yaml_params) { | ||
throw std::runtime_error("Failed to initialize yaml params struct"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be able to narrow this down to a better exception, like std::bad_alloc
. Looking at them now, the yaml parsing API in yaml leaves something to be desired in terms of error reporting and namespacing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this exception to bad_alloc
in ef427a7
CI as of ef427a7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, with one suggestion on how to make it even better.
rclcpp::exceptions::throw_from_rcl_error(ret); | ||
} | ||
for (int i = 0; i < num_yaml_files; ++i) { | ||
yaml_paths.emplace_back(param_files[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if this throws? Maybe a "scope exit" kind of thing to clean up the whole param_files
all at once would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleanup on scope exit in b9a7e74
combined_name = node_namespace + node_name; | ||
} else { | ||
combined_name = node_namespace + '/' + node_name; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
It looks like ros2/rmw_fastrtps#205 is the cause of the test failure on the OSX build. I'll re-write the test to avoid destroying clients as a workaround. |
|
||
std::map<std::string, rclcpp::Parameter> parameters; | ||
|
||
// TODO(sloretz) use rcl to parse yaml when circular dependency is solved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sloretz do you happen to remember the context around this todo? I came across it today and was confused since it seems like yaml parsing is done in rcl already, so it must be referring to something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since it seems like yaml parsing is done in rcl already
This is actually done in rcl_yaml_param_parser and not rcl.
That package is currently depending on rcl to be able to use rcl types, so rcl cannot depenc on rcl_yaml_param_parser to avoid circular dependency.
ros2/rcl#252
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I see! Thanks, I thought from the rcl_
prefix that they were in rcl, but of course that's not the case 😄 I'll reference that ticket in the todo.
Signed-off-by: Erik Boasson <eb@ilities.com>
…ros2#488) * Remove exception throw in dtor Update logging errors to avoid duplication Signed-off-by: Devin Bonnie <dbbonnie@amazon.com> * Cleanup reset logic Add reset documentation Signed-off-by: Devin Bonnie <dbbonnie@amazon.com> * Fix cpplint error Signed-off-by: Devin Bonnie <dbbonnie@amazon.com> * Fix docstring Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Blocks #477
Requires #485
Requires #486
Requires ros2/rcl#253
This initializes the parameters on a node from yaml files. Since there are multiple sources of initial values passed to a node (multiple yaml files on the command line, multiple yaml files on node specific CLI arugments, explicit
initial_values
passed to constructor) this PR merges all the sources such that constructorinitial_values
overwrites constructorarguments
which overwrites global command line arguments. Within CLI arguments Yaml files specified later overwrite ones specified earlier.i.e. this works
The only commit is 2cb890e. This PR needs to be rebased when #485 and #486 are merged. Also this PR needs tests.