Qos configurability

[ivanpauno]

Prototype of ros2/design#300.

Depends on #1410.
Diff omitting #1410: https://github.com/ros2/rclcpp/pull/1408/files/ec90cb53091e355dcf194c944fcaf239c5fa6699..9197a8f2bfe9da6e54f3e30779b3b1c0236339f2.

---

Not quite ready yet.
Things to solve before merging:

1. Get the final topic name before creating a publisher/subscription

   • rcl support for expand and remap.
   • Wrap the rcl function in an rclcpp method for Node (PR).

2. create_subscription and create_publisher can be called without a parameters interface

   I'm not quite sure what to do about this.
   If the user wants to only provide a topics interface (without using qos overrides), that should be ok.
   Maybe fail when qos overrides are enabled and a parameters interface isn't provided?

   • Add create_publisher/create_subscription overload taking both a topics interface and a parameters interface (4d6ad65).
   • Handle error when qos overrides are enabled and a parameters interface isn't provided: A warning is being logged in this case.
   • See places that need special handling in rclcpp code in create_publisher.hpp/create_subscription.hpp comments.
     Added notes, this can be resolved in a follow-up.

3. Improve QosOverridingOptions API

   Not a blocker for merging this, but we should have minimal API that won't be deprecated.
   Maybe QosOverridingOptions could use the builder pattern or the parameter idiom to make things easier.
   I definitely also want either builder pattern/parameters idiom in PublisherOptions/SubscriptionOptions, but that can be done in a follow up.

4. Test coverage

   • Improve test coverage (note: this can still be improved)

5. Move things that can be shared between rclcpp/rclpy to rcl/rmw

   • Conversions between policy values and strings (PR).
   • Extend rmw_qos_policy_kind_t to cover all possible policies (and move that to a more appropriate header). Define rclcpp::QosPolicyKind values based on that. Conversions to/from strings can be implemented there (PR).
   • Use the new rmw functions in this PR (77eec6d).

6. Support in other client libraries

   • Support in rclpy (PR)

[ivanpauno]

FYI @jacobperron

[ivanpauno]

I was thinking that if QosOverridingOptions is an argument separated of PublisherOptions/SubscriptionOptions, this runtime check can we avoided (in that case, not providing a parameters interface while passing QosOverridingOptions can be detected at build time).

@wjwwood @jacobperron opinions?

(edit) See #1408 (comment) for a more clear description of the issue.

[ivanpauno]

I still have to write a bunch of tests before merging, but this is otherwise ready for review.

[jacobperron]

I was thinking that if QosOverridingOptions is an argument separated of PublisherOptions/SubscriptionOptions, this runtime check can we avoided

I'm not sure I follow exactly. It what situation will the node not have a parameters interface? Does this mean that someone has created a Node-like object that doesn't implement that interface? I think I prefer bundling the QoS overrides with the pub/sub options to keep the API tidy, but I don't have a strong preference.

[ivanpauno]

I'm not sure I follow exactly. It what situation will the node not have a parameters interface? Does this mean that someone has created a Node-like object that doesn't implement that interface? I think I prefer bundling the QoS overrides with the pub/sub options to keep the API tidy, but I don't have a strong preference.

You can currently do the following:

rclcpp::create_publisher(node_topics_interface, "my/topic/name", qos, options);
//                              ^^^^^^^
//                                ||------------------Type `rclcpp::node_interfaces::NodeTopicsInterface`

(same for create_subscription)
Here an example.

In that case, NodeTopicsInterface doesn't provide a NodeParametersInterface.

The current approach is that a warning is logged if you wanted to generate qos parameters and didn't provide a parameters interface, but I would prefer if the error is detected at compile time.
If create_publisher takes QosOverridingOptions as a separated argument, that can be achieved.

[ivanpauno]

I noticed that this can cause a parameter already declare error.

When creating two subscriptions in the same topic, the same qos might be desired for both (i.e. not passing an id to disambiguate) , and in that case the second creation will fail.
Maybe the QosOverridingOptions should have a "parameters already declared" option, that uses get_parameter instead of declare_parameter.

"/clock" is an special case, because we can create many time sources with different subscriptions, and we don't know what's the first one (I think that never happens in a real use case, but it's happening in tests).
Here is how we handle that for use_sim_time (by the way, there's a TOCTTOU race condition there).
I'm not sure what's the right way to handle this last case, @jacobperron @wjwwood any ideas?

[jacobperron]

What about wrapping this in a try-catch and if we catch a ParameterAlreadyDeclaredException then use get_parameter?

[jacobperron]

Perhaps the race you found can be fixed in a similar way.

[ivanpauno]

What about wrapping this in a try-catch and if we catch a ParameterAlreadyDeclaredException then use get_parameter?

That sounds ok to me, but maybe in the general case it should be an error (the /clock case sounds pretty exceptional).

Perhaps the race you found can be fixed in a similar way.

👍

[jacobperron]

I guess we still need to resolve this. In the general case, how should users workaround this issue?

IIUC, the situation arises if I do something like this:

...
 rclcpp::QosOverridingOptions qos_overrides{rclcpp::QosOverridingOptions::kDefaultPolicies};
 auto sub_options = rclcpp::SubscriptionOptions();
 sub_options.qos_overriding_options = qos_overrides;
 auto sub1 = rclcpp::create_subscription<test_msgs::msg::Empty>(
    node, "same_topic", foo_qos, foo_callback, sub_options);
 auto sub2 = rclcpp::create_subscription<test_msgs::msg::Empty>(
    node, "same_topic", bar_qos, bar_callback, sub_options);
...

At first glance, I would expect things to work without issue; the user shouldn't have to worry about parameters already being declared since it's an implementation detail. But, then it looks a little confusing since sub1 and sub2 have different "default" QoS settings, but the same override abilities. And configuring one QoS policy should modify both. It seems a bit strange, and is probably a exceptional use-case, but seems like something we could easily support.

Correct me if I'm wrong or missing something.

[ivanpauno]

As I understand, even if we introduce the proposed functions we still rely on the user knowing about this pitfall

It doesn't sound like a pitfall in get_subscription_qos_from_overrides (or with qos_overriding_options.declare_parameters = false), as in that case the user would be explicitly requesting: "take this qos profile and always replace the policies that have a declared parameter with the parameter value".

Ok, I understand that using (read-only) parameters is going to be part of the interface to the user and not a hidden implementation detail

Yeah, that was the result of some discussions in ros2/design#296.

Given that, could the interface be simplified a bit in its operations? The user could perhaps:

That sounds ok to me.
It's a bit too verbose in the case you want to only create one subscription/publisher in that topic, so I wanted to provide extra sugar on top of that to avoid the verbosity.

[mjeronimo]

True, it may be more lines of code, but I think the programming model of: Declare, Override, Create, Override, Create is simpler and easier to understand than DeclareAndOverride, Create, OverrideWithSavedQoS, Create

I'm assuming that this operation is returning a saved QoS from the initial DeclareAndOverride (could be wrong about that)

rclcpp::QoS qos = get_subscription_qos_from_overrides(
  node_parameters, node_topics->resolve_topic_name(topic));  // qos overriding options aren't needed here

[jacobperron]

Here's a straw-man example of what I think we should do here:

if (!parameters_interface.has_parameter()) {
  auto value = parameters_interface.declare_parameter(
    param_name.str(), get_default_qos_param_value(policy, qos), descriptor);
  ::rclcpp::detail::apply_qos_override(policy, value, qos);
} else {
  auto value = parameters_interface.get_parameter(param_name.str());
  // TODO: We would need to add the "is_default()" API, but it should be easy
  if (value.is_default() && value != get_default_qos_param_value(policy, qos)) {
    // Log or output an error here (optionally, we could throw an exception instead)
    std::cerr << "Default setting for QoS policy " << policy << " is being ignored. Previously overridden by another subscription." << std::endl; 
  }
  ::rclcpp::detail::apply_qos_override(policy, value, qos);
}

[jacobperron]

To clarify, I think it is a pitfall in the sense that the user must be aware that they can't write code like my example (#1408 (comment)), furthermore, they must also be aware of these additional functions to workaround the issue (which are essentially setting the default QoS profile to the same value as a previously declared sub/pub).

[ivanpauno]

cab74d8 takes the approach of #1408 (comment).

[ivanpauno]

Force pushed to resolve conflicts with master.

[jacobperron]

The current approach is that a warning is logged if you wanted to generate qos parameters and didn't provide a parameters interface, but I would prefer if the error is detected at compile time.
If create_publisher takes QosOverridingOptions as a separated argument, that can be achieved.

Creating a separate arg for QosOverridingOptions is fine to me. I don't have a strong feeling either way.

[jacobperron]

Left some more minor comments, but LGTM overall.

I think it would be useful to have an example (perhaps in the ros2/demos repository) demonstrating how to use this feature. Maybe just a modified talker demo that has some QoS policies that are configurable and exercises the callback feature; what do you think?

Regarding changing QosOverridingOptions to be a separate argument from the publisher/subscription options, I don't have a strong opinion. It would be good to hear from @mabelzhang and/or @wjwwood.

[ivanpauno]

Coverage of added lines isn't good, I'm going to improve the tests before merging.

[ivanpauno]

Local coverage testing: new files have 100% line coverage, new lines in modified files are completely covered.

Last round of CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[wjwwood]

🎉 This has turned out to be a really cool feature, imo.