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

Example of using Deadline, Liveliness, and Lifespan QoS in Client #314

Closed
wants to merge 0 commits into from
Closed

Conversation

mm318
Copy link
Member

@mm318 mm318 commented Mar 8, 2019

As part of implementation for Deadline, Liveliness, and Lifespan QoS. The design is at ros2/design#212

The changes are examples of how a user would create subscribers and publishers in their client-side application code.

@tfoote tfoote added the in progress Actively being worked on (Kanban column) label Mar 8, 2019
auto event_callback = [](rclcpp::ResourceStatusEvent event_type) -> void
{
if (rclcpp::ResourceStatusEvent::DEADLINE_MISSED == event_type) {
printf("deadline event occured");
Copy link
Member

@wjwwood wjwwood Mar 8, 2019

Choose a reason for hiding this comment

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

As we spoke about in our meeting, I think it would be best to have separate callbacks for each type of event, so that you could pass additional information about that event clearly as arguments to the callback. For example, here it would be good to know which instance/publisher or instance/subscription had a deadline missed. For the subscription/publisher, you can perhaps automatically associate which one it was by passing a different callback to each one when you add the callback, but for the instance (looking to the future when we'll have keys) you'd need more information than you have when you register the callback initially.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, we are currently working on that.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, should I hold off reviewing for now? Or do you want more early feedback?

Copy link
Member

@wjwwood wjwwood Mar 8, 2019

Choose a reason for hiding this comment

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

(I was going to do a full review, and accidentally clicked "add single comment" mid typing 🤦‍♂️)

Copy link
Member Author

Choose a reason for hiding this comment

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

We would like more early feedback. We believe that rcl (ros2/rcl#396) and rmw (ros2/rmw#167) interfaces are relatively final.

rclcpp (ros2/rclcpp#655) is probably the most in flux, but for now can you take a look at how we're planning to have PublisherBase and SubscriptionBase to implement the Waitable interface and removing a bunch of vectors of rcl handles in AllocatorMemoryStrategy?

@mm318 mm318 marked this pull request as ready for review March 13, 2019 20:47
@mm318
Copy link
Member Author

mm318 commented Mar 14, 2019

Hi @wjwwood, I've updated the sample code to show the use of separate callbacks for each type of event.

Copy link

@nburek nburek left a comment

Choose a reason for hiding this comment

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

Should we create a demo specific to the QoS settings showing how to actually use them and giving examples that actually trigger the callbacks?


rclcpp::PublisherOptions<MyAllocator<void>> pub_options;
pub_options.qos_history_depth(10)
.deadline_callback(deadline_event_callback)
Copy link

Choose a reason for hiding this comment

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

Let's not add all of these callbacks to the allocator example. We should try to keep each example small and specific. Adding these is unnecessary noise in a demo about allocators.

latched_qos.durability = RMW_QOS_POLICY_DURABILITY_TRANSIENT_LOCAL;
auto map_pub = node->create_publisher<nav_msgs::msg::OccupancyGrid>(
"map", latched_qos);
rclcpp::PublisherOptions<> latched_qos;
Copy link

Choose a reason for hiding this comment

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

Rename this variable as it's no longer just qos settings.


RCLCPP_INFO(node_logger, "Publishing data on topic '%s'", topic.c_str());
// Create the image publisher with our custom QoS profile.
auto pub = node->create_publisher<sensor_msgs::msg::Image>(
topic, custom_camera_qos_profile);
auto pub = node->create_publisher<sensor_msgs::msg::Image>(topic, nullptr,
Copy link

Choose a reason for hiding this comment

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

If something is getting passed in as a nullptr should it also be moved to the Options class?

@@ -105,26 +105,26 @@ int main(int argc, char * argv[])

// Set the parameters of the quality of service profile. Initialize as the default profile
// and set the QoS parameters specified on the command line.
rmw_qos_profile_t custom_camera_qos_profile = rmw_qos_profile_default;
rclcpp::PublisherOptions<> custom_camera_qos_profile;
Copy link

Choose a reason for hiding this comment

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

Rename variable as it is no longer just qos settings.

@@ -139,11 +139,11 @@ int main(int argc, char * argv[])
};

// Set the QoS profile for the subscription to the flip message.
rmw_qos_profile_t custom_flip_qos_profile = rmw_qos_profile_sensor_data;
custom_flip_qos_profile.depth = 10;
rclcpp::SubscriptionOptions<> custom_flip_qos_profile(rmw_qos_profile_sensor_data);
Copy link

Choose a reason for hiding this comment

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

Update variable name here and all the others below too.

@mm318 mm318 closed this Mar 29, 2019
@tfoote tfoote removed the in progress Actively being worked on (Kanban column) label Mar 29, 2019
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