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

Update graph API return codes. #243

Merged
merged 1 commit into from
Sep 22, 2020
Merged

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Sep 18, 2020

Connected to ros2/rmw#272. This patch affects:

  • rmw_get_node_names()
  • rmw_get_node_names_with_enclaves()
  • rmw_get_topic_names_and_types()
  • rmw_get_service_names_and_types()
  • rmw_get_publishers_info_by_topic()
  • rmw_get_subscriptions_info_by_topic()
  • rmw_get_subscriber_names_and_types_by_node()
  • rmw_get_publisher_names_and_types_by_node()
  • rmw_get_service_names_and_types_by_node()
  • rmw_get_client_names_and_types_by_node()
  • rmw_count_publishers()
  • rmw_count_subscribers()

Copy link
Collaborator

@eboasson eboasson left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for going over all the argument checking and return values with such care!

Copy link
Contributor

@Lobotuerk Lobotuerk left a comment

Choose a reason for hiding this comment

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

LGTM

@hidmic
Copy link
Contributor Author

hidmic commented Sep 21, 2020

CI up to test_rmw_implementation:

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

@jacobperron
Copy link
Member

jacobperron commented Sep 21, 2020

@hidmic I suggest testing additional packages above test_rmw_implementation. E.g. #241 seems to have introduced test regressions in several other packages further downstream.

@hidmic
Copy link
Contributor Author

hidmic commented Sep 22, 2020

@jacobperron I believe the problem with #241 was that CI was not properly run against actual tests (see here). Every modification here is checked against a corresponding test.

@hidmic
Copy link
Contributor Author

hidmic commented Sep 22, 2020

CI up to test_rmw_implementation, rcl, and rclcpp:

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

This patch affects:
- rmw_get_node_names()
- rmw_get_node_names_with_enclaves()
- rmw_get_topic_names_and_types()
- rmw_get_service_names_and_types()
- rmw_get_publishers_info_by_topic()
- rmw_get_subscriptions_info_by_topic()
- rmw_get_subscriber_names_and_types_by_node()
- rmw_get_publisher_names_and_types_by_node()
- rmw_get_service_names_and_types_by_node()
- rmw_get_client_names_and_types_by_node()
- rmw_count_publishers()
- rmw_count_subscribers()

Signed-off-by: Michel Hidalgo <[email protected]>
@hidmic
Copy link
Contributor Author

hidmic commented Sep 22, 2020

Argh, I had to rebase after #245.


CI up to test_rmw_implementation, rcl, and rclcpp:

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

@hidmic
Copy link
Contributor Author

hidmic commented Sep 22, 2020

Alright, going in! Thanks for all the good reviews!

@hidmic hidmic merged commit 7bf58ec into master Sep 22, 2020
@delete-merged-branch delete-merged-branch bot deleted the hidmic/update-graph-api-ret branch September 22, 2020 19:54
ahcorde pushed a commit that referenced this pull request Oct 9, 2020
This patch affects:
- rmw_get_node_names()
- rmw_get_node_names_with_enclaves()
- rmw_get_topic_names_and_types()
- rmw_get_service_names_and_types()
- rmw_get_publishers_info_by_topic()
- rmw_get_subscriptions_info_by_topic()
- rmw_get_subscriber_names_and_types_by_node()
- rmw_get_publisher_names_and_types_by_node()
- rmw_get_service_names_and_types_by_node()
- rmw_get_client_names_and_types_by_node()
- rmw_count_publishers()
- rmw_count_subscribers()

Signed-off-by: Michel Hidalgo <[email protected]>
ahcorde pushed a commit that referenced this pull request Oct 15, 2020
This patch affects:
- rmw_get_node_names()
- rmw_get_node_names_with_enclaves()
- rmw_get_topic_names_and_types()
- rmw_get_service_names_and_types()
- rmw_get_publishers_info_by_topic()
- rmw_get_subscriptions_info_by_topic()
- rmw_get_subscriber_names_and_types_by_node()
- rmw_get_publisher_names_and_types_by_node()
- rmw_get_service_names_and_types_by_node()
- rmw_get_client_names_and_types_by_node()
- rmw_count_publishers()
- rmw_count_subscribers()

Signed-off-by: Michel Hidalgo <[email protected]>
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