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 #997

Merged
merged 6 commits into from
Feb 28, 2023
Merged

Service introspection #997

merged 6 commits into from
Feb 28, 2023

Conversation

ihasdapie
Copy link
Member

This PR is part of a prototype implementation for ros-infrastructure/rep#360 ros2/ros2#1285.

Elements of note

  • service introspection logic is encapsulated in introspection.{c.h} (file name subject to change) and hooks into the service init/fini and various send/take request/response functions.

  • runtime configuration done via rcl_service_introspection_configure_client_events etc. functions exposed to client library to be called on parameter event callbacks

  • Services and headers have been broken up into service_impl.h and service.h in line with what is done for publishers to improve dependency ergonomics with service introspection (and same for clients)

  • Service event message creationg and population is delegated to typesupport libraries

  • rcl_{service,client}_options_t now has a clock and enable_service_introspection member.

  • Related issue: Service Introspection ros2#1285

  • Note: this PR replaces Service Introspection #990. Reviews left on that PR have yet be resolved as of time of creating this PR but will be resolved in this PR.

@ihasdapie
Copy link
Member Author

ihasdapie commented Aug 10, 2022

b40a655 is a refactor partially addressing the review in #990.

  • introspection.h to service_event_publisher.h
  • rcl_service_introspection_utils_t rename to rcl_service_event_publisher_t and accompanying refactor
  • Hiding service introspection implementation by introducing src/service_event_publisher.h and exposing public API via service.h, client.h, and include/service_introspection.h
  • Refactor to use pimpl design pattern

Commits reconciling these changes with rclcpp and rclpy are here:

@ihasdapie
Copy link
Member Author

service_event_publisher qos is now exposed in rcl and configurable in rclcpp and rclpy
See: #990 (comment)

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/include/rcl/client.h Outdated Show resolved Hide resolved
rcl/include/rcl/client.h Outdated Show resolved Hide resolved
rcl/include/rcl/client.h Outdated Show resolved Hide resolved
rcl/include/rcl/client.h Outdated Show resolved Hide resolved
rcl/include/rcl/node_options.h Outdated Show resolved Hide resolved
rcl/include/rcl/service_introspection.h Outdated Show resolved Hide resolved
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.

Partial review of the client code changes. Some of the comments apply to services as well.

rcl/CMakeLists.txt Show resolved Hide resolved
rcl/CMakeLists.txt Outdated Show resolved Hide resolved
rcl/CMakeLists.txt Outdated Show resolved Hide resolved
rcl/CMakeLists.txt Outdated Show resolved Hide resolved
rcl/include/rcl/client.h Outdated Show resolved Hide resolved
rcl/include/rcl/service.h Outdated Show resolved Hide resolved
rcl/include/rcl/service_introspection.h Outdated Show resolved Hide resolved
rcl/src/rcl/client.c Outdated Show resolved Hide resolved
rcl/src/rcl/client.c Outdated Show resolved Hide resolved
rcl/src/rcl/client.c Outdated Show resolved Hide resolved
rcl/src/rcl/service_event_publisher.h Outdated Show resolved Hide resolved
rcl/src/rcl/service_event_publisher.c Outdated Show resolved Hide resolved
rcl/src/rcl/service_event_publisher.c Outdated Show resolved Hide resolved
@jacobperron jacobperron self-assigned this Sep 1, 2022
@clalancette clalancette force-pushed the service_introspection branch 2 times, most recently from 484c73e to 07fb3ba Compare February 6, 2023 17:49
@clalancette clalancette force-pushed the service_introspection branch 2 times, most recently from 400b531 to c12351c Compare February 21, 2023 18:44
@clalancette
Copy link
Contributor

I'm in the middle of a rewrite here, so actually changing back to a draft for now.

@clalancette clalancette marked this pull request as draft February 22, 2023 19:04
@clalancette clalancette force-pushed the service_introspection branch from f79e6d2 to db4ebe7 Compare February 23, 2023 18:46
@clalancette
Copy link
Contributor

This has been substantially rewritten now, so I'm going to dismiss all of the review comments, and pull this out of draft.

@clalancette clalancette marked this pull request as ready for review February 23, 2023 22:10
@clalancette clalancette changed the title Service Introspection Service introspection Feb 23, 2023
ihasdapie and others added 4 commits February 24, 2023 14:51
This PR adds in the rcl implementation of service introspection.
In particular, what it adds in are the implementations of enabling
and disabling service introspection, as well as creating the publisher
when the introspection is enabled.

Signed-off-by: Brian Chen <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
The idea here is that by default, clients and services are
created just like they were before.  However, we add an
additional API where the user can choose to configure
service introspection, either to turn it OFF, send out
METADATA, or send out CONTENTS (and METADATA).

There are 4 different events that can get sent out if
introspection is configured for METADATA or CONTENTS;
REQUEST_SENT (from the client), REQUEST_RECEIVED (from
the service), RESPONSE_SENT (from the service), or
RESPONSE_RECEIVED (from the client).  For each of these,
a separate message is sent out a topic called
<service_name>_service_event , so an outside observer can
listen.

Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Along the way, I noticed quite a few potential bugs, so
this ends up mostly being a rewrite of the tests.  With
this in place, tests pass on all RMWs.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette clalancette force-pushed the service_introspection branch from 75ea33e to fe72a35 Compare February 24, 2023 15:01
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
@clalancette clalancette merged commit 894bbd1 into rolling Feb 28, 2023
@delete-merged-branch delete-merged-branch bot deleted the service_introspection branch February 28, 2023 18:42
@clalancette
Copy link
Contributor

I should mention that CI for this is in ros2/ros2#1285 (comment)

danthony06 pushed a commit to danthony06/rcl that referenced this pull request Jun 14, 2023
* Add in the APIs to enable service introspection.

This PR adds in the rcl implementation of service introspection.
In particular, what it adds in are the implementations of enabling
and disabling service introspection, as well as creating the publisher
when the introspection is enabled.

The idea here is that by default, clients and services are
created just like they were before.  However, we add an
additional API where the user can choose to configure
service introspection, either to turn it OFF, send out
METADATA, or send out CONTENTS (and METADATA).

There are 4 different events that can get sent out if
introspection is configured for METADATA or CONTENTS;
REQUEST_SENT (from the client), REQUEST_RECEIVED (from
the service), RESPONSE_SENT (from the service), or
RESPONSE_RECEIVED (from the client).  For each of these,
a separate message is sent out a topic called
<service_name>_service_event , so an outside observer can
listen.

Signed-off-by: Brian Chen <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
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.

6 participants