Skip to content
This repository has been archived by the owner on Jun 21, 2023. It is now read-only.

Update graph API return codes. #459

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()

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 <michel@ekumenlabs.com>
Comment on lines +45 to 47
if (RMW_RET_OK != rmw_check_zero_rmw_string_array(node_names)) {
return RMW_RET_INVALID_ARGUMENT;
}
Copy link
Member

Choose a reason for hiding this comment

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

this seems inconsistent with:

  ret = rmw_names_and_types_check_zero(topic_names_and_types);
  if (RMW_RET_OK != ret) {
    return ret;
  }

Copy link
Contributor

Choose a reason for hiding this comment

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

My guess is that rmw_check_zero_rmw_string_array(node_names) does not return RMW_RET_INVALID_ARGUMENT

Copy link
Member

Choose a reason for hiding this comment

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

ok, it's a bit weird the semantic different between the two, but it doesn't matter much,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. rmw_check_zero_rmw_string_array returns RMW_RET_INVALID_ARGUMENT if the argument is NULL, and RMW_RET_ERROR if it's not zero initialized. We can change rmw_names_and_types_check_zero call sites to the do the same for consistency's sake though.

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

@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

@hidmic
Copy link
Contributor Author

hidmic commented Sep 22, 2020

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

@hidmic hidmic merged commit afe8b1e 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 <michel@ekumenlabs.com>
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 <michel@ekumenlabs.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants