add use_global_arguments for node options of component nodes by gezp · Pull Request #1776 · ros2/rclcpp

gezp · 2021-09-16T03:59:09Z

Signed-off-by: zhenpeng ge zhenpeng.ge@qq.com

related to #978.

i'm trying to implement composed bringup for Nav2, but it can't work as expected. the reason is that internal nodes (like local_costmap, global_costmap) in Nav2 couldn't load parameters from param's file(use_global_arguments is setting false)

rclcpp/rclcpp_components/src/component_manager.cpp

Line 164 in f30329f

.use_global_arguments(false)

according to @wjwwood opinion( #978 (comment) ), allow_undeclared_parameters and use_global_arguments shouldn't be a parameter, but the use_global_arguments does have a legitimate use case here (for Nav2), and i think it's useful when we have a large params file to configure all nodes (or component nodes).

use_global_arguments = true is default behavior for all other situations so its strange that we cannot set it to true at all due to "its generally bad" advise for the component container. It creates a situations where component container nodes have a different behavior from everywhere else and it silently fails if a user is clever enough to know that that is the problem.

I still offer a warning to tell people not to do it but gives the flexibility to get the behavior that is expected every where else from the component container if explicitly called out in the extra parameters.

some extra details here: #978 (comment)

rclcpp_components/src/component_manager.cpp

fujitatomoya

I think this extension makes sense for use case of ComponentManager.

SteveMacenski

@fujitatomoya any other changes required?

rclcpp_components/src/component_manager.cpp

fujitatomoya · 2021-10-04T16:31:37Z

@ivanpauno @hidmic @wjwwood there's been discussion on this via #978, what would you think? i think extending this option makes sense for user application.

fujitatomoya

im good to go.

hidmic

@gezp I'm onboard with this change. This patch is missing a test though.

hidmic · 2021-10-05T14:24:35Z

rclcpp_components/src/component_manager.cpp

                "Extra component argument 'use_intra_process_comms' must be a boolean");
      }
      options.use_intra_process_comms(extra_argument.get_value<bool>());
+    } else if (extra_argument.get_name() == "use_global_arguments") {


@gezp meta: thinking about your use case, I wonder if it would make sense for the component_manager node to expose, e.g. as a parameter, the default value of use_global_arguments for the nodes it composes. Seems convenient if you're already passing a large parameter file to it.

i agree with you, for my use case (use a large parameter file for all components), it seems better to use a parameter of component_manager to configure all components use_global_arguments, which we needn't set this for components one by one in launch file.
I'm not good at naming parameter, could you give me some suggestions ? @hidmic . how about use_global_arguments (i think it's bad, because it seems that we set component_manager nodes to use global arguments, but we want to set this for components) or component_use_global_arguments?

@gezp perhaps forward_global_arguments? I'm not very creative either. At any rate, I won't block this patch on it.

gezp · 2021-10-14T01:32:59Z

i add some test for use_global_arguments , please review again. @fujitatomoya @hidmic

SteveMacenski · 2021-10-28T16:18:40Z

Any update?

fujitatomoya · 2021-10-28T23:32:20Z

I am good to go, https://github.com/ros2/rclcpp/pull/1776/files#r722910471 also makes sense though.

fujitatomoya · 2021-10-28T23:35:34Z

CI:

Linux
Linux-aarch64
macOS
Windows

SteveMacenski · 2021-10-29T01:15:22Z

Build failure seems unrelated

23:55:16 /home/jenkins-agent/workspace/ci_linux/ws/src/ros2/rclcpp/rclcpp_lifecycle/include/rclcpp_lifecycle/state.hpp:25:16: error: using typedef-name ‘rcl_lifecycle_state_t’ after ‘struct’
23:55:16    25 | typedef struct rcl_lifecycle_state_t rcl_lifecycle_state_t;

gezp · 2021-10-29T01:23:08Z

23:55:16 /home/jenkins-agent/workspace/ci_linux/ws/src/ros2/rclcpp/rclcpp_lifecycle/include/rclcpp_lifecycle/state.hpp:25:16: error: using typedef-name ‘rcl_lifecycle_state_t’ after ‘struct’
23:55:16    25 | typedef struct rcl_lifecycle_state_t rcl_lifecycle_state_t;

this issue was solved by #1788. i think i need rebase.

Signed-off-by: zhenpeng ge <zhenpeng.ge@qq.com> keep use_global_arguments false default Signed-off-by: zhenpeng ge <zhenpeng.ge@qq.com> update warning message Signed-off-by: zhenpeng ge <zhenpeng.ge@qq.com> update warnning message Signed-off-by: zhenpeng ge <zhenpeng.ge@qq.com> add test Signed-off-by: zhenpeng ge <zhenpeng.ge@qq.com>

ros-discourse · 2021-11-01T20:52:32Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2021-10-28/22947/1

SteveMacenski · 2021-11-18T23:00:07Z

@fujitatomoya @hidmic can we run CI again to merge it?

fujitatomoya · 2021-11-19T00:12:29Z

CI retry:

Linux
Linux-aarch64
macOS
Windows

ros-discourse · 2021-11-22T22:12:09Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2021-11-18/23209/1

hidmic

Sorry for the delay (I was out for some time).

Overall makes sense to me. Tests could use some improvement though.

hidmic · 2021-12-06T19:03:35Z

rclcpp_components/test/test_component_manager_api.cpp

+    EXPECT_EQ(result->error_message, "");
+    std::cout << result->full_node_name << std::endl;
+    EXPECT_EQ(result->full_node_name, "/test_component_global_arguments");
+    EXPECT_EQ(result->unique_id, 7u);


@gezp nit: this tests that the manager did not fail but not that it did anything about use_global_arguments.

yes, use_global_arguments is set true in NodeOption , but this component didn't use any global arguments, i don't know how to test for this.

hidmic · 2021-12-06T19:03:52Z

rclcpp_components/src/component_manager.cpp

                "Extra component argument 'use_intra_process_comms' must be a boolean");
      }
      options.use_intra_process_comms(extra_argument.get_value<bool>());
+    } else if (extra_argument.get_name() == "use_global_arguments") {


@gezp perhaps forward_global_arguments? I'm not very creative either. At any rate, I won't block this patch on it.

SteveMacenski · 2021-12-06T19:23:05Z

@gezp can you make the testing related changes so we can get this merged in and get dynamic composition in Nav2? 🥳

Signed-off-by: zhenpeng ge <zhenpeng.ge@qq.com>

SteveMacenski · 2021-12-16T17:36:05Z

@hidmic is this ready to review again?

hidmic · 2022-01-12T22:29:35Z

@hidmic is this ready to review again?

Argh. This fell through the cracks again. Sorry about that.

Last CI (hopefully):

Linux
Linux-aarch64
Windows

hidmic · 2022-01-19T13:29:25Z

Alright, going in.

SteveMacenski reviewed Sep 16, 2021

View reviewed changes

rclcpp_components/src/component_manager.cpp Outdated Show resolved Hide resolved

fujitatomoya reviewed Sep 27, 2021

View reviewed changes

SteveMacenski approved these changes Oct 1, 2021

View reviewed changes

fujitatomoya reviewed Oct 4, 2021

View reviewed changes

rclcpp_components/src/component_manager.cpp Outdated Show resolved Hide resolved

fujitatomoya approved these changes Oct 5, 2021

View reviewed changes

hidmic reviewed Oct 5, 2021

View reviewed changes

clalancette assigned hidmic Oct 28, 2021

hidmic approved these changes Dec 6, 2021

View reviewed changes

use forward_global_arguments instead of use_global_arguments

32f449e

Signed-off-by: zhenpeng ge <zhenpeng.ge@qq.com>

hidmic merged commit bca3fd7 into ros2:master Jan 19, 2022

Conversation

gezp commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

SteveMacenski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fujitatomoya commented Oct 4, 2021

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

hidmic Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

gezp Oct 6, 2021

Choose a reason for hiding this comment

Uh oh!

hidmic Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

gezp commented Oct 14, 2021

Uh oh!

SteveMacenski commented Oct 28, 2021

Uh oh!

fujitatomoya commented Oct 28, 2021

Uh oh!

fujitatomoya commented Oct 28, 2021

Uh oh!

SteveMacenski commented Oct 29, 2021

Uh oh!

gezp commented Oct 29, 2021

Uh oh!

ros-discourse commented Nov 1, 2021

Uh oh!

SteveMacenski commented Nov 18, 2021

Uh oh!

fujitatomoya commented Nov 19, 2021

Uh oh!

ros-discourse commented Nov 22, 2021

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

hidmic Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

gezp Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

hidmic Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

SteveMacenski commented Dec 6, 2021

Uh oh!

SteveMacenski commented Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hidmic commented Jan 12, 2022

Uh oh!

hidmic commented Jan 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gezp commented Sep 16, 2021 •

edited

Loading

SteveMacenski commented Dec 16, 2021 •

edited

Loading