Skip to content

Migrate qos event APIs to pybind11#723

Merged
hidmic merged 3 commits intomasterfrom
hidmic/pybind-events
Mar 22, 2021
Merged

Migrate qos event APIs to pybind11#723
hidmic merged 3 commits intomasterfrom
hidmic/pybind-events

Conversation

@hidmic
Copy link
Copy Markdown
Contributor

@hidmic hidmic commented Mar 19, 2021

Precisely what the title says. Part of #665.

CI up to rclpy:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@hidmic hidmic requested review from gbalke and sloretz March 19, 2021 18:07
@hidmic hidmic force-pushed the hidmic/pybind-events branch from 0f3f8d5 to 6935e66 Compare March 19, 2021 18:30
Copy link
Copy Markdown
Contributor

@gbalke gbalke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

}
if (RCL_RET_UNSUPPORTED == ret) {
rcl_reset_error();
throw std::runtime_error("take event is not implemented");
Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor

@sloretz sloretz left a 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

py::capsule
event_wrap_in_capsule(unique_cstruct_ptr<rcl_event_t> event, py::capsule pyparent)
{
rclpy_handle_t * event_handle = _rclpy_create_handle(
Copy link
Copy Markdown
Contributor

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.

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;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 2795b16.

hidmic added 3 commits March 22, 2021 11:28
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic force-pushed the hidmic/pybind-events branch from 6ba21f2 to 2795b16 Compare March 22, 2021 14:28
@hidmic
Copy link
Copy Markdown
Contributor Author

hidmic commented Mar 22, 2021

Rebased to solve merge conflicts. PTAL !

@hidmic hidmic requested a review from sloretz March 22, 2021 14:29
@hidmic
Copy link
Copy Markdown
Contributor Author

hidmic commented Mar 22, 2021

CI up to rclpy:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@hidmic
Copy link
Copy Markdown
Contributor Author

hidmic commented Mar 22, 2021

Going in!

@hidmic hidmic merged commit 75d9948 into master Mar 22, 2021
@delete-merged-branch delete-merged-branch bot deleted the hidmic/pybind-events branch March 22, 2021 17:25
@sloretz sloretz mentioned this pull request Mar 22, 2021
34 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants