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

Add node path and timestamp fields to event msg #51

Merged
merged 2 commits into from Dec 4, 2018

Conversation

bpwilcox
Copy link
Contributor

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.

Copy link
Contributor

@tfoote tfoote left a 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.

@bpwilcox
Copy link
Contributor Author

@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.

@norro
Copy link

norro commented Nov 30, 2018

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).

Copy link
Contributor

@tfoote tfoote left a 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.

@bpwilcox
Copy link
Contributor Author

bpwilcox commented Dec 3, 2018

The merge looks valid to me.

@tfoote
Copy link
Contributor

tfoote commented Dec 4, 2018

CI with all three repos rclcpp, rclpy and rcl_interfaces

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@tfoote tfoote merged commit b0bc045 into ros2:master Dec 4, 2018
@tfoote tfoote removed the in review Waiting for review (Kanban column) label Dec 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants