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?

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

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

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.

👍

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.

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.

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

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);
}

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).

cab74d8 takes the approach of #1408 (comment).