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

Implement generic client #2358

Merged
2 changes: 2 additions & 0 deletions rclcpp/CMakeLists.txt
Expand Up @@ -45,6 +45,7 @@ set(${PROJECT_NAME}_SRCS
src/rclcpp/clock.cpp
src/rclcpp/context.cpp
src/rclcpp/contexts/default_context.cpp
src/rclcpp/create_generic_client.cpp
src/rclcpp/detail/add_guard_condition_to_rcl_wait_set.cpp
src/rclcpp/detail/resolve_parameter_overrides.cpp
src/rclcpp/detail/rmw_implementation_specific_payload.cpp
Expand Down Expand Up @@ -73,6 +74,7 @@ set(${PROJECT_NAME}_SRCS
src/rclcpp/experimental/executors/events_executor/events_executor.cpp
src/rclcpp/experimental/timers_manager.cpp
src/rclcpp/future_return_code.cpp
src/rclcpp/generic_client.cpp
src/rclcpp/generic_publisher.cpp
src/rclcpp/generic_subscription.cpp
src/rclcpp/graph_listener.cpp
Expand Down
56 changes: 56 additions & 0 deletions rclcpp/include/rclcpp/create_generic_client.hpp
@@ -0,0 +1,56 @@
// Copyright 2023 Sony Group 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__CREATE_GENERIC_CLIENT_HPP_
#define RCLCPP__CREATE_GENERIC_CLIENT_HPP_

#include <memory>
#include <string>

#include "rclcpp/generic_client.hpp"
#include "rclcpp/node_interfaces/node_base_interface.hpp"
#include "rclcpp/node_interfaces/node_graph_interface.hpp"
#include "rclcpp/node_interfaces/node_services_interface.hpp"
fujitatomoya marked this conversation as resolved.
Show resolved Hide resolved
#include "rclcpp/qos.hpp"

namespace rclcpp
{
/// Create a generic service client with a name of given type.
/**
* \param[in] node_base NodeBaseInterface implementation of the node on which
* to create the client.
* \param[in] node_graph NodeGraphInterface implementation of the node on which
* to create the client.
* \param[in] node_services NodeServicesInterface implementation of the node on
* which to create the client.
* \param[in] service_name The name on which the service is accessible.
* \param[in] service_type The name of service type, e.g. "test_msgs/srv/BasicTypes"
* \param[in] qos Quality of service profile for client.
* \param[in] group Callback group to handle the reply to service calls.
* \return Shared pointer to the created client.
*/
wjwwood marked this conversation as resolved.
Show resolved Hide resolved
RCLCPP_PUBLIC
rclcpp::GenericClient::SharedPtr
create_generic_client(
std::shared_ptr<node_interfaces::NodeBaseInterface> node_base,
std::shared_ptr<node_interfaces::NodeGraphInterface> node_graph,
std::shared_ptr<node_interfaces::NodeServicesInterface> node_services,
const std::string & service_name,
const std::string & service_type,
const rclcpp::QoS & qos = rclcpp::ServicesQoS(),
rclcpp::CallbackGroup::SharedPtr group = nullptr);
wjwwood marked this conversation as resolved.
Show resolved Hide resolved

} // namespace rclcpp

#endif // RCLCPP__CREATE_GENERIC_CLIENT_HPP_
204 changes: 204 additions & 0 deletions rclcpp/include/rclcpp/generic_client.hpp
@@ -0,0 +1,204 @@
// Copyright 2023 Sony Group 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__GENERIC_CLIENT_HPP_
#define RCLCPP__GENERIC_CLIENT_HPP_

#include <map>
#include <memory>
#include <future>
#include <string>
#include <vector>
#include <utility>
fujitatomoya marked this conversation as resolved.
Show resolved Hide resolved

#include "rcl/client.h"

#include "rclcpp/client.hpp"
#include "rclcpp/visibility_control.hpp"
#include "rcpputils/shared_library.hpp"
fujitatomoya marked this conversation as resolved.
Show resolved Hide resolved

#include "rosidl_typesupport_introspection_cpp/message_introspection.hpp"

namespace rclcpp
{
class GenericClient : public ClientBase
{
public:
using Request = void *; // Serialized data pointer of request message
using Response = void *; // Serialized data pointer of response message

using SharedResponse = std::shared_ptr<void>;

using Promise = std::promise<SharedResponse>;
using SharedPromise = std::shared_ptr<Promise>;

using Future = std::future<SharedResponse>;
using SharedFuture = std::shared_future<SharedResponse>;

RCLCPP_SMART_PTR_DEFINITIONS(GenericClient)

/// A convenient GenericClient::Future and request id pair.
/**
* Public members:
* - future: a std::future<void *>.
* - request_id: the request id associated with the future.
*
* All the other methods are equivalent to the ones std::future provides.
*/
struct FutureAndRequestId
: detail::FutureAndRequestId<Future>
{
using detail::FutureAndRequestId<Future>::FutureAndRequestId;

/// See std::future::share().
SharedFuture share() noexcept {return this->future.share();}

/// Move constructor.
FutureAndRequestId(FutureAndRequestId && other) noexcept = default;
/// Deleted copy constructor, each instance is a unique owner of the future.
FutureAndRequestId(const FutureAndRequestId & other) = delete;
/// Move assignment.
FutureAndRequestId & operator=(FutureAndRequestId && other) noexcept = default;
/// Deleted copy assignment, each instance is a unique owner of the future.
FutureAndRequestId & operator=(const FutureAndRequestId & other) = delete;
/// Destructor.
~FutureAndRequestId() = default;
};

GenericClient(
rclcpp::node_interfaces::NodeBaseInterface * node_base,
rclcpp::node_interfaces::NodeGraphInterface::SharedPtr node_graph,
const std::string & service_name,
const std::string & service_type,
rcl_client_options_t & client_options);

RCLCPP_PUBLIC
SharedResponse create_response() override;

RCLCPP_PUBLIC
std::shared_ptr<rmw_request_id_t> create_request_header() override;

RCLCPP_PUBLIC
void handle_response(
std::shared_ptr<rmw_request_id_t> request_header,
std::shared_ptr<void> response) override;
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: if you need to multi-line the function prototypes, I think it's cleaner to put the return code on its own line first, then the arguments, and if you do that for one function, it's helpful for readability (again in my opinion) to put them all on their own line even for functions that are short. This squarely falls into developer discretion, but I wanted to point it out. To be clear, I'm suggesting:

Suggested change
RCLCPP_PUBLIC
void handle_response(
std::shared_ptr<rmw_request_id_t> request_header,
std::shared_ptr<void> response) override;
RCLCPP_PUBLIC
void
handle_response(
std::shared_ptr<rmw_request_id_t> request_header,
std::shared_ptr<void> response) override;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will update other declarations to use this coding style.


/// Send a request to the service server.
/**
* This method returns a `FutureAndRequestId` instance
* that can be passed to Executor::spin_until_future_complete() to
* wait until it has been completed.
*
* If the future never completes,
* e.g. the call to Executor::spin_until_future_complete() times out,
* GenericClient::remove_pending_request() must be called to clean the client internal state.
* Not doing so will make the `Client` instance to use more memory each time a response is not
* received from the service server.
*
* ```cpp
* auto future = client->async_send_request(my_request);
* if (
* rclcpp::FutureReturnCode::TIMEOUT ==
* executor->spin_until_future_complete(future, timeout))
* {
* client->remove_pending_request(future);
* // handle timeout
* } else {
* handle_response(future.get());
* }
* ```
*
* \param[in] request request to be send.
* \return a FutureAndRequestId instance.
*/
RCLCPP_PUBLIC
FutureAndRequestId async_send_request(const Request request);

/// Clean all pending requests older than a time_point.
/**
* \param[in] time_point Requests that were sent before this point are going to be removed.
* \param[inout] pruned_requests Removed requests id will be pushed to the vector
* if a pointer is provided.
* \return number of pending requests that were removed.
*/
template<typename AllocatorT = std::allocator<int64_t>>
size_t
prune_requests_older_than(
std::chrono::time_point<std::chrono::system_clock> time_point,
std::vector<int64_t, AllocatorT> * pruned_requests = nullptr)
{
std::lock_guard guard(pending_requests_mutex_);
auto old_size = pending_requests_.size();
for (auto it = pending_requests_.begin(), last = pending_requests_.end(); it != last; ) {
if (it->second.first < time_point) {
if (pruned_requests) {
pruned_requests->push_back(it->first);
}
it = pending_requests_.erase(it);
} else {
++it;
}
}
return old_size - pending_requests_.size();
}
Copy link
Member

Choose a reason for hiding this comment

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

Copy and pasting large chunks of logic like this one (pulled from

/// Clean all pending requests older than a time_point.
/**
* \param[in] time_point Requests that were sent before this point are going to be removed.
* \param[inout] pruned_requests Removed requests id will be pushed to the vector
* if a pointer is provided.
* \return number of pending requests that were removed.
*/
template<typename AllocatorT = std::allocator<int64_t>>
size_t
prune_requests_older_than(
std::chrono::time_point<std::chrono::system_clock> time_point,
std::vector<int64_t, AllocatorT> * pruned_requests = nullptr)
{
std::lock_guard guard(pending_requests_mutex_);
auto old_size = pending_requests_.size();
for (auto it = pending_requests_.begin(), last = pending_requests_.end(); it != last; ) {
if (it->second.first < time_point) {
if (pruned_requests) {
pruned_requests->push_back(it->first);
}
it = pending_requests_.erase(it);
} else {
++it;
}
}
return old_size - pending_requests_.size();
}
) is really bad imo. If we discover and fix a bug in one we may fail to update the other. It would be better to factor these into common functions, so that it is more obvious that this logic is used in two places.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@wjwwood

"Currently, pending_requests_ is defined in GenericClient class and Client class. The operations on pending_requests_ involve duplicated code.
There are two ways to avoid duplicated codes:

  1. Move pending_requests_ and the corresponding mutex lock into the client base class. Since pending_requests_ contains different contents, the BaseClient class must be changed to a template class. This change would be quite significant.

  2. Use macros to include the same code and place the macros in client.hpp."

I am inclined towards 2.
I would like to hear your opinion.

Copy link
Member

Choose a reason for hiding this comment

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

Why not have a free function that handles most of the logic and call it from the two classes? I don't think changing the base class to a template is an option. The base class needs to be concrete for storing them in containers. I don't think you need macros either. If a mutex or other objects are needed, pass them in to the free function by reference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why not have a free function that handles most of the logic and call it from the two classes? I don't think changing the base class to a template is an option. The base class needs to be concrete for storing them in containers. I don't think you need macros either. If a mutex or other objects are needed, pass them in to the free function by reference.

Yes. Using function is okay.
pending_requests_ must be passed, and the type of pending_requests_ is different for Client and GenericClient. So the function have to be a template function.


RCLCPP_PUBLIC
size_t prune_pending_requests();

RCLCPP_PUBLIC
bool remove_pending_request(int64_t request_id);

/// Take the next response for this client.
/**
* \sa ClientBase::take_type_erased_response().
*
* \param[out] response_out The reference to a Service Response into
* which the middleware will copy the response being taken.
* \param[out] request_header_out The request header to be filled by the
* middleware when taking, and which can be used to associate the response
* to a specific request.
* \returns true if the response was taken, otherwise false.
* \throws rclcpp::exceptions::RCLError based exceptions if the underlying
* rcl function fail.
*/
RCLCPP_PUBLIC
bool take_response(Response response_out, rmw_request_id_t & request_header_out)
{
return this->take_type_erased_response(response_out, request_header_out);
}

protected:
using CallbackInfoVariant = std::variant<
std::promise<SharedResponse>>; // Use variant for extension

int64_t
async_send_request_impl(const Request request, CallbackInfoVariant value);

std::optional<CallbackInfoVariant>
get_and_erase_pending_request(int64_t request_number);

RCLCPP_DISABLE_COPY(GenericClient)

std::map<int64_t, std::pair<
std::chrono::time_point<std::chrono::system_clock>,
CallbackInfoVariant>> pending_requests_;
std::mutex pending_requests_mutex_;

private:
std::shared_ptr<rcpputils::SharedLibrary> ts_lib_;
const rosidl_typesupport_introspection_cpp::MessageMembers * response_members_;
};
} // namespace rclcpp

#endif // RCLCPP__GENERIC_CLIENT_HPP_
17 changes: 17 additions & 0 deletions rclcpp/include/rclcpp/node.hpp
Expand Up @@ -42,6 +42,7 @@
#include "rclcpp/clock.hpp"
#include "rclcpp/context.hpp"
#include "rclcpp/event.hpp"
#include "rclcpp/generic_client.hpp"
#include "rclcpp/generic_publisher.hpp"
#include "rclcpp/generic_subscription.hpp"
#include "rclcpp/logger.hpp"
Expand Down Expand Up @@ -320,6 +321,22 @@ class Node : public std::enable_shared_from_this<Node>
const rclcpp::QoS & qos = rclcpp::ServicesQoS(),
rclcpp::CallbackGroup::SharedPtr group = nullptr);

/// Create and return a GenericClient.
/**
* \param[in] service_name The name on which the service is accessible.
* \param[in] service_type The name of service type, e.g. "std_srvs/srv/SetBool"
* \param[in] qos Quality of service profile for client.
* \param[in] group Callback group to handle the reply to service calls.
* \return Shared pointer to the created GenericClient.
*/
RCLCPP_PUBLIC
rclcpp::GenericClient::SharedPtr
create_generic_client(
const std::string & service_name,
const std::string & service_type,
const rclcpp::QoS & qos = rclcpp::ServicesQoS(),
rclcpp::CallbackGroup::SharedPtr group = nullptr);

/// Create and return a GenericPublisher.
/**
* The returned pointer will never be empty, but this function can throw various exceptions, for
Expand Down
44 changes: 44 additions & 0 deletions rclcpp/src/rclcpp/create_generic_client.cpp
@@ -0,0 +1,44 @@
// Copyright 2023 Sony Group 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.

#include "rclcpp/create_generic_client.hpp"
#include "rclcpp/generic_client.hpp"

namespace rclcpp
{
rclcpp::GenericClient::SharedPtr
create_generic_client(
std::shared_ptr<node_interfaces::NodeBaseInterface> node_base,
std::shared_ptr<node_interfaces::NodeGraphInterface> node_graph,
std::shared_ptr<node_interfaces::NodeServicesInterface> node_services,
const std::string & service_name,
const std::string & service_type,
const rclcpp::QoS & qos,
rclcpp::CallbackGroup::SharedPtr group)
{
rcl_client_options_t options = rcl_client_get_default_options();
options.qos = qos.get_rmw_qos_profile();

auto cli = rclcpp::GenericClient::make_shared(
node_base.get(),
node_graph,
service_name,
service_type,
options);

auto cli_base_ptr = std::dynamic_pointer_cast<rclcpp::ClientBase>(cli);
node_services->add_client(cli_base_ptr, group);
return cli;
}
} // namespace rclcpp