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 cpp #885

Closed
3 tasks
Yadunund opened this issue Apr 5, 2023 · 9 comments
Closed
3 tasks

Service introspection cpp #885

Yadunund opened this issue Apr 5, 2023 · 9 comments

Comments

@Yadunund
Copy link
Member

Yadunund commented Apr 5, 2023

Setup

  • DDS vendor: Cyclone
  • BuildType: Debian
  • Os: Ubuntu Jammy
  • Chip: Amd64

Links

Checks

  • Run the client and server

    details

    Try

    1. # User input in terminal 1
      ros2 launch demo_nodes_cpp introspect_services_launch.py
    2. # User input in terminal 2
      ros2 topic list --include-hidden-topics

    Expect

    1. # stdout in terminal 1
      [introspection_service-1] a: 2 b: 3
      [introspection_client-2] [INFO] [1680585324.964598437] [introspection_client]: Result of add_two_ints: 5
    2. # stdout in terminal 2
      /parameter_events
      /rosout
  • Configure service to send introspection metadata

    details

    Try

    1. # User input in terminal 1
      ros2 launch demo_nodes_cpp introspect_services_launch.py
    2. # User input in terminal 2
      ros2 param set /introspection_service service_configure_introspection metadata
    3. # User input in terminal 2
      ros2 topic echo /add_two_ints/_service_event

    Expect

    1. # stdout in terminal 1
      [introspection_service-1] a: 2 b: 3
      [introspection_client-2] [INFO] [1680585324.964598437] [introspection_client]: Result of add_two_ints: 5
    2. # stdout in terminal 2
      info:
        event_type: 1
        stamp:
          sec: 1680586531
          nanosec: 726539263
        client_gid:
        - 83
        - 143
        - 248
        - 38
        - 47
        - 231
        - 185
        - 230
        - 98
        - 17
        - 45
        - 187
        - 128
        - 124
        - 253
        - 156
        sequence_number: 46
      request: []
      response: []
      ---
      info:
        event_type: 2
        stamp:
          sec: 1680586531
          nanosec: 726695003
        client_gid:
        - 83
        - 143
        - 248
        - 38
        - 47
        - 231
        - 185
        - 230
        - 98
        - 17
        - 45
        - 187
        - 128
        - 124
        - 253
        - 156
        sequence_number: 46
      request: []
      response: []
  • Configure client to send introspection metadata

    details

    Try

    1. # User input in terminal 1
      ros2 launch demo_nodes_cpp introspect_services_launch.py
    2. # User input in terminal 2
      ros2 param set /introspection_client client_configure_introspection metadata
    3. # User input in terminal 2
      ros2 topic echo /add_two_ints/_service_event

    Expect

    1. # stdout in terminal 1
      [introspection_service-1] a: 2 b: 3
      [introspection_client-2] [INFO] [1680585324.964598437] [introspection_client]: Result of add_two_ints: 5
    2. # stdout in terminal 2
      info:
        event_type: 0
        stamp:
          sec: 1680586637
          nanosec: 158736272
        client_gid:
        - 1
        - 16
        - 34
        - 142
        - 114
        - 128
        - 207
        - 116
        - 98
        - 246
        - 204
        - 52
        - 0
        - 0
        - 0
        - 7
        sequence_number: 48
      request: []
      response: []
      ---
      info:
        event_type: 3
        stamp:
          sec: 1680586637
          nanosec: 159157853
        client_gid:
        - 1
        - 16
        - 34
        - 142
        - 114
        - 128
        - 207
        - 116
        - 98
        - 246
        - 204
        - 52
        - 0
        - 0
        - 0
        - 7
        sequence_number: 48
      request: []
      response: []

You can find the code used to generate this test case here

@jacobperron
Copy link

The commands work for me, however when trying to disable service events by setting the ROS parameter to off or "off" from the command-line, we get the following error:

$ ros2 param set /introspection_client client_configure_introspection off
Setting parameter failed: Wrong parameter type, parameter {client_configure_introspection} is of type {string}, setting it to {bool} is not allowed.

A workaround is to pass in an explicit set of quotes, like:

$ ros2 param set /introspection_client client_configure_introspection \"off\"

Perhaps it is easily fixed by picking a different keyword for the demo. Instead of "off", we could use "none" (for example) here:

https://github.com/ros2/demos/blob/ab645317895b36e04753b269a9ce11b532023533/demo_nodes_cpp/src/services/introspection_client.cpp#L87

@nirwester
Copy link

nirwester commented May 9, 2023

Hi, I tested using a simplified version of the code that only enables the full introspection on both the client and the server, full code here:

https://github.com/nirwester/ros2_journey_examples/tree/master/homeworks/chap4/service_with_introspection
Running on this docker container (jammy):
osrf/docker_images#673

And FastRTPS (I couldn't find an open ticket specific for it). I encontered the following issue:

If executing the following sequence of actions:

  • starting the server
  • starting the client
  • wait some seconds to make sure that the execution is finished
  • subscribe: ros2 topic echo /add_two_ints/_service_event

The following last response sent by the server is received, even if the service is no more running:

info:
event_type: 2
stamp:
sec: 1683653877
nanosec: 751816926
client_gid:

  • 1
  • 15
  • 149
  • 130
  • 165
  • 6
  • 126
  • 77
  • 0
  • 0
  • 0
  • 0
  • 0
  • 0
  • 18
  • 4
    sequence_number: 1
    request: []
    response:
  • sum: 3

This would seem an issue in the durability of the corresponding publisher, probably set to transient_local. I don't know if there's any technical reason for this kind of implementation, but getting the last published results upon subscription is confusing.

Apart from this issue, the feature is working (all 4 messages are received if the subscription is performed before running the client).

@Yadunund
Copy link
Member Author

@jacobperron thanks for the feedback! I agree none would be a better option and have opened a ticket ros2/demos#625

@nirwester are you able to confirm if you see this problem outside a docker container?

@MarcoMatteoBassa
Copy link

@Yadunund not within a short time, sorry, but I would be surprised if this would be docker-specific. Maybe @jacobperron could try to briefly reproduce it too?

@clalancette
Copy link
Collaborator

This would seem an issue in the durability of the corresponding publisher, probably set to transient_local. I don't know if there's any technical reason for this kind of implementation, but getting the last published results upon subscription is confusing.

I was able to reproduce the issue, and I see what you mean.

I could see us going one of two ways here:

  1. Leave it as TRANSIENT_LOCAL, but give it a large queue depth. That means that late-joining subscribers would be able to get all of the previous data, at the expense of using a lot of memory.
  2. Change it to VOLATILE and just don't worry about late-joining subscribers.

I lean towards the latter, but maybe there was some other thinking here. @jacobperron do you know if it was ever discussed to keep this TRANSIENT_LOCAL for some reason?

Regardless, I think we should do some follow-up work here. @nirwester would you mind opening a bug about this on https://github.com/ros2/rcl ? We can follow-up more there.

@nirwester
Copy link

Thanks @clalancette , I agree that the volatile choice sounds more reasonable. I opened an issue here ros2/rcl#1068

@clalancette
Copy link
Collaborator

Thanks, much appreciated.

@jacobperron
Copy link

@clalancette I don't recall a discussion about using transient local durability. The proposed design document suggest we should use the default QoS for service event topics (ros-infrastructure/rep#360):

The service event topics proposed in this REP shall use the default quality of service settings.

@clalancette
Copy link
Collaborator

@clalancette I don't recall a discussion about using transient local durability. The proposed design document suggest we should use the default QoS for service event topics (ros-infrastructure/rep#360):

All right, thanks. It may just be a bug in the implementation then; I'll take a look and see what I can find.

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

No branches or pull requests

5 participants