Refactor to remove SubscriptionHandle/ClientHandle/ServiceHandle

[nnmm]

The contents of SubscriptionHandle are moved into Subscription.

This creates the problem that the SubscriptionHandle was returned by SubscriptionBase. We could also return the raw rcl_subscription_t, but I'd like to keep these types out of the public API.

Instead, I changed the SubscriptionBase trait to not return the thing to be added to the WaitSet, but to take the WaitSet as an argument and add the rcl_subscription_t directly to it. This has the benefit that this trait now is general enough to also cover clients, services etc. and thus I renamed it to Waitable.

[esteve]

@nnmm what is the rationale for this? I don't really see the point, is it to get rid of SubscriptionHandle?

On a general note, if this gets approved, I'd like to wait to merge this after the first release. I'd rather not refactor #146 to include these changes and apply them to ClientHandle and ServiceHandle

[nnmm]

@esteve You suggested to remove the *Handle structs here, and I think that makes sense. They are not useful for users, so removing them removes internal details from our public API. I already removed PublisherHandle here, with SubscriptionHandle left for a future PR.

The generalization of SubscriptionBase to Waitable is kind of forced by removing SubscriptionHandle, as described in the PR text – but I think this generalization is a good thing. It unifies ClientBase, SubscriptionBase, ServiceBase etc. all into one, because they do the same thing: be added to a wait set, and then be executed when ready.

[nnmm]

I'm okay with waiting for clients & services.

[esteve]

but I think this generalization is a good thing

How would that work with GuardConditions? They are entities that are added to a waitset, but behave differently than subscriptions, clients, services, etc. In rclcpp, ClientBase, SubscriptionBase and ServiceBase inherit from Waitable, but GuardCondition doesn't

[nnmm]

How would that work with GuardConditions? They are entities that are added to a waitset, but behave differently than subscriptions, clients, services, etc. In rclcpp, ClientBase, SubscriptionBase and ServiceBase inherit from Waitable, but GuardCondition doesn't

To be honest, I'm not too familiar with guard conditions. But if guard conditions don't fit this trait, that's no problem, we can just have a add_guard_condition() function on WaitSet.

[nnmm]

@esteve I had an idea. Currently, after this PR, the Node would have members

    pub(crate) clients: Vec<Weak<dyn Waitable>>,
    pub(crate) services: Vec<Weak<dyn Waitable>>,
    pub(crate) subscriptions: Vec<Weak<dyn Waitable>>,

– same for the WaitSet. It's possible to make a copy-paste error or so and mix up two different kinds of Waitable.

The idea is to keep ClientBase, ServiceBase, SubscriptionBase, but make them empty sub-traits of Waitable:

trait ClientBase : Waitable {}

That's a bit more type-safe. Though maybe ClientWaitable would be a better name.

[nnmm]

I've applied the changes to SubscriptionHandle/SubscriptionBase also to clients and services. The diff is a bit large as a result – let me know if you'd prefer me to split it up into several commits or PRs.

[esteve]

trait ClientBase : Waitable {}

That's a bit more type-safe. Though maybe ClientWaitable would be a better name.

Good idea! I like it, in fact, that's the same class hierarchy as rclcpp 😉 I prefer to keep it as ClientBase, it'll be consistent with rclcpp

Edit: I was mistaken, I thought that Subscriptions et al inherited from Waitable, turns out they don't.

[esteve]

Given that the Waitables are now typed, we can keep the old style (Waitset::add_X(X)). IMHO, it reads more logically that we add a Subscription to a Waitset. I'd agree more with the new style if we would have an arbitrary set of entities to be added to a Waitset (the responsibility of adding an entity should therefore be in the Waitable trait), but in this case, we know we won't have anything else other than Subscriptions, Clients, Services and Timers, so we can just have methods for each entity in Waitset

[nnmm]

Ok, we need to keep the actual logic in Waitable::add_to_wait_set (see PR description for why), but I added back the old functions as syntactic sugar.

[esteve]

I've read the description again, but I'm afraid I'm not following, sorry. Is it because you don't want to give pub(crate) visibility to rcl_wait_set in the Waitset? Is there a reason for that? To me Waitset::add_subscription seems the natural "translattion" for rcl_wait_set_add_subscription, so I would expect the logic for adding a subscription to be in Waitset.

[nnmm]

Hmm, no, I'm fine with pub(crate) visibility for rcl types. But the issue is that WaitSet::add_subscription() needs an rcl_subscription_t.

