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

Parameters in LifecycleNodes #855

Closed
buschbapti opened this issue Sep 17, 2019 · 3 comments · Fixed by #1249
Closed

Parameters in LifecycleNodes #855

buschbapti opened this issue Sep 17, 2019 · 3 comments · Fixed by #1249
Assignees
Labels
enhancement New feature or request

Comments

@buschbapti
Copy link

buschbapti commented Sep 17, 2019

Hi all,

I am trying to add parameters to a node following the demo at https://github.com/ros2/demos/blob/master/demo_nodes_cpp/src/parameters/set_and_get_parameters.cpp

Problem is, the node is based on the Lyfecycle interface and I suspect rclcpp::SyncParametersClient does not accept them yet. I have seen that this problem appears in multiple other places as the NodeInterface is not yet fully integrated (see
ros-perception/image_common#108 for example)

For reference, the code I use is:

this->parameters_client_ = std::make_shared<rclcpp::SyncParametersClient>(this);

where this extends from rclcpp_lifecycle::LifecycleNode. I have also tried with std::make_shared<rclcpp::SyncParametersClient>(this->shared_from_this()); in vain. The error is listed as follow:

/opt/ros/eloquent/include/rclcpp/parameter_client.hpp:201:12: note:   no known conversion for argument 1 from ‘std::shared_ptr<rclcpp_lifecycle::LifecycleNode>’ to ‘rclcpp::Node::SharedPtr {aka std::shared_ptr<rclcpp::Node>}’

which makes me suggest it is the same problem as ros-perception/image_common#108

Thanks in advance for your support.

EDIT

After looking closely at the constructors for rclcpp::SyncParametersClient it seems that the rclcpp::node_interfaces::NodeBaseInterface::SharedPtr is partially integrated, i.e. only when passing a rclcpp::executor::Executor::SharedPtr along with it. This means we should be able to initialize the parameters_client outside of the node class.

Could it be possible to extend the Constructor

rclcpp::SyncParametersClient::SyncParametersClient(rclcpp::Node::SharedPtr, const string&, const rmw_qos_profile_t&)

to accept a rclcpp::node_interfaces::NodeBaseInterface::SharedPtr and not only a rclcpp::Node::SharedPtr?

@mabelzhang mabelzhang added the enhancement New feature or request label Sep 26, 2019
@buschbapti
Copy link
Author

Hello,

Any news on this issue? I am still unable to declare the parameter server in Lifecycle nodes and that is becoming problematic as this is an important feature. One workaround would be to declare a service for each parameters but it is bit tedious.

Thanks in advance.

@fujitatomoya
Copy link
Collaborator

i guess that is related to #898, the problem is that we have separated classes rclcpp::Node and rclcpp_lifecycle::LifercycleNode. but i believe that rclcpp_lifecycle::LifercycleNode is built on top of rclcpp::Node. so rclcpp_lifecycle::LifecycleNode Class inherits based on rclcpp::Node and featured with lifecycle interfaces.

@sparkskraps
Copy link

sparkskraps commented Feb 28, 2020

I noticed this problem and began to deal with it these days.

As @buschbapti metioned, we can extend the constructor SyncParametersClient(...) to accept not only rclcpp::Node. Just like the way this file handled. It is easy to implement.

If you want the rclcpp_lifecycle::LifecycleNode as parameter of constructor SyncParametersClient(...), just to be consistent with the original constructor that rclcpp::Node takes as an argument. Need to do more work.
Cause the file parameter_client.hpp/cpp is located in the rclcpp package, which was compiled before pacakge rclcpp_lifecycle.

Here are some thoughts.

  1. Copy the relevant files to the package rclcpp_lifecycle and modify the interface to support rclcpp_lifecycle :: LifecycleNode as parameter. The disadvantage is obvious, this will generate redundant code with the same logic.
  2. We can put the common files that Node and LifecycleNode depend on in a new package. Both Node and LifecycleNode are built on this new packages. The build sequence of package will be new package-> rclcpp_lifecycle-> rclcpp-> others. Instead of rclcpp -> others.

Any thoughts on this ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants