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

Implement functions to get publisher and subcription informations like QoS policies from topic name #454

Merged
merged 24 commits into from
Feb 14, 2020

Conversation

jaisontj
Copy link
Contributor

NOTE: DO NOT MERGE until rmw #186, rmw_implementation #72 and rcl #511 are merged.

This PR makes the necessary changes to implement this feature request. The RCLPY layer needs to expose functions to the ros2cli layer such that ros2 topic info <topic_name> can display publisher and subscriptions information for the given topic_name.

Summary of changes:

  • Added a new helper function in common.h/c to convert rmw_topic_info_array_t to a Python list of dictionaries.
  • Added get_publishers_info_by_topic and get_subscriptions_info_by_topic in node.py
  • Added rclpy_get_publishers_info_by_topic and
    rclpy_get_subscriptions_information_by_topic in _rclpy.c
  • Added tests for node.get_publishers_info_by_topic and
    node.get_subscriptions_info_by_topic

Related to issues in aws-roadmap #85

@jaisontj jaisontj changed the title - Added and implemented get_publishers_info_by_topic and get_subscrip… Implement functions to get publisher and subcription informations like QoS policies from topic name Oct 29, 2019
rclpy/test/test_node.py Outdated Show resolved Hide resolved
rclpy/rclpy/node.py Outdated Show resolved Hide resolved
rclpy/test/test_node.py Outdated Show resolved Hide resolved
rclpy/test/test_node.py Outdated Show resolved Hide resolved
@jaisontj jaisontj force-pushed the jaisontj/impl_qos_in_topic_info branch 2 times, most recently from 08075e4 to 982a88e Compare November 12, 2019 19:27
Copy link

@piraka9011 piraka9011 left a comment

Choose a reason for hiding this comment

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

LGTM

@jaisontj jaisontj force-pushed the jaisontj/impl_qos_in_topic_info branch from a89e0a9 to c17c401 Compare November 19, 2019 22:16
rclpy/test/test_node.py Outdated Show resolved Hide resolved
rclpy/src/rclpy/_rclpy.c Outdated Show resolved Hide resolved
rclpy/src/rclpy/_rclpy.c Outdated Show resolved Hide resolved
rclpy/src/rclpy/_rclpy.c Outdated Show resolved Hide resolved
rclpy/src/rclpy/_rclpy.c Outdated Show resolved Hide resolved
rclpy/src/rclpy_common/src/common.c Outdated Show resolved Hide resolved
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

Looks good. Two minor comments

rclpy/src/rclpy_common/src/common.c Outdated Show resolved Hide resolved
rclpy/src/rclpy_common/src/common.c Outdated Show resolved Hide resolved
rclpy/rclpy/node.py Outdated Show resolved Hide resolved
rclpy/src/rclpy/_rclpy.c Show resolved Hide resolved
rclpy/src/rclpy/_rclpy.c Outdated Show resolved Hide resolved
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM but for the last nit standing

rclpy/src/rclpy/_rclpy.c Show resolved Hide resolved
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

Left a minimal comment, otherwise LGTM!

rclpy/src/rclpy/_rclpy.c Outdated Show resolved Hide resolved
@ivanpauno ivanpauno requested a review from hidmic January 20, 2020 19:52
@mm318 mm318 force-pushed the jaisontj/impl_qos_in_topic_info branch from a3272a5 to 50018e8 Compare January 20, 2020 22:30
@mm318
Copy link
Member

mm318 commented Feb 7, 2020

Sorry for the delay. I haven't been able to get around to finishing the rclpy implementation to match rclcpp. It will be finished by tomorrow.

@mm318 mm318 force-pushed the jaisontj/impl_qos_in_topic_info branch from cc07683 to 3a0c105 Compare February 7, 2020 19:42
@mm318
Copy link
Member

mm318 commented Feb 7, 2020

I have added topic name remapping to _get_info_by_topic() in rclpy, so now it behaves like the rclcpp one. Please re-review, @wjwwood and @ivanpauno. Thanks!

@mm318 mm318 force-pushed the jaisontj/impl_qos_in_topic_info branch from 3a0c105 to b34b127 Compare February 7, 2020 20:08
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm, setting aside some small comments.

return infos

def get_publishers_info_by_topic(
self, topic_name: str, no_mangle: bool = False) -> List[TopicEndpointInfo]:
Copy link
Member

Choose a reason for hiding this comment

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

This style doesn't look correct to me, I'd expect:

    def get_publishers_info_by_topic(
        self, topic_name: str, no_mangle: bool = False
    ) -> List[TopicEndpointInfo]:
        """
        ...
        """
        # ...

Or

    def get_publishers_info_by_topic(
        self,
        topic_name: str,
        no_mangle: bool = False
    ) -> List[TopicEndpointInfo]:
        """
        ...
        """
        # ...

There are a few other places this applies in this pull request.

Copy link
Member

Choose a reason for hiding this comment

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

Done.

def get_publishers_info_by_topic(
self, topic_name: str, no_mangle: bool = False) -> List[TopicEndpointInfo]:
"""
Return a list of publishers publishing to a given topic.
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: a publisher doesn't need to be "publishing" on the topic to be seen here, technically I'd say "list of publishers on a given topic" or "list of publishers that have advertised on a given topic"

Copy link
Member

Choose a reason for hiding this comment

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

Done.

Signed-off-by: Miaofei <[email protected]>
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM!

@ivanpauno
Copy link
Member

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • macOS Build Status

@ivanpauno
Copy link
Member

@mm318 can you check the build failures?

@mm318
Copy link
Member

mm318 commented Feb 10, 2020

@mm318 can you check the build failures?

Hi @ivanpauno, did you include the changes in ros2/rcl#558?

@ivanpauno
Copy link
Member

ivanpauno commented Feb 10, 2020

Hi @ivanpauno, did you include the changes in ros2/rcl#558?

Thanks! I didn't remember about that PR.

Another run:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member

@mm318 The errors seem to be due to the following:

  • Linters errors have probably already been solved in master, can you rebase?
  • macOS failure is unrelated (I will run it again when that's fixed).
  • Windows failures are because of RCLCPP PUBLIC missing in some places.

Signed-off-by: Miaofei <[email protected]>
@mm318
Copy link
Member

mm318 commented Feb 12, 2020

The issues found on Linux and Windows should be fixed now.

@ivanpauno
Copy link
Member

ivanpauno commented Feb 12, 2020

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status (unrelated problem in macOS CI)
  • Windows Build Status

@wjwwood
Copy link
Member

wjwwood commented Feb 12, 2020

Once the fastrtps issue is fixed, can you re-run these @ivanpauno?

@ivanpauno
Copy link
Member

ivanpauno commented Feb 13, 2020

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status (unrelated failures)
  • Windows Build Status

@mm318
Copy link
Member

mm318 commented Feb 14, 2020

I think the test failures in the Windows build are unrelated. What do you think?

@ivanpauno
Copy link
Member

I think the test failures in the Windows build are unrelated. What do you think?

Yes, I'm wating the macOS job.

@ivanpauno
Copy link
Member

Merging! Thanks @jaisontj @mm318 for pushing this!!

@ivanpauno ivanpauno merged commit 17a0854 into ros2:master Feb 14, 2020
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.

6 participants