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

Add ~/get_type_description service (rep2011) #1052

Merged

Conversation

achim-k
Copy link
Contributor

@achim-k achim-k commented Mar 29, 2023

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

@achim-k achim-k changed the title [REP-2011] Add ~/get_type_description service Add ~/get_type_description service (rep2011) Mar 30, 2023
rcl/include/rcl/type_description_conversions.h Outdated Show resolved Hide resolved
rcl/src/rcl/node_type_cache_init.h Outdated Show resolved Hide resolved
rcl/include/rcl/node_type_cache.h Outdated Show resolved Hide resolved
rcl/src/rcl/node.c Outdated Show resolved Hide resolved
rcl/src/rcl/node_impl.h Outdated Show resolved Hide resolved
rcl/src/rcl/node.c Outdated Show resolved Hide resolved
rcl/src/rcl/node_type_cache.c Outdated Show resolved Hide resolved
rcl/src/rcl/node_type_cache.c Outdated Show resolved Hide resolved
rcl/src/rcl/node_type_cache.c Outdated Show resolved Hide resolved
rcl/src/rcl/type_description_conversions.c Outdated Show resolved Hide resolved
rcl/src/rcl/node.c Outdated Show resolved Hide resolved
@achim-k achim-k force-pushed the achim-k/rep2011_get_type_description_srv branch from 5fbe4c9 to 9253ea9 Compare April 5, 2023 16:21

rosidl_type_hash_t type_hash;
if (RCUTILS_RET_OK !=
rosidl_parse_type_hash_string(request.type_hash.data, &type_hash))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

request.type_name is currently ununsed

@achim-k 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
@james-rms
Copy link

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 Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@james-rms
Copy link

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

Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

LGTM

@emersonknapp
Copy link
Collaborator

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
Copy link
Contributor Author

achim-k commented Apr 13, 2023

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

rcl/include/rcl/node.h Outdated Show resolved Hide resolved
rcl/include/rcl/type_description_conversions.h Outdated Show resolved Hide resolved
rcl/include/rcl/type_description_conversions.h Outdated Show resolved Hide resolved
rcl/include/rcl/type_description_conversions.h Outdated Show resolved Hide resolved
rcl/include/rcl/type_description_conversions.h Outdated Show resolved Hide resolved
rcl/src/rcl/node.c Outdated Show resolved Hide resolved
rcl/src/rcl/node.c Outdated Show resolved Hide resolved
rcl/src/rcl/node.c Outdated Show resolved Hide resolved
rcl/src/rcl/node.c Outdated Show resolved Hide resolved
rcl/src/rcl/node_options.c Outdated Show resolved Hide resolved
@achim-k 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
Copy link
Contributor Author

achim-k commented Jun 9, 2023

@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
Copy link
Contributor

@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.

achim-k and others added 7 commits June 22, 2023 21:26
…e type cache

Signed-off-by: Hans-Joachim Krauch <achim.krauch@gmail.com>
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>
@emersonknapp emersonknapp force-pushed the achim-k/rep2011_get_type_description_srv branch from 92e1382 to 9d345d6 Compare June 23, 2023 04:27
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
Copy link
Collaborator

@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
Copy link
Collaborator

emersonknapp commented Jun 28, 2023

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 Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines 391 to 397
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;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

@emersonknapp emersonknapp Jul 3, 2023

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
rcl/src/rcl/publisher.c Outdated Show resolved Hide resolved
rcl_action/src/rcl_action/action_server.c Outdated Show resolved Hide resolved
rcl_action/src/rcl_action/action_server.c Outdated Show resolved Hide resolved
… 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
Copy link
Collaborator

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 Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@emersonknapp
Copy link
Collaborator

emersonknapp commented Jul 4, 2023

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 Build Status
  • Linux-aarch64 Build Status
  • Build Status
  • Windows Build Status

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

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

rcl/src/rcl/node.c Outdated Show resolved Hide resolved
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Copy link
Collaborator

emersonknapp commented Jul 5, 2023

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 Build Status
    • Build Status
    • Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@emersonknapp emersonknapp merged commit a4633b8 into ros2:rolling Jul 6, 2023
@emersonknapp
Copy link
Collaborator

@Mergifyio backport iron

@mergify
Copy link

mergify bot commented Jul 6, 2023

backport iron

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jul 6, 2023
* 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
Copy link
Collaborator

emersonknapp commented Jul 7, 2023

Rolling post-merge, no repos file

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

clalancette pushed a commit that referenced this pull request Jul 18, 2023
…#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>
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.

4 participants