Add `~/get_type_description` service (rep2011) #1052

achim-k · 2023-03-29T21:38:11Z

Part of ros2/ros2#1159

~~Depends on ros2/rosidl#727 (ros2/rosidl#735)~~

Features:

Implements the ~/get_type_description which will be offered by each node (can be disabled via node options)
Adds a type cache for each node which holds all types that are registered by subscribers, publishers, services and action servers
Adds conversion functions to convert between relevant rosidl_runtime_c and type_description_interfaces structs

rcl/include/rcl/type_description_conversions.h

rcl/src/rcl/node_type_cache_init.h

rcl/include/rcl/node_type_cache.h

rcl/src/rcl/node.c

rcl/src/rcl/node_impl.h

rcl/src/rcl/node.c

rcl/src/rcl/node_type_cache.c

rcl/src/rcl/type_description_conversions.c

rcl/src/rcl/node.c

achim-k · 2023-04-05T16:32:22Z

rcl/src/rcl/node.c

+
+  rosidl_type_hash_t type_hash;
+  if (RCUTILS_RET_OK !=
+    rosidl_parse_type_hash_string(request.type_hash.data, &type_hash))


request.type_name is currently ununsed

james-rms · 2023-04-10T00:20:34Z

Gist: https://gist.githubusercontent.com/james-rms/22e9369a4fd54e4d18d0e0323d88c489/raw/47868239244ef18091c0f06c166de7a54e361caa/ros2.repos
BUILD args: --packages-above-and-dependencies rcl rcl_action
TEST args: --packages-above rcl rcl_action
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11831

Linux
Linux-aarch64
Windows

james-rms · 2023-04-10T00:21:13Z

@clalancette @wjwwood are you able to take a look at this PR?

emersonknapp

LGTM

emersonknapp · 2023-04-10T18:40:52Z

We probably won't get any reviews from the others until at least tomorrow - today is rmw freeze day, but the deadline for this PR is next Monday so we're OK. I'm actually going to hold off on re-running CI too just to leave the build servers open for more pressing issues for today's EOD freeze.

achim-k · 2023-04-13T19:58:00Z

Rebased to include #1060. I have also squashed commits to make it easier reviewable.

rcl/include/rcl/node.h

rcl/include/rcl/type_description_conversions.h

rcl/src/rcl/node.c

rcl/src/rcl/node_options.c

achim-k · 2023-06-09T09:05:57Z

@clalancette thanks for your review. I have addressed most of your comments and added two commits that contain the changes. What's the policy here, is it OK to do a rebase? If so, I could squash the changes into previous commits to make it easier to review (commit by commit). Also, the base is rather outdated (959ef28), which might be a reason why CI is failing

clalancette · 2023-06-09T15:23:29Z

@clalancette thanks for your review. I have addressed most of your comments and added two commits that contain the changes. What's the policy here, is it OK to do a rebase? If so, I could squash the changes into previous commits to make it easier to review (commit by commit). Also, the base is rather outdated (959ef28), which might be a reason why CI is failing

Please feel free to rebase as you need.

…e type cache Signed-off-by: Hans-Joachim Krauch <achim.krauch@gmail.com>

Signed-off-by: Hans-Joachim Krauch <achim.krauch@gmail.com>

This reverts commit 61ca3dc. Signed-off-by: Hans-Joachim Krauch <achim.krauch@gmail.com>

Signed-off-by: Hans-Joachim Krauch <achim.krauch@gmail.com>

… is used Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

RCL does not initialize the get_type_description service itself, instead providing a full enough API for full language clients to initialize it and register its callback within their threading/execution framework Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

emersonknapp · 2023-06-27T01:32:39Z

@clalancette this is ready for next pass review now, its new state is successfully utilized by ros2/rclcpp#2224. See achim-k#5 for the exact changes made, or just the squashed commit 48a611a

emersonknapp · 2023-06-28T19:36:05Z

Pulls: #1052, ros2/rclcpp#2224
Gist: https://gist.githubusercontent.com/emersonknapp/c4a4fc1d37b707ec959808ee6dd2d189/raw/f6081a66740b791e891c43d39ace0c33004c8312/ros2.repos
BUILD args: --packages-above-and-dependencies rcl rcl_action rclcpp rclcpp_lifecycle
TEST args: --packages-above rcl rcl_action rclcpp rclcpp_lifecycle
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12306

