rclcpp QoS implementation

[emersonknapp]

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[mm318]

In allocator_memory_strategy.hpp, you seem to have left out the event Waitables of publishers and subscriptions. This applies to remove_null_handles(), collect_entities(), add_handles_to_wait_set(), and possibly more.

EDIT: My mistake, it only applies to collect_entities().

[emersonknapp]

@mm318

2:01:48 ➜  rclcpp git:(qos-min) ✗ git diff qos-min upstream/master rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp
diff --git a/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp b/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp
index 6c76cad..ca1d2fb 100644
--- a/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp
+++ b/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp
@@ -430,16 +430,6 @@ public:
     return number_of_services;
   }

-  size_t number_of_ready_events() const
-  {
-    size_t number_of_events = 0;
-    for (auto waitable : waitable_handles_) {
-      number_of_events += waitable->get_number_of_ready_events();
-    }
-    return number_of_events;
-  }
-
-
   size_t number_of_ready_clients() const
   {
     size_t number_of_clients = client_handles_.size();

I put all the stuff back to the original, did I delete functionality in the process?

[emersonknapp]

@mm318 if there's parts that are missing here for the bare minimum qos functionality, maybe this is a useful starting place for you to add just that part back in?

[mm318]

@emersonknapp Right, the original is missing functionality that's been added. Sure, I can add them back.

[nburek]

There are a lot of comments of me trying to figure things out during my first pass of all this. I'll make another pass after I read more of the rcl changes tomorrow and hopefully that should close out a number of my questions.

[nburek]

Hmmm, I wonder if it would be better to specify the callback_group for event callbacks as part of the optional class where the callback functions are set instead. What do you think of that idea?

[mm318]

I think @ross-desmond said CallbackGroup is used often, which is why I didn't fold it into the options class. I'm open to changing it.

[nburek]

Yeah, I'm not as concerned about exposing it to the user. I was more thinking that the callback group for publishers really only applies to the event callbacks and that for subscribers you may want to separate your event handlers and your message handler into different callback groups. Maybe I'm over thinking it though so I'm fine leaving this as is to not add more work for now if we don't think it's absolutely the right thing to do.

[nburek]

This looks like it needs a //TODO marker

[ross-desmond]

In this implementation, waitables will not have any events, since it was stripped.

[mm318]

Let's leave QOSEventHandlerBase/QOSEventHandler as a Waitable, since they are new classes. The Waitable interface will then continue to need a get_number_of_ready_events() member function.

[nburek]

If this has reached the complexity of needing to template initialization functions and types that relate to the specific callback then it seems like we should be using polymorphism to extend the base class instead of templating further.

[mm318]

I don't think defining n number of derived classes with a bunch of member functions here can scale with the n number of simple QoS status struct types defined in rmw/types.h

[nburek]

I don't think it needs a bunch of member functions. Each derived class would probably only need a constructor or to implement a virtual initialize function that the constructor calls with the event_handle_, which you already need to create for every status type. I think it's fine to still keep the base class templated so that the destructor and execute functions can be reused, but templating an initialize functions is where it starts to feel like a hack around inheritance to me. Yes it's a couple extra lines of code for every status, but in my opinion it's easier to understand code.

[dabonnie]

Though I generally prefer polymorphism I don't know if it's the best path forward because there's no real commonality between the base events defined in rmw (e.g. a base class doesn't make sense). We're lucky that 3/4 have the same fields, but that won't be the case if we look to QoS types beyond DDS.

[nburek]

The commonality is that these are all interfaces for handling events. All events come out of the same take function in RMW when an event is ready and they all pass that along to some user provided callback function. The divergence is in the type for the event. If the code is exactly the same and the only difference is in the type then templates are definitely the right way to go. But in this case it's actually using another template parameter to change out the code that's being executed by providing different initialization functions based on the type. To me that seems like a much better case for polymorphism.

From the offline conversation, I still feel like the combination of polymorphism and templating is the cleaner approach, but I'm willing to consider it a stylistic difference because I don't know that there is a strong technical argument for going one way or the other.

---EDIT---
I take that back, the technical reason for using the polymorphism is to set ourselves up for the situation in the future where there is an initialize function that takes a different number of parameters and we can't just rely on a templated function call because it happens to have the same number of parameters. If we don't think that will ever happen then it's a stylistic difference.

[mm318]

Calling initialization functions that have different number of parameters can still be handled by variadic templates, so I think this is still a stylistic difference.

[ross-desmond]

C++ function style comments

[mm318]

@ross-desmond What did you mean by this?

[dabonnie]

@mm318 check this out: https://google.github.io/styleguide/cppguide.html#Function_Comments

[ross-desmond]

Adding a few comments here. Will get back to it :)

[emersonknapp]

reminder: restore pre-existing create_publisher and create_subscriber APIs, and mark as deprecated

[mm318]

@emersonknapp Will you be carving out the PublisherOptions/SubscriptionOptions PR? Can you restore the to-be-deprecated APIs as part of that? I want to leave them out here so that uses of the to-be-deprecated APIs will error out instead of just producing some warnings.

[emersonknapp]

Yes, I am working on that today - once that's ready we can rebase this review onto that branch