I took advantage of modifying this to homogenize the cmake to match our other interface packages.
Add new message and dependency: c1e4dbe ff8c8f9
Use the new message in existing ParameterEvent messages d5beca1
Cmake homogenization: b308ffc b18ec75
Ready for feedback / review (Although I wont merge this before merging a PR using it, I'd like feedback on the message definition)

Hmm, so looking at this. I think the ParameterEventDescriptor is now deprecated since we implemented the Parameter which includes "type" and has "PARAMETER_NOT_SET" support for the events they just use the Parameter in the deletion too. And the descriptors are not published in an event stream I think it might make sense to clear out that message.

The other places that it might make sense to include this header is the ListParameterResult.msg Though that may be considered redundant as that's usually just inside the srv response.

Hmm, so looking at this. I think the ParameterEventDescriptor is now deprecated since we implemented the Parameter which includes "type" and has "PARAMETER_NOT_SET" support for the events they just use the Parameter in the deletion too. And the descriptors are not published in an event stream I think it might make sense to clear out that message.

oh good point, I didnt have all the context here. My understanding based on the description from the readme:

parameter_event_descriptors: ParameterEventDescriptors
This topic provides a way to subscribe to all parameter updates occuring on the node, including addition removal and changes in value. Every atomic change will be published separately. This is provided if large parameter values are expected to slow down the system.

was that we'd like to keep this around to allow users to publish only descriptors event rather than the full parameter event when they happen to use very long parameters (e.g. a robot description or a massive binary blob)

The other places that it might make sense to include this header is the ListParameterResult.msg Though that may be considered redundant as that's usually just inside the srv response.

I'm not sure what the last part of this sentence means. Do you mean we could add a header in the service Response message rather than the message itself? or that this information is already provided with the current service definition?

Hmm, so looking at this. I think the ParameterEventDescriptor is now deprecated since we implemented the Parameter which includes "type" and has "PARAMETER_NOT_SET" support for the events they just use the Parameter in the deletion too. And the descriptors are not published in an event stream I think it might make sense to clear out that message.

oh good point, I didnt have all the context here. My understanding based on the description from the readme:

parameter_event_descriptors: ParameterEventDescriptors
This topic provides a way to subscribe to all parameter updates occuring on the node, including addition removal and changes in value. Every atomic change will be published separately. This is provided if large parameter values are expected to slow down the system.

was that we'd like to keep this around to allow users to publish only descriptors event rather than the full parameter event when they happen to use very long parameters (e.g. a robot description or a massive binary blob)

I'd forgotten about that design use case. It looks like that never got implemented. This could be quite valuable for light weight introspection too where a GUI could subscribe to the stream of event descriptors and then only query for a filtered set. Then we should leave this as is and probably ticket or TODO adding that topic publishing

The other places that it might make sense to include this header is the ListParameterResult.msg Though that may be considered redundant as that's usually just inside the srv response.

I'm not sure what the last part of this sentence means. Do you mean we could add a header in the service Response message rather than the message itself? or that this information is already provided with the current service definition?

The only place that this is used is in the service. https://github.com/ros2/rcl_interfaces/blob/master/rcl_interfaces/srv/ListParameters.srv#L11 which is already inherently one to one and known. However if that return value is ever stored, having the node name and timestamp would be valuable to be included in the content. Although it's only expected to be used in the service, we shouldn't cut corners assuming it will never be reused elsewhere.

Then we should leave this as is and probably ticket or TODO adding that topic publishing

I was considering adding this publication to the prototype c implementation. If I dont get around doing it this cycle I'll ticket it for later.

However if that return value is ever stored, having the node name and timestamp would be valuable to be included in the content. Although it's only expected to be used in the service, we shouldn't cut corners assuming it will never be reused elsewhere.

Yeah I see your point. I think I'll add it only to these 2 for now as we have a concrete need for them. I'll open an issue to consider providing more information in more messages (ListParameters message included) as it could be handy to also add it to the requests messages to provide a "sender name" that could be then used to populate the Event messages with information about which node triggered the parameter change

issue created. I believe this is ready for review.
In particular, I'm not very happy about the "sender_node_name" field name, and would appreciate feedback in that matter.

In particular, I'm not very happy about the "sender_node_name" field name, and would appreciate feedback in that matter.

For inspiration: rosgraph_msgs just uses "node" in Log.msg (https://github.com/ros/ros_comm_msgs/blob/b3562776bd62ebbedf022ed15222e4d8f8b70639/rosgraph_msgs/msg/Log.msg#L14) or "node_pub" for topic statistics (https://github.com/ros/ros_comm_msgs/blob/b3562776bd62ebbedf022ed15222e4d8f8b70639/rosgraph_msgs/msg/TopicStatistics.msg#L5) (because there's node_sub too)

thanks @dhood and @tfoote for the feedback on the field name.
I like node if we decide we don't want to include information about "another node" than the one publishing in this header.
As suggested in #27 (comment), we could add a custom field for parameter events if that's the only place where including information about the requesting node makes sense

I am not sure that embedding the node name into these messages is a good path forward. Arguable this information could be useful for any kind of message when an introspection tool want't to know e.g. where it comes from. I would prefer if this kind of information is passed "out-of-band". E.g. a subscriber callback could have a MessageEvent<Msg> like argument (as in ROS ) instead of the plain message type to access additional information.

The node name is an integral part of the Parameter Event information. If I give you a parameter event it cannot be interpreted without knowing the originating node. And although we have mechanisms to to capture the origin of a message out of band, that is not robust to aggregation, forwarding, throttling, muxing or any other storage/playback mechanism where the event is no longer directly published by the node.

The node name is an integral part of the Parameter Event information.

I could be convinced to explicitly embed the node name which stores the parameter in the event message (though it doesn't address the case where the node name isn't unique).

But how about the request_origin as well as the time stamp? I don't see a strong rational why those should be added. Also it is unclear what the origin would contain in cases where the event is not triggered by a service call from another node.

The time stamp seems to be valuable information for monitoring, aggregation and replay as well as debugging. There is no guarantee that the messages are arriving in order and code processing these events should have a way to know what the "last parameter change" was rather than assuming the last received is the latest one.
The origin will also be valuable for auditing your system and post-processing logs. One would like to know why and especially who triggered an unexpected parameter change when debugging a system.

I simply don't believe that embedding those information into every message like this is the right way to support these debugging cases. Arguably the same argument applies to each an every ROS message being exchanged and I doubt that we want to add this to every message definition.

Arguably the same argument applies to each an every ROS message being exchanged and I doubt that we want to add this to every message definition.

I disagree, in the current ROS 2 design and demos there is no "topic on which every node should publish updates" except this one. That makes it in my opinion the only topic that needs to provide this kind of "request_origin" information.
As this is not targeted to get in for this release we can postpone the decision after more in-depth discussion on that matter. I'm also welcoming feedback on the various interface changes I'm considering regarding refactoring the parameters: ros2/ros2#432, including not making this topic a single topic for all parameter updates but a topic located on the node (that may impact the information we want to embed in these messages)

Sorry, I'm a little late to the party, but I have some comments.

Re: the new message

I'm not a huge fan of the name RclHeader, because it doesn't really describe the content to me and because I don't know how Rcl would differentiate it in this way from the existing Header message in ROS. I'm not sure what a better name would be, maybe SourceHeader or OriginHeader or MessageOrigin, I don't know.

I'm also not convinced we need a new message for this. We could just put a field for the stamp and the origin node in each of those two messages. I don't completely follow @tfoote's logic about adding this information to the other services, perhaps it would be useful, but it's not clear to me that it is. So combining these two fields into a separate message right now seems like we're getting ahead of ourselves. But I could be wrong about that.

Re: originating node name in the event messages

For messages (sent over topics), I think the "out of band" communication of the sending node's name is useful (mentioned by @dirk-thomas ), but can be lost in relaying and during playback from a bag file, unless we find a way to make a rosbag playback look like the original node is sending it, but I'm not convinced that's a good thing to do. It might be useful to know that the message is coming from rosbag and not something else or that the source of data is from two places and not one (in the case rosbag recorded from two publishers but only uses one on playback). Imagine the practical case that you've recorded the /parameter_events topic for several nodes and you're playing it back to debug a GUI tool for visualizing parameter changes. If you rely on the "caller id", then it will look like all parameter events happened for the rosbag node.

Anyways, there's enough questions around that for me to believe that putting the source node's name in the event message makes sense. Also as @mikaelarguedas mentioned, it's possible but more inconvenient in this case since everyone is publishing to the same topic. Add to that, it is not currently possible to know which node sent a message (you can know which publisher by guid, but not which node), and it's clear to me (at least for the near term) that adding the node name to the message is a decent idea.

Re: timestamp for event in messages

It should be possible to get the "sent time" as well as the "received time" from the middleware for any given message instance (at least DDS already tracks that information in the sample info I think), but we don't have that in our API right now. Also, it has the same issues with relayed topics and rosbag playback. So if we need the time stamp, it would mean we need to communicate it in the message.

So then, do we need the time stamp? I think we do, for the reason that @mikaelarguedas suggested (event messages come out of order and we could use the timestamp to detect that in a tool). Now, we could also detect that with a sequence id, which we removed from the Header but again we could require the middleware to give us this information (DDS has this info I think, but we don't currently expose it). But only if that sequence number is unique or is paired with a publisher guid (and it's unique for that publisher). Again, there's enough not implemented or supposed that adding the timestamp in the message is easier and has extra debugging and human readability benefits.

For both this and the originating node name fields, I think it makes since for this particular message, but does not automatically extend to being equally needed/useful for all messages as @dirk-thomas suggested in #27 (comment)

Re: request_origin

Unlike the other two fields in question, this one cannot be provided by the middleware. So if we need it, then we have to include it in the message. However, for this one I'm not 100% convinced it is required. It might be just a good to put this information in the console logs, given that I don't think it will be used that often.

I've been working with the navigation2 stack, particularly with dynamic changes to parameters in the past few weeks. In the nav2_dynamic_params package, I've created a class to handle dynamic parameter updates, in a fashion suitable to replace previous calls to DynamicReconfigure. One of the main concerns recently has been resolving the source node of the parameter event. I've highlighted some of these concerns in an issue on our repo #286 and with limited fixes in PR #298.

It seems to me that this PR, adding a header with node source name, would resolve many of the issues, even duplicate parameter names under different nodes, enabling more robust handling of parameters in our dynamic parameter client model. What is the current status of this PR and discussion? It seems to address some of the refactor notes in #27. Our team would like to help this issue move forward soon and be merged, if possible.

I think everything I said in my last comment still applies: #27 (comment)

• I don't care for the name
  • I would probably go for MessageOrigin myself, but I'm more against RclHeader than for one of the alternatives because I think it's ambiguous
• I don't think it's general enough to be reused in lots of messages
  • would rather see this information just placed into things like the parameter event message directly (without a new intermediate header type)
  • I could be convinced otherwise, however, especially if we came up with a more descriptive name
• I think we should have the (full) name of the node that generated the event in the message (directly or indirectly)
  • because I think the alternatives are not ready for prime time (e.g. sending outside of the message)
  • and because it's more human readable and friendlier to rosbag playback
• I think we should have the timestamp in the message too and use it to detect out of order events (if they occur)
  • sequence number is not ready for prime time, needs to discriminate between publishers
• I don't think we need the "request origin" because I don't think it will be used that often
  • but I could be convinced otherwise on that one

So my proposal (based on my opinion) is that we should decline this pr and add the "origin node name" and "event timestamp" to the parameter event message directly.

My secondary proposal would be to select a different name for the message, updating this pr, and then use that in the parameter event message.

But this is again, just my opinion. This is not a dictatorship, but unfortunately it's also not a voting system, so we just have to seek consensus. So maybe the first step is to poke people to keep the discussion going until a proposal is accepted.

I agree with your above points @wjwwood, my original suggestion was to include those fields in the event message directly. I could submit a PR to add the new fields directly and then subsequently another PR in rclcpp to fill in the field info.

Feel free to do so as a way to motivate participation in the conversation (or to have a patch testers can try locally), but personally I'd like to get some feedback from others on this thread before committing too much effort. Specifically it would be good to hear from @tfoote and @dirk-thomas.

I agree with your above points @wjwwood, my original suggestion was to include those fields in the event message directly. I could submit a PR to add the new fields directly and then subsequently another PR in rclcpp to fill in the field info.

This sounds like a good approach. The parameter events are node specific so embedding that is required to fully describe the event. (If there are two laser nodes it's important to know upon which one the parameters were changed. This is different than who is telling you the parameter changed.). And I don't think that this header has more general use cases as generally with the anonymous publish/subscribe data centric model the publisher is not considered part of the model. Trying for that level of generalization I think is adding complexity without a benefit of a simpler system. I'd also suggest leaving out the request_origin since that's also breaking the abstraction.

Should this be closed with #51?

Given the age of this PR and the fact that we got at least part of it in in #51, I'm going to go ahead and close it. Feel free to reopen if you are going to pursue it further in the future.