Convert Node and Context to use C++ Classes by ahcorde · Pull Request #771 · ros2/rclpy

ahcorde · 2021-04-15T16:36:48Z

Related to #665

This converts the Node and Context functions to a C++ class.

This PR depends on this other PR #728 and include the changes of this branch about GuardCondition to use C++ classes.

There is one test which is failing:

test_declare_qos_parameters_with_unhappy_callback

I will create this as a draft, we need to decide if we are going to backport the signal PR or find a workaround.

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde · 2021-04-16T16:51:53Z

The GuardCondition PR is in, I open this PR for review.

The test test_declare_qos_parameters_with_unhappy_callback is still failing:, do you mind to have a look @hidmic ?

hidmic

First pass !

rclpy/rclpy/executors.py

rclpy/src/rclpy/action_client.hpp

rclpy/src/rclpy/context.cpp

rclpy/src/rclpy/context.hpp

rclpy/src/rclpy/logging.cpp

rclpy/src/rclpy/node.hpp

hidmic · 2021-04-16T18:02:41Z

rclpy/test/test_destruction.py

        node.destroy_node()
-        with pytest.raises(InvalidHandle):
-            node.destroy_node()
+        # with pytest.raises(InvalidHandle):


@ahcorde hmm, doesn't it throw anymore? Can we destroy a node twice?

rclpy/test/test_subscription.py

Signed-off-by: ahcorde <ahcorde@gmail.com>

rclpy/src/rclpy/node.cpp

rclpy/src/rclpy/node.hpp

hidmic · 2021-04-19T13:22:41Z

rclpy/test/test_destruction.py

    try:
        node = rclpy.create_node('test_node2', context=context)
        node.destroy_node()
-        # with pytest.raises(InvalidHandle):


@ahcorde #771 (comment) still holds.

rclpy/src/rclpy/node.hpp

hidmic · 2021-04-19T13:27:58Z

rclpy/test/test_destruction.py

-            node.destroy_node()
-            # handle valid because it's still being used
-            with node.handle:
-                pass


@ahcorde why the change?

# handle valid because it's still being used

This was an intentional change in #739 with Destroyable. Once someone asks to destroy a Destroyable, it will block new uses of it. The Handle class allowed new uses as long as someone else was already using it, but that could lead to two threads blocking destruction forever as long as their uses overlapped.

That makes total sense. I still think we should test that usage attempts after destruction requests fail.

hidmic · 2021-04-19T13:28:21Z

rclpy/test/test_destruction.py

        node.destroy_node()
-        with pytest.raises(InvalidHandle):
-            node.destroy_node()
+        node.destroy_node()


@ahcorde #771 (comment) still applies.

hidmic · 2021-04-19T13:28:36Z

rclpy/test/test_handle.py

        with node.handle:
-            node.handle.destroy()
-            with node.handle:
-                pass


@ahcorde why the change?

rclpy/src/rclpy/node.cpp

rclpy/src/rclpy/context.cpp

sloretz · 2021-04-19T16:56:47Z

rclpy/rclpy/node.py

+    def __del__(self):
+        self.destroy()
+
+    def destroy(self):


unnecessary addition?

@ahcorde mind removing this method?

@sloretz it's already removed https://github.com/ros2/rclpy/blob/ahcorde/pybind11-node/rclpy/rclpy/node.py#L310-L313

Ah weird, the github UI is still showing it for me. I'll look at the new commits locally :-/

sloretz · 2021-04-19T16:57:21Z

rclpy/rclpy/node.py

    def handle(self, value):
        raise AttributeError('handle cannot be modified after node creation')

+    def __del__(self):


This seems unnecessary. All the python objects on the node will already destroy themselves when they're garbage collected.

But we should call here destroy_node() otherwise the destruction fails

But we should call here destroy_node() otherwise the destruction fails

What does that mean? What does the failure look like?

@ahcorde mind removing this method?

@sloretz it's already removed https://github.com/ros2/rclpy/blob/ahcorde/pybind11-node/rclpy/rclpy/node.py#L310-L313

rclpy/src/rclpy/_rclpy_action.cpp

rclpy/src/rclpy/context.cpp

Signed-off-by: ahcorde <ahcorde@gmail.com>

This along with an extra call to `destroy_node()` in `Node.__del__` was causing a segfault in the test_publisher test. The problem is `destroy_when_not_in_use()` was being called twice, and the second time is now a problem because rcl_ptrs_ is dereferenced in the qos_event destroy() method. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

This should not be required. If it is, it means there's a bug that shows when the Node is garbage collected. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

The order of the pointers in the RclPtrs structs is important. Things at the bottom are destroyed first, so children must be placed underneath the parent's that must be kept alive. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

…773) * Rough pass converting everything Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Fixes to get copy destruction order working Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Fix qos_event test flakiness Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde · 2021-04-23T10:45:14Z

Linux
Linux-aarch64
macOS
Windows

ahcorde · 2021-04-23T11:09:19Z

@sloretz, what about the other functions in rclpy that also take a node as the first argument? for example in names.hpp or graph.cpp. Should I move them to the Node class?

This comment is still pending, what do you think @sloretz ?

sloretz · 2021-04-23T16:41:26Z

@sloretz, what about the other functions in rclpy that also take a node as the first argument? for example in names.hpp or graph.cpp. Should I move them to the Node class?

I think those are fine actually. I would like to get rid of the _rcl_action.cpp file, but I think I was wrong to say everything should be a method on the Node. Maybe some of the action functions could be moved to names.hpp or graph.hpp themselves?

…de/pybind11-node

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde · 2021-04-26T07:35:57Z

Linux
Linux-aarch64
macOS
Windows

sloretz

Just some nitpicks, otherwise this PR looks pretty good!

rclpy/rclpy/context.py

sloretz · 2021-04-26T20:43:54Z

rclpy/src/rclpy/init.cpp


 void
-init(py::list pyargs, py::capsule pycontext, size_t domain_id)
+init(py::list pyargs, Context & context, size_t domain_id)


Is the idea to move the logic in init(...) into a constructor of Context? That sounds fine to me. I don't think the zero initialized context is used anywhere else.

rclpy/src/rclpy/publisher.cpp

rclpy/src/rclpy/subscription.hpp

rclpy/src/rclpy/timer.cpp

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde · 2021-04-27T09:22:30Z

Linux
Linux-aarch64
macOS
Windows

rclpy/rclpy/context.py

rclpy/test/test_init_shutdown.py

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde · 2021-04-28T07:32:04Z

Linux
Linux-aarch64
macOS
Windows

sloretz

LGTM! Thanks for iterating! 🎉

