Add ParameterEventsSubscriber class

[bpwilcox]

This PR adds a new class ParameterEventsSubscriber which provides function interfaces to handle parameter events.

The aim of this class is to address shortcomings and one of the unimplemented features (see https://design.ros2.org/articles/ros_parameters.html#unimplemented) for parameter events. Namely, this class:

• can register multiple custom callbacks by parameter name and node (either your own or remote)
• register a parameter to automatically update a reference variable
• add a custom event callback by namespace (same behavior as on_parameter_event() in AsyncParametersClient)
• can track different namespaces for parameter events (I believe the subscription created in the parameter client doesn't necessarily subscribe to parameter events on the namespace of the remote node passed in but of its own node)

In my opinion, this class would separate the parameter event subscriptions created in the parameter client (I'd remove the on_parameter_event in the future)

[ivanpauno]

In general, the idea looks great to me.
I left some comments and questions.

[bpwilcox]

@ivanpauno Added a commit to address most of your feedback, including a test case for having the same parameter name on two separate nodes.

[ivanpauno]

@bpwilcox thanks for iterating on this, LGTM!

[ivanpauno]

CI up to rclcpp test_rclcpp, only fastrtps:

• Linux
• Linux-aarch64
• macOS
• Windows

[ivanpauno]

Windows build failed because of missing visibility macros.
Please take a look to my other comments.

[ivanpauno]

Also, check this warnings from the windows build:

10:17:38 C:\J\workspace\ci_windows\ws\src\ros2\rclcpp\rclcpp\test\test_parameter_events_subscriber.cpp(214): warning C4244: '=': conversion from 'int64_t' to 'int', possible loss of data [C:\J\workspace\ci_windows\ws\build\rclcpp\test_parameter_events_subscriber.vcxproj]
10:17:38 C:\J\workspace\ci_windows\ws\src\ros2\rclcpp\rclcpp\include\rclcpp/parameter_events_subscriber.hpp(116): warning C4244: '=': conversion from 'int64_t' to 'ParameterT', possible loss of data [C:\J\workspace\ci_windows\ws\build\rclcpp\test_parameter_events_subscriber.vcxproj]
10:17:38           with
10:17:38           [
10:17:38               ParameterT=int
10:17:38           ]
10:17:38   C:\J\workspace\ci_windows\ws\src\ros2\rclcpp\rclcpp\include\rclcpp/parameter_events_subscriber.hpp(92): note: see reference to function template instantiation 'void rclcpp::ParameterEventsSubscriber::get_parameter_update<ParameterT>(const rclcpp::Parameter &,ParameterT &)' being compiled
10:17:38           with
10:17:38           [
10:17:38               ParameterT=int
10:17:38           ]
10:17:38   C:\J\workspace\ci_windows\ws\src\ros2\rclcpp\rclcpp\test\test_parameter_events_subscriber.cpp(150): note: see reference to function template instantiation 'void rclcpp::ParameterEventsSubscriber::register_parameter_update<int>(const std::string &,ParameterT &,const std::string &)' being compiled
10:17:38           with
10:17:38           [
10:17:38               ParameterT=int
10:17:38           ]

[wjwwood]

Thanks for working on this feature! I have a few issues that I think we should try to resolve first, however.

First, I think the feature of updating the variable automatically is inherently unsafe. I could perhaps be convinced otherwise, but not without a new interface in place to protect the user.

Second, I think there needs to be more documentation about how to use this class. There are some not-so-obvious details about it for users, like the fact that you need to spinning on the Node for it to work.

---

Some what out-of-scope, but I think it applies to this classes future at least, is that when using this on a local node (if you have a reference rather than just a name), then you should be able to get updates without using the topic and without spinning. This is really needed for things like the TimeSource, see this issue:

#609

---

Also perhaps out-of-scope, but it would be a preferable end state, I think that this should support multiple callbacks not just one per node/parameter. For instance more than one party might care when use_sim_time changes. Right now they'd step on each other if they both tried to use the same instance of this class. So you could use two instances of this class, but then you'd need additional subscriptions which is wasteful. Therefore it would be better to support more than one callback at a time (at any granularity).

Also, it is not currently possible with this class to get a set of parameters, which is a common use case. For example, "if any of P, I, or D change, give me all of them" kind of thing. A stretch goal for sure, but it would require either changing this classes interface or adding nearly identical functionality along side of it in the future. Seems like it would be better to consider this case from the beginning.

[wjwwood]

For topics and logging they don't exist yet, but we should add them, it's just copy-paste update the names.

[bpwilcox]

I'm not sure I understand, can you clarify what you mean?

[ivanpauno]

That something similar to

rclcpp/rclcpp/include/rclcpp/node_interfaces/get_node_base_interface.hpp

Lines 120 to 144 in 64c0f86

	/// Get the NodeBaseInterface as a pointer from a pointer to a "Node like" object.
	template<
	typename NodeType,
	typename std::enable_if<std::is_pointer<NodeType>::value, int>::type = 0
	>
	rclcpp::node_interfaces::NodeBaseInterface *
	get_node_base_interface(NodeType node_pointer)
	{
	// Forward pointers to detail implmentation directly.
	return detail::get_node_base_interface_from_pointer(node_pointer);
	}
	/// Get the NodeBaseInterface as a pointer from a "Node like" object.
	template<
	typename NodeType,
	typename std::enable_if<
	!std::is_pointer<typename std::remove_reference<NodeType>::type>::value, int
	>::type = 0
	>
	rclcpp::node_interfaces::NodeBaseInterface *
	get_node_base_interface(NodeType && node_reference)
	{
	// Forward references to detail implmentation as a pointer.
	return detail::get_node_base_interface_from_pointer(&node_reference);
	}

has to be implemented for the node logging interface.

[wjwwood]

This still applies, as @ivanpauno said, you need the equivalent of these files:

• https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/node_interfaces/get_node_base_interface.hpp
• https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/node_interfaces/get_node_topics_interface.hpp

for the logging interface. It's essentially copy-paste-replace "base" or "topics" with "logging". And then use those here.

For example it is used here:

rclcpp/rclcpp/include/rclcpp/create_publisher.hpp

Lines 52 to 54 in 81049c4

	// Extract the NodeTopicsInterface from the NodeT.
	using rclcpp::node_interfaces::get_node_topics_interface;
	auto node_topics = get_node_topics_interface(node);

[bpwilcox]

Added a get_node_logging_interface.hpp

[wjwwood]

Also here, thread-safety?

[wjwwood]

What if this is set to nullptr with set_event_callback() after this line, but before the next?

[ivanpauno]

First, I think the feature of updating the variable automatically is inherently unsafe. I could perhaps be convinced otherwise, but not without a new interface in place to protect the user.

I agree with @wjwwood that is unsafe.
I originally thought that it could be useful in some single threaded scenarios, but after reading the last comments I agree that it doesn't add extra value compared with using get_parameter directly.

---

I also agree with the other thread-safety concerns that I originally missed.

[bpwilcox]

First, I think the feature of updating the variable automatically is inherently unsafe. I could perhaps be convinced otherwise, but not without a new interface in place to protect the user.

I'd like to look into making this feature thread-safe as well as address thread-safety through the class

Second, I think there needs to be more documentation about how to use this class. There are some not-so-obvious details about it for users, like the fact that you need to spinning on the Node for it to work.

Yes, I'll be working on documentation.

Some what out-of-scope, but I think it applies to this classes future at least, is that when using this on a local node (if you have a reference rather than just a name), then you should be able to get updates without using the topic and without spinning. This is really needed for things like the TimeSource, see this issue:

#609

I agree this would be preferable behavior, maybe a subsequent PR

Also perhaps out-of-scope, but it would be a preferable end state, I think that this should support multiple callbacks not just one per node/parameter. For instance more than one party might care when use_sim_time changes. Right now they'd step on each other if they both tried to use the same instance of this class. So you could use two instances of this class, but then you'd need additional subscriptions which is wasteful. Therefore it would be better to support more than one callback at a time (at any granularity).

Hmm, so perhaps saving a vector of callbacks per node/parameter, similar to what's being done in https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/node.hpp#L962?

Also, it is not currently possible with this class to get a set of parameters, which is a common use case. For example, "if any of P, I, or D change, give me all of them" kind of thing. A stretch goal for sure, but it would require either changing this classes interface or adding nearly identical functionality along side of it in the future. Seems like it would be better to consider this case from the beginning.

This is interesting. I suppose you could add a custom callback on each parameter and provide your own inspection into the other parameters, but it sounds like there could be a set_parameter_group_callback type function that accepts multiple parameter names and has a callback associated with that grouping if any change? You might have to create a client to remote nodes unless you're caching their values somewhere, though.

[wjwwood]

I agree this would be preferable behavior, maybe a subsequent PR

👍

Hmm, so perhaps saving a vector of callbacks per node/parameter, similar to what's being done in https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/node.hpp#L962?

I think that's the best way to go about it, especially since unlike that set of callbacks these callbacks can not influence one another. What I mean is that the other ones you linked to can each "reject" changes by returning something, whereas these callbacks are informative only, and so you don't need to decide whether or not these callbacks will affect each other.

So, it's more work, but not quite as much work as the other callbacks already using this pattern, and I think generally more useful.

This is interesting. I suppose you could add a custom callback on each parameter and provide your own inspection into the other parameters, but it sounds like there could be a set_parameter_group_callback type function that accepts multiple parameter names and has a callback associated with that grouping if any change? You might have to create a client to remote nodes unless you're caching their values somewhere, though.

I was thinking more like a callback that would only get called if all the parameters changed or something, but maybe the use case for this is less concrete than I imagined. You're right that the user can request the group if they want separately. Maybe we just table this feature.

[bpwilcox]

@wjwwood I've addressed much of your feedback in my latest commits. Namely:

• adding mutexes for thread safety
• removed register_parameter_update feature
  • given our conversation and further investigation, I decided it is best for now to just remove
• made get_parameter_from_event a static method
• changed naming to from register_parameter_callback to add_parameter_callback
• added feature to add multiple callbacks per {parameter, node}
• added functions to remove parameter callbacks (either by callback handle or entire parameter) and removal of event callback
• internally, subscriptions will be erased if there are no more callbacks associated with the <namespace>/parameter_events topic
• updated tests to cover added features

Can you review the changes and let me know if there are any remaining blocking issues for this PR?

[mkhansenbot]

@wjwwood - can you review again please?

[wjwwood]

I might be able to get to it this week. It's already on my list, but there are other big pull requests that are older (e.g. #778) which I am working on now.

[mkhansenbot]

Thanks for the update!

[ivanpauno]

I left some comments.

@wjwwood Why do we have a parameter event topic per namespace? Was that intentional?
I think it's ok, but also having only one is reasonable.

[ivanpauno]

That something similar to

rclcpp/rclcpp/include/rclcpp/node_interfaces/get_node_base_interface.hpp

Lines 120 to 144 in 64c0f86

	/// Get the NodeBaseInterface as a pointer from a pointer to a "Node like" object.
	template<
	typename NodeType,
	typename std::enable_if<std::is_pointer<NodeType>::value, int>::type = 0
	>
	rclcpp::node_interfaces::NodeBaseInterface *
	get_node_base_interface(NodeType node_pointer)
	{
	// Forward pointers to detail implmentation directly.
	return detail::get_node_base_interface_from_pointer(node_pointer);
	}
	/// Get the NodeBaseInterface as a pointer from a "Node like" object.
	template<
	typename NodeType,
	typename std::enable_if<
	!std::is_pointer<typename std::remove_reference<NodeType>::type>::value, int
	>::type = 0
	>
	rclcpp::node_interfaces::NodeBaseInterface *
	get_node_base_interface(NodeType && node_reference)
	{
	// Forward references to detail implmentation as a pointer.
	return detail::get_node_base_interface_from_pointer(&node_reference);
	}

has to be implemented for the node logging interface.

[ivanpauno]

Use const rcl_interfaces::msg::ParameterEvent & event

[bpwilcox]

The ParameterEventsFilter used in this function requires a shared ptr to the event msg,

rclcpp/rclcpp/include/rclcpp/parameter_events_filter.hpp

Line 63 in 59e7bbb

rcl_interfaces::msg::ParameterEvent::SharedPtr event,

[ivanpauno]

I really don't like the ParameterEventsFilter class design.
The problem could be solved with a free function, without the need of taking a shared_ptr to an event.
But that discussion is probably out of the scope of this PR.

[wjwwood]

I think this is a valid critique of the existing filter class and would be worth changing now so that we don't have to compromise this new API in order to use the existing filter class API...

[bpwilcox]

I've removed the ParameterEventsFilter dependency from the get_parameter_from_event function and modified code from the filter to work for this use case.

[ivanpauno]

Maybe, you can use this https://github.com/ros2/rcpputils/blob/master/include/rcpputils/join.hpp, https://github.com/ros2/rcpputils/blob/master/include/rcpputils/split.hpp.

[bpwilcox]

I will see if I can use these.

[bpwilcox]

Though, if we remove the namespace from the topic name as your PR proposes, then I no longer need to worry about splitting/joining strings.

[ivanpauno]

Probably, using this

rclcpp/rclcpp/include/rclcpp/logger.hpp

Line 134 in 3e08d9e

get_child(const std::string & suffix)

[ivanpauno]

I understand that the methods that take a namespace, defaults it to the current node namespace.
But when you take a vector of namespaces, I don't think that applying that rule is good.

[bpwilcox]

Would it be better to create a function overload then that doesn't take any namespaces but only applies callback to itself?

As mentioned in another comment, I am thinking to set the callbacks on the nodes as opposed to the namespace.

[ivanpauno]

nit: Using the move constructor might be better.
Prefer not using auto, except the type is obvious or too complicated to write.

  std::vector<std::string> temp_namesaces(std::move(event_namespaces_));

[ivanpauno]

Shouldn't call this one only when the namespace is in event_namespaces_;?

[bpwilcox]

I agree that makes more sense.

Actually, I had a couple more thoughts I'm considering:

1. Apply the event callbacks per node as opposed to namespace
2. Support multiple event callbacks, either multiple on a same node, or multiple different nodes. This is similar to what I've done for the individual parameter callbacks

[wjwwood]

@wjwwood Why do we have a parameter event topic per namespace? Was that intentional?
I think it's ok, but also having only one is reasonable.

Can you link to the line where this is done (where the topic name is specified)?

I believe this was discussed somewhere else, but I can't find it. Basically, if the topic is currently parameter_events, then I agree it's not right and should be changed. I think either /parameter_events or ~/parameter_events makes sense, but not parameter_events. I don't know what the hangup was on changing that, if there was any. Personally, I'd go for /parameter_events, because I judge that filtering undesired events is more efficient then having many subscriptions (one for each node) when you want to monitor all changes. Plus we could consider using a keyed topic (keyed on node name most likely) in the future to gain some performance back.

[ivanpauno]

Can you link to the line where this is done (where the topic name is specified)?

rclcpp/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp

Line 72 in 59e7bbb

"parameter_events",

I believe this was discussed somewhere else, but I can't find it.

I remember an offline discussion, but no idea is something was written about it.

I'd go for /parameter_events, because I judge that filtering undesired events is more efficient then having many subscriptions

I agree. I'm going to open a PR proposing that.

[wjwwood]

But it's not documented that letting the shared pointer returned by add_parameter_event_callback go out of scope will automatically remove it, unlike the inspiration for these signatures, Node::add_on_set_parameters_callback(). As far as I can tell from the code, it is not the case that letting this shared pointer go out of scope will result in the callback getting removed. So I don't see any reason for it to be returned as a shared_ptr.

So going back to my original thing, I think the add function should return a raw pointer (as there's no need to have shared ownership) or the remove function should take a shared pointer so it is symmetric with the add function.

Since automatic removal is not happening, it sounds like it should never be a shared pointer in the first place.

And since the handle is trivially copied, maybe it should just return a OnSetParametersCallbackHandle from add and take a const OnSetParametersCallbackHandle & in remove?

I still just don't see why we're returning a shared ptr from add but taking a raw pointer here.

[wjwwood]

There are some lingering questions, but this is really close.

[mjeronimo]

@fujitatomoya @wjwwood Any further input?

[wjwwood]

Mostly a bunch of nitpicks about the documentation.

I think this is ready to merge regardless. So take or leave the suggestions and lgtm 👍

[wjwwood]

nitpick: There is a stream operator for parameters:

• rclcpp/rclcpp/include/rclcpp/parameter.hpp

  Lines 224 to 226 in b9ffd72

  	RCLCPP_PUBLIC
  	std::ostream &
  	operator<<(std::ostream & os, const rclcpp::Parameter & pv);

• rclcpp/rclcpp/src/rclcpp/parameter.cpp

  Lines 175 to 180 in b9ffd72

  	std::ostream &
  	rclcpp::operator<<(std::ostream & os, const rclcpp::Parameter & pv)
  	{
  	os << std::to_string(pv);
  	return os;
  	}

• rclcpp/rclcpp/src/rclcpp/parameter.cpp

  Lines 189 to 197 in b9ffd72

  	std::string
  	std::to_string(const rclcpp::Parameter & param)
  	{
  	std::stringstream ss;
  	ss << "{\"name\": \"" << param.get_name() << "\", ";
  	ss << "\"type\": \"" << param.get_type_name() << "\", ";
  	ss << "\"value\": \"" << param.value_to_string() << "\"}";
  	return ss.str();
  	}

Which you could use like this (test it because I'm not 100% sure on the syntax):

Suggested change

	* RCLCPP_INFO(
	* node->get_logger(), "cb1: Received an update to parameter \"%s\" of type %s: \"%ld\"",
	* p.get_name().c_str(),
	* p.get_type_name().c_str(),
	* p.as_int());
	* RCLCPP_INFO_STREAM(node->get_logger(), << "cb1: Received an update to parameter " << p);

So long as the format is what you want. Just a suggestion, feel free to ignore it.

[wjwwood]

Suggested change

	* a regular expression on the node names or namespaces of interest. For example,
	* a regular expression on the node names or namespaces of interest. For example:

[wjwwood]

Did the documentation mention that the handle was scoped?

[nuclearsandwich]

Merging this introduced build failures on Windows . We reverted the change in #1572 and there is a new PR in #1573 to re-apply them.

[wjwwood]

@bpwilcox we're investigating some strange behavior and this class is involved, but during that investigation I noticed this mutex is recursive, but it wasn't immediately obvious to me why that's required. The callback is used in a subscription which is used in a mutually exclusive callback group (the default) and so it will never be called in parallel with itself. Likewise I don't see why any of the other functions (add/remove ones) use the mutex in a way where recursion is needed.

I know this is an ancient issue, but it was just a shot in the dark if you remembered why that was the case.