Add TakerSubscription #499
Conversation
| @@ -292,6 +255,52 @@ struct SubscriptionHandle { | |||
| } | |||
|
|
|||
| impl SubscriptionHandle { | |||
| fn create<'a, T: Message>( | |||
There was a problem hiding this comment.
Moved this logic into SubscriptionHandle to be used by both SubscriptionBase and TakerSubscription
| /// | ||
| /// [1]: crate::NodeState::create_taker_subscription | ||
| /// [2]: crate::Executor::spin | ||
| pub struct TakerSubscription<T: Message> { |
There was a problem hiding this comment.
Named TakerSubscription rather than the discussed SubscriptionTaker to be closer to WorkerSubscription
There was a problem hiding this comment.
Personally I think SubscriptionTaker makes more sense grammatically.
In this case Taker is the main object, and Subscription is describing the taker. Whereas for WorkerSubscription, the Subscription is the main object, and Worker is describing the subscription.
There was a problem hiding this comment.
If we want Take to come first, then maybe TakingSubscription could work grammatically. That way Taking is describing the Subscription.
| } | ||
| } | ||
|
|
||
| impl<T: Message> RclPrimitive for Arc<TakerSubscription<T>> { |
There was a problem hiding this comment.
Implemented for use with WaitSet, see examples in unit test below and in NodeState::create_taker_subscription docs
There was a problem hiding this comment.
One recommendation I would make for the executor is to include a watch sender and trigger it each time a new message is available. That way a user with a TakerSubscription could use Receiver::changed to .await an update signal so they know that a message is ready to be taken.
This will allow the TakerSubscription to work well with async programs.
There was a problem hiding this comment.
But also we can't use Arc<TakerSubscription<T>> for this because then the wait set will never know when to drop this from its list.
If you take a look at Subscription::lifecycle you'll see that the struct the user holds onto needs to contain a WaitableLifecycle. That struct keeps the associated RclPrimitive inside the wait set. Once the WaitableLifecycle gets dropped, the RclPrimitive will be removed from the wait set.
In the current state of this PR, this RclPrimitive will be dropped from the wait set immediately because you're dropping the lifecycle right away (and you named it _lifecycle up above to escape the compiler warning). Since this RclPrimitive implementation is doing nothing, I suppose it doesn't matter that it's being dropped from the wait set, but then we should do one of the following:
- Don't bother implementing
RclPrimitivehere and don't bother adding anything to the wait set for aTakerSubscription. - Follow the pattern of the other primitive implementations and create separate structs for the
RclPrimitiveversus the user-facing object. Then have theRclPrimitivejust trigger a signal so users can know when a message is available.
harrisonmg
force-pushed
the
h/take
branch
2 times, most recently
from
ef57cc0 to
8b3d65e
Compare
|
@harrisonmg thanks for your contribution. Is this PR ready for review? If not, could you move this to a draft and let us know when it's ready? Thanks |
Unless anybody has comments on the API itself, I think it's ready for review. Sorry if I spammed CI notifications while I tried to fix those tests. |
I took a shot at implementing the API discussed in #493.
I didn't implement the
take_withmethod that was previously discussed becausercl_takecopies the new message into anRmwMessage, not aMessage. It is not obvious to me how to avoid constructing a newMessagewhen converting fromRmwMessagetoMessageviafrom_rmw_message.