Prior to this PR, the situation is as follows: WaitSet::add_subscription() takes the subscription in the form of a dyn SubscriptionBase, and therefore that trait must be able to provide the rcl_subscription_t. We don't want to have the rcl_subscription_t in the public API, so SubscriptionBase returned the rcl_subscription_t in an opaque wrapper type, the SubscriptionHandle.

The main goal of this PR is to remove the SubscriptionHandle, since even though we now don't have the rcl_subscription_t in the public API, we now have that SubscriptionHandle in the public API which is also "internal" and has no useful functionality for the user. Fewer public types are better, as I'm sure you agree. It does that by changing the handle() method in SubscriptionBase to not return the rcl_subscription_t handle, but just call the function that needs it directly, in this case rcl_wait_set_add_subscription(). (To have a more fitting name, that method was then renamed to add_to_wait_set(), and the trait generalized into Waitable.)

So, I hope that makes sense – in Rust you cannot have a pub(crate) method on a trait, so all trait functions are public, so the way that the SubscriptionBase trait gives the rcl_subscription_t to the WaitSet is also public, unless we use the design in this PR.

We could maybe make the organization feel more "natural" like you say by moving the impls of Waitable into wait.rs. That way, the only place we deal with rcl_wait_set_* functions is in the wait module.

A third design would be to make WaitSet::add_subscription() take an actual Subscription<T>. Then we can just access the private parts, i.e. rcl_subscription_t. This would mean the add_to_wait_set() function is removed. It has the drawback of causing a bit more monomorphization, and being more restrictive on user code, since they'll not be able to e.g. downcast several subscriptions into a SubscriptionBase (or SubscriptionWaitable, if you're fine with that) and store them into a Vec, before adding them to a wait set.

[nnmm]

A fourth way is to keep Handle structs, but mark them as #[doc(hidden)]. They will still be a bit annoying to rclrs developers though.

[nnmm]

I'd go with the third option, we know that Waitsets will only deal with Subscriptions, Clients, Services and Timers, so I think it's fine to be that restrictive.

Actually, sorry I only realize this now, but I don't think option 3 would work. The node needs to store the subscriptions (and clients and services), and it must do so by storing trait objects, not the fully-typed Subscription<T>, or T would need to become a type parameter of Node itself, which is impossible since there may be many subscriptions with different Ts. And if it doesn't have the concrete type anymore, it cannot call a WaitSet::add_subscription() that takes the concrete Subscription<T> type.

So my opinion is that the design in this PR is the best one I can think of. It The way things are currently done + #[doc(hidden)] is also acceptable – but if you don't like the design in this PR, I would like to learn more about why.

[esteve]

but if you don't like the design in this PR, I would like to learn more about why.

Not that I actively dislike it, but I find it unnecessarily complicated. The responsibility of adding a subscription to a waitset should belong in a waitset IMHO, as it's the one that's storing the subscription's handle. But with this PR, in order to allow the entities to be added to a waitset, the waitset handle needs to be made public, which does not seem to me necessary.

[nnmm]

If you mean the rcl_wait_set_t by "waitset handle", that's not public. The trait uses an rclrs::WaitSet.

[esteve]

Yes, we've used that nomenclature for a while, so yeah, by "waitset handle", I mean rcl_wait_set_t. It is public in the sense that a Waitable needs access to it so it can call rcl_wait_set_add_X(). I prefer that we keep the handle structures (i.e. rcl_X_t) encapsulated in Handle structs and move them to a separate module (or declare them as [doc(hidden)]) so that developers know that these are internal.

IMHO the design in this PR would be akin to having the logic of adding an item to a std::vec inside the item that is being added.

[nnmm]

Then let's leave this PR aside for a while and let me rework #224 to work with SubscriptionHandle. Then we can release and later worry about removing SubscriptionHandle.

[nnmm]

Good idea! I like it, in fact, that's the same class hierarchy as rclcpp wink I prefer to keep it as ClientBase, it'll be consistent with rclcpp

Edit: I was mistaken, I thought that Subscriptions et al inherited from Waitable, turns out they don't.

I thought about it some more and imo ClientWaitable is more fitting, since "base class" is an OOP concept that doesn't map 1:1 to Rust, and like you said, the correspondence to rclcpp doesn't really hold. In rclcpp, the *Base classes hold a lot of functionality, but this trait is an empty (marker) trait. Are you ok with keeping the Waitable name as well?

[esteve]

Are you ok with keeping the Waitable name as well?

Yeah, that's fine, makes total sense.

[esteve]

Closing due to inactivity, we might revisit this in the future. Thanks @nnmm