Created public get_topic_list() function for use in other scripts. by EliteMasterEric · Pull Request #1154 · ros/ros_comm

EliteMasterEric · 2017-08-24T18:04:42Z

A third pull request since #1146 required changes and #1151 was based on kinetic-devel. I would have reopened #1146 but I broke it or something.

Resolves issue #946.

The only function in rostopic for listing the available topics is built exclusively for the command line interface; this adds a public function for other applications that want a list of topics.

Usage:

import rostopic
publishers, subscribers = rostopic.get_topic_list()
for topic, type, nodes in publishers:
    print("Publisher: %s, [%s] (%d nodes)" % (topic, type, len(nodes)))
for topic, type, nodes in subscribers:
    print("Subscriber: %s, [%s] (%d nodes)" % (topic, type, len(nodes)))

dirk-thomas

publishers, subscribers = rostopic.get_topic_list()
for topic, type, node_count in publishers:

The usage seems to be wrong. As far as I can see the code returns a list of publishers as the third element - not a count?

dirk-thomas · 2017-08-29T16:27:29Z

tools/rostopic/src/rostopic/__init__.py

-            topic_ns = rosgraph.names.make_global_ns(topic)        
-            subs = (x for x in subs if x[0] == topic or x[0].startswith(topic_ns))
-            pubs = (x for x in pubs if x[0] == topic or x[0].startswith(topic_ns))
+        pubs, subs = get_topic_list()


Why is the filtering by the optional topic not needed here anymore?

dirk-thomas · 2017-08-29T16:28:36Z

tools/rostopic/test/test_rostopic.py

+        # test through public function
+        topics = ['/chatter', '/foo/chatter', '/bar/chatter', '/rosout', '/rosout_agg'] 
+
+        with fakestdout() as b:


This shouldn't be necessary?

dirk-thomas · 2017-08-29T16:29:50Z

tools/rostopic/src/rostopic/__init__.py

        topics.sort()
        print('\n'.join(["%s%s"%(indent, t) for t in topics]))

+def get_topic_list():


It might be good to pass in the master since each caller already has a master variable.

dirk-thomas · 2017-08-29T16:32:09Z

tools/rostopic/src/rostopic/__init__.py

        if not subscribers_only:
            print("\n%sPublished topics:"%indent)
-            for t, l in pubs:
+            for t, y, l in pubs:


Please use a different variable name than y.

Same below.

dirk-thomas · 2017-08-29T16:33:22Z

tools/rostopic/src/rostopic/__init__.py

    else:
        if publishers_only:
-            topics = [t for t,_ in pubs]
+            topics = [t for t,_,_ in pubs]


Please add a space after each comma (PEP 8).

Same below.

This comment is still pending.

Same several times below.

EliteMasterEric · 2017-08-29T21:08:32Z

@dirk-thomas Commits have been made resolving the issues you pushed, and the outdated usage example in the description has been fixed.

dirk-thomas

While new / modified lines should follow PEP8 unchanged lines shouldn't be changed since that makes backporting changes to other branch much more difficult. Please revert any unrelated / unnecessary (whitespace) change in your patch.

dirk-thomas · 2017-09-14T22:50:13Z

tools/rostopic/src/rostopic/__init__.py

-            topic_ns = rosgraph.names.make_global_ns(topic)        
-            subs = (x for x in subs if x[0] == topic or x[0].startswith(topic_ns))
+        pubs, subs = get_topic_list(master=master)
+        if topic is not None:


Why was this condition changed from if topic to if topic is not None?

dirk-thomas · 2017-09-14T22:50:57Z

tools/rostopic/src/rostopic/__init__.py

+            topic_ns = rosgraph.names.make_global_ns(topic)
            pubs = (x for x in pubs if x[0] == topic or x[0].startswith(topic_ns))
-
+            subs = (x for x in subs if x[0] == topic or x[0].startswith(topic_ns))


Please do not change the order of lines for no reason.

EliteMasterEric · 2017-11-01T16:56:47Z

@dirk-thomas Sorry for the delay, I edited some of the commented lines. Please review these changes and provide feedback.

…1154)

EliteMasterEric · 2017-11-03T19:50:56Z

@mikepurvis Did you just push a commit with my changes in it rather than merging my pull request with the base branch?

mikepurvis · 2017-11-03T20:01:47Z

@mastereric Yes, lunar-edge is a branch that Clearpath uses for our internal development builds, which are tested across a variety of use-cases (robots, simulation, development). Patching changes on to the edge branch is a way for us to build confidence in them before merging to devel.

(Others are welcome to build from edge as well; we're the only current consumer that I'm directly aware of.)

…ipts. (#1154)" This reverts commit f901429.

mikepurvis · 2017-11-07T19:22:50Z

I've backed this PR out of our testing. Please see the regression below:

$ roscore &
$ rostopic info /rosout
Traceback (most recent call last):
  File "/opt/clearpath/2.8devel/sdk/bin/rostopic", line 35, in <module>
    rostopic.rostopicmain()
  File "/opt/clearpath/2.8devel/sdk/lib/python2.7/dist-packages/rostopic/__init__.py", line 2111, in rostopicmain
    _rostopic_cmd_info(argv)
  File "/opt/clearpath/2.8devel/sdk/lib/python2.7/dist-packages/rostopic/__init__.py", line 2068, in _rostopic_cmd_info
    exitval = _rostopic_info(topic) or 0
  File "/opt/clearpath/2.8devel/sdk/lib/python2.7/dist-packages/rostopic/__init__.py", line 1302, in _rostopic_info
    print(get_info_text(topic))
  File "/opt/clearpath/2.8devel/sdk/lib/python2.7/dist-packages/rostopic/__init__.py", line 1289, in get_info_text
    for p in itertools.chain(*[l for x, l in subs]):
ValueError: too many values to unpack

EliteMasterEric · 2017-11-08T22:05:58Z

@mikepurvis Fixed the error you're getting, a test looks fine on my machine

…1154)

EliteMasterEric · 2018-02-26T17:25:19Z

Hey @mikepurvis,
The changes in this pull request were merged into lunar-edge back in November. If you require no further changes for them and they appear to function without issues, can you please merge them into the main branch?
Note that another pull request (ros-visualization/rqt#124) is relying on these changes to be implemented properly.

mikepurvis · 2018-02-26T19:11:55Z

tools/rostopic/src/rostopic/__init__.py

        topics.sort()
        print('\n'.join(["%s%s"%(indent, t) for t in topics]))

+def get_topic_list(master=rosgraph.Master('/rostopic')):


I actually ran into a small issue with this just today— it's a bit of a corner case, but when ROS_MASTER_URI isn't set, this bombs out on import, for example:

$ unset ROS_MASTER_URI administrator@vm-mpurvis-xenial-ws:~$ python Python 2.7.12 (default, Nov 19 2016, 06:48:10) [GCC 5.4.0 20160609] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> import rostopic Traceback (most recent call last): File "<stdin>", line 1, in <module> File "workspace/lib/python2.7/dist-packages/rostopic/__init__.py", line 1152, in <module> def get_topic_list(master=rosgraph.Master('/rostopic')): File "workspace/lib/python2.7/dist-packages/rosgraph/masterapi.py", line 100, in __init__ self._reinit(master_uri) File "workspace/lib/python2.7/dist-packages/rosgraph/masterapi.py", line 113, in _reinit raise ValueError("ROS master URI is not set") ValueError: ROS master URI is not set >>>

I believe this will be corrected if you instead do:

def get_topic_list(master=None): if not master: master = rosgraph.Master('/rostopic')

Other than this, I'd be happy to merge this to mainline.

Thanks for the fix. Please also address the style concern from @dirk-thomas and then this is good to go in.

EliteMasterEric · 2018-04-02T15:50:20Z

@mikepurvis I am apparently blind and didn't notice the leftover spacing fix had yet to be done. The fix has been pushed for review.

dirk-thomas · 2018-04-02T18:10:00Z

While new / modified lines should follow PEP8 unchanged lines shouldn't be changed since that makes backporting changes to other branch much more difficult. Please revert any unrelated / unnecessary (whitespace) change in your patch.

This comment still applies.

EliteMasterEric · 2018-04-05T13:36:12Z

@dirk-thomas Sorry for making that mistake again, after my current PRs are done I'll look into making separate PEP8 pull requests for this and other repositories.

dirk-thomas · 2018-04-05T15:28:04Z

My previous comment still applies. See the diff should not touch any line just for style adjustments. If the line isn't being changed fo a functional reason in this patch please avoid the change to keep this patch minimal.

after my current PRs are done I'll look into making separate PEP8 pull requests for this and other repositories.

For newly added code we aim to follow common style guides. But don't want to reformat existing code because this repository has currently three (soon four) actively maintained branches and a code reformatting patch would severely hinder the ability to backport changes to other branch.

EliteMasterEric · 2018-04-09T16:39:44Z

@dirk-thomas I have double-checked with the diff and ensured the changes are as requested.

If you have any other issues please cite specific line numbers.

EliteMasterEric · 2018-04-16T13:03:58Z

@dirk-thomas It has been one week since I pushed these changes. Can you please review and approve them? I would like for them to be approved before the Melodic release.

mikepurvis · 2018-04-16T13:40:11Z

Please fix the remaining PEP8 issues (spacing around commas).

mikepurvis · 2018-04-20T18:48:59Z

Looks great, thanks for this contribution— going to merge into lunar-devel for now, and then it will be part of melodic-devel when we create that branch.

mikepurvis · 2018-04-20T18:49:24Z

The failing test on Melodic/debian is unrelated to this change (see #1358)

dirk-thomas · 2018-05-23T18:39:41Z

tools/rostopic/src/rostopic/__init__.py

-                    print(indent+" * %s [%s] %s subscribers"%(t, topic_type(t, topic_types), len(l)))
+            for t, ttype, tlist in subs:
+                if len(tlist) > 1:
+                    print(indent+" * %s [%s] %s subscribers"%(t, ttype, len(llist)))


llist doesn't exist in this context. See #1407 for a follow up patch.

Created public get_topic_list() function for use in other scripts.

1b610a5

EliteMasterEric mentioned this pull request Aug 24, 2017

Created standardized ROS message selector interfaces ros-visualization/rqt#124

Closed

dirk-thomas reviewed Aug 29, 2017

View reviewed changes

dirk-thomas reviewed Sep 14, 2017

View reviewed changes

dirk-thomas added enhancement more-information-needed requires-changes and removed more-information-needed labels Sep 14, 2017

EliteMasterEric force-pushed the topic-list branch from d511ea8 to ab8fa1d Compare November 1, 2017 16:18

mikepurvis added the hitlist label Nov 3, 2017

mikepurvis added a commit that referenced this pull request Nov 3, 2017

Created public get_topic_list() function for use in other scripts. (#…

f901429

…1154)

mikepurvis added a commit that referenced this pull request Nov 7, 2017

Revert "Created public get_topic_list() function for use in other scr…

5046fed

…ipts. (#1154)" This reverts commit f901429.

mikepurvis added a commit that referenced this pull request Nov 15, 2017

Created public get_topic_list() function for use in other scripts. (#…

a421714

…1154)

mikepurvis reviewed Feb 26, 2018

View reviewed changes

EliteMasterEric force-pushed the topic-list branch from ddd069c to f7b683a Compare April 5, 2018 13:34

EliteMasterEric force-pushed the topic-list branch from f7b683a to a361328 Compare April 9, 2018 14:17

Spacing corrections and bug fixes

30fc379

EliteMasterEric force-pushed the topic-list branch from a361328 to 30fc379 Compare April 9, 2018 14:19

Spacing fixes.

fb01aca

EliteMasterEric force-pushed the topic-list branch from 04305a3 to fb01aca Compare April 20, 2018 13:22

mikepurvis merged commit e96c407 into ros:lunar-devel Apr 20, 2018

EliteMasterEric mentioned this pull request Apr 23, 2018

PEP8 compliance for existing codebase #1375

Closed

dirk-thomas mentioned this pull request May 23, 2018

Fix the count of subscribers. #1407

Merged

dirk-thomas reviewed May 23, 2018

View reviewed changes

dirk-thomas mentioned this pull request Aug 20, 2018

changes between 1.12.13 and 1.13.7 for backporting #1490

Merged

Conversation

EliteMasterEric commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dirk-thomas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EliteMasterEric commented Aug 29, 2017

Uh oh!

dirk-thomas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EliteMasterEric commented Nov 1, 2017

Uh oh!

EliteMasterEric commented Nov 3, 2017

Uh oh!

mikepurvis commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikepurvis commented Nov 7, 2017

Uh oh!

EliteMasterEric commented Nov 8, 2017

Uh oh!

EliteMasterEric commented Feb 26, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

EliteMasterEric commented Apr 2, 2018

Uh oh!

dirk-thomas commented Apr 2, 2018

Uh oh!

EliteMasterEric commented Apr 5, 2018

Uh oh!

dirk-thomas commented Apr 5, 2018

Uh oh!

EliteMasterEric commented Apr 9, 2018

Uh oh!

EliteMasterEric commented Apr 16, 2018

Uh oh!

mikepurvis commented Apr 16, 2018

Uh oh!

mikepurvis commented Apr 20, 2018

Uh oh!

mikepurvis commented Apr 20, 2018 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

EliteMasterEric commented Aug 24, 2017 •

edited

Loading

mikepurvis commented Nov 3, 2017 •

edited

Loading

EliteMasterEric commented Feb 26, 2018 •

edited

Loading

mikepurvis commented Apr 20, 2018 •

edited

Loading