[rostopic] Get multiple topics with rostopic hz by wkentaro · Pull Request #712 · ros/ros_comm

wkentaro · 2015-12-04T06:53:44Z

No description provided.

wkentaro · 2015-12-04T06:55:06Z

it looks like

wkentaro · 2015-12-04T06:58:05Z

This is designed to find bottleneck of topic node flow. So it supports --search-parent option which automatically detect parent topics of specified one.

dirk-thomas · 2015-12-10T00:08:02Z

tools/rostopic/src/rostopic/__init__.py

This line will fail if ret is None but not self.times.

Thanks. fixed.

wkentaro · 2015-12-14T02:39:57Z

Maybe this can be supported by rostopic hz by rostopic hz [topic1] [topic2] ....
What do you think about this?

wkentaro · 2015-12-14T09:50:10Z

Now it looks like

dirk-thomas · 2015-12-14T18:11:30Z

tools/rostopic/src/rostopic/__init__.py

msg_class is only set in the previous loop (line 230) which doesn't seem to be what you want?

dirk-thomas · 2015-12-14T18:19:09Z

Can you please describe what a parent topic is?

Also the two features (multiple topics for hz and parent topics) seem to be independent from each other (except that the later one needs the former one). Please use separate PRs or at least commits for this in the future since it simplifies the review significantly.

Parent topics means that topics to which the specified topic subscribes to Depends on ros#712

dirk-thomas · 2016-03-02T00:55:45Z

tools/rostopic/src/rostopic/__init__.py

        import threading
        self.lock = threading.Lock()
-        self.last_printed_tn = 0
+        self.last_msg_tn = 0


Please do not change the name of this variable. It is publically visible and it would break the API to rename it.

wkentaro · 2016-03-02T04:43:39Z

I updated the commit aside from above comment.

dirk-thomas · 2016-03-02T19:46:29Z

@ros-pull-request-builder retest this please

dirk-thomas · 2016-03-02T22:16:51Z

Jenkins fails to build this branch. Please make sure it is rebased to the latest devel branch to avoid potential problems.

dirk-thomas · 2016-03-03T16:59:32Z

The test still times out.

dirk-thomas · 2016-05-10T17:45:12Z

tools/rostopic/src/rostopic/__init__.py

+        print the average publishing rate to screen
+        """
+        if not self.times:
+            return


If get_hz would distinguish this case with a different return value this extra check would not be necessary.

dirk-thomas · 2016-05-10T17:51:24Z

Thanks for fixing the build and the tests. I added a few more comments inline but they should be fairly easy to address.

wkentaro · 2016-05-11T04:05:19Z

Thanks, I applied changes according to your comments. Could you please check them again?

dirk-thomas · 2016-05-18T23:22:47Z

tools/rostopic/src/rostopic/__init__.py

    args = argv[2:]
    from optparse import OptionParser
-    parser = OptionParser(usage="usage: %prog hz /topic", prog=NAME)
+    parser = OptionParser(usage="usage: %prog hz [optioncs] /topic_0 [/topic_1 [[topic_2 [..]]]]", prog=NAME)


Spelling.

The previous question remains:

What are the trailing two dots for?

I mean topic_0 [/topic_1 [[topic_2 [topic_3 [topic_4 [topic_5 [..]]]]]]]

wkentaro · 2016-05-19T09:25:15Z

Fixed spelling, and fixed the breaking public api with default value for argument.

dirk-thomas · 2016-06-27T17:57:13Z

tools/rostopic/src/rostopic/__init__.py

    args = argv[2:]
    from optparse import OptionParser
-    parser = OptionParser(usage="usage: %prog hz /topic", prog=NAME)
+    parser = OptionParser(usage="usage: %prog hz [options] /topic_0 [/topic_1 [[topic_2 [..]]]]", prog=NAME)


I will replace the double [[ with a single [ when cherry-picking the changes.

dirk-thomas · 2016-06-27T17:58:10Z

Thank you for providing this enhancement. I have cherry-picked the changes to the kinetic-devel branch: a6a4e57

wkentaro · 2016-06-27T20:31:14Z

Thank you!

dirk-thomas · 2016-09-02T21:24:33Z

I just run into a problem with this new feature when testing #874. If you pass two topic names but only one of them has any activity the command only prints "no new messages" and doesn't show the information about the topic which does have data.

@wkentaro Can you please look into this problem and make a PR for it (the tests should probably also cover that case).

Close ros#712 (comment)

wkentaro · 2016-09-03T08:18:09Z

I think #886 fixes the problem, but not sure how to test it.
Currently the testing is disabled, and enabling it leads test fails.
https://github.com/ros/ros_comm/blob/kinetic-devel/tools/rostopic/test/check_rostopic_command_line_online.py#L116-L127

dirk-thomas · 2016-09-09T17:56:19Z

Since this PR changes the public API of ROSTopicHz it breaks downstream code which uses the class. E.g. it breaks rqt_topic. While this specific code can be updated to work with the old API (list) as well as the new API (dict) I am not sure if it is a good idea risking to break more downstream code. Maybe this PR (as well as all other ones building on top) should be reverted? Or this can be updated somehow to maintain backward compatibility?

wkentaro · 2016-09-09T23:06:26Z

I agree that the code should be changed for the backward compatibility, but currently I don't have time to work on that, so it cannot be done soon.
So could you please revert this at this time, and open an issue for this? I will work for that later.
The better implementation seems using multiple ROSTopicHz class in rostopic_hz command and print hz with get_hz and _get_ascii_table, without changes on member variables of ROSTopicHz.

dirk-thomas · 2016-09-14T22:22:18Z

Since we have already merged quite a bit of stuff on-top of this patch I rather spent time on actually fixing the API than trying to revert all the changes (knowing that I will have to reapply them in the near future again). Therefore I created #896 to restore API compatibility..

dirk-thomas reviewed Dec 10, 2015
View reviewed changes

wkentaro force-pushed the topic-hz-monitor branch from 318087e to 9ff24cb Compare December 11, 2015 17:39

wkentaro force-pushed the topic-hz-monitor branch from ed30602 to 8e46e40 Compare December 14, 2015 09:47

dirk-thomas reviewed Dec 14, 2015
View reviewed changes

wkentaro force-pushed the topic-hz-monitor branch from c6b3723 to 00e7912 Compare December 15, 2015 04:23

wkentaro added a commit to wkentaro/ros_comm that referenced this pull request Dec 15, 2015

[rostopic] Get parent topics in rostopic hz

d04775f

Parent topics means that topics to which the specified topic subscribes to Depends on ros#712

wkentaro mentioned this pull request Dec 15, 2015

[rostopic] Get parent topics in rostopic hz #718

Closed

wkentaro added a commit to wkentaro/ros_comm that referenced this pull request Dec 15, 2015

[rostopic] Get parent topics in rostopic hz

eb3f805

Parent topics means that topics to which the specified topic subscribes to Depends on ros#712

wkentaro changed the title ~~topic_hz_monitor: Script to monitor multiple topic hz rate~~ rostopic Jan 5, 2016

wkentaro changed the title ~~rostopic~~ rostopic hz: Get multiple topics' hz Jan 5, 2016

dirk-thomas reviewed Mar 2, 2016
View reviewed changes

wkentaro force-pushed the topic-hz-monitor branch from 7358b45 to 863f099 Compare March 2, 2016 23:10

dirk-thomas added the requires-changes label Mar 3, 2016

Get multiple topics for rostopic hz

6a02ef8

dirk-thomas reviewed May 10, 2016
View reviewed changes

wkentaro added 2 commits May 11, 2016 13:00

Print multiple topics in print_hz method for hz check

50372e3

Remove redundant check in ROSTopicHz.print_hz

5f93f62

wkentaro changed the title ~~rostopic hz: Get multiple topics' hz~~ [rostopic] Get multiple topics with rostopic hz May 11, 2016

dirk-thomas reviewed May 18, 2016
View reviewed changes

wkentaro added 2 commits May 19, 2016 18:15

Make command usage easy to understand .. -> [options]

ecd5f6f

Does not break the public api

f4d57a1

wkentaro force-pushed the topic-hz-monitor branch from b4c68c8 to f4d57a1 Compare May 19, 2016 09:24

dirk-thomas reviewed Jun 27, 2016
View reviewed changes

dirk-thomas removed the requires-changes label Jun 27, 2016

dirk-thomas closed this Jun 27, 2016

wkentaro deleted the topic-hz-monitor branch June 27, 2016 20:24

dirk-thomas mentioned this pull request Sep 2, 2016

[rostopic] Get parent topics (upstream in pipeline) with rostopic hz #874

Closed

wkentaro added a commit to wkentaro/ros_comm that referenced this pull request Sep 3, 2016

Fix for no messages for one topic in two topics

98437ec

Close ros#712 (comment)

wkentaro mentioned this pull request Sep 3, 2016

[rostopic] Fix for no messages for one topic in two topics #886

Merged

dirk-thomas mentioned this pull request Sep 14, 2016

restore API compatibility of rostopic.ROSTopicHz #896

Merged

dirk-thomas mentioned this pull request Mar 1, 2017

changes between 1.11.20 and 1.12.7 for backporting #1008

Merged

Conversation

wkentaro commented Dec 4, 2015

Uh oh!

wkentaro commented Dec 4, 2015

Uh oh!

wkentaro commented Dec 4, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wkentaro commented Dec 14, 2015

Uh oh!

wkentaro commented Dec 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dirk-thomas commented Dec 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wkentaro commented Mar 2, 2016

Uh oh!

dirk-thomas commented Mar 2, 2016

Uh oh!

dirk-thomas commented Mar 2, 2016

Uh oh!

dirk-thomas commented Mar 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dirk-thomas commented May 10, 2016

Uh oh!

wkentaro commented May 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wkentaro commented May 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dirk-thomas commented Jun 27, 2016

Uh oh!

wkentaro commented Jun 27, 2016

Uh oh!

dirk-thomas commented Sep 2, 2016

Uh oh!

wkentaro commented Sep 3, 2016

Uh oh!

dirk-thomas commented Sep 9, 2016

Uh oh!

wkentaro commented Sep 9, 2016

Uh oh!

dirk-thomas commented Sep 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wkentaro commented May 19, 2016 •

edited

Loading