Add rmw count clients,services impl

[leeminju531]

this PR is for service counts using it's name like topic.
Related to ros2/ros2cli#771

Signed-off-by: leeminju531 dlalswn531@naver.com

[eboasson]

I have a few comments. I've only made them for rmw_count_clients, obviously they also apply to rmw_count_services.

[eboasson]

I think it would be better to have a const std::string for the "Request" and "Reply" strings and use that everywhere (in particular also in rmw_init_cs).

[eboasson]

This'll lead to spurious error returns: discovery of publishers and subscribers happens independently, and so the odd discrepancy between the two is to be expected. It is not trivial to decide what to do, retrying is an option, but then you could end up in a live lock if someone is creating and deleting clients all the time, or in an even worse situation if someone deliberately creates a DDS reader but no writer (or vice versa, but the reader seems more likely to me).

Given that all the (Cyclone DDS) clients have a "client id" in the user data (

rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp

Line 4688 in 5b67ae4

std::string user_data = std::string(is_service ? "serviceid=" : "clientid=") + csid_to_string(

), one could also take the union of all of these ids and then return the size of the set. The vast majority of the time it'll be exactly the same as the number of response subscribers ...

[leeminju531]

I've searched methods to get the size of client ids.

for this, to my knowledge, a type of thedds_entity_t should be needed like below.

rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp

Lines 5103 to 5116 in 5b67ae4

	if (get_matched_endpoints(client.pub->enth, dds_get_matched_subscriptions, rds) < 0 ||
	get_matched_endpoints(client.sub->enth, dds_get_matched_publications, wrs) < 0)
	{
	RMW_SET_ERROR_MSG("rmw_service_server_is_available: failed to get reader/writer matches");
	return RMW_RET_ERROR;
	}
	// first extract all service ids from matched readers
	std::set<std::string> needles;
	for (const auto & rdih : rds) {
	auto rd = get_matched_subscription_data(client.pub->enth, rdih);
	std::string serviceid;
	if (rd && get_user_data_key(rd->qos, "serviceid", serviceid)) {
	needles.insert(serviceid);
	}

however, the function i made have no topic entity and i tried to find it (or the type of rmw_client_t , rmw_publisher_t ... ) using node and service_name provided by the input parameter of the function, but i'd not find it.

to implement the service count, there are two ways in my idea.

1. search all nodes (

   rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp

   Lines 4963 to 4966 in 5b67ae4

   	extern "C" rmw_ret_t rmw_get_node_names(
   	const rmw_node_t * node,
   	rcutils_string_array_t * node_names,
   	rcutils_string_array_t * node_namespaces)

   )
   and count the matched service.
   (

   rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp

   Lines 5387 to 5392 in 5b67ae4

   	extern "C" rmw_ret_t rmw_get_client_names_and_types_by_node(
   	const rmw_node_t * node,
   	rcutils_allocator_t * allocator,
   	const char * node_name,
   	const char * node_namespace,
   	rmw_names_and_types_t * sntyp)

   )

2. immediately return number_of_response_subscribers without comparing with number_of_request_publishers

   rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp

   Line 5233 in 5b67ae4

   return common_context->graph_cache.get_reader_count(mangled_topic_name, count);

What should I do? could you tell me if there's a good way I haven't found?

[eboasson]

It really depends on what you want to use it for, right?

If all it is ever going to be used for is for a user to get an indication of the number of clients/services from the CLI, then I'd say just look at the number of response subscribers and be done with it. (It would also make it really trivial to have a single implementation for counting clients and subscribers: all you need to do is pass in the topic name pattern you're looking for.)

If you do want to protect against all kinds of things that might happen if people start to write parts of the system in DDS without using the ROS libraries, I'd go a step further. If you want to have a well-defined notion of what number you actually return (there is no such thing as the "current" number of subscribers in a distributed system that's not in steady state) then the first problem is to define what exactly the returned value means.

I suspect it is, at least for now, the first case. In that case, it is probably best to keep it as simple as possible.

I just checked the related PRs to get a better sense of the goal, and I noticed that the comment in the RMW says "does not allocate memory". I'd say that's not true, e.g., just constructing the topic node likely involves memory allocation.

[leeminju531]

thank you for your information and sorry for misunderstanding

I suspect it is, at least for now, the first case. In that case, it is probably best to keep it as simple as possible.

Does the first case mean just looking at the number of response subscribers ?

[leeminju531]

i just saw comments again. i don't yet have a deep understanding of DDS and ROS.
my current estimate is below.

2. immediately return number_of_response_subscribers without comparing with number_of_request_publishers.
   

   rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp

   Line 5233 in 5b67ae4

   return common_context->graph_cache.get_reader_count(mangled_topic_name, count);

this just get the number of data readers for a DDS topic. it can't protect for those to use reading/writing parts in DDS without ROS as you mentioned.

1. rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp

   Lines 5387 to 5392 in 5b67ae4

   	extern "C" rmw_ret_t rmw_get_client_names_and_types_by_node(
   	const rmw_node_t * node,
   	rcutils_allocator_t * allocator,
   	const char * node_name,
   	const char * node_namespace,
   	rmw_names_and_types_t * sntyp)

in this case, it can get the numbers by Node which is a concept of ROS, so this is likely to protect from reading/writing parts in DDS without using the ROS

I suspect it is, at least for now, the first case. In that case, it is probably best to keep it as simple as possible.

this means the way which get the numbers using Node, right?

i'll post PR again as soon as you respond.

[mjcarroll]

@eboasson Friendly ping, I think we are waiting for a response here.

[eboasson]

Indeed ... sorry that I let it slip through the cracks ...

I think for the purpose of showing the number of clients and services in the ros2 tool, there is no harm in assuming that every part of the system is either built on ROS 2, or plays by the rules that ROS 2 follows. That is, that there are no other readers/writers for the service topics.

I also think it is fine to not worry about all the edge cases one I come up with (years of practice there ...), again, given the purpose. I would put a note in the documentation that the number is only reliable "in steady state", or something to that effect, just so people will realise there are limitations to these operations.

And then, just go with: return common_context->graph_cache.get_reader_count(mangled_topic_name, count);

Does that sound reasonable?

[leeminju531]

absolutely.
I fixed.

[leeminju531]

ping @eboasson, could you review this again?
I'm going to quickly update other PRs related to this upon your acceptance.

[eboasson]

If you consider it an error when the number of publisher & the number of subscribers don't match, then arguably you should here return number_of_response_subscribers, because the graph cache may already have changed here.

[eboasson]

LGTM, @leeminju531! And thanks for your patience with me ☺️

[methylDragon]

rmw interface changes have been merged

• Add rmw count clients, services rmw#334

Wait on:

• Add rmw_count_clients,services & test rmw_implementation#208

[methylDragon]

rmw interface changes have been merged

• Add rmw count clients, services rmw#334

[clalancette]

CI for this is in ros2/rmw_implementation#208 (comment)