Linux
Linux-aarch64
Windows

clalancette

I've gone through another pass here. Things generally look good to me, though I've left a few things to take a look at.

rcl/include/rcl/node_type_cache.h

clalancette · 2023-06-29T13:36:37Z

rcl/src/rcl/node.c

@@ -284,6 +283,20 @@ rcl_node_init(
    // error message already set
    goto fail;
  }
+  // Initialize the node type cache hash map
+  ret = rcl_node_type_cache_init(node);


I think we may have lost a bug fix here, because this seems to be an issue again. For instance, if rcl_logging_rosout_init_publisher_for_node fails below, then we'll leak any memory associated with the node type cache.

clalancette · 2023-06-29T13:40:32Z

rcl/src/rcl/node.c

+  rcl_ret = rcl_node_type_description_service_fini(node);
+  if (rcl_ret == RCL_RET_NOT_INIT) {
+    rcl_reset_error();
+  } else if (rcl_ret != RCL_RET_OK) {
+    RCL_SET_ERROR_MSG("Unable to fini ~/get_type_description service for node.");
+    result = RCL_RET_ERROR;
+  }


It seems somewhat asymmetric to fini the service here when we didn't init the service in rcl_node_init. Do we actually need to call this here, or should we leave it to the client libraries to do this?

I suppose that rmw_destroy_node will destroy the service anyways - and invalidating node->impl will get rid of the pointer to this service, so there's no harm in not calling fini here

EDIT: rmw_destroy_node requires that everything is destroyed first. Makes sense to require client libraries to fini the service if they inited it, just like with everything else on a node.

rcl/src/rcl/publisher.c

rcl/test/rcl/test_get_type_description_service.cpp

rcl_action/src/rcl_action/action_server.c

… formatting touchups Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

…educing error logging on error-case shutdown Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

emersonknapp · 2023-07-04T00:12:21Z

Pulls: #1052, ros2/rclcpp#2224
Gist: https://gist.githubusercontent.com/emersonknapp/e64c966d59d7e2081d4d296fa756a1e3/raw/f6081a66740b791e891c43d39ace0c33004c8312/ros2.repos
BUILD args: --packages-above-and-dependencies rcl rcl_action rclcpp rclcpp_lifecycle
TEST args: --packages-above rcl rcl_action rclcpp rclcpp_lifecycle
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12316

Linux
Linux-aarch64
Windows

emersonknapp · 2023-07-04T16:04:35Z

Pulls: #1052, ros2/rclcpp#2224, ros2/system_tests#520
Gist: https://gist.githubusercontent.com/emersonknapp/b250eee074072eeb11c1a6d544866179/raw/e787e5a7cbeadb0ba5865e439578867d581b682d/ros2.repos
BUILD args: --packages-above-and-dependencies rcl rcl_action rclcpp rclcpp_lifecycle test_rclcpp
TEST args: --packages-above rcl rcl_action rclcpp rclcpp_lifecycle test_rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12318

Linux
Linux-aarch64
Windows

clalancette

I've left one extremely trivial nit, so I'll approve anyway.

rcl/src/rcl/node.c

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

emersonknapp · 2023-07-05T19:51:12Z

Pulls: #1052, ros2/rclcpp#2224, ros2/system_tests#520
Gist: https://gist.githubusercontent.com/emersonknapp/61666ed4bfef7aa7b2f349cd87122a42/raw/e787e5a7cbeadb0ba5865e439578867d581b682d/ros2.repos
BUILD args: --packages-above-and-dependencies rcl rcl_action rclcpp rclcpp_lifecycle test_rclcpp
TEST args: --packages-above rcl rcl_action rclcpp rclcpp_lifecycle test_rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12324

Linux
Linux-aarch64
Windows

emersonknapp · 2023-07-06T19:45:36Z

@Mergifyio backport iron

mergify · 2023-07-06T19:45:40Z

backport iron

✅ Backports have been created

#1082 Add ~/get_type_description service (rep2011) (backport #1052) has been created for branch iron

* Add ~/get_type_description service API * Add node type cache * Register subscription, publication, service and action types with node type cache * Add functions to convert between rosidl_runtime_c / type_description_interfaces structs RCL does not initialize the get_type_description service itself, instead providing a full enough API for full language clients to initialize it and register its callback within their threading/execution framework Signed-off-by: Hans-Joachim Krauch <achim.krauch@gmail.com> Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com> (cherry picked from commit a4633b8)

emersonknapp · 2023-07-07T01:16:40Z

Rolling post-merge, no repos file

Linux
Linux-aarch64
Windows

…#1082) * Add `~/get_type_description` service (rep2011) (#1052) * Add ~/get_type_description service API * Add node type cache * Register subscription, publication, service and action types with node type cache * Add functions to convert between rosidl_runtime_c / type_description_interfaces structs RCL does not initialize the get_type_description service itself, instead providing a full enough API for full language clients to initialize it and register its callback within their threading/execution framework Signed-off-by: Hans-Joachim Krauch <achim.krauch@gmail.com> Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

achim-k requested review from audrow, ivanpauno and wjwwood as code owners March 29, 2023 21:38

emersonknapp mentioned this pull request Mar 30, 2023

Type Description Distribution (REP-2011 subset / REP-2016) - formerly "message definitions on-the-wire" ros2/ros2#1159

Closed

14 tasks

achim-k changed the title ~~[REP-2011] Add ~/get_type_description service~~ Add ~/get_type_description service (rep2011) Mar 30, 2023

emersonknapp requested changes Mar 30, 2023

View reviewed changes

achim-k commented Mar 31, 2023

View reviewed changes

rcl/src/rcl/node.c Outdated Show resolved Hide resolved

achim-k force-pushed the achim-k/rep2011_get_type_description_srv branch from 5fbe4c9 to 9253ea9 Compare April 5, 2023 16:21

achim-k commented Apr 5, 2023

View reviewed changes

achim-k force-pushed the achim-k/rep2011_get_type_description_srv branch 3 times, most recently from 23667d4 to f8749e9 Compare April 5, 2023 20:23

This was referenced Apr 10, 2023

Add ~/get_type_description service (rep2011) (dupe of @achim-k) achim-k/rcl#2

Closed

