Add ros2 node get_type_description initial implementation #846

emersonknapp · 2023-07-28T07:15:49Z

Replaces #817

Implements a first pass at ros2 node get_type_description. It does its best to autofill the type hash so the user doesn't have to, but given the limitations of the current API this is only possible for msg on pub/sub topics, not for srv/action. For those, the hash must be provided manually by the user, though it is not discoverable via any existing API.

Sample output:

$ ros2 node get-type-description /talker rcl_interfaces/msg/Log
Waiting for service to become available...
Requesting type hash RIHS01_e28ce254ca8abc06abf92773b74602cdbf116ed34fbaf294fb9f81da9f318eac
Sending request...
Response successful:

rcl_interfaces/msg/Log
Fields:
  stamp: FIELD_TYPE_NESTED_TYPE (builtin_interfaces/msg/Time)
  level: FIELD_TYPE_UINT8
  name: FIELD_TYPE_STRING
  msg: FIELD_TYPE_STRING
  file: FIELD_TYPE_STRING
  function: FIELD_TYPE_STRING
  line: FIELD_TYPE_UINT32

Referenced Type Descriptions:

  builtin_interfaces/msg/Time
  Fields:
    sec: FIELD_TYPE_INT32
    nanosec: FIELD_TYPE_UINT32

$ ros2 node get-type-description /talker type_description_interfaces/srv/GetTypeDescription RIHS01_69b9c19c1021405984cc60dbbb1edceb147a6538b411d812ba6afabeed962cd5
Sending request...                                                                                                                                                                               
Response successful:

type_description_interfaces/srv/GetTypeDescription
Fields:
  request_message: FIELD_TYPE_NESTED_TYPE (type_description_interfaces/srv/GetTypeDescription_Request)
  response_message: FIELD_TYPE_NESTED_TYPE (type_description_interfaces/srv/GetTypeDescription_Response)
  event_message: FIELD_TYPE_NESTED_TYPE (type_description_interfaces/srv/GetTypeDescription_Event)

Referenced Type Descriptions:

  builtin_interfaces/msg/Time
  Fields:
    sec: FIELD_TYPE_INT32
    nanosec: FIELD_TYPE_UINT32

  service_msgs/msg/ServiceEventInfo
  Fields:
    event_type: FIELD_TYPE_UINT8
    stamp: FIELD_TYPE_NESTED_TYPE (builtin_interfaces/msg/Time)
    client_gid: FIELD_TYPE_UINT8_ARRAY - capacity 16
    sequence_number: FIELD_TYPE_INT64

  type_description_interfaces/msg/Field
  Fields:
    name: FIELD_TYPE_STRING
    type: FIELD_TYPE_NESTED_TYPE (type_description_interfaces/msg/FieldType)
    default_value: FIELD_TYPE_STRING

  type_description_interfaces/msg/FieldType
  Fields:
    type_id: FIELD_TYPE_UINT8 = 0
    capacity: FIELD_TYPE_UINT64
    string_capacity: FIELD_TYPE_UINT64
    nested_type_name: FIELD_TYPE_BOUNDED_STRING - string_capacity 255

  type_description_interfaces/msg/IndividualTypeDescription
  Fields:
    type_name: FIELD_TYPE_BOUNDED_STRING - string_capacity 255
    fields: FIELD_TYPE_NESTED_TYPE_UNBOUNDED_SEQUENCE (type_description_interfaces/msg/Field)

  type_description_interfaces/msg/KeyValue
  Fields:
    key: FIELD_TYPE_STRING
    value: FIELD_TYPE_STRING

  type_description_interfaces/msg/TypeDescription
  Fields:
    type_description: FIELD_TYPE_NESTED_TYPE (type_description_interfaces/msg/IndividualTypeDescription)
    referenced_type_descriptions: FIELD_TYPE_NESTED_TYPE_UNBOUNDED_SEQUENCE (type_description_interfaces/msg/IndividualTypeDescription)

  type_description_interfaces/msg/TypeSource
  Fields:
    type_name: FIELD_TYPE_STRING
    encoding: FIELD_TYPE_STRING
    raw_file_contents: FIELD_TYPE_STRING

  type_description_interfaces/srv/GetTypeDescription_Event
  Fields:
    info: FIELD_TYPE_NESTED_TYPE (service_msgs/msg/ServiceEventInfo)
    request: FIELD_TYPE_NESTED_TYPE_BOUNDED_SEQUENCE (type_description_interfaces/srv/GetTypeDescription_Request) - capacity 1
    response: FIELD_TYPE_NESTED_TYPE_BOUNDED_SEQUENCE (type_description_interfaces/srv/GetTypeDescription_Response) - capacity 1

  type_description_interfaces/srv/GetTypeDescription_Request
  Fields:
    type_name: FIELD_TYPE_STRING
    type_hash: FIELD_TYPE_STRING
    include_type_sources: FIELD_TYPE_BOOLEAN = True

  type_description_interfaces/srv/GetTypeDescription_Response
  Fields:
    successful: FIELD_TYPE_BOOLEAN
    failure_reason: FIELD_TYPE_STRING
    type_description: FIELD_TYPE_NESTED_TYPE (type_description_interfaces/msg/TypeDescription)
    type_sources: FIELD_TYPE_NESTED_TYPE_UNBOUNDED_SEQUENCE (type_description_interfaces/msg/TypeSource)
    extra_information: FIELD_TYPE_NESTED_TYPE_UNBOUNDED_SEQUENCE (type_description_interfaces/msg/KeyValue)

I think we should move forward with putting this in, as it covers the majority use case. Further discussion follows:

It's awkward that we need to get names_and_types, then use that to go to get_x_info_by_topic in order to get TopicEndpointInfo to find the hash. Extend names_and_types to contain type hashes rmw#356 would make this part easier and support srv/action
The fact that we need to do this graph-munging to "discover" the hash for a typename seems unnecessary. Given that a specific node has a 1:1 relationship of type name and type hash, it seems like GetTypeDescription should allow for either (but not both) to be empty in the request, so that the requesting client doesn't necessarily even need to know both pieces of information. From a user point of view, it seems very natural to say "you node, give me your description for mypackage/msg/Foo". It also seems like it could come up, though less commonly, to request "you node, what is the type that has hash RIHS01_e28ce254ca8abc06abf92773b74602cdbf116ed34fbaf294fb9f81da9f318eac". The current underlying implementation in rcl assumes the hash is the more important piece of information, using it as the key to its cache, but this usage reveals that it's probably more likely we wish to fetch this information by typename, using the hash as a potential validation. HOWEVER - all that aside, with Extend names_and_types to contain type hashes rmw#356 the "discovery" would be much simpler and so alleviate some of my concern with this point.

emersonknapp · 2023-08-27T08:22:15Z

@clalancette could I get a reviewer assignment for this? (or @audrow @gbiggs friendly ping)

Signed-off-by: Emerson Knapp <[email protected]>

fujitatomoya

it would be nice to have test for topic.

fujitatomoya · 2023-09-14T05:47:32Z

ros2node/ros2node/verb/get_type_description.py

+    if ns != 'msg':
+        raise RuntimeError(
+            f'Currently cannot auto-discover hashes for "{ns}", only "msg". '
+            'Please provide type_hash value to command.')


out of curiosity, how user can get type_hash for srv or action? that would be helpful to add here.

Unfortunately it is not exposed to user anywhere except in the source code ☹️ so you would already have the hash.

ros2/rmw#356 might help a little, but it's not the whole story. I think we need to change the signatures of Node::get_[service|topic]_names_and_types, or provide a new alternative API something like

struct InterfaceType { std::string name; rosidl_type_hash_t hash; }; std::vector<std::string, std::vector<InterfaceType>> get_topic_names_and_types;

fujitatomoya · 2023-09-14T06:02:26Z

ros2node/ros2node/verb/get_type_description.py

+
+        rclpy.init()
+        node = rclpy.create_node(f'{NODE_NAME_PREFIX}_td_requester_{os.getpid()}')
+        cli = node.create_client(GetTypeDescription, '/talker/get_type_description')


Suggested change

cli = node.create_client(GetTypeDescription, '/talker/get_type_description')

cli = node.create_client(GetTypeDescription, service_name)

fujitatomoya · 2023-09-14T06:25:33Z

ros2node/ros2node/verb/get_type_description.py

+            if not cli.wait_for_service(timeout_sec=5.0):
+                raise RuntimeError(f'Service {service_name} not found')


how about waiting until service is ready, unless user sends the signal to stop? i would have timeout option instead of static timeout that user cannot even see how long it waits.

Suggested change

if not cli.wait_for_service(timeout_sec=5.0):

raise RuntimeError(f'Service {service_name} not found')

cli.wait_for_service()

MichaelOrlov

@emersonknapp In general looks good to me.
I only have a few minor concerns, especially about formatting output

formatting with - capacity 255 could be a bit confusing especially if the afterward will be printed out the default value. It is gonna look like FIELD_TYPE_INT8_ARRAY - capacity 25 = 0

I agree that if we had ros2/rmw#356 implemented it would significantly simplify the search for hashes.
I also thinking about why we require to provide a hash when doing a service request for get_type_description?
What if make the hash field optional and the service will return a list of types with hashes?
It will be a client-side responsibility to verify hashes and determine which type is really interesting.
IMO this way everybody's life would be much easier 😃

