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

move qos_profile_rosout_default from rcl. #381

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from

Conversation

fujitatomoya
Copy link
Collaborator

related and need to be merged before ros2/rclpy#1376

@@ -87,6 +87,19 @@ static const rmw_qos_profile_t rmw_qos_profile_parameter_events =
false
};

static const rmw_qos_profile_t rmw_qos_profile_rosout_default =
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

qos profile for rosout publisher is defined in rcl. it should be moved here with other profiles. see more details for ros2/rcl#1195

@fujitatomoya
Copy link
Collaborator Author

Pulls: ros2/rclpy#1376, #381, ros2/rcl#1195, ros2/rclcpp#2663
Gist: https://gist.githubusercontent.com/fujitatomoya/e4ad476b64d3f274940ae31ebe7149e3/raw/2fa0833cd5e3edd9602385b88d556cb0d18ad054/ros2.repos
BUILD args: --packages-above-and-dependencies rclpy rmw rcl rclcpp
TEST args: --packages-above rclpy rmw rcl rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14837

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@christophebedard
Copy link
Member

christophebedard commented Nov 19, 2024

I think we talked about this in the waffle meeting last week and it was mentioned that, since rosout is an rcl-level thing and not an rmw-level thing, the QoS profile should stay in rcl.

@fujitatomoya
Copy link
Collaborator Author

@christophebedard thanks for the comment!

hmmm okay, in that case why clients default profiles are defined in rmw? for example, rmw_qos_profile_parameter_events. if those are supposed to be defined on where it is used, it should be defined in client libraries? i was thinking that those default profiles are defined in rmw so that all dependent libraries on top of that, can be consistent profile... maybe i am missing something... ❓❓❓

@christophebedard
Copy link
Member

christophebedard commented Nov 19, 2024

Then maybe rmw_qos_profile_parameter_events doesn't belong in rmw. If it's meant to be used by client libraries (rclcpp, rclpy) and not rmw itself, and is a client library concept instead of an rmw concept, then it should go in rcl.

Parameter events are a client library concept, but clients and services are an rmw concept, so rmw_qos_profile_services_default probably belongs in rmw. So I wouldn't say "defined where used" but "defined where it belongs," if that makes sense.

Anyway, I was just relaying what was mentioned in the waffle meeting last week. We may want to talk about it again.

@fujitatomoya
Copy link
Collaborator Author

yeah i know you are just sharing the information, thanks!

since rosout is an rcl-level thing and not an rmw-level thing, the QoS profile should stay in rcl.

if that is a really requirement, we need to update the pybind to expose the rosout qos from rcl instead of rmw.

IMO, having rosout default profile in rmw should be okay, lets keep this open.

@fujitatomoya
Copy link
Collaborator Author

@ahcorde @clalancette @mjcarroll @wjwwood any thoughts above discussion? thanks in advance.

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.

3 participants