Add ~/get_type_description service (rep2011) (dupe of #1052) #1059

Closed

achim-k mentioned this pull request Apr 10, 2023

Add ~/get_type_description service (rep2011) [merged] achim-k/rcl#3

Merged

achim-k force-pushed the achim-k/rep2011_get_type_description_srv branch from 9a0f81b to bd6941d Compare April 10, 2023 15:34

emersonknapp approved these changes Apr 10, 2023

View reviewed changes

emersonknapp mentioned this pull request Apr 11, 2023

Add enable_type_description_service node option - API only (rep2011) #1060

Merged

achim-k force-pushed the achim-k/rep2011_get_type_description_srv branch from bd6941d to 180e68f Compare April 13, 2023 19:56

emersonknapp mentioned this pull request Jun 6, 2023

Dynamic Subscription to locally-unknown types ros2/rosbag2#1373

Open

clalancette requested changes Jun 8, 2023

View reviewed changes

clalancette mentioned this pull request Jun 8, 2023

ros2node: Add verb get_type_description (rep2011) ros2/ros2cli#817

Closed

achim-k force-pushed the achim-k/rep2011_get_type_description_srv branch 2 times, most recently from 6ad2b3a to 0e7d4c5 Compare June 9, 2023 08:51

achim-k and others added 7 commits June 22, 2023 21:26

Register subscription, publication, service and action types with nod…

fbf8e2c

…e type cache Signed-off-by: Hans-Joachim Krauch <achim.krauch@gmail.com>

Add ~/get_type_description service

62cdf1a

Signed-off-by: Hans-Joachim Krauch <achim.krauch@gmail.com>

Enable get_type_description service by default

3459edf

Signed-off-by: Hans-Joachim Krauch <achim.krauch@gmail.com>

Revert "Enable get_type_description service by default"

1a6eec2

This reverts commit 61ca3dc. Signed-off-by: Hans-Joachim Krauch <achim.krauch@gmail.com>

Remove warning

4f301d3

Signed-off-by: Hans-Joachim Krauch <achim.krauch@gmail.com>

Optionally initialize and use type cache based on whether type server…

398e5af

… is used Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Enable type description service in its test

9d345d6

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

emersonknapp force-pushed the achim-k/rep2011_get_type_description_srv branch from 92e1382 to 9d345d6 Compare June 23, 2023 04:27

clalancette requested changes Jun 29, 2023

View reviewed changes

Unconditional type registrations, add to service/action client, minor…

fdff80e

… formatting touchups Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

emersonknapp requested a review from clalancette July 3, 2023 20:22

Avoid attempting unregister if no successful registration occurred, r…

37f9436

…educing error logging on error-case shutdown Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

emersonknapp mentioned this pull request Jul 4, 2023

new builtin parameter: fix test expectations ros2/system_tests#520

Merged

clalancette approved these changes Jul 5, 2023

View reviewed changes

rcl/src/rcl/node.c Outdated Show resolved Hide resolved

remove unneeded (void)

26d02ad

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

emersonknapp mentioned this pull request Jul 5, 2023

GetTypeDescription service ros2/rclpy#1139

Merged

emersonknapp merged commit a4633b8 into ros2:rolling Jul 6, 2023
2 of 3 checks passed

mergify bot mentioned this pull request Jul 6, 2023

[Iron]Add ~/get_type_description service (rep2011) (backport #1052) #1082

Merged

Acuadros95 mentioned this pull request Jul 11, 2023

Disable get_type_description service micro-ROS/rcl#22

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `~/get_type_description` service (rep2011) #1052

Add `~/get_type_description` service (rep2011) #1052

achim-k commented Mar 29, 2023 •

edited

Loading

achim-k Apr 5, 2023

james-rms commented Apr 10, 2023

james-rms commented Apr 10, 2023

emersonknapp left a comment

emersonknapp commented Apr 10, 2023

achim-k commented Apr 13, 2023

achim-k commented Jun 9, 2023

clalancette commented Jun 9, 2023

emersonknapp commented Jun 27, 2023

emersonknapp commented Jun 28, 2023 •

edited

Loading

clalancette left a comment

clalancette Jun 29, 2023

clalancette Jun 29, 2023

emersonknapp Jul 3, 2023 •

edited

Loading

emersonknapp commented Jul 4, 2023

emersonknapp commented Jul 4, 2023 •

edited

Loading

clalancette left a comment

emersonknapp commented Jul 5, 2023 •

edited

Loading

emersonknapp commented Jul 6, 2023

mergify bot commented Jul 6, 2023 •

edited

Loading

emersonknapp commented Jul 7, 2023 •

edited

Loading

Add ~/get_type_description service (rep2011) #1052

Add ~/get_type_description service (rep2011) #1052

Conversation

achim-k commented Mar 29, 2023 • edited Loading

achim-k Apr 5, 2023

Choose a reason for hiding this comment

james-rms commented Apr 10, 2023

james-rms commented Apr 10, 2023

emersonknapp left a comment

Choose a reason for hiding this comment

emersonknapp commented Apr 10, 2023

achim-k commented Apr 13, 2023

achim-k commented Jun 9, 2023

clalancette commented Jun 9, 2023

emersonknapp commented Jun 27, 2023

emersonknapp commented Jun 28, 2023 • edited Loading

clalancette left a comment

Choose a reason for hiding this comment

clalancette Jun 29, 2023

Choose a reason for hiding this comment

clalancette Jun 29, 2023

Choose a reason for hiding this comment

emersonknapp Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

emersonknapp commented Jul 4, 2023

emersonknapp commented Jul 4, 2023 • edited Loading

clalancette left a comment

Choose a reason for hiding this comment

emersonknapp commented Jul 5, 2023 • edited Loading

emersonknapp commented Jul 6, 2023

mergify bot commented Jul 6, 2023 • edited Loading

✅ Backports have been created

emersonknapp commented Jul 7, 2023 • edited Loading

Add `~/get_type_description` service (rep2011) #1052

Add `~/get_type_description` service (rep2011) #1052

achim-k commented Mar 29, 2023 •

edited

Loading

emersonknapp commented Jun 28, 2023 •

edited

Loading

emersonknapp Jul 3, 2023 •

edited

Loading

emersonknapp commented Jul 4, 2023 •

edited

Loading

emersonknapp commented Jul 5, 2023 •

edited

Loading

mergify bot commented Jul 6, 2023 •

edited

Loading

emersonknapp commented Jul 7, 2023 •

edited

Loading