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.

Sign up for GitHub

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

Jump to bottom

Make loaned messages const on the subscription side #1971

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

Make loaned messages const on the subscription side #1971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9270074
Make loaned messages const on the subscription side
nnmm Jul 12, 2022
b702380
Make a copy of the const message when callback requires non-const mes…
nnmm Jul 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions rclcpp/include/rclcpp/any_subscription_callback.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,9 +487,13 @@ class AnySubscriptionCallback
}

// Dispatch when input is a ros message and the output could be anything.
void
template<typename MaybeConstROSMessageType>
typename std::enable_if<std::is_same<ROSMessageType,
typename std::remove_const<MaybeConstROSMessageType>::type
>::value
>::type
dispatch(
Copy link
Member

Choose a reason for hiding this comment

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

could you add tests making sure that loaned messages work well with all types of callbacks?
There're a lot of codepaths here, and it's pretty hard to make sure you didn't break anything else.
So it would be nice to make sure all cases are covered in tests (some of the cases might already be covered).

std::shared_ptr<ROSMessageType> message,
std::shared_ptr<MaybeConstROSMessageType> message,
const rclcpp::MessageInfo & message_info)
{
TRACEPOINT(callback_start, static_cast<const void *>(this), false);
Expand All @@ -505,6 +509,7 @@ class AnySubscriptionCallback
[&message, &message_info, this](auto && callback) {
using T = std::decay_t<decltype(callback)>;
static constexpr bool is_ta = rclcpp::TypeAdapter<MessageT>::is_specialized::value;
static constexpr bool is_const = std::is_const<MaybeConstROSMessageType>::value;

// conditions for output is custom message
if constexpr (is_ta && std::is_same_v<T, ConstRefCallback>) {
Expand Down Expand Up @@ -548,6 +553,10 @@ class AnySubscriptionCallback
callback(create_ros_unique_ptr_from_ros_shared_ptr_message(message));
} else if constexpr (std::is_same_v<T, UniquePtrWithInfoROSMessageCallback>) {
callback(create_ros_unique_ptr_from_ros_shared_ptr_message(message), message_info);
} else if constexpr (is_const && std::is_same_v<T, SharedPtrROSMessageCallback>) {
callback(std::make_shared<ROSMessageType>(*message));
} else if constexpr (is_const && std::is_same_v<T, SharedPtrWithInfoROSMessageCallback>) {
callback(std::make_shared<ROSMessageType>(*message), message_info);
} else if constexpr ( // NOLINT[readability/braces]
std::is_same_v<T, SharedConstPtrROSMessageCallback>||
std::is_same_v<T, ConstRefSharedConstPtrROSMessageCallback>||
Expand Down
2 changes: 1 addition & 1 deletion rclcpp/include/rclcpp/generic_subscription.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ class GenericSubscription : public rclcpp::SubscriptionBase
/// This function is currently not implemented.
RCLCPP_PUBLIC
void handle_loaned_message(
void * loaned_message, const rclcpp::MessageInfo & message_info) override;
const void * loaned_message, const rclcpp::MessageInfo & message_info) override;

// Same as return_serialized_message() as the subscription is to serialized_messages only
RCLCPP_PUBLIC
Expand Down
8 changes: 4 additions & 4 deletions rclcpp/include/rclcpp/subscription.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ class Subscription : public SubscriptionBase

void
handle_loaned_message(
void * loaned_message,
const void * loaned_message,
const rclcpp::MessageInfo & message_info) override
{
if (matches_any_intra_process_publishers(&message_info.get_rmw_message_info().publisher_gid)) {
Expand All @@ -369,10 +369,10 @@ class Subscription : public SubscriptionBase
return;
}

auto typed_message = static_cast<ROSMessageType *>(loaned_message);
const auto typed_message = static_cast<const ROSMessageType *>(loaned_message);
// message is loaned, so we have to make sure that the deleter does not deallocate the message
auto sptr = std::shared_ptr<ROSMessageType>(
typed_message, [](ROSMessageType * msg) {(void) msg;});
auto sptr = std::shared_ptr<const ROSMessageType>(
typed_message, [](const ROSMessageType * msg) {(void) msg;});

std::chrono::time_point<std::chrono::system_clock> now;
if (subscription_topic_statistics_) {
Expand Down
2 changes: 1 addition & 1 deletion rclcpp/include/rclcpp/subscription_base.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ class SubscriptionBase : public std::enable_shared_from_this<SubscriptionBase>
RCLCPP_PUBLIC
virtual
void
handle_loaned_message(void * loaned_message, const rclcpp::MessageInfo & message_info) = 0;
handle_loaned_message(const void * loaned_message, const rclcpp::MessageInfo & message_info) = 0;

/// Return the message borrowed in create_message.
/** \param[in] message Shared pointer to the returned message. */
Expand Down
2 changes: 1 addition & 1 deletion rclcpp/src/rclcpp/executor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,7 +598,7 @@ Executor::execute_subscription(rclcpp::SubscriptionBase::SharedPtr subscription)
// This is the case where a loaned message is taken from the middleware via
// inter-process communication, given to the user for their callback,
// and then returned.
void * loaned_msg = nullptr;
const void * loaned_msg = nullptr;
// TODO(wjwwood): refactor this into methods on subscription when LoanedMessage
// is extened to support subscriptions as well.
take_and_do_error_handling(
Expand Down
2 changes: 1 addition & 1 deletion rclcpp/src/rclcpp/generic_subscription.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ GenericSubscription::handle_serialized_message(
}

void GenericSubscription::handle_loaned_message(
void * message, const rclcpp::MessageInfo & message_info)
const void * message, const rclcpp::MessageInfo & message_info)
{
(void) message;
(void) message_info;
Expand Down
2 changes: 1 addition & 1 deletion rclcpp/test/rclcpp/node_interfaces/test_node_topics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ class TestSubscription : public rclcpp::SubscriptionBase
create_serialized_message() override {return nullptr;}

void handle_message(std::shared_ptr<void> &, const rclcpp::MessageInfo &) override {}
void handle_loaned_message(void *, const rclcpp::MessageInfo &) override {}
void handle_loaned_message(const void *, const rclcpp::MessageInfo &) override {}
void handle_serialized_message(
const std::shared_ptr<rclcpp::SerializedMessage> &, const rclcpp::MessageInfo &) override {}
void return_message(std::shared_ptr<void> &) override {}
Expand Down