Skip to content
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 loaned message listener. #526

Open
wants to merge 2 commits into
base: rolling
Choose a base branch
from
Open

Conversation

llapx
Copy link

@llapx llapx commented Jul 5, 2021

Signed-off-by: Lei Liu Lei.Liu.AP@sony.com

Signed-off-by: Lei Liu <Lei.Liu.AP@sony.com>
Signed-off-by: Lei Liu <Lei.Liu.AP@sony.com>
@llapx
Copy link
Author

llapx commented Jul 5, 2021

During my test for zero copy, I found there's only talker_loaned_message, so I add a pair for it, hope it's useful.
Thanks.

@Karsten1987
Copy link
Contributor

isn't that an exact duplicate of the listener demo? While I am all up for consistency, I am not really a fan of copying code.

@llapx
Copy link
Author

llapx commented Jul 7, 2021

isn't that an exact duplicate of the listener demo? While I am all up for consistency, I am not really a fan of copying code.

Thanks for your reply!
listener.cpp only accept type String, can not be used for the case of loaned message, so I add it which can be paired with talker_loaned_message.cpp.

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the inline comments, I think it is important to point out what the expectation of this example is. That is, in what circumstances should the user expect this to be zero-copy? How would they verify this is the case?

Comment on lines +32 to +34
// Create a callback function for when messages are received.
// Variations of this function also exist using, for example UniquePtr for zero-copy transport.
setvbuf(stdout, NULL, _IONBF, BUFSIZ);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we have this in our other examples, but it actually isn't necessary anymore. Since Foxy, we print out all RCLCPP_* output to stderr. So just remove this.

Comment on lines +35 to +36
auto callback =
[this](const std_msgs::msg::Float64::SharedPtr msg) -> void
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is import to have a comment that shows that this can only be zero-copy for fixed-sized types.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could print some information about the listener in the constructor of this class and print whether the middleware supports zero-copy.
We could enhance the talker class by printing the pointer address of the message to be published and print the pointer address of the message you get here in the callback. In the case of zero-copy these addresses are the same.

That way we can demonstrate both, a loaned message where the middleware allocates all messages and a zero-copy transport in case the middleware supports it.

{
// Create a Listener class that subclasses the generic rclcpp::Node base class.
// The main function below will instantiate the class as a ROS node.
class LoanedMessageListener : public rclcpp::Node
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I would rename this class. The fact that it is a loaned message under the hood isn't super-interesting to the user. What's more interesting is that it is zero-copy, so I'd rename this to ZeroCopyListener or whatever.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of disagree with this. We have the equivalent talker which is called LoanedMessageTalker and I believe this pattern makes sense, nonetheless for consistency - we also have a pair of SerializedMessage[Talker|Listener].

The idiom of a loaned message is needed for zero-copy, but you can perfectly fine use a loaned message without the middleware supporting zero-copy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of disagree with this. We have the equivalent talker which is called LoanedMessageTalker and I believe this pattern makes sense, nonetheless for consistency - we also have a pair of SerializedMessage[Talker|Listener].

The idiom of a loaned message is needed for zero-copy, but you can perfectly fine use a loaned message without the middleware supporting zero-copy.

OK, that makes sense. I think the thing I'm stuck on here is how the user knows that loaning is happening, and what they should expect different when loaning is used.

@Karsten1987
Copy link
Contributor

listener.cpp only accept type String, can not be used for the case of loaned message, so I add it which can be paired with talker_loaned_message.cpp.

This makes total sense. Thanks for the clarification.

@clalancette clalancette added the more-information-needed Further information is required label Aug 5, 2021
@audrow audrow changed the base branch from master to rolling June 28, 2022 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more-information-needed Further information is required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants