Exposed rcl_actions to the NodeGraph class by CursedRock17 · Pull Request #2188 · ros2/rclcpp

CursedRock17 · 2023-05-07T21:32:41Z

This pull request is meant to address issue #1473 which needs rcl_action functions exposed to the NodeGraph class. As mentioned:

rcl_action_get_client_names_and_types_by_node
rcl_action_get_server_names_and_types_by_node
rcl_action_get_names_and_types

were all meant to be exposed from rcl to rclcpp, which based on the previous comment meant to make methods similar to rclpy. So each of the functions developed in this PR all follow a similar format of gaining access to the original rcl_action methods with a rcl_ret_t type and return a map of all the names and types.

fujitatomoya · 2023-05-08T17:58:21Z

@CursedRock17 thanks for the PR, i will take a look. in the meantime, can you address DCO error? https://github.com/ros2/rclcpp/pull/2188/checks?check_run_id=13298073729

fujitatomoya

in addition to comments, it would be ideal to add the test cases to rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpp.

CursedRock17 · 2023-05-11T02:27:26Z

in addition to comments, it would be ideal to add the test cases to rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpp.

The most recent commit I pushed contained the tests for the rcl_action_get_names_and_types I followed format for other similar methods in the file. Was that the way that made testing the most efficient for those types of functions, if so, I can replicate with so more tests for the other new functions.

fujitatomoya · 2023-05-11T03:45:03Z

@CursedRock17 thanks for iterating and being responsive. i will take a look tomorrow, in the meantime can you address DCO error?

fujitatomoya

Can you also add the test cases to check if it can actually get names and types? for example,

rclcpp/rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpp

Line 195 in fce021b

TEST_F(TestNodeGraph, get_service_names_and_types_by_node)

fujitatomoya

@CursedRock17 can you check building and testing with your local environment? i cannot build or test this PR... thanks in advance.

fujitatomoya · 2023-05-23T00:20:20Z


+  RCLCPP_PUBLIC
+  std::map<std::string, std::vector<std::string>>
+  get_action_names_and_types() const override;


I think this cannot be built since there is no virtual function exists. Could you update NodeGraphInterface accordingly?

see https://build.ros2.org/job/Rpr__rclcpp__ubuntu_jammy_amd64/887/console

fujitatomoya · 2023-05-23T00:23:02Z

+  EXPECT_THROW(
+    node_graph()->get_action_names_and_types("action", false),
+    rclcpp::exceptions::RCLError);
+


Needs brackets at the end of this test case?

Suggested change

}

fujitatomoya · 2023-05-23T00:30:29Z

+  auto client =
+    node()->create_client<test_msgs::srv::Empty>("node1_service");


i think we need to create action but service for this test case.

fujitatomoya · 2023-05-23T00:32:03Z

+
+TEST_F(TestNodeGraph, get_action_client_names_and_types_by_node_rcl_error)
+{
+  auto client = node()->create_client<test_msgs::srv::Empty>("node1_service");


same here, this is creating service but action. rclcpp_action::create_client should be called instead.

While this makes sense, I cannot include the rclcpp_action part of the library in the rclcpp section, otherwise a linking error is thrown because rclcpp_action uses rclcpp, so this would result in an infinite loop. We need to both, have these functions in the node_graph_interface section, and utilize the rclcpp_action commands.

One thing that we could do is add specifically the tests to the rclcpp_action part of the rclcpp library with a new file along the names of test_action_calls. Since rclcpp_action has to include rclcpp in its cmake file and already has the ability to include rclcpp_action::create_server there would be no linking issues. The problem is just the location of these files because the rcl_action wrapper interfaces are in the rclcpp/nodeinterfaces section of the codebase, which makes this location a bit awkward for naming purpose. The functions like get_action_types_and_names make the most sense where they are now, but it seems the tests have to be moved simply because including rclcpp_action in rclcpp isn't possible.

This conversation is making me question whether we want to expose this as part of rclcpp at all. That is, I wonder if it would make more sense to move all of this to rclcpp_action (possibly with a new NodeActionGraph class), which avoids this whole problem. That said, I haven't looked at this implementation closely yet, so maybe this doesn't make much sense.

I would have to agree with moving this entire implementation to rclcpp_action. The folder is already quite slim with files and seems to be a better place to place wrapped rcl_action functions. While keeping all three of these functions in the node_graph.cpp section would be simpler; the fact that we would need to move tests to a different part of the project entirely showcases a design flaw.

I'm now less confident in moving the entire implementation of NodeGraph into rclcpp_action as NodeActionGraph. While we are only using it for action based activities, it's not action specific, simply just an rcl_action wrapper, we then have to move all of the other general overrides to this new class. This will add another ~500 lines that we probably could have avoided.

Additionally, if the user has some long-running process using an action server/client, while they have a small publisher/subscriber maybe keeping track of the environment, they won't be able to track these processes from one NodeGraph. If we did move the action side of things over to a new class, they have to create both a NodeGraph and NodeActionGraph, so if the user were to call get_graph_event and gain another user on the NodeGraph, the NodeActionGraph wouldn't see that change, thus won't be able to accurately track the processes running.

fujitatomoya · 2023-05-23T00:32:21Z

+
+TEST_F(TestNodeGraph, get_action_client_names_and_types_by_node_rcl_fini_error)
+{
+  auto client = node()->create_client<test_msgs::srv::Empty>("node1_service");


fujitatomoya · 2023-05-23T00:33:09Z

+  };
+  auto handle_accepted = [](std::shared_ptr<test_msgs::msg::Empty>){};
+
+  const server = node()->create_server<test_msgs::msg::Empty(


same here, this is service.

CursedRock17 · 2023-05-23T17:32:02Z

@CursedRock17 can you check building and testing with your local environment? i cannot build or test this PR... thanks in advance.

Yes I can test, I can do it on both my computer and Docker for Ubuntu 22.04 correct?

fujitatomoya · 2023-05-23T23:19:49Z

Docker for Ubuntu 22.04 correct?

yes, as long as the userland is Ubuntu 22.04, i believe there should be no problem for this PR verification.

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>

Signed-off-by: CursedRock17 <mtglucas1@gmail.com> Making requests action related Signed-off-by: CursedRock17 <mtglucas1@gmail.com> Testing suite Signed-off-by: CursedRock17 <mtglucas1@gmail.com> Moving Tests Signed-off-by: CursedRock17 <mtglucas1@gmail.com>

CursedRock17 requested review from hidmic, ivanpauno and wjwwood as code owners May 7, 2023 21:32

fujitatomoya self-requested a review May 8, 2023 17:57

fujitatomoya self-assigned this May 8, 2023

CursedRock17 force-pushed the actions_exposed branch from 7c7bb78 to f79c1f8 Compare May 8, 2023 20:41

fujitatomoya requested changes May 8, 2023

View reviewed changes

CursedRock17 force-pushed the actions_exposed branch from 8672eaa to b8229db Compare May 8, 2023 21:42

CursedRock17 force-pushed the actions_exposed branch from 05ce539 to 0218076 Compare May 11, 2023 03:55

fujitatomoya requested changes May 19, 2023

View reviewed changes

CursedRock17 force-pushed the actions_exposed branch from f44e614 to 5c37c93 Compare May 22, 2023 03:43

fujitatomoya requested changes May 23, 2023

View reviewed changes

CursedRock17 added 9 commits November 2, 2023 22:32

Exposed rcl_actions to the NodeGraph class

ad1be69

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>

fixed a typo

3c0fd86

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>

fixed a typo

bb4c0ef

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>

Prepping for signoff

c3fafe4

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>

Removing mistake

b081439

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>

removing unused types

76f528d

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>

Attempting this format

67febe0

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>

Adding necessary test cases

193829d

Signed-off-by: CursedRock17 <mtglucas1@gmail.com>

CursedRock17 force-pushed the actions_exposed branch from c6637db to 021568f Compare November 3, 2023 02:32

		auto client =
		node()->create_client<test_msgs::srv::Empty>("node1_service");

Conversation

CursedRock17 commented May 7, 2023

Uh oh!

fujitatomoya commented May 8, 2023

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CursedRock17 commented May 11, 2023

Uh oh!

fujitatomoya commented May 11, 2023

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CursedRock17 commented May 23, 2023

Uh oh!

fujitatomoya commented May 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants