search

ros2 / design

Design documentation for ROS 2.0 effort
http://design.ros2.org/
Apache License 2.0
224 stars 192 forks source link

QoS profile files proposal #296

Closed ivanpauno closed 4 years ago

ivanpauno commented 4 years ago

Fixes https://github.com/ros2/design/issues/280.

I have written down the ideas I have in a document, and I have included the points that were raised in previous discussions.

ivanpauno commented 4 years ago

For example, using a well-known set of ROS parameters was proposed. We should consider (and document in this design doc) the pros/cons of this approach versus a QoS specific file.

Also, what about providing individual QoS settings on the command-line (instead of bundling them in a file)? The idea is similar to what is possible with parameters; using parameters as a back-bone would give us the implementation for free. I haven't thought about it too much yet, but seems like it's worth considering.

@jacobperron I have added a comment about those alternatives in the document, see https://github.com/ros2/design/pull/296/commits/031543ef344ee5e84069437b597d4fd88f266da7

ivanpauno commented 4 years ago

@jacobperron @gbiggs I have addressed all your comments, can you take another look? When you think this is ready, I will ask the rest of the team for feedback.

gbiggs commented 4 years ago

I think it's ready. There may some small issues left but we can clear those up as the proposed implementation is worked on.

gbiggs commented 4 years ago

@ivanpauno What's your plan for moving forward with this? Do you plan to open it up to wider comments as a proposal, or are you going to take it as a design document and begin prototyping an implementation? If the latter, then I think you need to make it more concrete first. There are still a lot of coulds and shoulds in it.

ivanpauno commented 4 years ago

@ivanpauno What's your plan for moving forward with this? Do you plan to open it up to wider comments as a proposal, or are you going to take it as a design document and begin prototyping an implementation?

That's a great question. I'm planning to do the following:

If the latter, then I think you need to make it more concrete first. There are still a lot of coulds and shoulds in it.

I agree. I tend to left a lot of coulds and shoulds, but it's confusing for the reader.

I will reorganize the document before starting a prototype.

gbiggs commented 4 years ago

I think refining those coulds and shoulds into musts and shalls while you produce the prototype is a fine approach. I don't think there's a need for a perfect document before starting to implement something to confirm decisions.

ros-discourse commented 4 years ago

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/configuring-qos-profiles-from-an-external-file-proposal/15991/1

ivanpauno commented 4 years ago

@wjwwood can you share your thoughts on the matter?

dabonnie commented 4 years ago

As discussed in the middleware WG meeting, are you aware of the QoS policy definitions in rosbag2?

this feature allows a user to specify the QoS settings for any recorded topic.

ivanpauno commented 4 years ago

As discussed in the middleware WG meeting, are you aware of the QoS policy definitions in rosbag2?

I wasn't aware of that particular feature. I was aware of other ad-hoc mechanisms in different repositories, I've listed all of them in: https://github.com/ros2/design/pull/296/commits/bd5b4d37c70d693ea4b478964954a2597fe7b61d

I think that the community will be benefited if we have a single and standard mechanism to allow configuring QoS profiles. There might be some places where ad-hoc mechanisms still become handy, e.g.: configuring QoS of gazebo ros plugins in the sdf might still be useful.

The ros1_bridge also has an issue related to QoS configurability, and the idea was not to add another ad-hoc mechanism but wait until we add a general one (see here).

ivanpauno commented 4 years ago

I'm copying notes from the middleware working group meeting, thanks everybody for the feedback! If some of the notes are wrong, please let me know and I will correct them. I would like to receive more input about the topic.

Notes:

@wjwwood About using parameters, I would like to see a concrete proposal that can satisfy the following requirements:

dejanpan commented 4 years ago

@ivanpauno replying to https://github.com/ros2/design/pull/296#issuecomment-689774530:

Having to configure QoS profiles is a problem that needs to be solved in all nodes. Parameters are better for ad-hoc configurations of a node.

I am not sure that I understand what you mean by that. Are you saying that there are nodes that do not need any parameterization? If yes I agree with that but what is the penalty if all nodes get parameterizied with the QoS profiles?

The current parameters API isn't great for representing a "group" of things, e.g.: There's no parameter callback to check if the group is consistent or not. One parameter cannot represent the whole QoS profile.

I would first like that we agree that QoS settings in a node are no different than any other parameter that the node might have. Namely as an example, a camera node that sets the frequency at which an image is published out of the node, is almost the same thing as setting the reliability at which this node sends out those same images. Or consider Deadline - that is a totally node specific configuration that will be different for a camera node publishing messages at 25Hz and lidar node publishing messages at 10Hz.

Now if I am able to convince you that, then I would like to say that there are definitely use cases for parameterizing a "group" of things in the nodes. For instance, if you have 4 cameras on your robot car and you are processing the data from these 4 cameras. The downstream nodes will use the same configuration for queue size, debayerization, rectification, image size, ... With this logic, if "parameters API isn't great for representing a "group" of things", then that is really a flaw that should be fixed in the implementation of ROS 2 parameters.

Parameters are reconfigurable, QoS profiles don't need to be so (and allowing so would be quite complex).

We implemented the read only parameters: https://github.com/ros2/rclcpp/pull/495/commits.

We already have several different ways to configure things, this seems like adding more.

I'm not sure if we have several ways of configure QoS profiles. The only one that you can always apply is to specify the QoS profile in code. DDS profiles file can be used in DDS based rmw implementations but:

What I mean is that we already have 3 different ways to configure ROS 2 applications which you list in the design document:

Now you are adding the fourth one and this complicates things in that the other 3 ways will not go away and it is also impossible to prohibit users to not use the above 3 ways to also configure QoS settings via them.

