Add design document discussing QoS reconfigurability

[ivanpauno]

Alternative to #296.
Based on feedback, the proposal leverages ROS parameters instead of proposing an ad-hoc file format.

[ivanpauno]

@jacobperron this is ready for a first pass.

[jacobperron]

Nice! I think the motivation section really helps focus the article.

I've left some wording suggestions and other opinions.

[mjeronimo]

I don't think the topic name is sufficient. It seems to me that topic+message_type would properly discriminate. In other words, not all message types using the same topic name should be expected to use the same QoS settings.

[ivanpauno]

In the most common use cases, each topic has only one message type.

Always including the topic type in the parameter sounds a bit verbose to me, and the "publisher/subscription id" trick mentioned later should cover this use case.
Anyways, including the topic type in the parameter name sounds like an acceptable proposal to me (though it will get problematic to make the parameters URI unique for a given fq node name, entity type, fq topic name and topic type).

[mjeronimo]

While it's true that each topic typically has only one associated message type, I don't think we should build in this assumption since, in general, ROS2 allows for multiple types per topic. Regardless of particular solution, there should be a way to differentiate between TopicA/MessageTypeA and TopicA/MessageTypeB when specifying QoS settings.

I'm wondering about using the ID approach (when creating a publisher)... I suppose this change would propagate to other functions, such as get_publishers_info_by_topic, where you'd need to augment the TopicEndpointInfo class to have a method to get this ID.

[ivanpauno]

I'm wondering about using the ID approach (when creating a publisher)... I suppose this change would propagate to other functions, such as get_publishers_info_by_topic

I don't think that propagating the concept to graph functions is a good idea.

Having an ad-hoc id only to be able to distinguish publishers/subscribers when re-configuring QoS might not be a great idea either.
If we think that distinguishing publishers in the same topic within a node is not necessary (see this comment), the concept of "id" can be deleted and the topic type can be added to the parameter name.

It's a bit problematic to add the topic type to the parameter URI though, e.g.:

  ros__parameters:
    qos_profiles:  # group for all the qos settings
      publisher:
        'my/fully/qualified/topic/name/here':
          'std_msgs.msg.String':

current addressing scheme:

ros_param://my/full/namespace/ny_node_name/qos_profiles.publisher.my/fully/qualified/topic/name/here.std_msgs.msg.String

alternative addressing scheme

ros_param://my.full.namespace.ny_node_name/qos_profiles/publisher/my/fully/qualified/topic/name/here.std_msgs.msg.String

well, that actually seems to work.

---