Update: @emersonknapp it would be nice to have some tests.

MichaelOrlov · 2023-09-17T22:15:43Z

ros2node/ros2node/verb/get_type_description.py

+from type_description_interfaces.srv import GetTypeDescription
+
+
+def print_field_type(ft):


The function name print_field_type is a bit misleading since it doesn't print anything.
IMO it would be more appropriate to rename it to the get_field_type_str or something similar.

MichaelOrlov · 2023-09-17T22:20:10Z

ros2node/ros2node/verb/get_type_description.py

+    if ft.string_capacity:
+        type_str += f' - string_capacity {ft.string_capacity}'
+    if ft.capacity:
+        type_str += f' - capacity {ft.capacity}'


Suggested change

if ft.string_capacity:

type_str += f' - string_capacity {ft.string_capacity}'

if ft.capacity:

type_str += f' - capacity {ft.capacity}'

if ft.string_capacity:

type_str += f' string_capacity({ft.string_capacity})'

if ft.capacity:

type_str += f' capacity({ft.capacity})'

IMO formatting with - capacity 255 could be a bit confusing especially if afterward will be printed out the dafult value. It is gonna look like FIELD_TYPE_INT8_ARRAY - capacity 25 = 0

MichaelOrlov · 2023-09-17T22:21:07Z

ros2node/ros2node/verb/get_type_description.py

+        if field.default_value:
+            field_string += f' = {field.default_value}'


Suggested change

if field.default_value:

field_string += f' = {field.default_value}'

if field.default_value:

field_string += f' default({field.default_value})'

MichaelOrlov · 2023-09-17T22:22:54Z

ros2node/ros2node/verb/get_type_description.py

+    print()
+    print('Referenced Type Descriptions:')
+    for rtd in td.referenced_type_descriptions:
+        print_individual_type_description(rtd, indent=2)


Printing 'Referenced Type Descriptions:' make sense only if td.referenced_type_descriptions is not empty

MichaelOrlov · 2023-09-17T22:23:57Z

ros2node/ros2node/verb/get_type_description.py

+
+
+def discover_hash_for_type(node, remote_node_name, type_name):
+    # TODO(emersonknapp) get hashes from names_and_types when that is implemented


Please add a reference to the tracking issue ros2/rmw#356 in the TODO comment.

emersonknapp requested review from audrow and gbiggs as code owners July 28, 2023 07:15

achim-k mentioned this pull request Jul 28, 2023

ros2node: Add verb get_type_description (rep2011) #817

Closed

Add ros2 node get_type_description initial implementation

1703c0b

Signed-off-by: Emerson Knapp <[email protected]>

emersonknapp force-pushed the emersonknapp/node-get-type-description branch from adba69e to 1703c0b Compare August 27, 2023 21:13

Fix flake8

b3d9c24

Signed-off-by: Emerson Knapp <[email protected]>

fujitatomoya reviewed Sep 14, 2023

View reviewed changes

MichaelOrlov suggested changes Sep 17, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ros2 node get_type_description initial implementation #846

Add ros2 node get_type_description initial implementation #846

emersonknapp commented Jul 28, 2023 •

edited

Loading

emersonknapp commented Aug 27, 2023

fujitatomoya left a comment

fujitatomoya Sep 14, 2023

emersonknapp Sep 14, 2023

fujitatomoya Sep 14, 2023

fujitatomoya Sep 14, 2023

MichaelOrlov left a comment •

edited

Loading

MichaelOrlov Sep 17, 2023

MichaelOrlov Sep 17, 2023

MichaelOrlov Sep 17, 2023

MichaelOrlov Sep 17, 2023

MichaelOrlov Sep 17, 2023

	cli = node.create_client(GetTypeDescription, '/talker/get_type_description')
	cli = node.create_client(GetTypeDescription, service_name)

		if not cli.wait_for_service(timeout_sec=5.0):
		raise RuntimeError(f'Service {service_name} not found')

	if not cli.wait_for_service(timeout_sec=5.0):
	raise RuntimeError(f'Service {service_name} not found')
	cli.wait_for_service()

		from type_description_interfaces.srv import GetTypeDescription


		def print_field_type(ft):

		if field.default_value:
		field_string += f' = {field.default_value}'



		def discover_hash_for_type(node, remote_node_name, type_name):
		# TODO(emersonknapp) get hashes from names_and_types when that is implemented

Add ros2 node get_type_description initial implementation #846

Are you sure you want to change the base?

Add ros2 node get_type_description initial implementation #846

Conversation

emersonknapp commented Jul 28, 2023 • edited Loading

emersonknapp commented Aug 27, 2023

fujitatomoya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelOrlov left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emersonknapp commented Jul 28, 2023 •

edited

Loading

MichaelOrlov left a comment •

edited

Loading