Replace node constructor arguments with NodeOptions #622
Conversation
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
|
CI (up to rclcpp)
|
|
Windows failures are likely due to missing symbol visibility. |
Signed-off-by: William Woodall <william@osrfoundation.org>
wjwwood
added
in review
|
|
Added missing lifecycle fixes:
|
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
force-pushed
the
node_arguments_constructor
branch
from
e3bdd3e
to
ea93848
Compare
|
I had to force-push to address the DCO issue. |
Signed-off-by: William Woodall <william@osrfoundation.org>
|
OK, I added a todo and @mjcarroll and I tested it locally, so here's full CI, hopefully it's green:
|
|
@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 |
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. |
Exactly.
It wouldn't affect those, as we'd only add new options to the
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. |
|
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/ |
|
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. |
|
It was actually my fault, I connected the |
|
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>
Creates a NodeOptions object to pass all of the necessary options to the node constructor.
Connects to #492