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

add verbose in service-info verb #916

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from

Conversation

leeminju531
Copy link
Contributor

@leeminju531 leeminju531 commented Jun 25, 2024

Verbose option in service info verb

Expected
image

Dependencies
rmw
rmw_implementation
rmw_fastrtps
rmw_cyclonedds
rmw_connextdds
rcl
rclcpp
rclpy

Refer to #877 (comment)

Signed-off-by: Minju, Lee <dlalswn531@naver.com>
@fujitatomoya
Copy link
Collaborator

@leeminju531 thanks for taking care of this! i will try to review all incoming PRs.

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

overall lgtm

@@ -231,6 +244,40 @@ def test_get_subscriptions_info_by_topic(daemon_node):
TEST_TOPIC_SUBSCRIPTION_QOS.reliability


def test_get_clients_info_by_service(daemon_node):
clients_info = daemon_node.get_clients_info_by_service(TEST_SERVICE_NAME)
assert len(clients_info) >= 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be one that is only matching with the service? if that is correct, probably we do not need the following for statement either.

Suggested change
assert len(clients_info) >= 1
assert len(clients_info) == 1


def test_get_servers_info_by_service(daemon_node):
servers_info = daemon_node.get_servers_info_by_service(TEST_SERVICE_NAME)
assert len(servers_info) >= 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here, should it be one?

Suggested change
assert len(servers_info) >= 1
assert len(servers_info) == 1

@fujitatomoya
Copy link
Collaborator

@leeminju531

I think that we need to have discussion on the behavior of this verbose option.
Currently suggestion is to display all hidden topics based on the service, like rq, rt internal topics as you demonstrated here.
although this is precise information, I am not sure if this is good for ROS 2 user perspective since it exposes internal hidden topics instead of services.

instead, i would suggest that it displays as service endpoint for client and server endpoint.
with this, we can conceal the hidden topics for ROS 2 users, and provide the information as service endpoint.
besides, when we create either service client or server, we can specify only one QoS as service endpoint but 2 underlying hidden topics.
that said, your suggested approach displays redundant information. (exact same QoS for 2 hidden topics)

i am now inclined to support this feature as service endpoint but exposing hidden topics. (rmw implementation needs to be aware of hidden topics but rcl or upper layer do not need to know that hidden topics at all.)

CC: @clalancette @sloretz @wjwwood

@leeminju531
Copy link
Contributor Author

@fujitatomoya

Sorry for coming back too late.

Displaying the service endpoint makes sense, even though DDS handles them internally as topics.
By doing this, other non-DDS systems that explicitly support services can also integrate with it more naturally.

However, I wonder if we could define and populate a ServiceEndpoint in the upper RCL layer?

The TopicEndpoint in the upper layer naturally pulls in the underlying TopicEndpoint like this:

/// A data structure that encapsulates the node name, node namespace,
/// topic_type, gid, and qos_profile of publishers and subscriptions
/// for a topic.
typedef struct RMW_PUBLIC_TYPE rmw_topic_endpoint_info_s
{
  /// Name of the node
  const char * node_name;
  /// Namespace of the node
  const char * node_namespace;
  /// The associated topic type's name
  const char * topic_type;
  /// Hashed value for topic type's description
  rosidl_type_hash_t topic_type_hash;
  /// The endpoint type
  rmw_endpoint_type_t endpoint_type;
  /// The GID of the endpoint
  uint8_t endpoint_gid[RMW_GID_STORAGE_SIZE];
  /// QoS profile of the endpoint
  rmw_qos_profile_t qos_profile;
} rmw_topic_endpoint_info_t;

To represent the ServiceEndpoint, though, it seems like we'd need to drop a few fields like topic_type, topic_type_hash, and endpoint_gid.

$ ros2 service info /add_two_ints -v
Type: example_interfaces/srv/AddTwoInts
Clients count: 0

Services count: 1

Node name: add_two_ints_server
Node namespace: /
Endpoint type: SERVER
QoS profile:
  Reliability: RELIABLE
  History (Depth): UNKNOWN
  Durability: VOLATILE
  Lifespan: Infinite
  Deadline: Infinite
  Liveliness: AUTOMATIC
  Liveliness lease duration: Infinite

Does that make sense?

@fujitatomoya
Copy link
Collaborator

@leeminju531 or how about landing in the middle? that means include the topic type, topic hash and GID under the ServiceEndpoint? i think these info are already collected in your implementation, and it does not have to be exclusive? for that, we can have general service information from rmw up to client libraries, and underlying topic information as well?

By doing this, other non-DDS systems that explicitly support services can also integrate with it more naturally.

if rmw implementation does not construct the service based on topics, these TopicEndpointInfo is going to be empty. besides, for having non-DDS support for ServiceEndpointInfo would be updated as it developed, i think. (e.g service endpoint itself has unique global identification but topic's.)

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.

2 participants