Skip to content

Conversation

@luca-della-vedova
Copy link
Collaborator

@luca-della-vedova luca-della-vedova commented Feb 1, 2024

This PR adds a default implementation for QoSProfile as well as a builder pattern based on the rclcpp API.
I split it into a first commit that should be fully uncontroversial and a second commit that contains the builder API for Duration based items, which should also be OK I believe.

I intentionally left out all the methods in the other APIs that just set a profile to the requested structure, since while in C++ people might have to do something like:

const auto reliability = QoSReliabilityPolicy::Reliable;
auto profile = QoSProfile::SystemDefault().reliability(reliability);

In Rust the API is (imho) not so necessary since they can just use struct initialization, such as:

let reliability = QoSReliabilityPolicy::Reliable;
let profile = QoSProfile {
   reliability,
   ..QOS_SYSTEM_DEFAULT
};

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
jhdcs
jhdcs previously approved these changes Feb 1, 2024
Copy link
Collaborator

@jhdcs jhdcs left a comment

Choose a reason for hiding this comment

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

These changes all look good to me! Only one minor comment/question about adding a comment to the default function, but I don't think it's enough to hold up the PR if we don't want to do it.

}

impl Default for QoSProfile {
fn default() -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we make a note here that this is setting the QOS profile to the RCL default? Just to avoid any confusion?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done! 6ac6caa

Copy link
Collaborator

Choose a reason for hiding this comment

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

@luca-della-vedova thanks 🙂

esteve
esteve previously approved these changes Feb 1, 2024
Copy link
Collaborator

@esteve esteve left a comment

Choose a reason for hiding this comment

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

@luca-della-vedova thanks. I agree with @jhdcs about adding a note about the RCL default, could you add that? Thanks

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
@luca-della-vedova luca-della-vedova dismissed stale reviews from esteve and jhdcs via 6ac6caa February 2, 2024 03:59
@esteve esteve merged commit 27bc355 into ros2-rust:main Feb 2, 2024
@luca-della-vedova luca-della-vedova deleted the luca/qos_builder branch February 2, 2024 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants