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

Fix std::function disambiguation in Windows #27

Merged
merged 9 commits into from
Apr 30, 2015
Merged

Fix std::function disambiguation in Windows #27

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2367549
Check callback arity and enable/disable compatible/incompatible versi…
Apr 29, 2015
db0238c
Formatting
Apr 29, 2015
ad3f5da
Let the compiler deduce the appropriate Service constructor to use
Apr 30, 2015
1181daf
Delegate service creation to SFINAE'd create_service_internal based o…
Apr 30, 2015
814f410
Check callback argument types
Apr 30, 2015
faba591
Moved std::enable_if to template parameters
Apr 30, 2015
bbd7e39
Use longer name
Apr 30, 2015
805d5cb
Fix code style
Apr 30, 2015
02a41cd
Removed check for third argument, fails with callbacks that don't tak…
Apr 30, 2015
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
84 changes: 74 additions & 10 deletions rclcpp/include/rclcpp/node.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <list>
#include <memory>
#include <string>
#include <tuple>

#include <rclcpp/callback_group.hpp>
#include <rclcpp/client.hpp>
Expand Down Expand Up @@ -46,6 +47,23 @@ class Executor;
namespace node
{

// TODO: add support for functors, std::function, lambdas and object members
template<typename FunctionT>
struct function_traits;

template<typename ReturnTypeT, typename ... Args>
struct function_traits<ReturnTypeT(Args ...)>
{
static constexpr std::size_t arity = sizeof ... (Args);

template<std::size_t N>
using argument_type = typename std::tuple_element<N, std::tuple<Args ...>>::type;
};

template<typename ReturnTypeT, typename ... Args>
struct function_traits<ReturnTypeT (*)(Args ...)>: public function_traits<ReturnTypeT(Args ...)>
{};

/* ROS Node Interface.
*
* This is the single point of entry for creating publishers and subscribers.
Expand Down Expand Up @@ -109,19 +127,11 @@ class Node
rclcpp::callback_group::CallbackGroup::SharedPtr group = nullptr);

/* Create and return a Service. */
template<typename ServiceT>
typename rclcpp::service::Service<ServiceT>::SharedPtr
create_service(
const std::string & service_name,
typename rclcpp::service::Service<ServiceT>::CallbackType callback,
rclcpp::callback_group::CallbackGroup::SharedPtr group = nullptr);

/* Create and return a Service. */
template<typename ServiceT>
template<typename ServiceT, typename F>
Copy link
Member

Choose a reason for hiding this comment

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

Can we rename F to something "longer" like CallbackT?

Copy link
Member

Choose a reason for hiding this comment

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

I agree that a more verbose name would be good.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

typename rclcpp::service::Service<ServiceT>::SharedPtr
create_service(
const std::string & service_name,
typename rclcpp::service::Service<ServiceT>::CallbackWithHeaderType callback_with_header,
F callback,
rclcpp::callback_group::CallbackGroup::SharedPtr group = nullptr);

private:
Expand All @@ -148,6 +158,60 @@ class Node
const std::string & service_name,
std::shared_ptr<rclcpp::service::ServiceBase> serv_base_ptr,
rclcpp::callback_group::CallbackGroup::SharedPtr group);

template<
typename ServiceT,
typename FunctorT,
typename std::enable_if<
function_traits<FunctorT>::arity == 2 &&
std::is_same<
typename function_traits<FunctorT>::template argument_type<0>,
typename std::shared_ptr<typename ServiceT::Request>
>::value &&
std::is_same<
typename function_traits<FunctorT>::template argument_type<1>,
typename std::shared_ptr<typename ServiceT::Response>
>::value>::type * = nullptr>
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as #27 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Do you want to reformat this the same way as before?

typename rclcpp::service::Service<ServiceT>::SharedPtr
create_service_internal(
rmw_service_t * service_handle,
const std::string & service_name,
FunctorT callback)
{
typename rclcpp::service::Service<ServiceT>::CallbackType callback_without_header =
callback;
return service::Service<ServiceT>::make_shared(
service_handle, service_name, callback_without_header);
}