Look what user now needs to do in their application to get through the configuration phase:

  1. parse command line arguments
  2. declare/get parameter
  3. check DDS settings as set externally by the underlying DDS implementation
  4. Some logic to check whether QoS settings were set via your approach and then to read them out (for e.g. printing or checking if the QoS settings are correct)

I think that this is insanely complicated and exactly error prone because there is too many knobs to turn.

ivanpauno commented 4 years ago

I am not sure that I understand what you mean by that. Are you saying that there are nodes that do not need any parameterization?

What I mean is that all nodes need some parameterization of QoS policies. Particularly: history kind, history depth and reliability should quite frequently be parameterized (I think we're saying the same thing).

If yes I agree with that but what is the penalty if all nodes get parameterizied with the QoS profiles?

The proposed issue is opt-in and by default it is not enabled, so there shouldn't be any surprises except the node's author consciously activated the feature. After the feature is enabled, I don't think it will be any "runtime" penalty nor anything like that. The file will be loaded when init is called, and then an easy look-up in a data structure will be done when creating a pub/sub/etc. I'm not sure if the question was referring to that.

About the difference between parameters and QoS settings:

It's true that qos settings can be considered as any other parameter. The main issue with using parameters is that you cannot make them look ergonomic, or at least, I haven't come up with something that is ergonomic. More on that later ...

Now if I am able to convince you that, then I would like to say that there are definitely use cases for parameterizing a "group" of things in the nodes With this logic, if "parameters API isn't great for representing a "group" of things", then that is really a flaw that should be fixed in the implementation of ROS 2 parameters.

I was thinking the same while I was writing my original comment, but I don't think that solving that problem will be enough.

We implemented the read only parameters: https://github.com/ros2/rclcpp/pull/495/commits.

Yeap!

About the current ways of configuring QoS settings:

Command line arguments ROS parameters

I don't think it will be confusing to have a mechanism specifically targeted to solve the QoS settings parameterization problems + this two "general" mechanisms to parameterize other things, but it might be arguable.

Combining system default QoS with vendor specific QoS profiles.

I think this doesn't solve the problem. If your node has publisher A and publisher B and you set both to the system default QoS profile, then both will have the same QoS settings.

If we make this mechanism more powerful, following the ideas of https://github.com/ros2/rmw_fastrtps/pull/335, I don't think that will look ergonomic. If the standard mechanism to parameterize QoS settings is a DDS QoS profiles, our middleware abstraction would be too leaky. Unfortunately when you want to tweak your system, you have to learn some details of your rmw implementation and be able to understand and tweak it. But if you need DDS specifics to do something that is frequently needed, I think we would be leaking details that we're trying to abstract away too early (if you only know ros, what is the data reader QoS?).

This mechanism cannot be applied to non-DDS rmw implementations, which I think it's not ideal.

IMO having DDS profile files as the only mechanism to solve the problem ergonomically is not an option, but I understand others might disagree.

Look what user now needs to do in their application to get through the configuration phase:

If we never use arguments and parameters to configure QoS, that won't be in the list of things you need to check. There are a few ad-hoc mechanisms to configure QoS here and there, but most places lack a way to parameterize QoS settings. Ideally, you would only need to check points 3. and 4. (and not points 1. and 2.).

I think that I need a more compelling argument about why I think that to manually parameterize QoS settings (i.e. not having a standard mechanism for it) don't work and why using parameters (even with the automatic creation of a known parameter subset) don't look ergonomic. I will try to write a full post about that.

ivanpauno commented 4 years ago

Ok I will try to answer why I think that we need an standard "built-in" mechanism to parameterize QoS settings and why parameters doesn't look ergonomic.

The need of a built-in mechanism

Suppose a node needs to parameterize the reliability/history kind/history depth of a publisher or subscription (which are required to be parameterized quite often):

So node A has only one publisher and one subscription, and the node uses the following parameters:

node B has also only one publisher and one subscription, and that nodes uses the following parameters:

I think those kind of differences will create a usability problem. This was a simple example with only two nodes and one publisher and subscription each. Imagine this in a real world use case, it will look bad. It will also need a lot of repeated code in many places, so at least a set of helper functions will be needed to avoid inconsistencies.

Ergonomics of a solution based on parameters

I will suppose that the parameters API is more powerful than now:

Based on that, I imagine that the best we can come up is something like this:

/**:
    ros__parameters:
        qos_profiles:  # group for all the qos settings
            # Using '/' will create problems with ros URLs to address parameters, thus a replacement is needed.
            # Using nested groups will create inconsistencies (and it would be a bit confusing).
            # We need a character that is not accepted in topic names but accepted in parameter names
            # I think both '/' and '#' aren't accepted in parameter or topic names currently (but we could fix that for parameters).
            'my#fully#qualified#topic#name#here': 
                publisher: &my_profile
                    reliability: reliable
                    history_depth: 100
                    history: keep_last
/my_ns/nested_ns/my_node:
    ros__parameters:
        qos_profiles:
            'my#fully#qualified#topic#name#here':
                publisher:
                    <<: *my_profile,
                    reliability: best_effort  # yeah, yaml anchors allow this :)

Maybe, it doesn't look that but after all ... If we add support to:

I think I will be happy with a solution based on parameters with all that solved ... (I have to put a bit more of thought on the parameters alternative, but it seems to be solvable).

ros-discourse commented 4 years ago

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-agenda-2020-09-17/16464/1

ivanpauno commented 4 years ago

I opened a new proposal, this time based on using const parameters https://github.com/ros2/design/pull/300. I think that it addresses most of the concerns that were raised before.

cc @gbiggs @dejanpan @budrus

ivanpauno commented 4 years ago

Closing in favor of https://github.com/ros2/design/pull/300. Thanks everybody for the feedback here!