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

Service Introspection #990

Closed
wants to merge 16 commits into from
Closed

Conversation

ihasdapie
Copy link
Member

@ihasdapie ihasdapie commented Jun 29, 2022

This PR is a prototype for the service introspection REP (ros-infrastructure/rep#360) implementing the proposed serialized service event approach.

Few items of note

  • This would introduce new dependencies for rcl on rosidl_typesupport_introspection_c and rosidl_typesupport_c
  • This implements a rather hacky way of loading the respective Request/Response typesupports given a Service typesupport by inspecting the service typesupport's symbol names to find the service name, then using that to build the name of the Request/Response typesupport libaries and loading them in.
    rcl_ret_t
    rcl_service_typesupport_to_message_typesupport(
    const rosidl_service_type_support_t * service_typesupport,
    rosidl_message_type_support_t ** request_typesupport,
    rosidl_message_type_support_t ** response_typesupport,
    const rcl_allocator_t * allocator)
    {
    rcutils_ret_t ret;
    type_support_map_t * map = (type_support_map_t *)service_typesupport->data;
    const char * typesupport_library_fmt = "lib%s__rosidl_typesupport_c.so";
    const char * service_message_fmt = // package_name, type name, Request/Response
    "rosidl_typesupport_c__get_message_type_support_handle__%s__srv__%s_%s";
    const char * service_type_name = rcl_service_get_service_type_name(service_typesupport);
    // build out typesupport library and symbol names
    char * typesupport_library_name = allocator->allocate(
    sizeof(char) * ((strlen(typesupport_library_fmt) - 2) +
    strlen(map->package_name) + 1),
    allocator->state);
    char * request_message_symbol = allocator->allocate(
    sizeof(char) *
    ((strlen(service_message_fmt) - 6) + strlen(map->package_name) +
    strlen(service_type_name) + strlen("Request") + 1),
    allocator->state);
    char * response_message_symbol = allocator->allocate(
    sizeof(char) *
    ((strlen(service_message_fmt) - 6) + strlen(map->package_name) +
    strlen(service_type_name) + strlen("Request") + 1),
    allocator->state);
    sprintf(typesupport_library_name, typesupport_library_fmt, map->package_name);
    sprintf(request_message_symbol, service_message_fmt, map->package_name,
    service_type_name, "Request");
    sprintf(response_message_symbol, service_message_fmt, map->package_name,
    service_type_name, "Response");
    rcutils_shared_library_t typesupport_library = rcutils_get_zero_initialized_shared_library();
    ret = rcutils_load_shared_library(&typesupport_library, typesupport_library_name, *allocator);
    if (RCUTILS_RET_OK != ret) {
    return RCL_RET_ERROR;
    }
    rosidl_message_type_support_t *(*req_typesupport_func_handle)() =
    (rosidl_message_type_support_t * (*)())
    rcutils_get_symbol(&typesupport_library, request_message_symbol);
    RCL_CHECK_FOR_NULL_WITH_MSG(req_typesupport_func_handle,
    "Looking up request type support failed", return RCL_RET_ERROR);
    rosidl_message_type_support_t *(*resp_typesupport_func_handle)() =
    (rosidl_message_type_support_t * (*)())
    rcutils_get_symbol(&typesupport_library, response_message_symbol);
    RCL_CHECK_FOR_NULL_WITH_MSG(resp_typesupport_func_handle,
    "Looking up response type support failed", return RCL_RET_ERROR);
    *request_typesupport = req_typesupport_func_handle();
    *response_typesupport = resp_typesupport_func_handle();
    allocator->deallocate(typesupport_library_name, allocator->state);
    allocator->deallocate(request_message_symbol, allocator->state);
    allocator->deallocate(response_message_symbol, allocator->state);
    return RCL_RET_OK;
    }
  • The client_id is not populated on client send request events due to the writer_guid not being accessible from the client_send_request scope. PRs will be made to the rmw implementations to make this accessible.
  • There are no tests/docs/minimal error handling. It also definitely doesn't pass linting and there are superfluous #includes. I will fix this later :)

For instructions on how to build and run this see the meta-ticket ros2/ros2#1285

Resolves ros2/ros2#1285

Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
…shed

Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@clalancette
Copy link
Contributor

  • This would introduce new dependencies for rcl on rosidl_typesupport_introspection_c and rosidl_typesupport_c

I just took a quick look, and it looks like there is already an (build) indirect dependency between rcl and rosidl_typesupport_introspection_c/rosidl_typesupport_c:

rcl -> rcl_interfaces -> rosidl_default_generators -> rosidl_typesupport_introspection_c (same goes for rosidl_typesupport_c). Thus I think turning this into a direct dependency is fine.

Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
…ervice introspection

Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
…tion configure

Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

partial review

rcl/CMakeLists.txt Show resolved Hide resolved
rcl/include/rcl/introspection.h Outdated Show resolved Hide resolved
rcl/include/rcl/introspection.h Show resolved Hide resolved
rcl/include/rcl/introspection.h Show resolved Hide resolved
rcl/include/rcl/introspection.h Show resolved Hide resolved
rcl/include/rcl/introspection.h Show resolved Hide resolved
rcl/include/rcl/introspection.h Outdated Show resolved Hide resolved
rcl/include/rcl/introspection.h Show resolved Hide resolved
rcl/include/rcl/service.h Show resolved Hide resolved
rcl/include/rcl/introspection.h Show resolved Hide resolved
rcl/src/rcl/introspection.c Outdated Show resolved Hide resolved
rcl/package.xml Outdated Show resolved Hide resolved
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie ihasdapie self-assigned this Jul 14, 2022
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

The approach looks good to me. Besides the string manipulation related to service typesupport, the code looks straight-forward 👍 I also agree with Ivan's comments.

I realize you haven't focused on error handling yet, but when you do you should look out for the following (which might help uncover bugs downstream):

  1. Always check the return values of functions (this isn't happening in many places)
  2. Make sure to do any necessary memory deallocation before returning from functions (pay attention to places where you are returning early due to an error).
  3. Check arguments to functions before using them (e.g. if they are null or invalid).

rcl/include/rcl/introspection.h Show resolved Hide resolved
rcl/include/rcl/introspection.h Show resolved Hide resolved
rcl/src/rcl/introspection.c Show resolved Hide resolved
rcl/src/rcl/introspection.c Show resolved Hide resolved
rcl/src/rcl/introspection.c Show resolved Hide resolved
rcl/src/rcl/introspection.c Show resolved Hide resolved
rcl/src/rcl/introspection.c Show resolved Hide resolved
rcl/src/rcl/service.c Show resolved Hide resolved
rcl/src/rcl/service.c Show resolved Hide resolved
rcl/include/rcl/introspection.h Show resolved Hide resolved
@ihasdapie ihasdapie mentioned this pull request Aug 10, 2022
@ihasdapie
Copy link
Member Author

Closing in lieu of #997 to keep all service introspection work under a single branch name

@ivanpauno
Copy link
Member

ivanpauno commented Aug 11, 2022

@ihasdapie I think you meant to close this PR (replaced with #997), so I'm closing it now.
Please, reopen if that was not the case.

@ivanpauno ivanpauno closed this Aug 11, 2022
@ivanpauno ivanpauno deleted the service_introspection-serialized branch August 11, 2022 20:57
@ihasdapie
Copy link
Member Author

ihasdapie commented Aug 11, 2022

@ivanpauno Yup, thanks! I had forgotten.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Service Introspection
4 participants