template<
typename ServiceT,
typename FunctorT,
typename std::enable_if<
function_traits<FunctorT>::arity == 3 &&
std::is_same<
typename function_traits<FunctorT>::template argument_type<0>,
std::shared_ptr<rmw_request_id_t>
>::value &&
std::is_same<
typename function_traits<FunctorT>::template argument_type<1>,
typename std::shared_ptr<typename ServiceT::Request>
>::value &&
std::is_same<
typename function_traits<FunctorT>::template argument_type<2>,
typename std::shared_ptr<typename ServiceT::Response>
>::value>::type * = nullptr>
typename rclcpp::service::Service<ServiceT>::SharedPtr
create_service_internal(
rmw_service_t * service_handle,
const std::string & service_name,
FunctorT callback)
{
typename rclcpp::service::Service<ServiceT>::CallbackWithHeaderType callback_with_header =
callback;
return service::Service<ServiceT>::make_shared(
service_handle, service_name, callback_with_header);
}
};

} /* namespace node */
Expand Down
46 changes: 6 additions & 40 deletions rclcpp/include/rclcpp/node_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,11 +182,11 @@ Node::create_client(
return cli;
}

template<typename ServiceT>
typename service::Service<ServiceT>::SharedPtr
template<typename ServiceT, typename FunctorT>
typename rclcpp::service::Service<ServiceT>::SharedPtr
Node::create_service(
const std::string & service_name,
typename rclcpp::service::Service<ServiceT>::CallbackType callback,
FunctorT callback,
rclcpp::callback_group::CallbackGroup::SharedPtr group)
{
using rosidl_generator_cpp::get_service_type_support_handle;
Expand All @@ -196,44 +196,9 @@ Node::create_service(
rmw_service_t * service_handle = rmw_create_service(
this->node_handle_, service_type_support_handle, service_name.c_str());

auto serv = service::Service<ServiceT>::make_shared(
service_handle,
service_name,
callback);
auto serv = create_service_internal<ServiceT>(service_handle, service_name,
callback);
Copy link
Member

Choose a reason for hiding this comment

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

Wrapping / indentation might not pass uncrustify?

Copy link
Member Author

Choose a reason for hiding this comment

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

I ran uncrustify before submitting this.

Copy link
Member

Choose a reason for hiding this comment

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

You are right - it passes. That looks like a bug in uncrustify to me then...

Since we want 2 space indentation can you please change it to the follow to pass uncrustify as well as fit with our style:

  auto serv = create_service_internal<ServiceT>(
    service_handle, service_name, callback);

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

auto serv_base_ptr = std::dynamic_pointer_cast<service::ServiceBase>(serv);
register_service(service_name, serv_base_ptr, group);
return serv;
}

template<typename ServiceT>
typename service::Service<ServiceT>::SharedPtr
Node::create_service(
const std::string & service_name,
typename rclcpp::service::Service<ServiceT>::CallbackWithHeaderType callback_with_header,
rclcpp::callback_group::CallbackGroup::SharedPtr group)
{
using rosidl_generator_cpp::get_service_type_support_handle;
auto service_type_support_handle =
get_service_type_support_handle<ServiceT>();

rmw_service_t * service_handle = rmw_create_service(
this->node_handle_, service_type_support_handle, service_name.c_str());

auto serv = service::Service<ServiceT>::make_shared(
service_handle,
service_name,
callback_with_header);
auto serv_base_ptr = std::dynamic_pointer_cast<service::ServiceBase>(serv);
register_service(service_name, serv_base_ptr, group);
return serv;
}

void
Node::register_service(
const std::string & service_name,
std::shared_ptr<rclcpp::service::ServiceBase> serv_base_ptr,
rclcpp::callback_group::CallbackGroup::SharedPtr group)
{
if (group) {
if (!group_in_node(group)) {
// TODO: use custom exception
Expand All @@ -244,6 +209,7 @@ Node::register_service(
default_callback_group_->add_service(serv_base_ptr);
}
number_of_services_++;
return serv;
}

#endif /* RCLCPP_RCLCPP_NODE_IMPL_HPP_ */