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
Add node path and timestamp fields to event msg #51
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. We should pair it with updates that fill these fields when published.
Update: Sorry I missed the related PR added "Connects to" at the top of the description to group it on Waffle.io. We still need a rclpy update too.
@tfoote I've just submitted a PR to add the update to ros2/rclpy to fill the ParameterEvent.msg. Can you update us on the status of this review and whether we could get these PRs merged for the Crystal release? A few of our issues in the navigation2 stack are held up on workarounds that this PR would resolve. |
This PR together with rclcpp PR 584 works like a charm for my use-case (rclcpp-based monitoring of the system-wide configuration / node parameters). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. It currently has @bpwilcox I rebased this on the upstream changes that already inserted the dependency, please check the merge is valid.
The merge looks valid to me. |
Per the discussion in #27, this PR adds a timestamp and node path name field to the ParameterEvent.msg. This would allow for parameter events to be resolved by their source node.