Added new constructors for SyncParameterClient

[alsora]

Following issue #599

Allows to create SyncParameterClient in a class which inherits from rclcpp::Node

[wjwwood]

Thanks for starting this pr, I have some requested changes and unfortunately we're past the point in the Crystal release where this could be included, instead it will go into post Crystal (for the D-turtle release) and therefore this pr might have to wait for a few days/weeks while we finish the release.

[wjwwood]

This is dangerous, because you're creating a shared pointer to Node (in node_) from a pointer that is already owned by another shared pointer (node is the pointer and all nodes are created as shared pointers to support std::enable_shared_from_this).

The SyncParametersClient needs to be changed to hold a raw pointer (optionally or always) rather than always having a shared pointer to node which cannot be safely fulfilled with a raw pointer as input.

[alsora]

I changed the SyncParametersClient member node_ to a raw pointer.
SyncParametersClient has still constructors which accept a raw or shared pointer to a node.

[wjwwood]

Instead a signature which took the component parts of the node that this client needs would be better, e.g. maybe the NodeParameterInterface and the NodeBaseInterface (and perhaps others?).

[wjwwood]

You could keep a signature like this one which simply extracted those components from a rclcpp::Node and delegated them to another constructor. That way this could be more easily used with other forms of Node's like rclcpp_lifecycle::LifecycleNode.

[alsora]

From what I have understood, SyncParameterClient only uses NodeBaseInterface for calling spin_until_future_complete.
All the other interfaces are used by AsyncParameterClient and are "extracted" from the node pointer in the AsyncParameterClient constructor.
I do not get what's the advantage of having a SyncParameterClient constructor signature which takes interfaces, since all the logic using them is contained in the AsyncParameterClient

[wjwwood]

I do not get what's the advantage of having a SyncParameterClient constructor signature which takes interfaces, since all the logic using them is contained in the AsyncParameterClient.

Doesn't the SyncParameterClient create an AsyncParameterClient internally? Therefore it needs those parts to pass to the constructor of the AsyncParameterClient (specifically this one:

rclcpp/rclcpp/include/rclcpp/parameter_client.hpp

Lines 49 to 56 in 69d7e69

	RCLCPP_PUBLIC
	AsyncParametersClient(
	const rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_base_interface,
	const rclcpp::node_interfaces::NodeTopicsInterface::SharedPtr node_topics_interface,
	const rclcpp::node_interfaces::NodeGraphInterface::SharedPtr node_graph_interface,
	const rclcpp::node_interfaces::NodeServicesInterface::SharedPtr node_services_interface,
	const std::string & remote_node_name = "",
	const rmw_qos_profile_t & qos_profile = rmw_qos_profile_parameters);

).

As to why the parts rather than an rclcpp::Node pointer only, the reason is that this won't be only used with rclcpp::Node, for instance if you wanted to use it with rclcpp_lifecycle::LifecycleNode (which does not inherit from rclcpp::Node), or some user defined node that doesn't inherit from rclcpp::Node, then you'd want the constructor that takes the interfaces.

If you want to be resilient to changes in the AsyncParameterClient's constructor, you could try to use perfect forwarding to forward the arguments that are only used by the AsyncParameterClient, but that makes it harder to document imo.

You can still have convenience constructors that take common node base types like rclcpp::Node and just extract the interfaces needed and then delegates to the other constructor.

[alsora]

In the last commit I took into account the possibility for objects which does not inherit from rclcpp::Node to create a SyncParametersClient.

I had to change again the node_ member.
I substituted it with const rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_base_interface_ .

Now there are 5 constructors for SyncParameterClient and 3 constructors for AsyncParameterClient.

These are examples of flow for the SyncParameterClient:

• shared_ptr constructor -> executor + shared_ptr constructor -> executor + interfaces constructor
• raw ptr constructor -> executor + raw ptr constructor -> executor + interfaces constructor

The just added "executor + interfaces constructor" instantiates the AsyncParameterClient using the provided interfaces.

I just noticed that there is a failed test (it was present also with the last commit, but I did not saw it since I only tested rclcpp package).
Do you have any idea why changing the ParameterClient can affect the rclcpp_action client ?
The test error is a timeout during async_cancel_some_goals

[wjwwood]

In the last commit I took into account the possibility for objects which does not inherit from rclcpp::Node to create a SyncParametersClient.

Thanks! I'll give it a look.

Do you have any idea why changing the ParameterClient can affect the rclcpp_action client ?
The test error is a timeout during async_cancel_some_goals

It may or may not be related, we do occasionally have flaky tests, @jacobperron and @sloretz do either of you know if that is a known flaky test?

[jacobperron]

It may or may not be related, we do occasionally have flaky tests, @jacobperron and @sloretz do either of you know if that is a known flaky test?

I'm not aware of any flaky tests in rclcpp_action.
With either master branch or this PR, the following command passes for me locally:

colcon test --executor sequential --retest-until-fail 10 --packages-select rclcpp_action

[alsora]

@jacobperron yes, you are right, now the checks are marked as passed.

I repeated all the rclcpp tests more than one time and apparently sometimes errors in the rclcpp_action package arise.

[wjwwood]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[wjwwood]

Restarted Windows CI due to failure:

[wjwwood]

The failing tests are due to another unrelated issue, so I'll merge this. Thanks @alsora!