* Convert Publisher and Subscription to use C++ Classes (#756) * Convert Publisher and Subscription to C++ Classes Signed-off-by: ahcorde <ahcorde@gmail.com> * Fixed destructor issues Signed-off-by: ahcorde <ahcorde@gmail.com> * [pub/sub 756 addition] use std::shared_ptr holder type and shared_from_this (#757) * Use std::shared_ptr holder type Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * Use shared_from_this for pub/sub Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * Make Timer keep Clock wrapper Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * Fixed publisher and subscription destructors Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Fixed docs Signed-off-by: ahcorde <ahcorde@gmail.com> Co-authored-by: Shane Loretz <sloretz@openrobotics.org> * Use py::class_ for rcl_action_goal_handle_t (#751) * Class object Signed-off-by: Greg Balke <greg@openrobotics.org> * Python adjustments Signed-off-by: Greg Balke <greg@openrobotics.org> * Adjustments to server and enum Signed-off-by: Greg Balke <greg@openrobotics.org> * Adjust destructor calls Signed-off-by: Greg Balke <greg@openrobotics.org> * Clear out destructor Signed-off-by: Greg Balke <greg@openrobotics.org> * Do what rclcpp did Signed-off-by: Greg Balke <greg@openrobotics.org> * Remove iostream (used for debugging) Signed-off-by: Greg Balke <greg@openrobotics.org> * Matching upstream changes Signed-off-by: Greg Balke <greg@openrobotics.org> * Convert ActionClient to use C++ classes (#759) * Convert ActionClient to use C++ classes Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Added convert_from_py to utils Signed-off-by: ahcorde <ahcorde@gmail.com> * improved docs Signed-off-by: ahcorde <ahcorde@gmail.com> * nits to adapt the code to pybind11 Signed-off-by: ahcorde <ahcorde@gmail.com> * throw a exception in convert_from_py Signed-off-by: ahcorde <ahcorde@gmail.com> * Fixed docs Signed-off-by: ahcorde <ahcorde@gmail.com> * Convert ActionServer to use C++ Classes (#766) * Convert ActionServer to use C++ Classes Signed-off-by: ahcorde <ahcorde@gmail.com> * Fixed Test_action_server Signed-off-by: ahcorde <ahcorde@gmail.com> * Improved docs Signed-off-by: ahcorde <ahcorde@gmail.com> * Improved docs and minor fixes Signed-off-by: ahcorde <ahcorde@gmail.com> * Convert WaitSet to use C++ Classes (#769) * Convert WaitSet to use C++ Classes Signed-off-by: ahcorde <ahcorde@gmail.com> * added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Removed unused structs (#770) Signed-off-by: ahcorde <ahcorde@gmail.com> * Convert Guardcondition to use C++ classes (#772) * Convert Guardcondition to use C++ classes Signed-off-by: ahcorde <ahcorde@gmail.com> * make destroy guard condition capsule function private Signed-off-by: ahcorde <ahcorde@gmail.com> * Pybind11 action goal handle nitpicks (#767) * Add GoalEvent back to module Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * self._goal -> self._goal_handle Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Remove unused API Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> Co-authored-by: ahcorde <ahcorde@gmail.com> * Pybind11 actionserver nitpicks and docblock improvements (#774) Signed-off-by: ahcorde <ahcorde@gmail.com> * Convert Node and Context to use C++ Classes (#771) * convert Node and Context to use C++ Classes Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Implement solution 1 Signed-off-by: ahcorde <ahcorde@gmail.com> * Prevent double early destruction of destroyable This along with an extra call to `destroy_node()` in `Node.__del__` was causing a segfault in the test_publisher test. The problem is `destroy_when_not_in_use()` was being called twice, and the second time is now a problem because rcl_ptrs_ is dereferenced in the qos_event destroy() method. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Remove unnecessary Node.__del__ This should not be required. If it is, it means there's a bug that shows when the Node is garbage collected. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Fix order of pointers in RclPtrs structs The order of the pointers in the RclPtrs structs is important. Things at the bottom are destroyed first, so children must be placed underneath the parent's that must be kept alive. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Intentionally copy rclpy:: objects instead of having RclPtrs structs (#773) * Rough pass converting everything Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Fixes to get copy destruction order working Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Fix qos_event test flakiness Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> Signed-off-by: ahcorde <ahcorde@gmail.com> * MacOS warning Signed-off-by: ahcorde <ahcorde@gmail.com> * added feedback and removed some files Signed-off-by: ahcorde <ahcorde@gmail.com> * Create only one context otherwise throw a exception Signed-off-by: ahcorde <ahcorde@gmail.com> Co-authored-by: Shane Loretz <sloretz@osrfoundation.org> Co-authored-by: Shane Loretz <sloretz@openrobotics.org> Co-authored-by: Alejandro Hernández Cordero <alejandro@openrobotics.org> Co-authored-by: Greg Balke <11024792+gbalke@users.noreply.github.com> Co-authored-by: ahcorde <ahcorde@gmail.com>

* convert Node and Context to use C++ Classes Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Implement solution 1 Signed-off-by: ahcorde <ahcorde@gmail.com> * Prevent double early destruction of destroyable This along with an extra call to `destroy_node()` in `Node.__del__` was causing a segfault in the test_publisher test. The problem is `destroy_when_not_in_use()` was being called twice, and the second time is now a problem because rcl_ptrs_ is dereferenced in the qos_event destroy() method. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Remove unnecessary Node.__del__ This should not be required. If it is, it means there's a bug that shows when the Node is garbage collected. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Fix order of pointers in RclPtrs structs The order of the pointers in the RclPtrs structs is important. Things at the bottom are destroyed first, so children must be placed underneath the parent's that must be kept alive. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Intentionally copy rclpy:: objects instead of having RclPtrs structs (#773) * Rough pass converting everything Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Fixes to get copy destruction order working Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Fix qos_event test flakiness Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> Signed-off-by: ahcorde <ahcorde@gmail.com> * MacOS warning Signed-off-by: ahcorde <ahcorde@gmail.com> * added feedback and removed some files Signed-off-by: ahcorde <ahcorde@gmail.com> * Create only one context otherwise throw a exception Signed-off-by: ahcorde <ahcorde@gmail.com> Co-authored-by: Shane Loretz <sloretz@osrfoundation.org> Co-authored-by: Shane Loretz <sloretz@openrobotics.org>

* convert Node and Context to use C++ Classes Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Implement solution 1 Signed-off-by: ahcorde <ahcorde@gmail.com> * Prevent double early destruction of destroyable This along with an extra call to `destroy_node()` in `Node.__del__` was causing a segfault in the test_publisher test. The problem is `destroy_when_not_in_use()` was being called twice, and the second time is now a problem because rcl_ptrs_ is dereferenced in the qos_event destroy() method. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Remove unnecessary Node.__del__ This should not be required. If it is, it means there's a bug that shows when the Node is garbage collected. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Fix order of pointers in RclPtrs structs The order of the pointers in the RclPtrs structs is important. Things at the bottom are destroyed first, so children must be placed underneath the parent's that must be kept alive. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Intentionally copy rclpy:: objects instead of having RclPtrs structs (#773) * Rough pass converting everything Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Fixes to get copy destruction order working Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Fix qos_event test flakiness Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> Signed-off-by: ahcorde <ahcorde@gmail.com> * MacOS warning Signed-off-by: ahcorde <ahcorde@gmail.com> * added feedback and removed some files Signed-off-by: ahcorde <ahcorde@gmail.com> * Create only one context otherwise throw a exception Signed-off-by: ahcorde <ahcorde@gmail.com> Co-authored-by: Shane Loretz <sloretz@osrfoundation.org> Co-authored-by: Shane Loretz <sloretz@openrobotics.org> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

ahcorde added the enhancement New feature or request label Apr 15, 2021

ahcorde requested review from hidmic and sloretz April 15, 2021 16:36

ahcorde self-assigned this Apr 15, 2021

ahcorde mentioned this pull request Apr 15, 2021

Use Pybind11 for rclpy Python Bindings #665

Closed

34 tasks

convert Node and Context to use C++ Classes

527fb10

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde force-pushed the ahcorde/pybind11-node branch from e9365a8 to 527fb10 Compare April 16, 2021 16:49

ahcorde marked this pull request as ready for review April 16, 2021 16:50

hidmic reviewed Apr 16, 2021

View reviewed changes

Added feedback

0532746

Signed-off-by: ahcorde <ahcorde@gmail.com>

hidmic reviewed Apr 19, 2021

View reviewed changes

rclpy/src/rclpy/node.cpp Show resolved Hide resolved

hidmic reviewed Apr 19, 2021

View reviewed changes

rclpy/src/rclpy/context.cpp Show resolved Hide resolved

sloretz reviewed Apr 19, 2021

View reviewed changes

ahcorde and others added 5 commits April 20, 2021 11:41

Added feedback

3d2c567

Signed-off-by: ahcorde <ahcorde@gmail.com>

Implement solution 1

7b0c4dc

Signed-off-by: ahcorde <ahcorde@gmail.com>

Remove unnecessary Node.__del__

21e80db

This should not be required. If it is, it means there's a bug that shows when the Node is garbage collected. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Fix order of pointers in RclPtrs structs

ad53180

The order of the pointers in the RclPtrs structs is important. Things at the bottom are destroyed first, so children must be placed underneath the parent's that must be kept alive. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz mentioned this pull request Apr 22, 2021

Intentionally copy rclpy:: objects instead of having RclPtrs structs #773

Merged

ahcorde force-pushed the ahcorde/pybind11-node branch from f36aaac to 33b848d Compare April 23, 2021 10:43

ahcorde added 2 commits April 26, 2021 09:20

Merge remote-tracking branch 'origin/post_galactic_freeze' into ahcor…

5b43d93

…de/pybind11-node

MacOS warning

a4e2fb1

Signed-off-by: ahcorde <ahcorde@gmail.com>

sloretz reviewed Apr 26, 2021

View reviewed changes

added feedback and removed some files

52c68cd

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde requested a review from sloretz April 27, 2021 09:22

sloretz reviewed Apr 27, 2021

View reviewed changes

rclpy/rclpy/context.py Outdated Show resolved Hide resolved

rclpy/test/test_init_shutdown.py Outdated Show resolved Hide resolved

Create only one context otherwise throw a exception

2ae5f2c

Signed-off-by: ahcorde <ahcorde@gmail.com>

sloretz approved these changes Apr 28, 2021

View reviewed changes

ahcorde merged commit afdd25e into post_galactic_freeze Apr 28, 2021

delete-merged-branch bot deleted the ahcorde/pybind11-node branch April 28, 2021 15:41

SpaceView mentioned this pull request Mar 23, 2022

Failed <<< rclpy [ Exited with code 1 ] #578

Closed

Conversation

ahcorde commented Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahcorde commented Apr 16, 2021

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

ahcorde commented Apr 23, 2021

Uh oh!

ahcorde commented Apr 23, 2021

Uh oh!

sloretz commented Apr 23, 2021

Uh oh!

ahcorde commented Apr 26, 2021

Uh oh!

sloretz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahcorde commented Apr 27, 2021

Uh oh!

Uh oh!

Uh oh!

ahcorde commented Apr 28, 2021

ahcorde commented Apr 15, 2021 •

edited

Loading