-
Notifications
You must be signed in to change notification settings - Fork 163
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
Add ~/get_type_description
service (rep2011)
#1052
Conversation
~/get_type_description
service~/get_type_description
service (rep2011)
5fbe4c9
to
9253ea9
Compare
rcl/src/rcl/node.c
Outdated
|
||
rosidl_type_hash_t type_hash; | ||
if (RCUTILS_RET_OK != | ||
rosidl_parse_type_hash_string(request.type_hash.data, &type_hash)) |
There was a problem hiding this comment.
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
23667d4
to
f8749e9
Compare
Gist: https://gist.githubusercontent.com/james-rms/22e9369a4fd54e4d18d0e0323d88c489/raw/47868239244ef18091c0f06c166de7a54e361caa/ros2.repos |
@clalancette @wjwwood are you able to take a look at this PR? |
9a0f81b
to
bd6941d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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. |
bd6941d
to
180e68f
Compare
Rebased to include #1060. I have also squashed commits to make it easier reviewable. |
6ad2b3a
to
0e7d4c5
Compare
@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>
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>
92e1382
to
9d345d6
Compare
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>
@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 |
Pulls: #1052, ros2/rclcpp#2224 |
There was a problem hiding this 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.
@@ -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); |
There was a problem hiding this comment.
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.
rcl/src/rcl/node.c
Outdated
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; | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 init
ed it, just like with everything else on a node.
… 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>
Pulls: #1052, ros2/rclcpp#2224 |
Pulls: #1052, ros2/rclcpp#2224, ros2/system_tests#520 |
There was a problem hiding this 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.
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Pulls: #1052, ros2/rclcpp#2224, ros2/system_tests#520 |
@Mergifyio backport iron |
✅ Backports have been created
|
* 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)
…#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>
Part of ros2/ros2#1159
Depends on ros2/rosidl#727 (ros2/rosidl#735)Features:
rosidl_runtime_c
andtype_description_interfaces
structs