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

Hz verb: Warn users about wrong reported freq #970

Closed
wants to merge 2 commits into from

Conversation

VictorLamoine
Copy link

@VictorLamoine VictorLamoine commented Feb 7, 2025

I think enough users have wasted time with this tool and I hope to be the last one.

#871
#843
https://robotics.stackexchange.com/questions/95613/ros2-topic-hz-provides-wrong-rate-for-larger-msgs

Please notify me if that modification is ok for you and I'll fix the tests

Copy link
Collaborator

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

@VictorLamoine thanks for creating PR, trying to add the notice.

IMO this message does not really help the user experience. meaning, if that just might not be correct, how can we know that is correct this time or that time? without that capability, i guess this is just what we cannot trust at all, that means pretty much same with do not use this feature for the topic statistics, because that might be just incorrect information... for me.

instead, i suggest the followings, what do you think?

  • adding the qos configuration option for ros2 topic hz. since QoS configuration affects the data reception on subscription with ros2 topic hz, besides this is also reported in ROS2 topic hz gives incorrect value with camera image #871 to solve the problem. we also can add this note on the command help message as well.
  • i also would want to add more description about how ros2 topic hz works. it actually creates the subscription and receives the data, and then calculate the frequency. with my experience from streaming framework, some developers would think this is statistics from the sender(publisher) side.

@VictorLamoine
Copy link
Author

I'm all in for improving the tool but until the problems are fixed warning the users is the least we can do.

@fujitatomoya
Copy link
Collaborator

@VictorLamoine could you add QoS improvement in this PR?

@audrow
Copy link
Member

audrow commented Feb 27, 2025

I've added this to discuss to the next ROS 2 PMC meeting.

@fujitatomoya
Copy link
Collaborator

#935 is adding the QoS argument for ros2 topic hz.

@fujitatomoya
Copy link
Collaborator

@VictorLamoine could you add QoS improvement in this PR?

this has been addressed by #970

@VictorLamoine
Copy link
Author

Sorry; I didn't have time to deal with this. Well test again and report!

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-pmc-minutes-for-2025-03-12/42493/1

@fujitatomoya
Copy link
Collaborator

@VictorLamoine can you close this one, instead i will come up with documentation described in #843 (comment). if you can review, that would be appreciated.

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.

4 participants