That will make the parameter names much more verbose, but I think that's ok.
The type cannot be optional, because that would imply declaring two parameters (except if we want to not use parameters for this, like in #296).

[mjeronimo]

Yes, it is more verbose, but I think it's consistent with the general idea of allowing for multiple types per topic name.

Regarding this:

If we think that distinguishing publishers in the same topic within a node is not necessary...

I would recommend starting with an addressing scheme that includes the type. Then, see if someone comes up with the requirement to distinguish between multiple publishers with the same topic and type in a single node. I'd like to hear what others have to say about this, though.

[mjeronimo]

@gbiggs One example is the ROS2 node's get_topic_names_and_types method, which returns a map of strings to vectors of strings:

  /// Return a map of existing topic names to list of topic types.
  /**
   * \sa rclcpp::Node::get_topic_names_and_types
   */
  RCLCPP_LIFECYCLE_PUBLIC
  std::map<std::string, std::vector<std::string>>
  get_topic_names_and_types(bool no_demangle = false) const;

Implying that each topic name can be associated with multiple types.

I don't want to get off-topic, but if this is indeed the case I think this also will break any tools that make the assumption of one topic name per one topic type. For example, rqt_bag, which I've been working on, has a row on the message timeline for each topic name. To properly support 1 topic name/n topic types, rqt_bag would need one row per combination.

[wjwwood]

My personal opinion after all of this is that if you use more than one type per topic, and you want to configure these, then you should use a named entity (so called "id").

---

I think it's somewhat unlikely that within a single node there are two entities with the same topic name but separate types. I would consider this a bug and discourage it. It is more likely, in my opinion, that two entities in different nodes have the same name but different types, even though I'd still discourage that. But in that situation you could still configure them separately as they are namespaced by the node name and namespace.

It is true that we allow for more than one type per topic, but that's not because we encourage it, but because of the nature of distributed discovery, since there might be remote entities with the same name but different types. That's the main reason for the discovery API allowing for that.

In fact, unless it changed, most DDS vendors will not allow you to create a topic with the same name but different types within a single participant. It only comes up when the conflicting entity is remote or at least in a different participant. So their behavior is a bit asymmetric on that point.

I think adding the type to the name of the parameter could work, but I'd prefer keeping the common case simple (each topic has a different type) and letting users use the named entity mechanism if they need to do the more complicated thing (entities share a topic name but not a type).

[ivanpauno]

My personal opinion after all of this is that if you use more than one type per topic, and you want to configure these, then you should use a named entity (so called "id").

My last question about this: Do we want the name of the entity ("id") to be complementary to the topic name (like described in the current proposal) or to be a replacement for it (i.e. the default "id" is the topic name and that can be overridden)?

More details in this comment.

I don't have a strong preference about either option, using the "id" as an optional complement to the topic name is more restrictive, but that can be seen as an advantage (less ways of people getting creative on how to provide qos profile overrides).

[wjwwood]

I don't have any opinion on id vs <entity>_id.

[ivanpauno]

Another alternative to solve the discussions here, here and here, I was thinking that having a "profile id" that can be independent of the topic name and type may be a good idea.

i.e.: the user provides a construction time an id

node->create_publisher(
  topic_name, qos,
  QosOverridingOptions::Builder()
    .id("my_profile_id")
    .policies({QoSPolicy::Reliability, QoSPolicy::History})
    .callback(callback)
    .build());

and then the file looks like:

/my/node/name:
  ros__parameters:
    qos_profiles:
      publisher:
        my_profile_id:
          reliability: reliable
          history: keep_last

In that way, the node's author can use the same profile name in several entities that must share the qos profile.
I would use as the default id the topic name (after expansion and remapping), but allowing the node author's to change it covers all possible use cases without the need of adding more levels of nesting (the type and an optional id).

The profile id could be remappable (using the same remapping rules that services and topic names) or not, I think it doesn't matter because all parameters are "private" to a node.
Caveat: topic names can be remapped, so if publisherA uses id1, and publisherB uses its topic name as the id and it gets remapped to id1, then you have a problem. But that can easily be avoided by the user doing systems integration.

[gbiggs]

In general we probably want to opt-out of infrastructure parameter events by default, not just the QoS ones.

[ivanpauno]

I think that's reasonable.

Do you mean?

• Turn off parameter events for read-only parameters.
• Turn off by default parameter events generated when declaring a parameter, but continue generating events when a parameter is set.
• Turn off by default all parameter events.

rclcpp currently depends on parameters events to make use_sim_time works, but that's fixable.

[gbiggs]

I mean to turn off parameter events for any parameters used internally by the ROS infrastructure, in the way these QoS ones are. I don't know how many of those there are at this time, though.

My reasoning is that a node author is going to care about events for parameters they declare themselves, but will find any additional events that they have to handle in some way to be noise.

[ivanpauno]

Ah, that makes sense.
I miss-read your previous comment.

[wjwwood]

I actually think we shouldn't do this. I think we should treat all of the parameters the same. It would be confusing to me as a user to see that some parameters cause events and others do not.

Answering the question "which parameters generate events" is not very trivial then.

That being said, if we do this, then it should be possible for the user to have parameters that do not generate events as well, i.e. we should dog food this feature.

[gbiggs]

I think the earlier approach combined with hidden parameters is more usable. As a bonus, hidden parameters will be useful in many other places in the ROS infrastructure.

[jacobperron]

Using a profile ID as an alternative seems viable and perhaps simpler. Instead of defaulting the ID to the topic name, I would rather make the profile ID a hard requirement if an author wants to make their publisher or subscription QoS configurable. For example:

node->create_publisher(
  topic_name, qos,
  QosOverridingOptions::Builder("my_profile_id")
    .policies({QoSPolicy::Reliability, QoSPolicy::History})
    .callback(callback)
    .build());

This avoids any issues with topic remapping.

We'll probably want tooling to be able to query what pub/subs are associated with what profile IDs (post-remapping).

[mjeronimo]

I agree with @jacobperron that using a profile ID but not defaulting to the topic name would a better way to go. It seems pretty straightforward and less error prone (remapping a topic name resulting in an expected change of QoS settings).

[gbiggs]

If the ID is specified in source code, then it would need to be scoped to the node. Otherwise we could have two nodes from different authors using the same ID and clashing.

[ivanpauno]

If the ID is specified in source code, then it would need to be scoped to the node. Otherwise we could have two nodes from different authors using the same ID and clashing.

Parameter names are all "private" to a node (and not global, like in ROS 1), so that's not an issue.

---

@jacobperron @mjeronimo My main motivation to use the "resolved" topic name as the default id is that it makes pretty easy to set the QoS when you want is to set the same profile for all nodes for all entities in the same topic:

/**:
  ros__parameters:
    qos_profiles:
      publisher:
        '/my/topic/name':
          reliability: reliable
          history_depth: best_effort

If desired, the qos parameters can be overridden for an specific node.
In the weird case that a node wants different qos parameters for the same topic, the you can override the id with something else.

But using the "resolved" topic name as the profile id makes configuration easy for the typical use case (and not impossible for strange use cases).

e.g.: node A publishes to /my_topic, node B publisher to /another_topic.
Both are remapped to /new_topic.
If the profile id was also modified to /new_topic, then you can configure the same qos easily using node wildcards (like the example above).

[jacobperron]

Defaulting to the topic name sounds okay too. The re-usability argument is convincing.
In any case, I think using the (optional) pub/sub ID as a QoS profile ID makes sense.

[wjwwood]

My personal opinion after all of this is that if you use more than one type per topic, and you want to configure these, then you should use a named entity (so called "id").

---

I think it's somewhat unlikely that within a single node there are two entities with the same topic name but separate types. I would consider this a bug and discourage it. It is more likely, in my opinion, that two entities in different nodes have the same name but different types, even though I'd still discourage that. But in that situation you could still configure them separately as they are namespaced by the node name and namespace.

It is true that we allow for more than one type per topic, but that's not because we encourage it, but because of the nature of distributed discovery, since there might be remote entities with the same name but different types. That's the main reason for the discovery API allowing for that.

In fact, unless it changed, most DDS vendors will not allow you to create a topic with the same name but different types within a single participant. It only comes up when the conflicting entity is remote or at least in a different participant. So their behavior is a bit asymmetric on that point.

I think adding the type to the name of the parameter could work, but I'd prefer keeping the common case simple (each topic has a different type) and letting users use the named entity mechanism if they need to do the more complicated thing (entities share a topic name but not a type).

[wjwwood]

I actually think we shouldn't do this. I think we should treat all of the parameters the same. It would be confusing to me as a user to see that some parameters cause events and others do not.

Answering the question "which parameters generate events" is not very trivial then.

That being said, if we do this, then it should be possible for the user to have parameters that do not generate events as well, i.e. we should dog food this feature.

[wjwwood]

You should at least set the QoS parameters for a single topic atomically, but honestly, maybe all of them should be set atomically. What if you wanted publisherA.history == subscriptionB.history?

So I agree we need a declare_parameters_atomically().

[ivanpauno]

What if you wanted publisherA.history == subscriptionB.history?

That's a good question, but I think in that case the user can control in which order they create entities:

publisher = node->create_publisher(
  "my_topic",
  my_pub_default_qos,
  [] (const QoSProfile & qos) -> bool {
      return qos_profile_is_valid(qos);
  });

subscription = node->create_subscription(
  "my_topic",
  my_sub_default_qos,
  subscription_callback
  [required_history=publisher->get_qos().get_history()] (const QoSProfile & qos) -> bool {
      return qos.get_history() == required_history && qos_profile_is_valid_2(qos);
  });

Declaring all qos parameters atomically sounds like a valid alternative, but I'm not sure when that should happen.

[ivanpauno]

FYI (to everyone in the thread), I have opened some PRs implementing the proposal:

• rclcpp: Qos configurability rclcpp#1408
• rclpy: Qos configurability rclpy#635
• demo: Add demo of how to use qos overrides demos#474

[jacobperron]

The document LGTM. I think most (or all?) comments have been addressed. I think we can merge this (maybe with a second approval, perhaps@mjeronimo). We can always make future amendments and additions where necessary.

[ivanpauno]

Merging, thanks @gbiggs @jacobperron @mjeronimo @wjwwood @dejanpan for the feedback in #296 and here.

If there's any extra comments I can address them in a follow-up.