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

Pass SharedPtrs callback remove functions instead of bare pointers #3

Merged
merged 1 commit into from
Feb 8, 2021
Merged

Pass SharedPtrs callback remove functions instead of bare pointers #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions rclcpp/include/rclcpp/parameter_event_handler.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,12 +98,12 @@ class ParameterEventHandler

/// Remove parameter event callback registered with add_parameter_event_callback.
/**
* \param[in] handle Pointer to the handle for the callback to remove.
* \param[in] callback_handle Handle of the callback to remove.
*/
RCLCPP_PUBLIC
void
remove_parameter_event_callback(
const ParameterEventCallbackHandle * const handle);
ParameterEventCallbackHandle::SharedPtr callback_handle);

using ParameterCallbackType = ParameterCallbackHandle::ParameterCallbackType;

Expand All @@ -129,12 +129,12 @@ class ParameterEventHandler
* is erased from the list of callback handles on the {parameter_name, node_name} in the map.
* An error is thrown if the handle does not exist and/or was already removed.
*
* \param[in] handle Pointer to callback handle to be removed.
* \param[in] callback_handle Handle of the callback to remove.
*/
RCLCPP_PUBLIC
void
remove_parameter_callback(
const ParameterCallbackHandle * const handle);
ParameterCallbackHandle::SharedPtr callback_handle);

/// Get an rclcpp::Parameter from a parameter event.
/**
Expand Down
7 changes: 4 additions & 3 deletions rclcpp/src/rclcpp/parameter_event_handler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,13 @@ struct HandleCompare

void
ParameterEventHandler::remove_parameter_event_callback(
const ParameterEventCallbackHandle * const handle)
ParameterEventCallbackHandle::SharedPtr callback_handle)
{
std::lock_guard<std::recursive_mutex> lock(mutex_);
auto it = std::find_if(
event_callbacks_.begin(),
event_callbacks_.end(),
HandleCompare<ParameterEventCallbackHandle>(handle));
HandleCompare<ParameterEventCallbackHandle>(callback_handle.get()));
if (it != event_callbacks_.end()) {
event_callbacks_.erase(it);
} else {
Expand Down Expand Up @@ -91,9 +91,10 @@ ParameterEventHandler::add_parameter_callback(

void
ParameterEventHandler::remove_parameter_callback(
const ParameterCallbackHandle * const handle)
ParameterCallbackHandle::SharedPtr callback_handle)
{
std::lock_guard<std::recursive_mutex> lock(mutex_);
auto handle = callback_handle.get();
auto & container = parameter_callbacks_[{handle->parameter_name, handle->node_name}];
auto it = std::find_if(
container.begin(),
Expand Down
25 changes: 18 additions & 7 deletions rclcpp/test/rclcpp/test_parameter_event_handler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ class TestParameterEventHandler : public rclcpp::ParameterEventHandler
{
event_callback(event);
}

size_t num_installed_handlers()
{
return event_callbacks_.size();
}
};

class TestNode : public ::testing::Test
Expand Down Expand Up @@ -282,7 +287,7 @@ TEST_F(TestNode, EventCallback)
}
};

auto event_handle = param_handler->add_parameter_event_callback(cb);
auto event_handle1 = param_handler->add_parameter_event_callback(cb);
auto event_handle2 = param_handler->add_parameter_event_callback(cb2);

bool_param = false;
Expand All @@ -297,15 +302,15 @@ TEST_F(TestNode, EventCallback)
// Test removal of event callback
received = false;
bool_param = false;
param_handler->remove_parameter_event_callback(event_handle.get());
param_handler->remove_parameter_event_callback(event_handle1);
param_handler->test_event(multiple);
param_handler->test_event(diff_ns_bool);
EXPECT_EQ(received, false);
EXPECT_EQ(bool_param, true);

// Should throw if callback handle no longer exists or already removed
EXPECT_THROW(
param_handler->remove_parameter_event_callback(event_handle.get()), std::runtime_error);
param_handler->remove_parameter_event_callback(event_handle1), std::runtime_error);
}

TEST_F(TestNode, MultipleParameterCallbacks)
Expand All @@ -329,18 +334,24 @@ TEST_F(TestNode, MultipleParameterCallbacks)
// Test removal of parameter callback by callback handle
received_1 = false;
received_2 = false;
param_handler->remove_parameter_callback(h1.get());
param_handler->remove_parameter_callback(h1);
param_handler->test_event(same_node_int);
EXPECT_EQ(received_1, false);
EXPECT_EQ(received_2, true);

// Test removal of parameter callback by name
received_2 = false;
param_handler->remove_parameter_callback(h2.get());
param_handler->remove_parameter_callback(h2);
param_handler->test_event(same_node_int);
EXPECT_EQ(received_2, false);

// Should throw if callback handle no longer exists or already removed
EXPECT_THROW(param_handler->remove_parameter_callback(h1.get()), std::runtime_error);
EXPECT_THROW(param_handler->remove_parameter_callback(h2.get()), std::runtime_error);
EXPECT_THROW(param_handler->remove_parameter_callback(h1), std::runtime_error);
EXPECT_THROW(param_handler->remove_parameter_callback(h2), std::runtime_error);

param_handler->remove_parameter_callback(h3);

// All callbacks should have been removed
EXPECT_EQ(received_2, 0);
EXPECT_EQ(param_handler->num_installed_handlers(), 0UL);
}