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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions demo_nodes_cpp/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ add_library(topics_library SHARED
src/topics/talker_loaned_message.cpp
src/topics/talker_serialized_message.cpp
src/topics/listener.cpp
src/topics/listener_loaned_message.cpp
src/topics/listener_serialized_message.cpp
src/topics/listener_best_effort.cpp)
add_demo_dependencies(timers_library)
Expand Down Expand Up @@ -121,6 +122,9 @@ rclcpp_components_register_node(topics_library
rclcpp_components_register_node(topics_library
PLUGIN "demo_nodes_cpp::Listener"
EXECUTABLE listener)
rclcpp_components_register_node(topics_library
PLUGIN "demo_nodes_cpp::LoanedMessageListener"
EXECUTABLE listener_loaned_message)
rclcpp_components_register_node(topics_library
PLUGIN "demo_nodes_cpp::SerializedMessageListener"
EXECUTABLE listener_serialized_message)
Expand Down
53 changes: 53 additions & 0 deletions demo_nodes_cpp/src/topics/listener_loaned_message.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
// Copyright 2021 Open Source Robotics Foundation, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#include "rclcpp/rclcpp.hpp"
#include "rclcpp_components/register_node_macro.hpp"

#include "std_msgs/msg/float64.hpp"
#include "demo_nodes_cpp/visibility_control.h"

namespace demo_nodes_cpp
{
// 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.

{
public:
DEMO_NODES_CPP_PUBLIC
explicit LoanedMessageListener(const rclcpp::NodeOptions & options)
: Node("listener_loaned_message", options)
{
// 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);
Comment on lines +32 to +34
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.

auto callback =
[this](const std_msgs::msg::Float64::SharedPtr msg) -> void
Comment on lines +35 to +36
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.

{
RCLCPP_INFO(this->get_logger(), "I heard: [%f]", msg->data);
};
// Create a subscription to the topic which can be matched with one or more compatible ROS
// publishers.
// Note that not all publishers on the same topic with the same type will be compatible:
// they must have compatible Quality of Service policies.
sub_ = create_subscription<std_msgs::msg::Float64>("chatter_pod", 10, callback);
}

private:
rclcpp::Subscription<std_msgs::msg::Float64>::SharedPtr sub_;
};

} // namespace demo_nodes_cpp

RCLCPP_COMPONENTS_REGISTER_NODE(demo_nodes_cpp::LoanedMessageListener)