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

Adding ability to add parameters through yaml at runtime #2406

Open
wants to merge 5 commits into
base: rolling
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 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
26 changes: 26 additions & 0 deletions rclcpp/include/rclcpp/copy_all_parameter_values.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,18 @@
#ifndef RCLCPP__COPY_ALL_PARAMETER_VALUES_HPP_
#define RCLCPP__COPY_ALL_PARAMETER_VALUES_HPP_

#include <rcl_yaml_param_parser/parser.h>

CursedRock17 marked this conversation as resolved.
Show resolved Hide resolved
#include <string>
#include <vector>

#include "rcl_interfaces/srv/list_parameters.hpp"
#include "rcl_interfaces/msg/parameter_descriptor.hpp"
#include "rcl_interfaces/msg/set_parameters_result.hpp"

#include "rclcpp/node_interfaces/node_interfaces.hpp"
#include "rclcpp/parameter.hpp"
#include "rclcpp/parameter_map.hpp"
#include "rclcpp/logger.hpp"
#include "rclcpp/logging.hpp"

Expand Down Expand Up @@ -77,6 +81,28 @@ copy_all_parameter_values(
}
}

/// Load a list of parameters from a yaml parameter file.
/**
* \param[in] yaml_name The name of the yaml file that needs to be loaded.
* \param[in] node_interfaces The list of variadic NodeInterfaces taken as a template to set parameters for
CursedRock17 marked this conversation as resolved.
Show resolved Hide resolved
*/
CursedRock17 marked this conversation as resolved.
Show resolved Hide resolved
template<typename NodeT>
std::vector<rcl_interfaces::msg::SetParametersResult>
load_parameters(
Copy link
Collaborator

Choose a reason for hiding this comment

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

this file name should be changed into something else like parameter_interfaces.hpp? copy_all_parameter_values.hpp sounds dedicated to rclcpp::copy_all_parameter_values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I agree, I just didn't want to change it too fast. parameter_interfaces.hpp sounds good to me.

const std::string & yaml_filepath, NodeT node_interface)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer if we didn't use a template here, but rather the NodeInterfaces class or directly passing the two required interface (node_base_interface and node_parameters_interface)

Copy link
Collaborator

Choose a reason for hiding this comment

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

that would be better for compile time.

{
rclcpp::ParameterMap parameter_map =
rclcpp::parameter_map_from_yaml_file(yaml_filepath, node_interface->get_name());
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this be get_fully_qualified_name ?


auto iter = parameter_map.find(node_interface->get_name());
if (iter == parameter_map.end() || iter->second.size() == 0) {
throw rclcpp::exceptions::InvalidParametersException("No valid parameter");
}
auto params_result = node_interface->set_parameters(iter->second);

return params_result;
}

} // namespace rclcpp

#endif // RCLCPP__COPY_ALL_PARAMETER_VALUES_HPP_
16 changes: 16 additions & 0 deletions rclcpp/test/rclcpp/test_copy_all_parameter_values.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,3 +86,19 @@ TEST_F(TestNode, TestParamCopyingExceptions)
node2->declare_parameter("Foo1", rclcpp::ParameterValue(0.123));
EXPECT_NO_THROW(rclcpp::copy_all_parameter_values(node1, node2, override));
}

TEST_F(TestNode, TestFileImport)
{
const uint64_t expected_param_count = 4;
auto load_node = std::make_shared<rclcpp::Node>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can add LifecycleNode test case here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would that not make a circular testing structure, as LifecycleNode in lifecycle_rclcpp needs to include rclcpp.

"load_node",
"namespace",
rclcpp::NodeOptions().allow_undeclared_parameters(true));

// load parameters
rcpputils::fs::path test_resources_path{TEST_RESOURCES_DIRECTORY};
const std::string parameters_filepath = (
test_resources_path / "test_node" / "load_parameters.yaml").string();
auto load_vector = rclcpp::load_parameters(parameters_filepath, load_node);
ASSERT_EQ(load_vector.size(), expected_param_count);
}