Adding required structs and methods to get a list of publishers or subscribers with their respective qos

[jaisontj]

Includes creating relevant data structures to enable the feature mentioned here. Relevant information about implementation is discussed here

Summary:

• Added two new structs
  • rmw_participant_qos_profile_t to store participant name and their qos policy in one struct
  • rmw_participants_t to store a list of rmw_participant_qos_profile_t
• Added two new functions:
  • rmw_participant_qos_profile_allocate
  • rmw_participant_qos_profile_free
• Tests for the above two functions

Note: colcon build --packages-up-to rmw && colcon test --packages-up-to rmw && colcon test-result --verbose --test-result-base build/rcl passes without any errors or failures.

[thomas-moulard]

Please add test

[emersonknapp]

lgtm

[ivanpauno]

I don't get why rmw_get_qos_for_publishers and rmw_get_qos_for_subscribers return a list of rmw_qos_profile_t (with the participant name), instead of just returning one.

IMO, both of them should return only one rmw_qos_profile_t, using the data discovered by the rmw_node_t passed as an argument.

[ivanpauno]

I don't get why rmw_get_qos_for_publishers and rmw_get_qos_for_subscribers return a list of rmw_qos_profile_t (with the participant name), instead of just returning one.

IMO, both of them should return only one rmw_qos_profile_t, using the data discovered by the rmw_node_t passed as an argument.

Sorry, now I see why you need a list (there may be many publishers/subscription on each topic, in different nodes).
Please, always refer to the node name and to the node.
Participant and its name are not a concept in ROS 2.

[jaisontj]

I don't get why rmw_get_qos_for_publishers and rmw_get_qos_for_subscribers return a list of rmw_qos_profile_t (with the participant name), instead of just returning one.
IMO, both of them should return only one rmw_qos_profile_t, using the data discovered by the rmw_node_t passed as an argument.

Sorry, now I see why you need a list (there may be many publishers/subscription on each topic, in different nodes).
Please, always refer to the node name or node.
Participant and its name are not a concept is ROS 2.

• I used Participant as a blanket term for Publishers and Subscribers. Since rmw_participant_qos_profile_t and rmw_partiticpants_t can be used for both publishers and subscribers.
• Since each topic can have multiple publishers/subscribers, the participant_name (aka publisher_name or subscriber_name) is important to differentiate between them when displaying from ros2cli

For eg: the command ros2 topic info my_topic should print

Topic: my_topic

Publisher count: 2

Publisher Name: _ros2cli_daemon_0
QoS Profile: 
    Reliability: RMW_QOS_POLICY_RELIABILITY_RELIABLE
    Durability: RMW_QOS_POLICY_RELIABILITY_RELIABLE
    Lifespan: 1000000000 nanoseconds
    Deadline: 2000000000 nanoseconds
    Liveness: RMW_QOS_POLICY_LIVELINESS_MANUAL_BY_TOPIC
    Liveness Lease Duration: 50000 nanoseconds

Publisher Name: talker
QoS Profile: 
    Reliability: RMW_QOS_POLICY_RELIABILITY_RELIABLE
    Durability: RMW_QOS_POLICY_RELIABILITY_RELIABLE
    Lifespan: 1000000000 nanoseconds
    Deadline: 2000000000 nanoseconds
    Liveness: RMW_QOS_POLICY_LIVELINESS_MANUAL_BY_TOPIC
    Liveness Lease Duration: 5 nanoseconds

Subscriber count: 0

What do you suggest is a better term to Participants as I see that Participants can be misleading?

[ivanpauno]

Topic: my_topic

Publisher count: 2

Publisher Name: _ros2cli_daemon_0
QoS Profile:
Reliability: RMW_QOS_POLICY_RELIABILITY_RELIABLE
Durability: RMW_QOS_POLICY_RELIABILITY_RELIABLE
Lifespan: 1000000000 nanoseconds
Deadline: 2000000000 nanoseconds
Liveness: RMW_QOS_POLICY_LIVELINESS_MANUAL_BY_TOPIC
Liveness Lease Duration: 50000 nanoseconds

Publisher Name: talker
QoS Profile:
Reliability: RMW_QOS_POLICY_RELIABILITY_RELIABLE
Durability: RMW_QOS_POLICY_RELIABILITY_RELIABLE
Lifespan: 1000000000 nanoseconds
Deadline: 2000000000 nanoseconds
Liveness: RMW_QOS_POLICY_LIVELINESS_MANUAL_BY_TOPIC
Liveness Lease Duration: 5 nanoseconds

Subscriber count: 0

It should say node name, Publishers or Subscriptions don't have a name (the name you're printing is the node name).
You can also create many publishers/subscriptions in a node in the same topic (and with different qos settings, or not).
When that happens, you don't have a way to distinguish between one publisher/subscription and the other.
It's a strange use case, but it's possible.

[jaisontj]

It should say node name, Publishers or Subscriptions don't have a name (the name you're printing is the node name).
You can also create many publishers/subscriptions in a node in the same topic (and with different qos settings, or not).
When that happens, you don't have a way to distinguish between one publisher/subscription and the other.
It's a strange use case, but it's possible.

Ah, I see what you mean.

Alright, so participant_name can be renamed to node_name.
Whats your take on renaming rmw_participants_t and rmw_participant_qos_profile_t? Since rmw_nodes_t might be misleading since we already have rmw_node_t. The same goes for rmw_node_qos_profile_t. Ideally, an umbrella term for publishers and subscribers is what we want.

[ivanpauno]

Whats your take on renaming rmw_participants_t and rmw_participant_qos_profile_t? Since rmw_nodes_t might be misleading since we already have rmw_node_t. The same goes for rmw_node_qos_profile_t. Ideally, an umbrella term for publishers and subscribers is what we want.

We currently have two types of graph introspection functions:

• The ones that list all available topics/services (https://github.com/ros2/rmw/blob/master/rmw/include/rmw/get_service_names_and_types.h, https://github.com/ros2/rmw/blob/master/rmw/include/rmw/get_topic_names_and_types.h).
• The ones that list entities by node (https://github.com/ros2/rmw/blob/master/rmw/include/rmw/get_node_info_and_types.h).

I think that getting the list of publishers and subscribers with its qos enters in the second group, we could have a *_by_node function.
In that case, it's not necessary to retrieve the node name in the output, as it was part of the arguments. We could just add a rmw_qos_profiles_t, which is a list of the current rmw_qos_profile_t.

Does that sound reasonable?

[jaisontj]

I think that getting the list of publishers and subscribers with its qos enters in the second group, we could have a *_by_node function.
In that case, it's not necessary to retrieve the node name in the output, as it was part of the arguments. We could just add a rmw_qos_profiles_t, which is a list of the current rmw_qos_profile_t.

I am a little confused. The only data available is the handle to rmw_node_t and the topic_name, with this we need to find all publishers and subscribers (along with their QoS Policies) to that topic_name. In this case aren't we also getting publishers and subscribers from other nodes as well?
How can node name not be part of the arguments when we do not know of the other nodes involved?

[ivanpauno]

@jaisontj I'm thinking in something like this:

rmw_get_publisher_qos_by_node(
  const rmw_node_t * node,
  rcutils_allocator_t * allocator,
  const char * node_name,
  const char * node_namespace,
  rmw_qos_profiles_t * qos_profiles);

I think that matches better the current API.

And actually, I would go with a method like this:

rmw_get_publisher_names_types_and_qos_by_node(
  const rmw_node_t * node,
  rcutils_allocator_t * allocator,
  const char * node_name,
  const char * node_namespace,
  bool demangle,
  rmw_names_and_types_t * topic_names_and_types,
  rmw_qos_profiles_t * qos_profiles);

rmw_names_and_types_t and rmw_qos_profiles_t will have in the same index info of the same publisher (it's not possible to ensure that if it's separated in two calls).

---

If you want to go with something similar to your first proposal, I would rather go with:

rmw_ret_t
rmw_get_qos_for_publishers(
  const rmw_node_t * node,
  const char * topic_name,
  const char * type_name,
  bool demangle,
  rmw_node_names_t * node_names,
  rmw_qos_profiles_t * qos_profiles);

I added type_name, as you probably also want to know it.
demangle option is taken by all graph API functions, it specifies if ROS topic and type name conventions are used or not.
rmw_node_names_t would be a list of node names (and namespaces).
rmw_qos_profiles_t will have the same indexing as rmw_node_names_t.

[jaisontj]

Thank you for this @ivanpauno. After looking at your suggestions, I have slight modifications in mind

 rmw_get_publisher_names_and_qos_for_topic(
   const rmw_node_t * node,
   rcutils_allocator_t * allocator,
   const char * topic_name,
   rcutils_string_array_t * node_names,
   rmw_qos_profiles_t * qos_profiles);

I've renamed the function from *_by_node to *_for_topic since we are getting a list of all publishers to a topic, regardless of which node it is a part of.

Parameters:

• allocator is required to allocate memory for node_names and qos_profiles since we do not know the size of the array before hand.
• topic_name: the topic name to get the list of publishers for; this must be a fully qualified topic_name
• node_names and qos_profiles: which have the same indexing for participants

I've taken off:

• type_name as I'm not sure why we need it. We can get all the data we need from the topic_name and rmw_node_t (from the data inside topic_cache). Moreover, when this call originates from the cli, the command would be ros2 topic info <topic_name>.
• demangle: similar to how rmw_count_publisher does it, could we not specify that the topic name must be fully qualified; like here (linking to rcl as the rmw package has no comments/documentation for the functions)?

Let me know if I've gotten this right. If so, I will update this PR with the same :)

[emersonknapp]

@ivanpauno I think some of the confusion here is that we are querying for information for everything except this node. There isn't a precedent in the graph API of "information_by_topic", but we need that info for this new functionality.

@jaisontj For naming consistency, it may make more sense to name it rmw_get_qos_profiles_by_topic since it seems the _by_X naming has precedent, whereas the _for_X doesn't.

Note: the name convention in ROS2 is subscription not subscriber. There is no subscriber in ROS2. Unfortunately it seems that a handful of functions that mention subscriber have made it into rmw. Lets make sure not to continue the trend.

So to summarize that, I think you're on the right track and we're looking at

rmw_get_publisher_names_and_qos_by_topic
rmw_get_subscription_names_and_qos_by_topic

@ivanpauno does that sound about right?

EDIT: more clarification - the purpose of this code change is to gain information about the rest of the running system, with the goal of later exposing QoS mismatch when we try to subscribe to a topic whose publisher has an incompatible QoS to our subscription.

[mm318]

Even though ros2/rclcpp#960 (which supersedes ros2/rclcpp#959) is out for review, are we we ready to merge the "rcl and below" subset now?

[wjwwood]

Mostly looks good to me, there was only one thing that I think must be fixed (related to not checking if allocate fails in one case), but there were lots of other comments that may or may not need to be addressed.

[wjwwood]

Sounds like they are allowed? Why not an error case?

[mm318]

I am not sure what error code to use in this case (a generic RMW_RET_ERROR? RCL_RET_TOPIC_NAME_INVALID migrated over to rmw?).

Also, this function shouldn't be used for detecting the existence of a topic; that feels like it is encroaching on the purpose of rmw_get_topic_names_and_types().

[wjwwood]

Sorry this comment doesn't make since if "non-existent topic" just means a topic that isn't on the graph. I was still thinking an empty string, in which case it should create an error, just like if it violated any of the other rules in the topic name rules set.

I think this can be resolved without changes. Though a follow up question is what happens if the value for topic_name violates one of the topic name rules, e.g. empty string, starts with a number, etc.

There is already this function:

rmw/rmw/include/rmw/validate_full_topic_name.h

Lines 37 to 90 in 813b94d

	/// Determine if a given fully qualified topic name is valid.
	/** Validity of a FQN for topic is determined based on rules defined here:
	*
	* http://design.ros2.org/articles/topic_and_service_names.html
	*
	* Note that this function expects any URL suffixes as described in the above
	* document to have already been removed.
	*
	* If either the C string or validation_result pointer are null, then
	* `RMW_RET_INVALID_ARGUMENT` will be returned.
	* The topic_name should be a valid, null-terminated C string.
	* The validation_result int pointer should point to valid memory so a result
	* can be stored in it as an output variable.
	* The invalid_index size_t pointer should either point NULL or to valid memory
	* so in the event of a validation error, the location in the input string can
	* be stored therein.
	* If NULL is passed in for invalid_index, it will be not be set.
	*
	* The invalid_index will not be assigned a value if the topic is valid.
	*
	* The int which validation_result points to will have a one of a few possible
	* results values (defined with macros) stored into it:
	*
	* - RMW_TOPIC_VALID
	* - RMW_TOPIC_INVALID_IS_EMPTY_STRING
	* - RMW_TOPIC_INVALID_NOT_ABSOLUTE
	* - RMW_TOPIC_INVALID_ENDS_WITH_FORWARD_SLASH
	* - RMW_TOPIC_INVALID_CONTAINS_UNALLOWED_CHARACTERS
	* - RMW_TOPIC_INVALID_CONTAINS_REPEATED_FORWARD_SLASH
	* - RMW_TOPIC_INVALID_NAME_TOKEN_STARTS_WITH_NUMBER
	* - RMW_TOPIC_INVALID_TOO_LONG
	*
	* The result value can be converted to a description with the
	* rmw_full_topic_name_validation_result_string() function.
	*
	* The `RMW_TOPIC_INVALID_TOO_LONG` is guaranteed to be checked last, such
	* that if you get that result, then you can assume all other checks succeeded.
	* This is done so that the length limit can be treated as a warning rather
	* than an error if desired.
	*
	* \param[in] topic_name topic name to be validated
	* \param[out] validation_result int in which the result of the check is stored
	* \param[out] invalid_index size_t index of the input string where an error occurred
	* \returns `RMW_RET_OK` on successfully running the check, or
	* \returns `RMW_RET_INVALID_ARGUMENT` on invalid parameters, or
	* \returns `RMW_RET_ERROR` when an unspecified error occurs.
	*/
	RMW_PUBLIC
	RMW_WARN_UNUSED
	rmw_ret_t
	rmw_validate_full_topic_name(
	const char * topic_name,
	int * validation_result,
	size_t * invalid_index);

[ivanpauno]

I see rcl and downstream PRs ready for another round of CI, @wjwwood what do you think?

[ivanpauno]

Just rmw_fastrtps_cpp:

• Linux
• Linux-aarch64
• macOS
  Including all the stubs:
• Windows

[ivanpauno]

@mm318 Can you check the macOS compiler warnings? https://ci.ros2.org/job/ci_osx/7362/warnings11Result/

[mm318]

Hmm, interesting this post here and these examples in rcl here and here seem to say these warnings would be false positives with GCC, and would not happen with Clang in the ROS2 codebase.

Anyway, I'll implement the solution found here: https://stackoverflow.com/a/9293087

[ivanpauno]

Hmm, interesting this post here and these examples in rcl here and here seem to say these warnings would be false positives with GCC, and would not happen with Clang in the ROS2 codebase.

Anyway, I'll implement the solution found here: https://stackoverflow.com/a/9293087

That's strange, ignoring the warning sounds reasonable.

[ivanpauno]

• macOS

[ivanpauno]

Merged!! Thanks for your work @mm318 @jaisontj !!

[mm318]

Thank you for merging, @ivanpauno! Let's continue the conversation at ros2/rclcpp#960.

[jaisontj]

Thank you @mm318 for following up on these :)