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

Adjust topic hz and bw command description. #987

Merged
merged 3 commits into from
Mar 14, 2025

Conversation

fujitatomoya
Copy link
Collaborator

@fujitatomoya fujitatomoya commented Mar 13, 2025

part of #843 (comment)

replaces #970

Copy link
Collaborator Author

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding the note for ros2 topic hz/bw command that these statistics are what we can get on this subscription, not expected publisher rate or bandwidth.

@fujitatomoya
Copy link
Collaborator Author

@mjcarroll @ahcorde @christophebedard @MichaelOrlov what do you think? at least, this could give some hint for users?

@VictorLamoine
Copy link

This is great thank you 👍🏼
Users will try to investigate their QoS configuration first instead of checking their publisher code.

@jmachowinski
Copy link

I would also add a warning before the start of the rate output e.g.:

$ ros2 topic hz /clock
Warning: This rate reflects the receiving rate on subscription, which might be affected by platform resources and QoS configuration, and may not exactly match the publisher rate. This is especially true for big or high frequency messages.
average rate: 100.006
        min: 0.010s max: 0.010s std dev: 0.00008s window: 102
average rate: 100.004
        min: 0.010s max: 0.010s std dev: 0.00008s window: 203

@fujitatomoya
Copy link
Collaborator Author

I would also add a warning before the start of the rate output e.g.:

i like this one, i am inclined to do this. but one minor concern is that changing output format/message would break user tools, sometime developers have some tools to grep the output from ros2cli... but i think this is not the case.

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with the message in the help message. Anyhow I will wait for other opinions

@fujitatomoya
Copy link
Collaborator Author

yeah it would be much better if we can address performance related enhancement like deserialization and executor.

btw, it is also important to know that statistics are different from sender and receiver. means, that if we want the precise statistics like this, we would need to provide the API/Interface to get the statistics on publisher and subscription each. (saying no matter we optimize the performance, there would be platform resource issue, network stability problem on receiver side.) i think that this is common for the use cases that rely on statistics such as live streaming and camera framework.
this additional note is meant to let the user know this is what you can see on your subscription, not publisher statistics.

Signed-off-by: Tomoya Fujita <[email protected]>
@fujitatomoya
Copy link
Collaborator Author

Pulls: #987
Gist: https://gist.githubusercontent.com/fujitatomoya/18dd221066730bb7a47229a6a51d5288/raw/9a015b9681ac5414e1753157e00f1800880141c3/ros2.repos
BUILD args: --packages-above-and-dependencies ros2topic
TEST args: --packages-above ros2topic
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15374

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

Copy link

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fujitatomoya I think the statement about

Display bandwidth used by the topic

It is a bit misleading and unclear. Bandwidth usually refers to the number of bytes per second. However, here, we are talking about the messages rate without considering the number of bytes transferred.

Also, I would add a note that this command will subscribe to the topic and receive messages from subscription during the measurement, which can affect overall system performance.

@MichaelOrlov
Copy link

MichaelOrlov commented Mar 13, 2025

Please disregard my concern above Bandwidth statement. I missed that it is actually in the BW (Bandwidth) command and not in the Hz.

@fujitatomoya
Copy link
Collaborator Author

@MichaelOrlov no worries, thanks for the comments.

Signed-off-by: Tomoya Fujita <[email protected]>
@fujitatomoya
Copy link
Collaborator Author

Pulls: #987
Gist: https://gist.githubusercontent.com/fujitatomoya/4bcb5cecf35c77b47388d1f791a553ff/raw/9a015b9681ac5414e1753157e00f1800880141c3/ros2.repos
BUILD args: --packages-above-and-dependencies ros2topic
TEST args: --packages-above ros2topic
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15382

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@fujitatomoya fujitatomoya merged commit 9a0c044 into rolling Mar 14, 2025
2 checks passed
@fujitatomoya
Copy link
Collaborator Author

@Mergifyio backport humble jazzy

Copy link

mergify bot commented Mar 14, 2025

backport humble jazzy

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Mar 14, 2025
* Adjust topic hz and bw command description.

Signed-off-by: Tomoya Fujita <[email protected]>

* note description fix.

Signed-off-by: Tomoya Fujita <[email protected]>

* address flake8 warnings.

Signed-off-by: Tomoya Fujita <[email protected]>

---------

Signed-off-by: Tomoya Fujita <[email protected]>
(cherry picked from commit 9a0c044)
mergify bot pushed a commit that referenced this pull request Mar 14, 2025
* Adjust topic hz and bw command description.

Signed-off-by: Tomoya Fujita <[email protected]>

* note description fix.

Signed-off-by: Tomoya Fujita <[email protected]>

* address flake8 warnings.

Signed-off-by: Tomoya Fujita <[email protected]>

---------

Signed-off-by: Tomoya Fujita <[email protected]>
(cherry picked from commit 9a0c044)
ahcorde pushed a commit that referenced this pull request Mar 17, 2025
Signed-off-by: Tomoya Fujita <[email protected]>
(cherry picked from commit 9a0c044)

Co-authored-by: Tomoya Fujita <[email protected]>
ahcorde pushed a commit that referenced this pull request Mar 17, 2025
Signed-off-by: Tomoya Fujita <[email protected]>
(cherry picked from commit 9a0c044)

Co-authored-by: Tomoya Fujita <[email protected]>
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.

5 participants