Need to specify NodeOption explicitly to allow declaration. #389

fujitatomoya · 2019-09-13T03:40:48Z

for EvenParameterNode sample, NodeOption should be specified explicitly to allow declaration the parameters from the other nodes.

Client(ros2 param cli to set parameter)

root@6cc2c62d0dd0:~/ros2_colcon_ws# ros2 param set /even_parameters_node foo 2
Setting parameter failed: parameter 'foo' cannot be set because it was not declared

EvenParameterNode

root@6cc2c62d0dd0:~/ros2_colcon_ws# ros2 run demo_nodes_cpp even_parameters_node
[WARN] [rclcpp]: Failed to set parameter: parameter 'foo' cannot be set because it was not declared

with above, expected behavior is to be able to set even number for EvenParameterNode.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>

ivanpauno

LGTM!

The example should also be updated to use the new API:
https://github.com/ros2/rclcpp/blob/dfb144d3cbadd3f5147d95ca6fcff7ad871804b5/rclcpp/include/rclcpp/node.hpp#L960-L962
https://github.com/ros2/rclcpp/blob/dfb144d3cbadd3f5147d95ca6fcff7ad871804b5/rclcpp/include/rclcpp/node.hpp#L987-L989

Could you give that a try?

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>

fujitatomoya · 2019-09-19T11:24:40Z

@ivanpauno

could you check bb1cf59 ?

demo_nodes_cpp/src/parameters/even_parameters_node.cpp

ivanpauno · 2019-09-19T12:44:51Z

Is this example also affected by ros2/rclcpp#851?

fujitatomoya · 2019-09-20T06:33:48Z

Is this example also affected by ros2/rclcpp#851?

yes, i will make the one PR for both of them.

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>

fujitatomoya · 2019-09-20T06:45:38Z

@ivanpauno

can you help me to merge this branch into the master?

Is this example also affected by ros2/rclcpp#851?

this fix also resolve the ros2/rclcpp#851

ivanpauno

LGTM! Thanks for iterating

ivanpauno · 2019-09-20T13:56:29Z

demo_nodes_cpp/src/parameters/parameter_blackboard.cpp

-      rclcpp::NodeOptions()
-      .allow_undeclared_parameters(true)
-      .automatically_declare_parameters_from_overrides(true)
-  ))


The default value was not being used, right?
IMO, it's ok to force allow_undeclared_parameters automatically_declare_parameters_from_overrides to true, because if not the example doesn't have sense.

these are default arguments, so not used with current implementation.

ivanpauno · 2019-09-20T15:09:20Z

Linux
Linux-aarch64
macOS
Windows

ivanpauno · 2019-09-23T12:49:19Z

@fujitatomoya Can you address the linter failures?

Lines should be <= 100 characters long [whitespace/line_length] [2] Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>

fujitatomoya · 2019-09-24T04:50:29Z

@ivanpauno

sorry about that i should have realized that before,

https://ci.ros2.org/job/ci_linux/8186/testReport/(root)/projectroot/cpplint/

is fixed.

ivanpauno

Sorry, but uncrustify is still not passing.

ivanpauno · 2019-09-24T12:18:56Z

demo_nodes_cpp/src/parameters/parameter_blackboard.cpp

-      .automatically_declare_parameters_from_overrides(true)
-  ))
-  : Node("parameter_blackboard", options)
+  rclcpp::NodeOptions options


extra tab missing

got it, fixed in 2dc4fb3

ivanpauno · 2019-09-24T12:20:51Z

demo_nodes_cpp/src/parameters/parameter_blackboard.cpp

+  )
+  : Node("parameter_blackboard",
+    options.allow_undeclared_parameters(true).
+    automatically_declare_parameters_from_overrides(true))


The correct style is:

: Node( "parameter_blackboard", options.allow_undeclared_parameters(true). automatically_declare_parameters_from_overrides(true))

got it, fixed in 2dc4fb3

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>

fujitatomoya · 2019-09-30T05:28:33Z

@ivanpauno

could you check once again? apologize for uncrustify messges.

thanks

ivanpauno · 2019-09-30T12:43:25Z

Linux
Linux-aarch64
macOS
Windows

hidmic

LGTM pending green CI

fujitatomoya · 2019-10-01T01:25:43Z

is there anything i gotta do? just let me know what to do.

ivanpauno · 2019-10-01T13:36:42Z

is there anything i gotta do? just let me know what to do.

Fast-RTPS builds were temporally broken, now it should be fixed.
Re-running CI:

Linux
Linux-aarch64
macOS
Windows

ivanpauno · 2019-10-01T16:00:52Z

@fujitatomoya there are some test failures that seem to be related with this change, can you check them?

fujitatomoya · 2019-10-02T01:03:20Z

@ivanpauno

could you tell me what exactly the test failure is? i do not see any failure on CI.

ivanpauno · 2019-10-02T13:12:42Z

could you tell me what exactly the test failure is? i do not see any failure on CI.

You can see the failures in the links above, e.g. here are the linux failures.

I tested locally, and the only problem seems to be that the branch isn't rebased with master.
Running CI again with a copy of your branch rebased on master:

Linux
Linux-aarch64
macOS
Windows

ivanpauno · 2019-10-02T17:18:55Z

Merged! Thanks for iterating @fujitatomoya!

fujitatomoya · 2019-10-02T23:18:45Z

@ivanpauno

and the only problem seems to be that the branch isn't rebased with master.

sorry about that, i just did not realize cz CI tells me that it can be merged.

anyway, thanks for your time!

ivanpauno · 2019-12-10T14:07:27Z

Removing from the Dashing patch release board, as #375 triggered the error, and that PR is not part of the Dashing branch.

specify NodeOption explicitly to allow declaration.

7d62931

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>

ivanpauno approved these changes Sep 13, 2019

View reviewed changes

replace deprecated API for parameter event into new API.

bb1cf59

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>

ivanpauno reviewed Sep 19, 2019

View reviewed changes

demo_nodes_cpp/src/parameters/even_parameters_node.cpp Show resolved Hide resolved

fix issue rclcpp#851.

346fbb1

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>

fujitatomoya mentioned this pull request Sep 20, 2019

parameter_blackboard node parameter cannot be set because it was not declared ros2/rclcpp#851

Closed

ivanpauno approved these changes Sep 20, 2019

View reviewed changes

ivanpauno requested a review from hidmic September 20, 2019 13:58

ivanpauno self-assigned this Sep 20, 2019

get rid of the following lint error,

23f9e39

Lines should be <= 100 characters long [whitespace/line_length] [2] Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>

ivanpauno requested changes Sep 24, 2019

View reviewed changes

mabelzhang added the enhancement New feature or request label Sep 26, 2019

make the source code beautifier.

2dc4fb3

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>

ivanpauno approved these changes Sep 30, 2019

View reviewed changes

hidmic approved these changes Sep 30, 2019

View reviewed changes

ivanpauno merged commit f3d61e6 into ros2:master Oct 2, 2019

fujitatomoya deleted the topic-20190913-even_parameter_node_option_fix branch October 2, 2019 23:16

nuclearsandwich added this to Needs Backport in Dashing Patch Release 5 Dec 5, 2019

ivanpauno removed this from Needs Backport in Dashing Patch Release 5 Dec 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Need to specify NodeOption explicitly to allow declaration. #389

Need to specify NodeOption explicitly to allow declaration. #389

fujitatomoya commented Sep 13, 2019 •

edited

Loading

ivanpauno left a comment

fujitatomoya commented Sep 19, 2019

ivanpauno commented Sep 19, 2019

fujitatomoya commented Sep 20, 2019

fujitatomoya commented Sep 20, 2019

ivanpauno left a comment

ivanpauno Sep 20, 2019

fujitatomoya Sep 22, 2019

ivanpauno commented Sep 20, 2019 •

edited

Loading

ivanpauno commented Sep 23, 2019 •

edited

Loading

fujitatomoya commented Sep 24, 2019

ivanpauno left a comment

ivanpauno Sep 24, 2019

fujitatomoya Sep 30, 2019

ivanpauno Sep 24, 2019

fujitatomoya Sep 30, 2019

fujitatomoya commented Sep 30, 2019

ivanpauno commented Sep 30, 2019 •

edited

Loading

hidmic left a comment

fujitatomoya commented Oct 1, 2019

ivanpauno commented Oct 1, 2019 •

edited

Loading

ivanpauno commented Oct 1, 2019

fujitatomoya commented Oct 2, 2019

ivanpauno commented Oct 2, 2019

ivanpauno commented Oct 2, 2019

fujitatomoya commented Oct 2, 2019

ivanpauno commented Dec 10, 2019

Need to specify NodeOption explicitly to allow declaration. #389

Need to specify NodeOption explicitly to allow declaration. #389

Conversation

fujitatomoya commented Sep 13, 2019 • edited Loading

ivanpauno left a comment

Choose a reason for hiding this comment

fujitatomoya commented Sep 19, 2019

ivanpauno commented Sep 19, 2019

fujitatomoya commented Sep 20, 2019

fujitatomoya commented Sep 20, 2019

ivanpauno left a comment

Choose a reason for hiding this comment

ivanpauno Sep 20, 2019

Choose a reason for hiding this comment

fujitatomoya Sep 22, 2019

Choose a reason for hiding this comment

ivanpauno commented Sep 20, 2019 • edited Loading

ivanpauno commented Sep 23, 2019 • edited Loading

fujitatomoya commented Sep 24, 2019

ivanpauno left a comment

Choose a reason for hiding this comment

ivanpauno Sep 24, 2019

Choose a reason for hiding this comment

fujitatomoya Sep 30, 2019

Choose a reason for hiding this comment

ivanpauno Sep 24, 2019

Choose a reason for hiding this comment

fujitatomoya Sep 30, 2019

Choose a reason for hiding this comment

fujitatomoya commented Sep 30, 2019

ivanpauno commented Sep 30, 2019 • edited Loading

hidmic left a comment

Choose a reason for hiding this comment

fujitatomoya commented Oct 1, 2019

ivanpauno commented Oct 1, 2019 • edited Loading

ivanpauno commented Oct 1, 2019

fujitatomoya commented Oct 2, 2019

ivanpauno commented Oct 2, 2019

ivanpauno commented Oct 2, 2019

fujitatomoya commented Oct 2, 2019

ivanpauno commented Dec 10, 2019

fujitatomoya commented Sep 13, 2019 •

edited

Loading

ivanpauno commented Sep 20, 2019 •

edited

Loading

ivanpauno commented Sep 23, 2019 •

edited

Loading

ivanpauno commented Sep 30, 2019 •

edited

Loading

ivanpauno commented Oct 1, 2019 •

edited

Loading