Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add non transform capabilities for intra-process #1849
Add non transform capabilities for intra-process #1849
Changes from 9 commits
10795a4
066c79b
1ceabc8
6dc5e4e
1334d33
df5f52e
54166ee
6fb0254
7925aee
a5c5504
89c70d1
b76af8e
42cad32
71c3239
da5f12e
f3b7616
3c29a57
1779cd8
c8c4851
6d47368
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the logic with the hierarchy in the Subscription IPC class.
We have that:
SubscriptionIntraProcess
is aSubscriptionIntraProcessBuffer
is aROSMessageIntraProcessBuffer
is aSubscriptionIntraProcessBase
is aWaitable
Do we really need all these layers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree this is confusing. The most confusing part to me is going from Subscription to ROSMessage back to Subscription. Maybe rename
ROSMessageIntraProcessBuffer
to something with subscription in it, or fold it in to another class in the inheritance hierarchy?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this is confusing, but I think we do need all of these layers.
So at the base, the whole thing needs to be a
Waitable
because it needs to be waited on by the executors. We need theSubscriptionIntraProcessBase
as a non-templated type so that we can store essentially untyped subscriptions in theIntraProcessManager
(rclcpp/rclcpp/include/rclcpp/experimental/intra_process_manager.hpp
Lines 319 to 320 in c8c4851
ROSMessageIntraProcessBuffer
andSubscriptionIntraProcessBuffer
is how the subscriber ultimately communicates whether it is a ROS type or not to theIntraProcessManager
. So we need both of those layers to distinguish between those two cases. And finally, we needSubscriptionIntraProcess
as the thing that theSubscription
class interacts with.I'll note that this hierarchy was largely here already; the only thing this PR does is to add in the
ROSMessageIntraProcessBuffer
.One thing we might be able to do is collapse the
SubscriptionIntraProcess
andSubscriptionIntraProcessBuffer
classes into one. Neither of them does too much on its own, and it isn't entirely clear why they are separated. If you'd like me to try to do that, I'll give that a whirl. I think we need the rest of the layers.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think that it makes sense to rename
ROSMessageIntraProcessBuffer
to something likeSubscriptionROSMessageIntraProcessBuffer
, or something similar? It is just a little odd to me that it's in the hierarchy but doesn't start withSubscription
, like the other classes after waitable.Maybe make an issue and possibly do it in another PR? This isn't that high priority for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me. I'll go ahead and rename it (probably to
SubscriptionROSMsgIntraProcessBuffer
, just to shorten it a bit).