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 ParameterEventsSubscriber class #829

Merged
merged 31 commits into from
Mar 4, 2021
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
6473041
add ParameterEventsSubscriber class and tests
Aug 22, 2019
3246e34
test improvements, path name fixes, and more documentation
Aug 22, 2019
cdf4156
fix lint and uncrustify issues
Aug 22, 2019
72817ab
add try-catch and warning around getting parameter value
Aug 22, 2019
884a2d8
pass rclcpp::Parameter object to callback, rename functions, get para…
Aug 23, 2019
f189327
use unordered map with string pair, add test for different nodes same…
Sep 9, 2019
a010d2d
address feedback (wjwwood) part 1
Sep 11, 2019
f67d829
add RCLCPP_PUBLIC macro
Sep 11, 2019
12329bf
address feedback part 1: static get_parameter method, remove register…
Sep 24, 2019
921d2b4
map parameters to list of callbacks
Sep 25, 2019
7c19dbb
use absolute parameter event topic for parameter event subscriber
Dec 9, 2019
9cc41c7
support multiple parameter event callbacks
Dec 10, 2019
2559434
use get_child for parameter event subscriber logger
Dec 10, 2019
0d5d75c
update utility function description
Dec 10, 2019
e4bc1d8
address feedback: move HandleCompare, test exceptions, reference code…
Dec 16, 2019
f360350
address feedback: replace ParameterEventsFilter dependency, fix copyr…
Dec 17, 2019
36e0190
Get rid of a few compiler warnings; add test to build
Nov 19, 2020
e41f75c
Remove some unneeded code
Nov 24, 2020
b3b683f
Remove a stray debug trace
Nov 24, 2020
8fb32de
Add a function to get all parameter values from a parameter event
Nov 24, 2020
60533db
Address code review feedback
Jan 7, 2021
ab4ff32
Merge pull request #1 from mjeronimo/mjeronimo/address-review-feedback
Jan 8, 2021
894da98
Another name change; using Handler instead of the more passive term, …
Jan 8, 2021
3404ec9
Merge pull request #2 from mjeronimo/mjeronimo/address-review-feedback
Jan 8, 2021
d4128de
Pass SharedPtrs callback remove functions instead of bare pointers
Feb 3, 2021
1201f4f
Merge pull request #3 from mjeronimo/mjeronimo/address-review-feedback
Feb 8, 2021
18e72cf
Add a comment block describing usage
Feb 11, 2021
e08612c
Address review feedback
Feb 11, 2021
a4f5533
Merge pull request #4 from mjeronimo/mjeronimo/add-block-comment
Feb 11, 2021
cbc8989
A couple more doc fixes from review comments
Mar 3, 2021
f011c3d
Merge pull request #5 from mjeronimo/mjeronimo/add-block-comment
Mar 4, 2021
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
1 change: 1 addition & 0 deletions rclcpp/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ set(${PROJECT_NAME}_SRCS
src/rclcpp/parameter_value.cpp
src/rclcpp/parameter_client.cpp
src/rclcpp/parameter_events_filter.cpp
src/rclcpp/parameter_events_subscriber.cpp
src/rclcpp/parameter_map.cpp
src/rclcpp/parameter_service.cpp
src/rclcpp/publisher_base.cpp
Expand Down
234 changes: 234 additions & 0 deletions rclcpp/include/rclcpp/parameter_events_subscriber.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,234 @@
// Copyright 2019 Intel Corporation
//
// 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.

#ifndef RCLCPP__PARAMETER_EVENTS_SUBSCRIBER_HPP_
#define RCLCPP__PARAMETER_EVENTS_SUBSCRIBER_HPP_

#include <list>
#include <memory>
#include <string>
#include <utility>
#include <unordered_map>
#include <vector>

#include "rcl_interfaces/msg/parameter_event.hpp"
#include "rclcpp/create_subscription.hpp"
#include "rclcpp/node_interfaces/get_node_base_interface.hpp"
#include "rclcpp/node_interfaces/get_node_logging_interface.hpp"
#include "rclcpp/node_interfaces/get_node_topics_interface.hpp"
#include "rclcpp/node_interfaces/node_base_interface.hpp"
#include "rclcpp/node_interfaces/node_logging_interface.hpp"
#include "rclcpp/node_interfaces/node_topics_interface.hpp"
#include "rclcpp/parameter.hpp"
#include "rclcpp/qos.hpp"
#include "rclcpp/subscription.hpp"

namespace rclcpp
{

struct ParameterCallbackHandle
{
RCLCPP_SMART_PTR_DEFINITIONS(ParameterCallbackHandle)

using ParameterCallbackType = std::function<void (const rclcpp::Parameter &)>;

std::string parameter_name;
std::string node_name;
ParameterCallbackType callback;
};

struct ParameterEventCallbackHandle
{
RCLCPP_SMART_PTR_DEFINITIONS(ParameterEventCallbackHandle)

using ParameterEventCallbackType =
std::function<void (const rcl_interfaces::msg::ParameterEvent::SharedPtr &)>;

ParameterEventCallbackType callback;
};

class ParameterEventsSubscriber
Copy link
Member

Choose a reason for hiding this comment

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

This class needs documentation on how to use it. Perhaps also a new example to show how it is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I was planning to provide additional documentation at some point. Would an example go in https://github.com/ros2/demos/tree/master/demo_nodes_cpp/src/parameters ?

Copy link
Member

Choose a reason for hiding this comment

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

I think this comment still applies, you could either have a small example in the doc block here, or you could add an example to either the demo_nodes_cpp package or this repository:

https://github.com/ros2/examples/tree/master/rclcpp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be more clear in either examples or demo_nodes_cpp. I can make a separate PR in one of those repos after this one?

Copy link
Member

Choose a reason for hiding this comment

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

Even if you put something in examples or demo_nodes_cpp this class needs documentation.

Copy link
Member

Choose a reason for hiding this comment

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

@mjeronimo this comment still applies I think.

{
public:
/// Construct a subscriber to parameter events
template<typename NodeT>
ParameterEventsSubscriber(
NodeT node,
const rclcpp::QoS & qos =
rclcpp::QoS(rclcpp::QoSInitialization::from_rmw(rmw_qos_profile_parameter_events)))
{
node_base_ = rclcpp::node_interfaces::get_node_base_interface(node);
node_logging_ = rclcpp::node_interfaces::get_node_logging_interface(node);
auto node_topics = rclcpp::node_interfaces::get_node_topics_interface(node);

event_subscription_ = rclcpp::create_subscription<rcl_interfaces::msg::ParameterEvent>(
node_topics, "/parameter_events", qos,
std::bind(&ParameterEventsSubscriber::event_callback, this, std::placeholders::_1));
}

using ParameterEventCallbackType =
ParameterEventCallbackHandle::ParameterEventCallbackType;

/// Set a custom callback for parameter events.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Set a custom callback for parameter events.
/// Add a custom callback for parameter events.

/**
* Multiple callbacks are allowed.
*
* \param[in] callback Function callback to be evaluated on event.
*/
RCLCPP_PUBLIC
ParameterEventCallbackHandle::SharedPtr
add_parameter_event_callback(
ParameterEventCallbackType callback);

/// Remove parameter event callback.
/**
* Calling this function will set the event callback to nullptr.
*/
RCLCPP_PUBLIC
void
remove_parameter_event_callback(
const ParameterEventCallbackHandle * const handle);
Copy link
Member

Choose a reason for hiding this comment

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

It's kind of odd that add_parameter_event_callback() returns a shared pointer but this takes a raw pointer.

I think this should either take a shared pointer (maybe as a const reference), or maybe a const reference to the handle. Is the address of the handle important or are the contents sufficient?

Choose a reason for hiding this comment

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

I believe @bpwilcox was following suit with a similar usage in the rclcpp::Node class:

  // node.hpp
  RCLCPP_PUBLIC
  void
  remove_on_set_parameters_callback(const OnSetParametersCallbackHandle * const handler);

Copy link
Member

Choose a reason for hiding this comment

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

I think that the original method in rclcpp::Node used that signature because:

  • you don't need shared ownership, so taking the shared pointer by value isn't needed (cppcoreguidelines R34).
  • you don't need to "maybe take ownership", so taking a const reference to the shared pointer also isn't needed (R36).

We're using a raw pointer to identify the handle, so we only need a raw pointer to remove it from the map.

Copy link
Member

Choose a reason for hiding this comment

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

But it's not documented that letting the shared pointer returned by add_parameter_event_callback go out of scope will automatically remove it, unlike the inspiration for these signatures, Node::add_on_set_parameters_callback(). As far as I can tell from the code, it is not the case that letting this shared pointer go out of scope will result in the callback getting removed. So I don't see any reason for it to be returned as a shared_ptr.

So going back to my original thing, I think the add function should return a raw pointer (as there's no need to have shared ownership) or the remove function should take a shared pointer so it is symmetric with the add function.

Since automatic removal is not happening, it sounds like it should never be a shared pointer in the first place.

And since the handle is trivially copied, maybe it should just return a OnSetParametersCallbackHandle from add and take a const OnSetParametersCallbackHandle & in remove?

I still just don't see why we're returning a shared ptr from add but taking a raw pointer here.

Copy link
Member

Choose a reason for hiding this comment

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

Since automatic removal is not happening, it sounds like it should never be a shared pointer in the first place.

That sounds good to me.
I think that the inconsistency with the "parameters acceptation callbacks" is a bit undesired, but it sounds ok.

i.e.: I would either always have scoped handles or unscoped ones.


using ParameterCallbackType = ParameterCallbackHandle::ParameterCallbackType;

/// Add a custom callback for a specified parameter.
/**
* If a node_name is not provided, defaults to the current node.
*
* \param[in] parameter_name Name of parameter.
* \param[in] callback Function callback to be evaluated upon parameter event.
* \param[in] node_name Name of node which hosts the parameter.
*/
RCLCPP_PUBLIC
ParameterCallbackHandle::SharedPtr
add_parameter_callback(
const std::string & parameter_name,
ParameterCallbackType callback,
const std::string & node_name = "");

/// Remove a custom callback for a specified parameter given its callback handle.
/**
* The parameter name and node name are inspected from the callback handle. The callback handle
* 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.
*/
RCLCPP_PUBLIC
void
remove_parameter_callback(
const ParameterCallbackHandle * const handle);

/// Get a rclcpp::Parameter from parameter event, return true if parameter name & node in event.
/**
* If a node_name is not provided, defaults to the current node.
*
* \param[in] event Event msg to be inspected.
* \param[out] parameter Reference to rclcpp::Parameter to be assigned.
* \param[in] parameter_name Name of parameter.
* \param[in] node_name Name of node which hosts the parameter.
* \returns true if requested parameter name & node is in event, false otherwise
*/
RCLCPP_PUBLIC
static bool
get_parameter_from_event(
const rcl_interfaces::msg::ParameterEvent & event,
rclcpp::Parameter & parameter,
const std::string parameter_name,
const std::string node_name = "");

/// Get a rclcpp::Parameter from parameter event
/**
* If a node_name is not provided, defaults to the current node.
*
* The user is responsible to check if the returned parameter has been properly assigned.
* By default, if the requested parameter is not found in the event, the returned parameter
* has parameter value of type rclcpp::PARAMETER_NOT_SET.
*
* \param[in] event Event msg to be inspected.
* \param[in] parameter_name Name of parameter.
* \param[in] node_name Name of node which hosts the parameter.
* \returns The resultant rclcpp::Parameter from the event
*/
RCLCPP_PUBLIC
static rclcpp::Parameter
get_parameter_from_event(
const rcl_interfaces::msg::ParameterEvent & event,
const std::string parameter_name,
const std::string node_name = "");

/// Get all rclcpp::Parameter values from a parameter event
/**
* \param[in] event Event msg to be inspected.
* \returns A vector rclcpp::Parameter values from the event
*/
RCLCPP_PUBLIC
static std::vector<rclcpp::Parameter>
get_parameters_from_event(
const rcl_interfaces::msg::ParameterEvent & event);

using CallbacksContainerType = std::list<ParameterCallbackHandle::WeakPtr>;

protected:
/// Callback for parameter events subscriptions.
void
event_callback(const rcl_interfaces::msg::ParameterEvent::SharedPtr event);

// Utility function for resolving node path.
std::string resolve_path(const std::string & path);

// Node Interfaces used for base and logging.
std::shared_ptr<rclcpp::node_interfaces::NodeBaseInterface> node_base_;
std::shared_ptr<rclcpp::node_interfaces::NodeLoggingInterface> node_logging_;

// *INDENT-OFF* Uncrustify doesn't handle indented public/private labels
// Hash function for string pair required in std::unordered_map
// See: https://stackoverflow.com/questions/35985960/c-why-is-boosthash-combine-the-best-way-to-combine-hash-values
class StringPairHash
{
public:
template<typename T>
inline void hash_combine(std::size_t & seed, const T & v) const
{
std::hash<T> hasher;
seed ^= hasher(v) + 0x9e3779b9 + (seed << 6) + (seed >> 2);
}

inline size_t operator()(const std::pair<std::string, std::string> & s) const
{
size_t seed = 0;
hash_combine(seed, s.first);
hash_combine(seed, s.second);
return seed;
}
};
// *INDENT-ON*

// Map container for registered parameters.
std::unordered_map<
std::pair<std::string, std::string>,
CallbacksContainerType,
StringPairHash
> parameter_callbacks_;

rclcpp::Subscription<rcl_interfaces::msg::ParameterEvent>::SharedPtr event_subscription_;

std::list<ParameterEventCallbackHandle::WeakPtr> event_callbacks_;

std::recursive_mutex mutex_;
};

} // namespace rclcpp

#endif // RCLCPP__PARAMETER_EVENTS_SUBSCRIBER_HPP_
1 change: 1 addition & 0 deletions rclcpp/include/rclcpp/rclcpp.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@
#include "rclcpp/parameter.hpp"
#include "rclcpp/parameter_client.hpp"
#include "rclcpp/parameter_service.hpp"
#include "rclcpp/parameter_events_subscriber.hpp"
#include "rclcpp/rate.hpp"
#include "rclcpp/time.hpp"
#include "rclcpp/utilities.hpp"
Expand Down
Loading