Use py::class_ for rcl_action_goal_handle_t by gbalke · Pull Request #751 · ros2/rclpy

gbalke · 2021-04-02T02:20:26Z

Part of #665.

rclpy/src/rclpy/action_goal_handle.cpp

rclpy/rclpy/action/server.py

gbalke · 2021-04-05T21:39:47Z

In a certain condition, the state of an allocator is in a questionable state. In this scenario, the de-allocate call inside of the fini causes a segfault. I'm still uncertain as to if this is a rcl bug or an rclpy issue.

Thread 1 "python3" hit Breakpoint 1, rcl_action_goal_handle_fini (goal_handle=0x1f7be10) at /rclpy_ws/src/rcl/rcl_action/src/rcl_action/goal_handle.c:79
79          goal_handle->impl->allocator.deallocate(goal_handle->impl, goal_handle->impl->allocator.state);
(gdb) p goal_handle->impl->allocator.state
$3 = (void *) 0x0
(gdb) p goal_handle->impl->allocator
$4 = {allocate = 0x555555555555, deallocate = 0x555555550000, reallocate = 0x0, zero_allocate = 0x555555550000, state = 0x0}
(gdb) n

Thread 1 "python3" received signal SIGSEGV, Segmentation fault.
0x0000555555550000 in ?? ()

gbalke · 2021-04-06T17:20:42Z

@ros-pull-request-builder retest this

rclpy/src/rclpy/action_goal_handle.cpp

gbalke · 2021-04-06T18:44:37Z

CI up to rclpy:

Linux
Linux-aarch64
macOS
Windows

ahcorde

Do you mind to add these changes 6939425 to your PR

hidmic

LGTM once @ahcorde's comments have been addressed

gbalke · 2021-04-07T19:10:05Z

CI up to rclpy:

Linux
Linux-aarch64
macOS
Windows

sloretz · 2021-04-07T22:37:48Z

Something seems off with the diff on github. It looks like this PR includes all of #756 - the conversion of Publisher and Subscription. Is that intended?

gbalke · 2021-04-07T23:35:39Z

Do you mind to add these changes 6939425 to your PR

@sloretz I thought that was what this was asking for?

ahcorde · 2021-04-08T06:22:00Z

@greg the Publisher and Subscriber PR is in (in the post_galactic_freeze branch). I think you should remove the merge with the branch pybind11-pubsub (which is adding my commits too) and then merge your branch with post_galactic_freeze.

Signed-off-by: Greg Balke <greg@openrobotics.org>

gbalke · 2021-04-08T17:27:47Z

Did all the rebasing shenanigans.

CI up to rclpy:

Linux
Linux-aarch64
macOS
Windows

sloretz · 2021-04-08T17:48:47Z

rclpy/rclpy/action/server.py

    ACCEPT = 2


-class GoalEvent(Enum):


Since GoalEvent was on the module before, put GoalEvent = _rclpy.GoalEvent as done here:

rclpy/rclpy/rclpy/clock.py

Lines 20 to 21 in a75529d

ClockType = _rclpy.ClockType

ClockChange = _rclpy.ClockChange

sloretz · 2021-04-08T17:51:38Z

