C clients/services #30

jacquelinekay · 2016-02-26T22:17:22Z

Connects to #27

Implement C clients and services.

Not ready for review quite yet, but I would welcome a preliminary review of the .h files and the tests.

TODO:

Discuss design choices
- Storing and incrementing the sequence_number happens at the rcl layer. Should this variable be atomic?
- Do the fini methods need the node argument? Should I be doing something with them? (Resolution: keep the unused node argument for now and consider changing signature in rmw)
- the taken boolean is not exposed as an argument to the take functions, like it is in rmw. Instead, if take failed, a new error code (CLIENT_TAKE_FAILED or SERVICE_TAKE_FAILED) is returned. Is this OK? I see that rcl_take in subscription.h has a pointer to a taken boolean in the function signature. I suppose the take functions should at least be consistent with each other. @wjwwood any thoughts?
- request_header. Currently it's type-erased in rcl and rmw. Does it need to be? See Don't type-erase request header rmw#57
Comments. I copy pasted the headers and the comments currently reflect that. I'll take the time to correct them and write comments that are at least as extensive as the publisher/subscription comments.
- In particular, comments about multithreading and allocation behavior
Code cleanup/linting
Multithreaded/multi-process test (optional?)

jacquelinekay · 2016-02-27T02:58:08Z

The documentation should be fairly coherent now (?).

wjwwood · 2016-02-29T01:17:57Z

rcl/include/rcl/client.h

+ *    rcl_node_options_t node_ops = rcl_node_get_default_options();
+ *    rcl_ret_t ret = rcl_node_init(&node, "node_name", &node_ops);
+ *    // ... error handling
+ *    rosidl_service_type_support_t * ts = ROSIDL_GET_MESSAGE_TYPE_SUPPORT(


Should be ROSIDL_GET_SERVICE_TYPE_SUPPORT?

wjwwood · 2016-02-29T02:02:28Z

I got through the headers with only minor comments and questions (I'll try to make a pr tonight to address the obvious changes).

As for your OP questions:

Storing and incrementing the sequence_number happens at the rcl layer. Should this variable be atomic?

Probably, since calling send_request and send_response should probably be thread safe, like publish is.

Do the fini methods need the node argument? Should I be doing something with them?

They are passed through to the rmw layer and at least one of the functions does need the original node handle to unregister types or something like that. We could argue that the client/service/publisher/etc should hold on to the dds participant or what ever it needs and use it during fini. The problem then becomes ownership of the pointer to that object. Since node has to be passed to the client's fini function, it's obvious that the node needs to be valid, otherwise it might make sense to fini the node and then the client (which would obviously be an issue if the client's fini function used a thing that is owned by the node that just got fini).

the taken boolean is not exposed as an argument to the take functions, like it is in rmw. Instead, if take failed, a new error code (CLIENT_TAKE_FAILED or SERVICE_TAKE_FAILED) is returned. Is this OK? I see that rcl_take in subscription.h has a pointer to a taken boolean in the function signature. I suppose the take functions should at least be consistent with each other. @wjwwood any thoughts?

This is probably historical. I think we either were copying the style of the matching DDS function or we have some part of the take function chain that doesn't have many error codes (just ok or error), so taken was passed separately back and we copied that behavior. Either way I think using an error code would be ok, since we also do this for timeout. The only semantic squabble is that take can technically succeed even if no data is taken so it's not quite correct to say that taking no data is a failure.

request_header. Currently it's type-erased in rcl and rmw. Does it need to be?

I don't know. If the type of request_header is something we can share (in an rcl header) and its type doesn't change based on the message type, then no it does not need to be type erased.

jacquelinekay · 2016-02-29T03:50:40Z

"They are passed through to the rmw layer and at least one of the functions does need the original node handle to unregister types or something like that. We could argue that the client/service/publisher/etc should hold on to the dds participant or what ever it needs and use it during fini. The problem then becomes ownership of the pointer to that object. Since node has to be passed to the client's fini function, it's obvious that the node needs to be valid, otherwise it might make sense to fini the node and then the client (which would obviously be an issue if the client's fini function used a thing that is owned by the node that just got fini)."

I guess I was confused because the function signatures rmw_destroy_client and rmw_destroy_service don't include a rmw_node pointer:

https://github.com/ros2/rmw/blob/master/rmw/include/rmw/rmw.h#L120
https://github.com/ros2/rmw/blob/master/rmw/include/rmw/rmw.h#L151

jacquelinekay · 2016-02-29T04:10:11Z

whoops, didn't see your thing about making a PR for the obvious stuff so I took care of it in 174feb6. More small adjustments are always welcome...

wjwwood · 2016-02-29T09:08:47Z

Oh, I was just going to make a pr to help you out, but if you already found time to fix it up then that's fine.

Yeah I'm not sure about node being passed to the client service finis. Maybe they shouldn't.

jacquelinekay · 2016-02-29T23:24:05Z

Regarding the node pointer in client_fini and service_fini:
Part of your original point was that the node pointer should be passed to fini to indicate ownership.
I agree with this in principal, but it bothers me that the rmw functions do not do this.
rmw_opensplice has a roundabout implementation of destroy_service (and destroy_client), but it does everything it eventually needs to do:

callbacks->destroy_responder(service_info->responder_, &rmw_free);

https://github.com/ros2/rmw_opensplice/blob/master/rmw_opensplice_cpp/src/rmw_service.cpp#L206
Which, through the typesupport callbacks, calls Responder::teardown. Responder stores a pointer to the DDS Participant and calls the Participant::delete_... functions you would expect to clean up the service:
https://github.com/ros2/rmw_opensplice/blob/master/rosidl_typesupport_opensplice_cpp/include/rosidl_typesupport_opensplice_cpp/responder.hpp#L168
So destroy_service eventually accesses the DDS Participant. The rmw_node that the service was added to points to the same participant, but the participant owned by Responder has no idea about the rmw_node pointer.
However, in Connext the Participant is never accessed directly in destroy due to the usage of Requester/Replier API.
I think I'm going to leave the node pointer in rcl_client_fini and rcl_service_fini and ticket adding the rmw_node pointer to rmw_destroy_client and rmw_destroy_service if needed.

Regarding request_header and response_header:
The type of this object seems poorly defined right now. Here's a rough sketch:
In rclcpp:
In Client and Service, create_request_header returns a shared_ptr, but the underlying type is rmw_request_id_t.
https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/client.hpp#L111
https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/service.hpp#L109

In rcl (current state of this PR):
The pointer is passed straight to rmw as type void *.

In rmw:
request_header in take_request, send_response, and take_response is of type void.
https://github.com/ros2/rmw/blob/master/rmw/include/rmw/rmw.h#L133-L137
https://github.com/ros2/rmw/blob/master/rmw/include/rmw/rmw.h#L156-L160
https://github.com/ros2/rmw/blob/master/rmw/include/rmw/rmw.h#L165-L168

In typesupport:
request_header is always cast to an rmw_request_id_t*.
https://github.com/ros2/rmw_opensplice/blob/master/rosidl_typesupport_opensplice_cpp/resource/srv__type_support.cpp.template#L434
https://github.com/ros2/rmw_connext/blob/master/rosidl_typesupport_connext_cpp/resource/srv__type_support.cpp.template#L132-L133

I don't see why the request_header argument is sometimes type-erased when the type is always the same. So I think we can make motions to make the type always rmw_request_id_t. The other option would be to create an rmw_request_header_t struct in case we anticipate adding more metadata at the rcl layer.

wjwwood · 2016-03-01T01:29:58Z

I think I'm going to leave the node pointer in rcl_client_fini and rcl_service_fini and ticket adding the rmw_node pointer to rmw_destroy_client and rmw_destroy_service if needed.

Sounds good to me.

I don't see why the request_header argument is sometimes type-erased when the type is always the same. So I think we can make motions to make the type always rmw_request_id_t. The other option would be to create an rmw_request_header_t struct in case we anticipate adding more metadata at the rcl layer.

That sounds appropriate, especially if there is both a request and a response header.

jacquelinekay · 2016-03-04T21:57:08Z

I think the C Services PRs are ready for review. I can try separating out the changes a bit more if it makes things easier.

http://ci.ros2.org/job/ci_linux/1071/
http://ci.ros2.org/job/ci_osx/872/

wjwwood · 2016-03-04T22:24:50Z

@jacquelinekay I'm starting a review now.

jacquelinekay · 2016-03-04T23:08:42Z

I fixed the warnings and the test failure in rclpy: http://ci.ros2.org/job/ci_linux/1073/

jacquelinekay · 2016-03-06T21:07:36Z

OSX passes after some fiddling with linking in the generators:
http://ci.ros2.org/job/ci_osx/875/

Onto Windows!

jacquelinekay · 2016-03-08T17:12:54Z

OK, this Windows CI job appears to have the same number of warnings and test failures as the nightly Release job:

http://ci.ros2.org/job/ci_windows/1140/

wjwwood · 2016-03-09T20:05:35Z

rcl/include/rcl/client.h

+/* After calling this function on a rcl_client_t, it can be used to send requests of the given
+ * type by calling rcl_send_request().
+ * Once a response has been sent by the service, the response is available to the client when
+ * rcl_take_response() is called.


Is it more correct to say "Once a response has been sent by the service, a response may become available in the future and if so it can be retrieved using rcl_take_response()."? As it is currently worded it sounds like calling rcl_take_response is what makes it available, which I think is misleading.

wjwwood · 2016-03-09T22:47:19Z

Other than my comments, lgtm.

Btw, won't this require a related change in rclpy since the signature of rcl_wait_set_init has changed?

jacquelinekay · 2016-03-09T22:48:53Z

Yep, that's one of the many PRs linked to #27 on Waffle: ros2/rclpy#7

The values for number of clients and number of services are currently both hardcoded as zero until Python services are implemented.

wjwwood · 2016-03-09T22:50:34Z

rcl/include/rcl/client.h

+ * \return RCL_RET_OK if the response was taken successfully, or
+ *         RCL_RET_INVALID_ARGUMENT if any arguments are invalid, or
+ *         RCL_RET_CLIENT_INVALID if the client is invalid, or
+ *         RCL_RET_ERROR if an unspecified error occurs.


RCL_RET_CLIENT_TAKE_FAILED doesn't seem to be documented here. Also did we ever decide if that was better (preferable) to passing a bool * taken parameter?

wjwwood · 2016-03-09T22:51:20Z

Yep, that's one of the many PRs linked to #27 on Waffle: ros2/rclpy#7

Awesome, sorry I missed that connection.

Fixed up include guard names to RMW

…_local_endpoints Ignore local endpoints: RCL

jacquelinekay added the in progress Actively being worked on (Kanban column) label Feb 26, 2016

jacquelinekay self-assigned this Feb 26, 2016

jacquelinekay mentioned this pull request Feb 26, 2016

Services #27

Closed

5 tasks

wjwwood reviewed Feb 29, 2016
View reviewed changes

This was referenced Mar 1, 2016

Don't type-erase request header ros2/rmw#57

Merged

Consider adding rmw_node * to rmw_destroy_client and rmw_destroy_service ros2/rmw#58

Closed

jacquelinekay mentioned this pull request Mar 2, 2016

Add service name to client and service ros2/rmw#56

Merged

jacquelinekay force-pushed the c_services branch 2 times, most recently from d5cd824 to 5655078 Compare March 4, 2016 18:08

wjwwood reviewed Mar 9, 2016
View reviewed changes

jacquelinekay force-pushed the c_services branch from 2eca373 to 61bb8fe Compare March 9, 2016 23:32

Add services and clients

534a49c

jacquelinekay force-pushed the c_services branch from fd20808 to 534a49c Compare March 24, 2016 22:24

jacquelinekay merged commit 494cfc5 into master Mar 24, 2016

jacquelinekay deleted the c_services branch March 24, 2016 23:55

jacquelinekay self-assigned this Mar 24, 2016

jacquelinekay removed the in progress Actively being worked on (Kanban column) label Mar 24, 2016

ivanpauno pushed a commit that referenced this pull request Jan 2, 2020

Merge pull request #30 from spaghetti-/gaurds

129b348

Fixed up include guard names to RMW

mauropasse pushed a commit to mauropasse/rcl that referenced this pull request May 1, 2024

Merge pull request ros2#30 from mauropasse/mauro/irobot-humble-ignore…

f5b0ea2

…_local_endpoints Ignore local endpoints: RCL

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C clients/services #30

C clients/services #30

jacquelinekay commented Feb 26, 2016

jacquelinekay commented Feb 27, 2016

wjwwood Feb 29, 2016

wjwwood commented Feb 29, 2016

jacquelinekay commented Feb 29, 2016

jacquelinekay commented Feb 29, 2016

wjwwood commented Feb 29, 2016

jacquelinekay commented Feb 29, 2016

wjwwood commented Mar 1, 2016

jacquelinekay commented Mar 4, 2016

wjwwood commented Mar 4, 2016

jacquelinekay commented Mar 4, 2016

jacquelinekay commented Mar 6, 2016

jacquelinekay commented Mar 8, 2016

wjwwood Mar 9, 2016

wjwwood commented Mar 9, 2016

jacquelinekay commented Mar 9, 2016

wjwwood Mar 9, 2016

wjwwood commented Mar 9, 2016

C clients/services #30

C clients/services #30

Conversation

jacquelinekay commented Feb 26, 2016

jacquelinekay commented Feb 27, 2016

wjwwood Feb 29, 2016

Choose a reason for hiding this comment

wjwwood commented Feb 29, 2016

jacquelinekay commented Feb 29, 2016

jacquelinekay commented Feb 29, 2016

wjwwood commented Feb 29, 2016

jacquelinekay commented Feb 29, 2016

wjwwood commented Mar 1, 2016

jacquelinekay commented Mar 4, 2016

wjwwood commented Mar 4, 2016

jacquelinekay commented Mar 4, 2016

jacquelinekay commented Mar 6, 2016

jacquelinekay commented Mar 8, 2016

wjwwood Mar 9, 2016

Choose a reason for hiding this comment

wjwwood commented Mar 9, 2016

jacquelinekay commented Mar 9, 2016

wjwwood Mar 9, 2016

Choose a reason for hiding this comment

wjwwood commented Mar 9, 2016