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

Initialize params via yaml from command line #488

Merged
merged 9 commits into from
Jun 9, 2018
Merged
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 90 additions & 2 deletions rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

#include "rclcpp/node_interfaces/node_parameters.hpp"

#include <rcl_yaml_param_parser/parser.h>

#include <map>
#include <memory>
#include <string>
Expand All @@ -22,6 +24,7 @@

#include "rcl_interfaces/srv/list_parameters.hpp"
#include "rclcpp/create_publisher.hpp"
#include "rclcpp/parameter_map.hpp"
#include "rcutils/logging_macros.h"
#include "rmw/qos_profiles.h"

Expand Down Expand Up @@ -52,10 +55,95 @@ NodeParameters::NodeParameters(
use_intra_process,
allocator);

// Get the node options
const rcl_node_t * node = node_base->get_rcl_node_handle();
if (nullptr == node) {
throw std::runtime_error("Need valid node handle in NodeParameters");
}
const rcl_node_options_t * options = rcl_node_get_options(node);
if (nullptr == options) {
throw std::runtime_error("Need valid node options NodeParameters");
}

// Get paths to yaml files containing initial parameter values
std::vector<std::string> yaml_paths;

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;
Copy link
Member

Choose a reason for hiding this comment

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

This is never deallocated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 934fa77

rcl_ret_t ret = rcl_arguments_get_param_files(args, options->allocator, &param_files);
if (RCL_RET_OK != ret) {
rclcpp::exceptions::throw_from_rcl_error(ret);
}
for (int i = 0; i < num_yaml_files; ++i) {
yaml_paths.emplace_back(param_files[i]);
Copy link
Member

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.

Copy link
Contributor Author

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

options->allocator.deallocate(param_files[i], options->allocator.state);
}
options->allocator.deallocate(param_files, options->allocator.state);
}
};

// global before local so that local overwrites global
if (options->use_global_arguments) {
get_yaml_paths(rcl_get_global_arguments());
}
get_yaml_paths(&(options->arguments));

// Get fully qualified node name post-remapping to use to find node's params in yaml files
const std::string node_name = node_base->get_name();
const std::string node_namespace = node_base->get_namespace();
if (0u == node_namespace.size() || 0u == node_name.size()) {
// Should never happen
throw std::runtime_error("Node name and namespace were not set");
}
std::string combined_name;
if ('/' == node_namespace.at(node_namespace.size() - 1)) {
combined_name = node_namespace + node_name;
} else {
combined_name = node_namespace + '/' + 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.

Should probably be a function for this in rcl, I wouldn't do it now, but an issue would be nice.

Copy link
Contributor Author

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

Copy link
Member

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) use rcl to parse yaml when circular dependency is solved
Copy link
Member

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.

Copy link
Member

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

Copy link
Member

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.

for (const std::string & yaml_path : yaml_paths) {
rcl_params_t * yaml_params = rcl_yaml_node_struct_init(options->allocator);
if (nullptr == yaml_params) {
throw std::bad_alloc();
}
if (!rcl_parse_yaml_file(yaml_path.c_str(), yaml_params)) {
throw std::runtime_error("Failed to parse parameters " + yaml_path);
}

rclcpp::ParameterMap initial_map = rclcpp::parameter_map_from(yaml_params);
Copy link
Member

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

Copy link
Contributor Author

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

rcl_yaml_node_struct_fini(yaml_params);
auto iter = initial_map.find(combined_name);
if (initial_map.end() == iter) {
continue;
}

// Combine parameter yaml files, overwriting values in older ones
for (auto & param : iter->second) {
parameters[param.get_name()] = param;
}
}

// initial values passed to constructor overwrite yaml file sources
for (auto & param : initial_parameters) {
parameters[param.get_name()] = param;
}

std::vector<rclcpp::Parameter> combined_values;
combined_values.reserve(parameters.size());
for (auto & kv : parameters) {
combined_values.emplace_back(kv.second);
}

// TODO(sloretz) store initial values and use them when a parameter is created ros2/rclcpp#475
// Set initial parameter values
if (!initial_parameters.empty()) {
rcl_interfaces::msg::SetParametersResult result = set_parameters_atomically(initial_parameters);
if (!combined_values.empty()) {
rcl_interfaces::msg::SetParametersResult result = set_parameters_atomically(combined_values);
if (!result.successful) {
throw std::runtime_error("Failed to set initial parameters");
}
Expand Down