Skip to content

Add Galactic feature note for externally configuring QoS #1033

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 21, 2021
Merged

Add Galactic feature note for externally configuring QoS #1033

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0747b9f
Add Galactic feature note for externally configuring QoS
jacobperron Jan 21, 2021
792ccbd
Update wording
jacobperron Jan 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions source/Releases/Release-Galactic-Geochelone.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,19 @@ Thus the default value stays the same: ``~/.ros/log``.

Related PRs: `ros2/rcl_logging#53 <https://github.com/ros2/rcl_logging/pull/53>`_ and `ros2/launch#460 <https://github.com/ros2/launch/pull/460>`_.

Externally configure QoS at start-up
------------------------------------

It is now possible to externally configure the QoS settings for a node at start-up time.
QoS settings are **not** configurable during runtime and node authors must opt-in to enable this feature.
Copy link
Member

@ivanpauno ivanpauno Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems a bit confusing to me, maybe:

Suggested change
QoS settings are **not** configurable during runtime and node authors must opt-in to enable this feature.
QoS settings are configurable during runtime only if the node authors have enabled this feature.

Copy link
Contributor

@clalancette clalancette Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ivanpauno I think that rewrite completely changes the meaning, and suggests that QoS can be changed via runtime. So I don't think we want to say that.

Really, I think this sentence should be split into two because it is talk about two separate topics. Something like:

QoS settings are **not** configurable during runtime; they are only configurable at start-up time.
In order to change the QoS settings at runtime, node authors must opt-in to enable this feature.

Or something like that. The previous sentence may need to be tweaked as well.

Copy link
Member

@ivanpauno ivanpauno Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok, I got it now.
The distinction between "runtime" and "startup time" can be confusing (because "startup time" is also "runtime").

Really, I think this sentence should be split into two because it is talk about two separate topics

👍 your suggested wording looks fine!

Copy link
Member Author

@jacobperron jacobperron Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the wording in 792ccbd

If the feature is enabled on a node, then QoS settings can be set with ROS parameters when a node first starts.

`Demos in C++ and Python can be found here. <https://github.com/ros2/demos/tree/a66f0e894841a5d751bce6ded4983acb780448cf/quality_of_service_demo#qos-overrides>`_

See the `design document for more details <http://design.ros2.org/articles/qos_configurability.html>`_.

Related PRs: `ros2/rclcpp#1408 <https://github.com/ros2/rclcpp/pull/1408>`_ and `ros2/rclpy#635 <https://github.com/ros2/rclpy/pull/635>`_

Changes since the Foxy release
------------------------------

Expand Down