-
Notifications
You must be signed in to change notification settings - Fork 222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate qos event APIs to pybind11 #723
Conversation
0f3f8d5
to
6935e66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
rclpy/src/rclpy/qos_events.cpp
Outdated
} | ||
if (RCL_RET_UNSUPPORTED == ret) { | ||
rcl_reset_error(); | ||
throw std::runtime_error("take event is not implemented"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this isn't documented in take_event
's header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. This is nonsense though. There's no documentation of rcl_take_event()
returning RCL_RET_UNSUPPORTED. There's no implementation that does that either. See 6ba21f2 for a fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple nice-to-have's
rclpy/src/rclpy/qos_events.cpp
Outdated
py::capsule | ||
event_wrap_in_capsule(unique_cstruct_ptr<rcl_event_t> event, py::capsule pyparent) | ||
{ | ||
rclpy_handle_t * event_handle = _rclpy_create_handle( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind using this pattern with rclpy_create_handle_capsule
instead of _rclpy_create_handle
? I think it's a little easier to follow.
rclpy/rclpy/src/rclpy/subscription.cpp
Lines 106 to 122 in 41f9c2f
PyObject * pysub_c = | |
rclpy_create_handle_capsule(sub.get(), "rclpy_subscription_t", _rclpy_destroy_subscription); | |
if (!pysub_c) { | |
throw py::error_already_set(); | |
} | |
auto pysub = py::reinterpret_steal<py::capsule>(pysub_c); | |
// pysub now owns the rclpy_subscription_t | |
sub.release(); | |
auto sub_handle = static_cast<rclpy_handle_t *>(pysub); | |
auto node_handle = static_cast<rclpy_handle_t *>(pynode); | |
_rclpy_handle_add_dependency(sub_handle, node_handle); | |
if (PyErr_Occurred()) { | |
throw py::error_already_set(); | |
} | |
return pysub; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 2795b16.
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
6ba21f2
to
2795b16
Compare
Rebased to solve merge conflicts. PTAL ! |
Going in! |
Precisely what the title says. Part of #665.
CI up to
rclpy
: