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

[Iron] Add new node interface TypeDescriptionsInterface to provide GetTypeDescription service (backport #2224) #2236

Merged
merged 5 commits into from Jul 18, 2023
Merged
Show file tree
Hide file tree
Changes from 4 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
1 change: 1 addition & 0 deletions rclcpp/CMakeLists.txt
Expand Up @@ -92,6 +92,7 @@ set(${PROJECT_NAME}_SRCS
src/rclcpp/node_interfaces/node_time_source.cpp
src/rclcpp/node_interfaces/node_timers.cpp
src/rclcpp/node_interfaces/node_topics.cpp
src/rclcpp/node_interfaces/node_type_descriptions.cpp
src/rclcpp/node_interfaces/node_waitables.cpp
src/rclcpp/node_options.cpp
src/rclcpp/parameter.cpp
Expand Down
9 changes: 9 additions & 0 deletions rclcpp/include/rclcpp/node.hpp
Expand Up @@ -56,6 +56,7 @@
#include "rclcpp/node_interfaces/node_time_source_interface.hpp"
#include "rclcpp/node_interfaces/node_timers_interface.hpp"
#include "rclcpp/node_interfaces/node_topics_interface.hpp"
#include "rclcpp/node_interfaces/node_type_descriptions_interface.hpp"
#include "rclcpp/node_interfaces/node_waitables_interface.hpp"
#include "rclcpp/node_options.hpp"
#include "rclcpp/parameter.hpp"
Expand Down Expand Up @@ -1454,6 +1455,11 @@ class Node : public std::enable_shared_from_this<Node>
rclcpp::node_interfaces::NodeTimeSourceInterface::SharedPtr
get_node_time_source_interface();

/// Return the Node's internal NodeTypeDescriptionsInterface implementation.
RCLCPP_PUBLIC
rclcpp::node_interfaces::NodeTypeDescriptionsInterface::SharedPtr
get_node_type_descriptions_interface();
emersonknapp marked this conversation as resolved.
Show resolved Hide resolved

/// Return the sub-namespace, if this is a sub-node, otherwise an empty string.
/**
* The returned sub-namespace is either the accumulated sub-namespaces which
Expand Down Expand Up @@ -1591,6 +1597,9 @@ class Node : public std::enable_shared_from_this<Node>
const rclcpp::NodeOptions node_options_;
const std::string sub_namespace_;
const std::string effective_namespace_;

class BackportMemberMaps;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note to reviewer: went with a generic name to leave it open for possible other members usage

Copy link
Contributor

Choose a reason for hiding this comment

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

At the risk of bikeshedding, I think the name is a little too generic. That is, the class embeds a

std::unordered_map<
    const Node *, rclcpp::node_interfaces::NodeTypeDescriptionsInterface::SharedPtr
  > type_descriptions_map_;

which feels very specific to this backport. I think I'd go with BackportTypeDescriptionsInterface, or something like that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe the intention wasn't clear. This class could have more maps within it, controlled by the same access mutex, that create more "members" for the owning class' instances. Given that members would be created all at once, and removed all at once, there would be no partial construction or partial deletion of these "members", it makes sense to use the same mutex and manage all the maps at once. That was the intention here - this thing is "all extra backported members" - and maybe that ends up being only one from this backport, but if we do want to backport more member-based features to Iron we don't need to create any new classes or static members, this one does the whole job via new maps within it. I like it as a generic backport pattern that could be reused.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The implementation could actually be a singular map to some "tuple" or struct of extra members, that is added and deleted atomically.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, OK. Thanks for the explanation. In that case, I'm fine with the name as-is. I'll just suggest you put some of that explanation there into a comment (either here in the .hpp or in the .cpp, I don't care which).

static BackportMemberMaps backport_member_maps_;
};

} // namespace rclcpp
Expand Down
2 changes: 2 additions & 0 deletions rclcpp/include/rclcpp/node_interfaces/node_interfaces.hpp
Expand Up @@ -30,6 +30,7 @@
rclcpp::node_interfaces::NodeTimeSourceInterface, \
rclcpp::node_interfaces::NodeTimersInterface, \
rclcpp::node_interfaces::NodeTopicsInterface, \
rclcpp::node_interfaces::NodeTypeDescriptionsInterface, \
rclcpp::node_interfaces::NodeWaitablesInterface


Expand Down Expand Up @@ -118,6 +119,7 @@ class NodeInterfaces
* - rclcpp::node_interfaces::NodeTimeSourceInterface
* - rclcpp::node_interfaces::NodeTimersInterface
* - rclcpp::node_interfaces::NodeTopicsInterface
* - rclcpp::node_interfaces::NodeTypeDescriptionsInterface
* - rclcpp::node_interfaces::NodeWaitablesInterface
*
* Or you use custom interfaces as long as you make a template specialization
Expand Down
@@ -0,0 +1,63 @@
// Copyright 2023 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.

#ifndef RCLCPP__NODE_INTERFACES__NODE_TYPE_DESCRIPTIONS_HPP_
#define RCLCPP__NODE_INTERFACES__NODE_TYPE_DESCRIPTIONS_HPP_

#include <memory>

#include "rclcpp/macros.hpp"
#include "rclcpp/node_interfaces/node_base_interface.hpp"
#include "rclcpp/node_interfaces/node_logging_interface.hpp"
#include "rclcpp/node_interfaces/node_parameters_interface.hpp"
#include "rclcpp/node_interfaces/node_services_interface.hpp"
#include "rclcpp/node_interfaces/node_topics_interface.hpp"
#include "rclcpp/node_interfaces/node_type_descriptions_interface.hpp"
#include "rclcpp/visibility_control.hpp"

namespace rclcpp
{
namespace node_interfaces
{

/// Implementation of the NodeTypeDescriptions part of the Node API.
class NodeTypeDescriptions : public NodeTypeDescriptionsInterface
{
public:
RCLCPP_SMART_PTR_ALIASES_ONLY(NodeTypeDescriptions)

RCLCPP_PUBLIC
explicit NodeTypeDescriptions(
NodeBaseInterface::SharedPtr node_base,
NodeLoggingInterface::SharedPtr node_logging,
NodeParametersInterface::SharedPtr node_parameters,
NodeServicesInterface::SharedPtr node_services);

RCLCPP_PUBLIC
virtual
~NodeTypeDescriptions();

private:
RCLCPP_DISABLE_COPY(NodeTypeDescriptions)

// Pimpl hides helper types and functions used for wrapping a C service, which would be
// awkward to expose in this header.
class NodeTypeDescriptionsImpl;
std::unique_ptr<NodeTypeDescriptionsImpl> impl_;
};

} // namespace node_interfaces
} // namespace rclcpp

#endif // RCLCPP__NODE_INTERFACES__NODE_TYPE_DESCRIPTIONS_HPP_
@@ -0,0 +1,44 @@
// Copyright 2023 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.

#ifndef RCLCPP__NODE_INTERFACES__NODE_TYPE_DESCRIPTIONS_INTERFACE_HPP_
#define RCLCPP__NODE_INTERFACES__NODE_TYPE_DESCRIPTIONS_INTERFACE_HPP_

#include "rclcpp/macros.hpp"
#include "rclcpp/node_interfaces/detail/node_interfaces_helpers.hpp"
#include "rclcpp/visibility_control.hpp"

namespace rclcpp
{
namespace node_interfaces
{

/// Pure virtual interface class for the NodeTypeDescriptions part of the Node API.
class NodeTypeDescriptionsInterface
{
public:
RCLCPP_SMART_PTR_ALIASES_ONLY(NodeTypeDescriptionsInterface)

RCLCPP_PUBLIC
virtual
~NodeTypeDescriptionsInterface() = default;
};

} // namespace node_interfaces
} // namespace rclcpp

RCLCPP_NODE_INTERFACE_HELPERS_SUPPORT(
rclcpp::node_interfaces::NodeTypeDescriptionsInterface, type_descriptions)

#endif // RCLCPP__NODE_INTERFACES__NODE_TYPE_DESCRIPTIONS_INTERFACE_HPP_
72 changes: 72 additions & 0 deletions rclcpp/src/rclcpp/node.cpp
Expand Up @@ -17,7 +17,10 @@
#include <limits>
#include <map>
#include <memory>
#include <mutex>
#include <shared_mutex>
#include <string>
#include <unordered_map>
#include <utility>
#include <vector>

Expand All @@ -36,6 +39,7 @@
#include "rclcpp/node_interfaces/node_time_source.hpp"
#include "rclcpp/node_interfaces/node_timers.hpp"
#include "rclcpp/node_interfaces/node_topics.hpp"
#include "rclcpp/node_interfaces/node_type_descriptions.hpp"
#include "rclcpp/node_interfaces/node_waitables.hpp"
#include "rclcpp/qos_overriding_options.hpp"

Expand Down Expand Up @@ -109,6 +113,65 @@ create_effective_namespace(const std::string & node_namespace, const std::string

} // namespace

/// \brief Associates new hidden backported members with instances of Node.
class Node::BackportMemberMaps
{
public:
BackportMemberMaps() = default;

/// \brief Add all backported members for a new Node.
/**
* \param[in] key Raw pointer to the Node instance that will use new members.
*/
void add(Node * key)
{
// Adding a new instance to the maps requires exclusive access
std::unique_lock lock(map_access_mutex_);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: #include <mutex> at the top of the file for this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a really good eye you have for these - do you know of any include-what-you-use linters that could handle this so you don't have to?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is iwyu: https://include-what-you-use.org/ . It's even packaged for Ubuntu under the same name. The major issue with it (from an automation standpoint) is that it relies on clang, so we could only conceivably run it in our clang CI jobs.

Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised that cpplint.py doesn't catch this, it used to catch things like this. Maybe its coverage is just not as good.

type_descriptions_map_.emplace(
key,
std::make_shared<rclcpp::node_interfaces::NodeTypeDescriptions>(
key->get_node_base_interface(),
key->get_node_logging_interface(),
key->get_node_parameters_interface(),
key->get_node_services_interface()));
}

/// \brief Remove the members for an instance of Node
/**
* \param[in] key Raw pointer to the Node
*/
void remove(const Node * key)
{
// Removing an instance from the maps requires exclusive access
std::unique_lock lock(map_access_mutex_);
type_descriptions_map_.erase(key);
}

/// \brief Retrieve the NodeTypeDescriptionsInterface for a Node.
/**
* \param[in] key Raw pointer to an instance of Node.
* \return A shared ptr to this Node's NodeTypeDescriptionsInterface instance.
*/
rclcpp::node_interfaces::NodeTypeDescriptionsInterface::SharedPtr
get_node_type_descriptions_interface(const Node * key) const
{
// Multiple threads can retrieve from the maps at the same time
std::shared_lock lock(map_access_mutex_);
emersonknapp marked this conversation as resolved.
Show resolved Hide resolved
return type_descriptions_map_.at(key);
}

private:
/// \brief Map that stored TypeDescriptionsInterface members
std::unordered_map<
emersonknapp marked this conversation as resolved.
Show resolved Hide resolved
const Node *, rclcpp::node_interfaces::NodeTypeDescriptionsInterface::SharedPtr
> type_descriptions_map_;

/// \brief Controls access to all private maps
mutable std::shared_mutex map_access_mutex_;
};
// Definition of static member declaration
Node::BackportMemberMaps Node::backport_member_maps_;

Node::Node(
const std::string & node_name,
const NodeOptions & options)
Expand Down Expand Up @@ -211,6 +274,8 @@ Node::Node(
sub_namespace_(""),
effective_namespace_(create_effective_namespace(this->get_namespace(), sub_namespace_))
{
backport_member_maps_.add(this);

// we have got what we wanted directly from the overrides,
// but declare the parameters anyway so they are visible.
rclcpp::detail::declare_qos_parameters(
Expand Down Expand Up @@ -272,6 +337,7 @@ Node::Node(
Node::~Node()
{
// release sub-interfaces in an order that allows them to consult with node_base during tear-down
backport_member_maps_.remove(this);
node_waitables_.reset();
node_time_source_.reset();
node_parameters_.reset();
Expand Down Expand Up @@ -591,6 +657,12 @@ Node::get_node_topics_interface()
return node_topics_;
}

rclcpp::node_interfaces::NodeTypeDescriptionsInterface::SharedPtr
Node::get_node_type_descriptions_interface()
{
return backport_member_maps_.get_node_type_descriptions_interface(this);
}

rclcpp::node_interfaces::NodeServicesInterface::SharedPtr
Node::get_node_services_interface()
{
Expand Down