Replace node constructor arguments with NodeOptions by mjcarroll · Pull Request #622 · ros2/rclcpp

mjcarroll · 2019-01-29T22:40:01Z

Creates a NodeOptions object to pass all of the necessary options to the node constructor.

Connects to #492

Signed-off-by: Michael Carroll <michael@openrobotics.org>

rclcpp/include/rclcpp/node.hpp

rclcpp/include/rclcpp/node_options.hpp

rclcpp/src/rclcpp/node_options.cpp

Signed-off-by: Michael Carroll <michael@openrobotics.org>

mjcarroll · 2019-01-30T19:37:27Z

CI (up to rclcpp)

Linux
Linux-aarch64
macOS
Windows

mjcarroll · 2019-01-30T21:45:40Z

Windows failures are likely due to missing symbol visibility.

rclcpp/test/test_node_global_args.cpp

Signed-off-by: William Woodall <william@osrfoundation.org>

mjcarroll · 2019-02-05T15:17:54Z

Linux
Linux-aarch64
macOS
Windows

mjcarroll · 2019-02-05T16:06:41Z

Added missing lifecycle fixes:

Linux
Linux-aarch64
macOS
Windows

hidmic

Left a few comments here only, ros2/rcl#379 looks fine.

rclcpp/include/rclcpp/node_options.hpp

rclcpp/src/rclcpp/node_options.cpp

rclcpp/include/rclcpp/node_options.hpp

Co-Authored-By: wjwwood <william+github@osrfoundation.org> Signed-off-by: William Woodall <william@osrfoundation.org>

Signed-off-by: William Woodall <william@osrfoundation.org>

wjwwood · 2019-02-05T18:55:16Z

I had to force-push to address the DCO issue.

hidmic

LGTM pending get_env TODO and green CI!

Signed-off-by: William Woodall <william@osrfoundation.org>

wjwwood · 2019-02-05T23:04:04Z

OK, I added a todo and @mjcarroll and I tested it locally, so here's full CI, hopefully it's green:

Linux
Linux-aarch64
macOS
Windows

wjwwood · 2019-02-06T19:28:14Z

@mjcarroll do you think it would be possible to make this change tick/tock? I guess we could have left the existing constructors and deprecated them?

A separate question is whether or not it is worth doing that? @nuclearsandwich do you have an opinion on whether or not we should try to do that retroactively?

Either way we need an entry in the change summary page for dashing:

https://index.ros.org/doc/ros2/Releases/Release-Dashing-Diademata/#id4

nuclearsandwich · 2019-02-06T19:42:58Z

@nuclearsandwich do you have an opinion on whether or not we should try to do that retroactively?

I'm slightly confused about the tick and tock. Would the tick deprecating the old constructors be Dashing and then they'd be removed from E-turtle? From offline conversation yesterday I think this new API is required for a lot of things we want to introduce. Does maintaining the old constructors interfere with that? APIs that land in Dashing will be with us until May 2021 whether they be deprecated or not. I think that as long as we update the change guide now that this has merged we will be giving people plenty of time to respond to it and make the necessary updates.

wjwwood · 2019-02-06T19:48:38Z

I'm slightly confused about the tick and tock. Would the tick deprecating the old constructors be Dashing and then they'd be removed from E-turtle?

Exactly.

From offline conversation yesterday I think this new API is required for a lot of things we want to introduce. Does maintaining the old constructors interfere with that? APIs that land in Dashing will be with us until May 2021 whether they be deprecated or not.

It wouldn't affect those, as we'd only add new options to the NodeOptions struct and not the constructor. So the existing constructor could be used for some existing options, but would be deprecated and new options would only be available via the new NodeOptions constructor.

I think that as long as we update the change guide now that this has merged we will be giving people plenty of time to respond to it and make the necessary updates.

That was my original thinking, but I just wanted to leave the door open if that wasn't ok with everyone.

nuclearsandwich · 2019-02-06T19:55:04Z

That was my original thinking, but I just wanted to leave the door open if that wasn't ok with everyone.

It's okay with me at least 👍 please do link the ros2_documentation PR when it's up.

dirk-thomas · 2019-02-06T19:58:37Z

Merging this PR while the corresponding rcl PR ros2/rcl#379 hasn't been merged breaks master: https://ci.ros2.org/job/ci_linux/6132/

wjwwood · 2019-02-06T20:24:29Z

I merged the outstanding pr, that's my fault. I opened all connected prs in waffle, but either that one didn't show in waffle or I misclicked. Should be fixed now.

mjcarroll · 2019-02-06T21:03:27Z

It was actually my fault, I connected the rcl issue to this pull request and not the other issue. Sorry about that.

wjwwood · 2019-02-06T21:26:46Z

No worries, I shouldn't 🙈 trust waffle :)

* Start work on creaating NodeOptions structure. Signed-off-by: Michael Carroll <michael@openrobotics.org> * Continue work on NodeOptions. Signed-off-by: Michael Carroll <michael@openrobotics.org> * Update tests for NodeOptions impl. Signed-off-by: Michael Carroll <michael@openrobotics.org> * Update documentation and copy/assignment. Signed-off-by: Michael Carroll <michael@openrobotics.org> * Update rclcpp_lifecycle to conform to new API. Signed-off-by: Michael Carroll <michael@openrobotics.org> * Use builder pattern with NodeOptions. Signed-off-by: Michael Carroll <michael@openrobotics.org> * Documentation updates. Signed-off-by: Michael Carroll <michael@openrobotics.org> * Update rclcpp_lifecycle to use NodeOptions. Signed-off-by: Michael Carroll <michael@openrobotics.org> * change to parameter idiom only, from builder pattern Signed-off-by: William Woodall <william@osrfoundation.org> * Update rclcpp/include/rclcpp/node_options.hpp Co-Authored-By: wjwwood <william+github@osrfoundation.org> Signed-off-by: William Woodall <william@osrfoundation.org> * follow up with more resets of the rcl_node_options_t Signed-off-by: William Woodall <william@osrfoundation.org> * todo about get env Signed-off-by: William Woodall <william@osrfoundation.org>

* Added documentation to rcl_lifecycle Signed-off-by: ahcorde <ahcorde@gmail.com> * Adding more documentation to rcl_lifecycle Signed-off-by: ahcorde <ahcorde@gmail.com> * Update rcl_lifecycle/include/rcl_lifecycle/default_state_machine.h Co-Authored-By: Jorge Perez <jjperez@ekumenlabs.com> Update rcl_lifecycle/include/rcl_lifecycle/default_state_machine.h Co-Authored-By: Jorge Perez <jjperez@ekumenlabs.com> Update rcl_lifecycle/include/rcl_lifecycle/transition_map.h Co-Authored-By: Jorge Perez <jjperez@ekumenlabs.com> Update rcl_lifecycle/include/rcl_lifecycle/rcl_lifecycle.h Co-Authored-By: Jorge Perez <jjperez@ekumenlabs.com> Update rcl_lifecycle/include/rcl_lifecycle/transition_map.h Co-Authored-By: Jorge Perez <jjperez@ekumenlabs.com> Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Added more feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Fixed uncrustify Signed-off-by: ahcorde <ahcorde@gmail.com> Co-authored-by: Jorge Perez <jjperez@ekumenlabs.com>

mjcarroll and others added 4 commits January 29, 2019 11:54

Start work on creaating NodeOptions structure.

ea3d706

Signed-off-by: Michael Carroll <michael@openrobotics.org>

Continue work on NodeOptions.

dab30ee

Signed-off-by: Michael Carroll <michael@openrobotics.org>

Update tests for NodeOptions impl.

baf1035

Signed-off-by: Michael Carroll <michael@openrobotics.org>

Update documentation and copy/assignment.

8f99dee

Signed-off-by: Michael Carroll <michael@openrobotics.org>

mjcarroll added the in progress Actively being worked on (Kanban column) label Jan 29, 2019

mjcarroll mentioned this pull request Jan 29, 2019

Support for rclcpp::NodeOptions ros2/rcl#379

Merged

mjcarroll self-assigned this Jan 29, 2019

Update rclcpp_lifecycle to conform to new API.

9f59557

Signed-off-by: Michael Carroll <michael@openrobotics.org>

wjwwood reviewed Jan 29, 2019

View reviewed changes

rclcpp/include/rclcpp/node.hpp Outdated Show resolved Hide resolved

rclcpp/include/rclcpp/node_options.hpp Outdated Show resolved Hide resolved

rclcpp/include/rclcpp/node_options.hpp Outdated Show resolved Hide resolved

sloretz reviewed Jan 30, 2019

View reviewed changes

rclcpp/include/rclcpp/node_options.hpp Outdated Show resolved Hide resolved

rclcpp/src/rclcpp/node_options.cpp Outdated Show resolved Hide resolved

Michael Carroll added 3 commits January 30, 2019 11:17

Use builder pattern with NodeOptions.

93b92e3

Signed-off-by: Michael Carroll <michael@openrobotics.org>

Documentation updates.

b368e90

Signed-off-by: Michael Carroll <michael@openrobotics.org>

Update rclcpp_lifecycle to use NodeOptions.

7bd7f1e

Signed-off-by: Michael Carroll <michael@openrobotics.org>

wjwwood mentioned this pull request Feb 1, 2019

Set Parameter Event Publisher settings #591 #614

Merged

wjwwood reviewed Feb 1, 2019

View reviewed changes

rclcpp/test/test_node_global_args.cpp Outdated Show resolved Hide resolved

change to parameter idiom only, from builder pattern

006d141

Signed-off-by: William Woodall <william@osrfoundation.org>

wjwwood self-assigned this Feb 5, 2019

wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Feb 5, 2019

hidmic reviewed Feb 5, 2019

View reviewed changes

hidmic and others added 2 commits February 5, 2019 10:54

Update rclcpp/include/rclcpp/node_options.hpp

c1c1240

Co-Authored-By: wjwwood <william+github@osrfoundation.org> Signed-off-by: William Woodall <william@osrfoundation.org>

follow up with more resets of the rcl_node_options_t

ea93848

Signed-off-by: William Woodall <william@osrfoundation.org>

wjwwood force-pushed the node_arguments_constructor branch from e3bdd3e to ea93848 Compare February 5, 2019 18:54

hidmic approved these changes Feb 5, 2019

View reviewed changes

todo about get env

41cb583

Signed-off-by: William Woodall <william@osrfoundation.org>

wjwwood merged commit 0f9098e into master Feb 6, 2019

wjwwood deleted the node_arguments_constructor branch February 6, 2019 07:10

wjwwood removed the in review Waiting for review (Kanban column) label Feb 6, 2019

wjwwood mentioned this pull request Feb 21, 2019

Port exceptions submodule of moveit_core moveit/moveit2#7

Merged

mjcarroll mentioned this pull request May 9, 2019

Update Release Notes for Dashing Features ros2/ros2_documentation#186

Merged

Conversation

mjcarroll commented Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mjcarroll commented Jan 30, 2019

Uh oh!

mjcarroll commented Jan 30, 2019

Uh oh!

Uh oh!

mjcarroll commented Feb 5, 2019

Uh oh!

mjcarroll commented Feb 5, 2019

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wjwwood commented Feb 5, 2019

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

wjwwood commented Feb 5, 2019

Uh oh!

wjwwood commented Feb 6, 2019

Uh oh!

nuclearsandwich commented Feb 6, 2019

Uh oh!

wjwwood commented Feb 6, 2019

Uh oh!

nuclearsandwich commented Feb 6, 2019

Uh oh!

dirk-thomas commented Feb 6, 2019

Uh oh!

wjwwood commented Feb 6, 2019

Uh oh!

mjcarroll commented Feb 6, 2019

Uh oh!

wjwwood commented Feb 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mjcarroll commented Jan 29, 2019 •

edited

Loading