-
Notifications
You must be signed in to change notification settings - Fork 418
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
Integrate topic statistics #1072
Merged
Merged
Changes from all commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
538fbcb
Add SubscriberTopicStatistics class
dabonnie a61ad41
Add SubscriberTopicStatistics Test
dabonnie fe4fad9
Address review comments
dabonnie fdb6c64
Modify constructor to allow a node to create necessary components
dabonnie 043b9a9
Fix docstring style
dabonnie 019c86d
Remove SetPublisherTimer method
dabonnie 8b8a31c
Change naming style to match rclcpp
dabonnie ade51ca
Address style issues
dabonnie 34a89ce
Use rclcpp:Time
dabonnie c9a6eb7
Address review comments
dabonnie 6f7060a
Remove unnecessary check for null publisher timer
dabonnie 9c19234
Update message dependency
dabonnie 9554784
Initial integration of Subscriber Topic Statistics
dabonnie c4519eb
Fix nanoseconds used for Topic Stats
dabonnie 598d045
Add simple publishing test
dabonnie ebc753e
Add test utils header
dabonnie 7c6bff4
Integrate with Topic Statistics options
dabonnie d88ec30
Update after rebasing
dabonnie 6ae2e93
Address minor review comments
dabonnie 80086d0
Move Topic Statistics instantiation to create_subscription
dabonnie 6b9eaa5
Fix rebase issue
dabonnie 5e69742
Move new timer creation method to relevant header
dabonnie 942d7e0
Add timers interface to topic interface
dabonnie 4c73fcf
Use new create timer method
dabonnie ec9bb73
Address review comments
dabonnie File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
qq: why does the topics statistics publisher use the same qos as the subscription being created?
I haven't checked the topics statistics demo, but it seems that if different entities are publishing statistics using different QoS, a variety of problems can happen (e.g.: pub/sub not matching).
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.
That's a good question. It's possible we could provide publishing QoS options as part of the topic statistics options. What do you think is the best path forward?
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.
I think that adding an extra option for that (like
options.topic_stats_options.publish_topic
) is a good idea.