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

Services post api review #11

Merged
merged 3 commits into from
Mar 9, 2015
Merged

Services post api review #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9daec00
Added support for RTI Connext services. Update rmw_take* signatures
Mar 6, 2015
858671a
Merge remote-tracking branch 'origin/master' into services-post-api-r…
Mar 6, 2015
bb8cad7
Fix order of parameters
Mar 6, 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
9 changes: 5 additions & 4 deletions rclcpp/include/rclcpp/client.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@ class ClientBase

virtual std::shared_ptr<void> create_response() = 0;
virtual std::shared_ptr<void> create_request_header() = 0;
virtual void handle_response(std::shared_ptr<void> &response, std::shared_ptr<void> &req_id) = 0;
virtual void handle_response(std::shared_ptr<void> &request_header,
std::shared_ptr<void> &response) = 0;
Copy link
Member

Choose a reason for hiding this comment

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

We really need to settle on the code style. I dislike this indentation so much...

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'm neutral about this, though. But I'd be more than happy if there's a code style and we all follow it.

Copy link
Member

Choose a reason for hiding this comment

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

Most of the code styles are not specific about how to deal with this particular case. I guess you would prefer:

virtual void handle_response(
    a,
    b) = 0;

Or

virtual void handle_response(
    a,
    b
) = 0;

Something else?


private:
RCLCPP_DISABLE_COPY(ClientBase);
Expand Down Expand Up @@ -106,11 +107,11 @@ class Client : public ClientBase
return std::shared_ptr<void>(new rmw_request_id_t);
}

void handle_response(std::shared_ptr<void> &response, std::shared_ptr<void> &req_id)
void handle_response(std::shared_ptr<void> &request_header, std::shared_ptr<void> &response)
{
auto typed_req_id = std::static_pointer_cast<rmw_request_id_t>(req_id);
auto typed_request_header = std::static_pointer_cast<rmw_request_id_t>(request_header);
auto typed_response = std::static_pointer_cast<typename ServiceT::Response>(response);
int64_t sequence_number = typed_req_id->sequence_number;
int64_t sequence_number = typed_request_header->sequence_number;
auto tuple = this->pending_requests_[sequence_number];
auto call_promise = std::get<0>(tuple);
auto callback = std::get<1>(tuple);
Expand Down
28 changes: 16 additions & 12 deletions rclcpp/include/rclcpp/executor.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,8 @@ class Executor
rclcpp::subscription::SubscriptionBase::SharedPtr &subscription)
{
std::shared_ptr<void> message = subscription->create_message();
auto taken = rmw_take(subscription->subscription_handle_, message.get());
// TODO(wjwwood): taken is no longer a boolean, check against return types.
bool taken = false;
rmw_take(subscription->subscription_handle_, message.get(), &taken);
if (taken)
{
subscription->handle_message(message);
Expand All @@ -176,14 +176,16 @@ class Executor
execute_service(
rclcpp::service::ServiceBase::SharedPtr &service)
{
std::shared_ptr<void> request = service->create_request();
std::shared_ptr<void> request_header = service->create_request_header();
bool taken = rmw_take_request(service->service_handle_,
request.get(),
request_header.get());
std::shared_ptr<void> request = service->create_request();
bool taken = false;
rmw_take_request(service->service_handle_,
request_header.get(),
request.get(),
&taken);
if (taken)
{
service->handle_request(request, request_header);
service->handle_request(request_header, request);
}
else
{
Expand All @@ -197,14 +199,16 @@ class Executor
execute_client(
rclcpp::client::ClientBase::SharedPtr &client)
{
std::shared_ptr<void> response = client->create_response();
std::shared_ptr<void> request_header = client->create_request_header();
bool taken = rmw_take_response(client->client_handle_,
response.get(),
request_header.get());
std::shared_ptr<void> response = client->create_response();
bool taken = false;
rmw_take_response(client->client_handle_,
request_header.get(),
response.get(),
&taken);
if (taken)
{
client->handle_response(response, request_header);
client->handle_response(request_header, response);
}
else
{
Expand Down
22 changes: 8 additions & 14 deletions rclcpp/include/rclcpp/node_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,8 @@ Node::create_publisher(std::string topic_name, size_t queue_size)
{
using rosidl_generator_cpp::get_type_support_handle;
auto type_support_handle = get_type_support_handle<MessageT>();
auto publisher_handle = rmw_create_publisher(node_handle_,
type_support_handle,
topic_name.c_str(),
queue_size);
rmw_publisher_t * publisher_handle = rmw_create_publisher(
node_handle_, type_support_handle, topic_name.c_str(), queue_size);

return publisher::Publisher::make_shared(publisher_handle);
}
Expand Down Expand Up @@ -98,10 +96,8 @@ Node::create_subscription(
{
using rosidl_generator_cpp::get_type_support_handle;
auto type_support_handle = get_type_support_handle<MessageT>();
auto subscriber_handle = rmw_create_subscription(node_handle_,
type_support_handle,
topic_name.c_str(),
queue_size);
rmw_subscription_t * subscriber_handle = rmw_create_subscription(
node_handle_, type_support_handle, topic_name.c_str(), queue_size);

using namespace rclcpp::subscription;

Expand Down Expand Up @@ -171,9 +167,8 @@ Node::create_client(
auto service_type_support_handle =
get_service_type_support_handle<ServiceT>();

auto client_handle = rmw_create_client(this->node_handle_,
service_type_support_handle,
service_name.c_str());
rmw_client_t * client_handle = rmw_create_client(
this->node_handle_, service_type_support_handle, service_name.c_str());

using namespace rclcpp::client;

Expand Down Expand Up @@ -211,9 +206,8 @@ Node::create_service(
auto service_type_support_handle =
get_service_type_support_handle<ServiceT>();

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

using namespace rclcpp::service;

Expand Down
9 changes: 5 additions & 4 deletions rclcpp/include/rclcpp/service.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ class ServiceBase

virtual std::shared_ptr<void> create_request() = 0;
virtual std::shared_ptr<void> create_request_header() = 0;
virtual void handle_request(std::shared_ptr<void> &request, std::shared_ptr<void> &req_id) = 0;
virtual void handle_request(std::shared_ptr<void> &request_header,
std::shared_ptr<void> &request) = 0;

private:
RCLCPP_DISABLE_COPY(ServiceBase);
Expand Down Expand Up @@ -103,13 +104,13 @@ class Service : public ServiceBase
return std::shared_ptr<void>(new rmw_request_id_t);
}

void handle_request(std::shared_ptr<void> &request, std::shared_ptr<void> &req_id)
void handle_request(std::shared_ptr<void> &request_header, std::shared_ptr<void> &request)
{
auto typed_request = std::static_pointer_cast<typename ServiceT::Request>(request);
auto typed_req_id = std::static_pointer_cast<rmw_request_id_t>(req_id);
auto typed_request_header = std::static_pointer_cast<rmw_request_id_t>(request_header);
auto response = std::shared_ptr<typename ServiceT::Response>(new typename ServiceT::Response);
callback_(typed_request, response);
send_response(typed_req_id, response);
send_response(typed_request_header, response);
}

void send_response(std::shared_ptr<rmw_request_id_t> &req_id,
Expand Down