Skip to content
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.

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

Add QoS overriding #13

Merged
merged 3 commits into from
Mar 26, 2021
Merged

Add QoS overriding #13

merged 3 commits into from
Mar 26, 2021

Conversation

jacobperron
Copy link
Member

Resolves #8

  • Adds support for configuring QoS policies per topic bridge.
  • Adds YAML support
  • Change default value for deadline and lifespan policies to be "infinite"
    • Users can opt-in to the automatic matching

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM!! (I've left some minor comments)

After seeing all the parsing code needed I wonder if we should have only enabled qos parameter overrides, though being able to configure everything from the same file looks much nicer!!

Comment on lines +217 to +218
qos.lifespan(
rclcpp::Duration::from_nanoseconds(std::numeric_limits<std::int64_t>::max()));
Copy link
Member

Choose a reason for hiding this comment

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

maybe:

Suggested change
qos.lifespan(
rclcpp::Duration::from_nanoseconds(std::numeric_limits<std::int64_t>::max()));
qos.lifespan(RMW_DURATION_INFINITE);

we should have something like rclcpp::Duration()::Infinity() in rclcpp though.

I'm not sure if your code above does exactly the same, thought it should.

Copy link
Member

Choose a reason for hiding this comment

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

actually, maybe the default value should be RMW_DURATION_INFINITE (instead of -1) and the if (lifespan_ns < 0) is never needed (?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted an easy way for users to explicitly specify an infinite duration in YAML (hence negative values), though in retrospect we could also use a string infinite or max.

Copy link
Member

Choose a reason for hiding this comment

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

const auto deadline_ns = qos_options.deadline().value();
if (deadline_ns < 0) {
qos.deadline(
rclcpp::Duration::from_nanoseconds(std::numeric_limits<std::int64_t>::max()));
Copy link
Member

Choose a reason for hiding this comment

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

same comment here

* Support configuring QoS policies per topic
  Add new QosOptions object accessible from TopicBridgeOptions for providing QoS overrides.
  Reliability and durability can be optionally configured, otherwise we default to the automatic matching mechanism.
  History and depth can be configured, and default to 'keep last' and '10' respectively.

* Support configuring QoS in YAML
  Add tests for parser involving QoS options.

* Update example

* Support configuring deadline and lifespan in YAML
  Users can override deadline and lifespan with number of nanoseconds, or a negative number to represent an infinite value.
  The default behavior was changed to be an infinite value for deadline and lifespan (instead of matching), which has a
  better change of not losing messages due to those policies.

* Make deadline and lifespan auto-matching opt-in behavior

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
This is temporary to fix test failures due that are resolved by this fix upstream: ros2/rclcpp#1584
After the next Rolling sync, we can revert this commit.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

jacobperron commented Mar 26, 2021

I had to add a fix to CI to get a newer version of rclcpp (3951467)

I've cleaned up the commits for a rebase-merge.

@jacobperron jacobperron merged commit 24e99ee into main Mar 26, 2021
@delete-merged-branch delete-merged-branch bot deleted the config_qos_per_topic branch March 26, 2021 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QoS overriding
2 participants