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 :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.

[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.