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 link to ROS QoS Policy docs to spec registry #1020

Conversation

defunctzombie
Copy link
Contributor

@defunctzombie defunctzombie commented Nov 17, 2023

Update the ros2 profile section of the registry with more details about the expectations for offered_qos_profiles and link to the official ROS docs.

Fixes: #1002


I do wonder if it would be more appropriate for the ROS docs to define these aspects of the profile and the MCAP registry links to those docs for details on the profile.

@defunctzombie
Copy link
Contributor Author

@MichaelOrlov @jhurliman fyi - if you have feedback.

…-documentation-for-offered_qos_profiles-foxglovemcap
@MichaelOrlov
Copy link
Contributor

@defunctzombie My apologies, a bit busy and don't have time to consider this PR.
However, I can get back to it after Thanksgiving holidays.

@jhurliman
Copy link
Contributor

LGTM

Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

@defunctzombie The wording and links looks good to me.

However, there are some subtle specifics that in theory could be more than one QoS profile stored in that field and it is actually a sequence of such serialized QoS profiles in yaml format. This is true for the case when exists different publishers with different QoS profiles on the same topic.
See for details https://github.com/ros2/rosbag2/blob/9855072566f8fb46a2bdd20cc217d1a0c2b03e82/rosbag2_storage_mcap/src/mcap_storage.cpp#L823-L825

    channel.metadata.emplace(
      "offered_qos_profiles",
      rosbag2_storage::serialize_rclcpp_qos_vector(topic_info.topic_metadata.offered_qos_profiles));

Maybe reword the first sentence The sequence of the QoS policy values for the topic in yaml format?

@defunctzombie
Copy link
Contributor Author

Maybe reword the first sentence The sequence of the QoS policy values for the topic in yaml format?

👍 sounds good. Do you know what the format of this sequence would look like? Would it be a typical yaml sequence of the existing QoS "objects"? The above code makes it look like it will always be a sequence even with 1 element. Is that correct or does the serialization of the vector special case single element vectors?

@MichaelOrlov
Copy link
Contributor

It is going to be always a YAML sequence.
The implementation is https://github.com/ros2/rosbag2/blob/542bda34358d03acf3c15228a60859e62f5b69ba/rosbag2_storage/src/rosbag2_storage/qos.cpp#L252-L259

Node convert<std::vector<rclcpp::QoS>>::encode(const std::vector<rclcpp::QoS> & rhs, int version)
{
  Node node{NodeType::Sequence};
  for (const auto & value : rhs) {
    node.push_back(convert<rclcpp::QoS>::encode(value, version));
  }
  return node;
}

@defunctzombie defunctzombie merged commit d40dba5 into main Dec 1, 2023
24 checks passed
@defunctzombie defunctzombie deleted the roman/fg-5520-missing-documentation-for-offered_qos_profiles-foxglovemcap branch December 1, 2023 14:12
pezy pushed a commit to pezy/mcap that referenced this pull request Jan 11, 2024
Update the `ros2` profile section of the registry with more details
about the expectations for `offered_qos_profiles` and link to the
official ROS docs.

Fixes: foxglove#1002
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Missing documentation for offered_qos_profiles
4 participants