rclpy/rclpy/action/server.py

        """
-        self._handle = _rclpy.rclpy_action_accept_new_goal(
-            action_server._handle, goal_info)
+        self._goal = _rclpy.ActionGoalHandle(action_server._handle, goal_info)


_goal means something else in this context. I would make this _goal_handle rather than _goal. An action server receives goals from clients, and then uses a goal handle to track the state of the goal.

sloretz · 2021-04-08T17:53:47Z

rclpy/src/rclpy/_rclpy_action.cpp

-    "rclpy_action_goal_handle_is_active", &rclpy_action_goal_handle_is_active,
-    "Check if a goal is active.");
+
+  define_action_goal_handle(m);


I would put this in _rclpy_pybind11.cpp. I hope that _rclpy_action.cpp will go away once everything is converted.

sloretz · 2021-04-08T17:55:06Z

rclpy/src/rclpy/_rclpy_action.cpp


-py::capsule
+rclpy::ActionGoalHandle
 rclpy_action_accept_new_goal(py::capsule pyaction_server, py::object pygoal_info_msg)


Recommend replacing the code calling rclpy_action_accept_new_goal with the creation of an _rclpy.ActionGoalHandle(...) in Python to get rid of this API.

sloretz · 2021-04-08T17:59:20Z

rclpy/src/rclpy/action_goal_handle.cpp

+ActionGoalHandle::ActionGoalHandle(
+  py::capsule pyaction_server, py::object pygoal_info_msg)
+{
+  auto action_server = get_pointer<rcl_action_server_t *>(


I think an ActionGoalHandle instance needs to keep the ActionServer alive - but that's hard because ActionServer isn't using the rclpy_handle_t wrapper. I don't think we can do anything about it in this PR, and it's a preexisting bug. This is just making a note that the ActionServer PR should add an std::shared_ptr to the ActionServer on ActionGoalHandle.

* 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>

* 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>

* 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> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

ahcorde reviewed Apr 2, 2021

View reviewed changes

rclpy/src/rclpy/action_goal_handle.cpp Outdated Show resolved Hide resolved

ahcorde reviewed Apr 2, 2021

View reviewed changes

rclpy/rclpy/action/server.py Outdated Show resolved Hide resolved

sloretz mentioned this pull request Apr 2, 2021

Use Pybind11 for rclpy Python Bindings #665

Closed

34 tasks

hidmic reviewed Apr 6, 2021

View reviewed changes

rclpy/src/rclpy/action_goal_handle.cpp Outdated Show resolved Hide resolved

gbalke requested review from ahcorde and hidmic April 6, 2021 20:28

ahcorde requested changes Apr 7, 2021

View reviewed changes

hidmic approved these changes Apr 7, 2021

View reviewed changes

gbalke requested a review from ahcorde April 7, 2021 19:10

sloretz changed the base branch from master to post_galactic_freeze April 7, 2021 22:38

Greg Balke added 7 commits April 8, 2021 10:16

Class object

5494bd5

Signed-off-by: Greg Balke <greg@openrobotics.org>

Python adjustments

9e208dc

Signed-off-by: Greg Balke <greg@openrobotics.org>

Adjustments to server and enum

5378186

Signed-off-by: Greg Balke <greg@openrobotics.org>

Adjust destructor calls

6b03ddd

Signed-off-by: Greg Balke <greg@openrobotics.org>

Clear out destructor

c9f4e4b

Signed-off-by: Greg Balke <greg@openrobotics.org>

Do what rclcpp did

612684e

Signed-off-by: Greg Balke <greg@openrobotics.org>

Remove iostream (used for debugging)

d3b8e9c

Signed-off-by: Greg Balke <greg@openrobotics.org>

gbalke force-pushed the classify-rcl_action_goal_handle branch 2 times, most recently from cefb06f to edcdc04 Compare April 8, 2021 17:20

Greg Balke added 2 commits April 8, 2021 10:23

Matching upstream changes

9de13da

Signed-off-by: Greg Balke <greg@openrobotics.org>

Merge branch 'post_galactic_freeze' into classify-rcl_action_goal_handle

a92ad1f

Signed-off-by: Greg Balke <greg@openrobotics.org>

gbalke force-pushed the classify-rcl_action_goal_handle branch from edcdc04 to a92ad1f Compare April 8, 2021 17:26

ahcorde approved these changes Apr 8, 2021

View reviewed changes

gbalke merged commit 6288657 into ros2:post_galactic_freeze Apr 8, 2021

gbalke deleted the classify-rcl_action_goal_handle branch April 8, 2021 19:55

sloretz reviewed Apr 8, 2021

View reviewed changes

sloretz mentioned this pull request Apr 9, 2021

Pybind11 action goal handle nitpicks #767

Merged

	ClockType = _rclpy.ClockType
	ClockChange = _rclpy.ClockChange

Conversation

gbalke commented Apr 2, 2021

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

gbalke commented Apr 6, 2021

Uh oh!

Uh oh!

gbalke commented Apr 6, 2021

Uh oh!

ahcorde 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!

gbalke commented Apr 7, 2021

Uh oh!

sloretz commented Apr 7, 2021

Uh oh!

gbalke commented Apr 7, 2021

Uh oh!

ahcorde commented Apr 8, 2021

Uh oh!

gbalke commented Apr 8, 2021

Uh oh!

sloretz Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

sloretz Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

sloretz Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

sloretz Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

sloretz Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gbalke commented Apr 5, 2021 •

edited

Loading