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

Fix dynamic windowing for Topic Statistics #1577

Closed
wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 10, 2019

Fix dynamic windowing for Topic Statistics to accurately adapt to slow or fast topics.

fixes #1562 (Topic statistics report 0Hz for topics under 1Hz)

@ghost
Copy link
Author

ghost commented Jan 11, 2019

The test that failed in above build on stretch (topic_tools.rosunit-throttle_hztest_explicit/test_hz) seems to have some probability of failing already, and from looking through it, seems like it couldn't have been affected by my change, since that test does not enable topic statistics.

@dirk-thomas
Copy link
Member

dirk-thomas commented Jan 31, 2019

Instead of breaking API to invert the semantic of the member variable and update all cases where it is being used it should be much less intrusive to invert the actual condition in this line: https://github.com/ros/ros_comm/pull/1577/files#diff-fafa872049c30ea0ac4fa62e11cd5ddeR262 ?

@emersonknapp emersonknapp force-pushed the melodic-devel branch 3 times, most recently from 8172e26 to 8add1eb Compare February 14, 2019 03:33
@emersonknapp
Copy link
Contributor

(merged eknapp-az account with this my main account)

Thanks for taking a look at this! Sorry for my slow update - just moved states and this fell by the wayside.

The linked publish? check isn't the only problematic part - the variable pub_frequency was used in all cases in the implementation as a period, not a frequency.

  • it is added to a time as a duration
  • it is compared directly to parameters called window, measured in seconds

In the interest of keeping the API untouched like you mentioned, I have uploaded a new version that corrects the semantics only in the implementation. There are two commits there - one is a failing test case for both rospy and roscpp for topic frequencies, the second is the fix to the clients.

This version is a little less intrusive, but still required fixing more than just the check since the semantics were globally wrong.

@emersonknapp
Copy link
Contributor

@dirk-thomas do you have any further thoughts or concerns about this?

@dirk-thomas
Copy link
Member

This needs a rebase after #1625 was merged.

@artivis
Copy link
Contributor

artivis commented Aug 1, 2019

Maybe this PR can be closed (cleanup) since #1695 replaces it.

dirk-thomas pushed a commit that referenced this pull request Aug 12, 2019
* Add failing tests for topic statistics frequency for rospy and roscpp

* Fix TopicStatistics dynamic windowing to adjust evaluation frequency in the right direction

* test_roscpp: fixed topic_statistic_frequency

* test_roscpp/topic_statistic_frequency: cleanup
dirk-thomas pushed a commit that referenced this pull request Aug 4, 2020
* Add failing tests for topic statistics frequency for rospy and roscpp

* Fix TopicStatistics dynamic windowing to adjust evaluation frequency in the right direction

* test_roscpp: fixed topic_statistic_frequency

* test_roscpp/topic_statistic_frequency: cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Topic Statistics reports 0 for topics under 1Hz
3 participants