[Iron] Add new node interface TypeDescriptionsInterface to provide GetTypeDescription service (backport #2224) #2236

emersonknapp · 2023-07-07T17:44:56Z

backports #2224 to Iron

Depends on ros2/rcl#1082
Linked to ros2/system_tests#521

TypeDescriptions interface with readonly param configuration
Add parameter descriptor, to make read only
example of spinning in thread for get_type_description service
Add a basic test for the new interface
Fix tests with new parameter
Add comments about builtin parameters

…scription service (ros2#2224) * TypeDescriptions interface with readonly param configuration * Add parameter descriptor, to make read only * example of spinning in thread for get_type_description service * Add a basic test for the new interface * Fix tests with new parameter * Add comments about builtin parameters Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com> Signed-off-by: William Woodall <william@osrfoundation.org>

rclcpp/include/rclcpp/node.hpp

rclcpp/src/rclcpp/node_interfaces/node_base.cpp

rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp

emersonknapp · 2023-07-07T21:18:25Z

Using https://github.com/ros2/rclcpp/pull/2183/files as a reference for ABI-stable backport

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

emersonknapp · 2023-07-07T22:23:08Z

rclcpp/include/rclcpp/node.hpp

@@ -1591,6 +1597,9 @@ class Node : public std::enable_shared_from_this<Node>
  const rclcpp::NodeOptions node_options_;
  const std::string sub_namespace_;
  const std::string effective_namespace_;
+
+  class BackportMemberMaps;


Note to reviewer: went with a generic name to leave it open for possible other members usage

At the risk of bikeshedding, I think the name is a little too generic. That is, the class embeds a

std::unordered_map< const Node *, rclcpp::node_interfaces::NodeTypeDescriptionsInterface::SharedPtr > type_descriptions_map_;

which feels very specific to this backport. I think I'd go with BackportTypeDescriptionsInterface, or something like that.

Maybe the intention wasn't clear. This class could have more maps within it, controlled by the same access mutex, that create more "members" for the owning class' instances. Given that members would be created all at once, and removed all at once, there would be no partial construction or partial deletion of these "members", it makes sense to use the same mutex and manage all the maps at once. That was the intention here - this thing is "all extra backported members" - and maybe that ends up being only one from this backport, but if we do want to backport more member-based features to Iron we don't need to create any new classes or static members, this one does the whole job via new maps within it. I like it as a generic backport pattern that could be reused.

The implementation could actually be a singular map to some "tuple" or struct of extra members, that is added and deleted atomically.

Ah, OK. Thanks for the explanation. In that case, I'm fine with the name as-is. I'll just suggest you put some of that explanation there into a comment (either here in the .hpp or in the .cpp, I don't care which).

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

emersonknapp · 2023-07-07T22:57:40Z

Pulls: ros2/rcl#1082, #2236, ros2/system_tests#521
Gist: https://gist.githubusercontent.com/emersonknapp/c9f2e76b044e7842b85a6ac7bb40dce3/raw/54f8236b734b80bddc3b50b3786083c2304c3ebf/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp rclcpp_lifecycle test_rclcpp
TEST args: --packages-above rclcpp rclcpp_lifecycle test_rclcpp
ROS Distro: iron
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12350

Linux
Linux-aarch64
Windows
- (manual rerun, flaky check)

clalancette

I've left only nitpicky stuff that we should fix before merging.

clalancette · 2023-07-12T15:57:46Z

rclcpp/include/rclcpp/node.hpp

@@ -1591,6 +1597,9 @@ class Node : public std::enable_shared_from_this<Node>
  const rclcpp::NodeOptions node_options_;
  const std::string sub_namespace_;
  const std::string effective_namespace_;
+
+  class BackportMemberMaps;


At the risk of bikeshedding, I think the name is a little too generic. That is, the class embeds a

std::unordered_map< const Node *, rclcpp::node_interfaces::NodeTypeDescriptionsInterface::SharedPtr > type_descriptions_map_;

which feels very specific to this backport. I think I'd go with BackportTypeDescriptionsInterface, or something like that.

clalancette · 2023-07-12T15:58:34Z

rclcpp/src/rclcpp/node.cpp

+  void add(Node * key)
+  {
+    // Adding a new instance to the maps requires exclusive access
+    std::unique_lock lock(map_access_mutex_);


Nit: #include <mutex> at the top of the file for this.

This is a really good eye you have for these - do you know of any include-what-you-use linters that could handle this so you don't have to?

There is iwyu: https://include-what-you-use.org/ . It's even packaged for Ubuntu under the same name. The major issue with it (from an automation standpoint) is that it relies on clang, so we could only conceivably run it in our clang CI jobs.

I'm surprised that cpplint.py doesn't catch this, it used to catch things like this. Maybe its coverage is just not as good.

rclcpp/src/rclcpp/node.cpp

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

clalancette

One more minor thing to fix, then this will be good to go.

clalancette · 2023-07-13T20:06:34Z

rclcpp/include/rclcpp/node.hpp

@@ -1591,6 +1597,9 @@ class Node : public std::enable_shared_from_this<Node>
  const rclcpp::NodeOptions node_options_;
  const std::string sub_namespace_;
  const std::string effective_namespace_;
+
+  class BackportMemberMaps;


Ah, OK. Thanks for the explanation. In that case, I'm fine with the name as-is. I'll just suggest you put some of that explanation there into a comment (either here in the .hpp or in the .cpp, I don't care which).

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

clalancette

This looks good to me.

@emersonknapp With this approved, I think that is everything needed for the Iron backport. I think I'd like to see one more CI run on all 5 of the PRs here. Assuming that is green, I'll go ahead and merge the group of them.

wjwwood

lgtm from an ABI/API stability point of view

emersonknapp · 2023-07-17T21:42:01Z

Pulls: ros2/rcl#1082, #2236, ros2/system_tests#521, ros2/rclpy#1140, ros2/ros2cli#839
Gist: https://gist.githubusercontent.com/emersonknapp/05bdac69737f3b9ead83ab407dacc401/raw/66da570b925141182d96d88ed28387f17f917899/ros2.repos
BUILD args:
TEST args:
ROS Distro: iron
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/12392

Linux
Linux-aarch64
Windows

…tTypeDescription service (backport ros2#2224) (ros2#2236) * Add new node interface TypeDescriptionsInterface to provide GetTypeDescription service (ros2#2224) * TypeDescriptions interface with readonly param configuration * Add parameter descriptor, to make read only * example of spinning in thread for get_type_description service * Add a basic test for the new interface * Fix tests with new parameter * Add comments about builtin parameters * Hide new member for ABI stability * Add typedescription interface smoke test Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com> Signed-off-by: William Woodall <william@osrfoundation.org> Signed-off-by: Ignacio Vizzo <ignacio@dexory.com>

…tTypeDescription service (backport ros2#2224) (ros2#2236) * Add new node interface TypeDescriptionsInterface to provide GetTypeDescription service (ros2#2224) * TypeDescriptions interface with readonly param configuration * Add parameter descriptor, to make read only * example of spinning in thread for get_type_description service * Add a basic test for the new interface * Fix tests with new parameter * Add comments about builtin parameters * Hide new member for ABI stability * Add typedescription interface smoke test Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com> Signed-off-by: William Woodall <william@osrfoundation.org>

…tTypeDescription service (backport ros2#2224) (ros2#2236) * Add new node interface TypeDescriptionsInterface to provide GetTypeDescription service (ros2#2224) * TypeDescriptions interface with readonly param configuration * Add parameter descriptor, to make read only * example of spinning in thread for get_type_description service * Add a basic test for the new interface * Fix tests with new parameter * Add comments about builtin parameters * Hide new member for ABI stability * Add typedescription interface smoke test Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com> Signed-off-by: William Woodall <william@osrfoundation.org> Signed-off-by: Oren Bell <oren.bell@wustl.edu>

…tTypeDescription service (backport ros2#2224) (ros2#2236) * Add new node interface TypeDescriptionsInterface to provide GetTypeDescription service (ros2#2224) * TypeDescriptions interface with readonly param configuration * Add parameter descriptor, to make read only * example of spinning in thread for get_type_description service * Add a basic test for the new interface * Fix tests with new parameter * Add comments about builtin parameters * Hide new member for ABI stability * Add typedescription interface smoke test Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com> Signed-off-by: William Woodall <william@osrfoundation.org>

emersonknapp mentioned this pull request Jul 7, 2023

[Iron] Add a fix for the tests given the new type description parameter (backport #520) ros2/system_tests#521

Merged

emersonknapp changed the title ~~Add new node interface TypeDescriptionsInterface to provide GetTypeDescription service (backport #2224)~~ [Iron] Add new node interface TypeDescriptionsInterface to provide GetTypeDescription service (backport #2224) Jul 7, 2023

emersonknapp commented Jul 7, 2023

View reviewed changes

Hide new member for ABI stability

75e180c

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

emersonknapp force-pushed the emersonknapp/iron-backport-2224 branch from d7c8669 to 75e180c Compare July 7, 2023 22:22

emersonknapp commented Jul 7, 2023

View reviewed changes

emersonknapp marked this pull request as ready for review July 7, 2023 22:24

emersonknapp requested review from ivanpauno, hidmic and wjwwood as code owners July 7, 2023 22:24

Add typedescription interface smoke test

1d8c616

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

emersonknapp mentioned this pull request Jul 7, 2023

Implement get_node_type_descriptions_interface for lifecyclenode and add test #2237

Merged

clalancette requested changes Jul 12, 2023

View reviewed changes

include what you use

037b8db

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

clalancette requested changes Jul 13, 2023

View reviewed changes

Add more detailed doc comments on BackportMembers, slight name change

ea2db31

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

clalancette approved these changes Jul 17, 2023

View reviewed changes

wjwwood approved these changes Jul 17, 2023

View reviewed changes

emersonknapp mentioned this pull request Jul 17, 2023

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

Merged

This was referenced Jul 17, 2023

[Iron] get_type_description service (backport #1139) ros2/rclpy#1140

Merged

[Iron] Fix tests with get_type_description service and param present (backport #838) ros2/ros2cli#839

Merged

clalancette merged commit 6d4a99f into ros2:iron Jul 18, 2023
2 checks passed

clalancette mentioned this pull request Aug 24, 2023

[humble] Add pause and resume service calls for rosbag2 recorder (Backport #1131) ros2/rosbag2#1453

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Iron] Add new node interface TypeDescriptionsInterface to provide GetTypeDescription service (backport #2224) #2236

[Iron] Add new node interface TypeDescriptionsInterface to provide GetTypeDescription service (backport #2224) #2236

emersonknapp commented Jul 7, 2023 •

edited

Loading

emersonknapp commented Jul 7, 2023

emersonknapp Jul 7, 2023

clalancette Jul 12, 2023

emersonknapp Jul 12, 2023 •

edited

Loading

emersonknapp Jul 12, 2023

clalancette Jul 13, 2023

emersonknapp commented Jul 7, 2023 •

edited

Loading

clalancette left a comment

clalancette Jul 12, 2023

clalancette Jul 12, 2023

emersonknapp Jul 12, 2023

clalancette Jul 13, 2023

wjwwood Jul 17, 2023

clalancette left a comment

clalancette Jul 13, 2023

clalancette left a comment

wjwwood left a comment

emersonknapp commented Jul 17, 2023 •

edited by clalancette

Loading

[Iron] Add new node interface TypeDescriptionsInterface to provide GetTypeDescription service (backport #2224) #2236

[Iron] Add new node interface TypeDescriptionsInterface to provide GetTypeDescription service (backport #2224) #2236

Conversation

emersonknapp commented Jul 7, 2023 • edited Loading

emersonknapp commented Jul 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emersonknapp Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emersonknapp commented Jul 7, 2023 • edited Loading

clalancette 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

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

wjwwood left a comment

Choose a reason for hiding this comment

emersonknapp commented Jul 17, 2023 • edited by clalancette Loading

emersonknapp commented Jul 7, 2023 •

edited

Loading

emersonknapp Jul 12, 2023 •

edited

Loading

emersonknapp commented Jul 7, 2023 •

edited

Loading

emersonknapp commented Jul 17, 2023 •

edited by clalancette

Loading