Convert more node accessors to pybind11 by gbalke · Pull Request #730 · ros2/rclpy

gbalke · 2021-03-22T19:21:27Z

Part of #665.

Handles:

rclpy_get_node_logger_name
rclpy_get_node_names_and_namespaces
rclpy_get_node_names_and_namespaces_with_enclaves
rclpy_node_get_fully_qualified_name

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

gbalke · 2021-03-22T19:39:55Z

CI up to rclpy:

Linux
Linux-aarch64
macOS
Windows

rclpy/src/rclpy/node.cpp

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

rclpy/src/rclpy/node.cpp

hidmic · 2021-03-23T12:44:12Z

rclpy/src/rclpy/node.cpp

+          "failed to fini node names during error handling: ");
+        RCUTILS_SAFE_FWRITE_TO_STDERR(rcl_get_error_string().str);
+        RCUTILS_SAFE_FWRITE_TO_STDERR("\n");
+        rcl_reset_error();


@gbalke if an error was set by function A, resetting it before calling function B avoids warnings about error overwrite if function B happens to set an error as well. That is, consider moving each of these if clauses after the corresponding function call. It's pure printing, so no risk of an exception getting thrown there.

I think I got mixed up because of #730 (comment). I'll change it back since I'm not throwing now.

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

hidmic

LGTM but for one comment, pending green CI

hidmic · 2021-03-23T20:44:54Z

rclpy/src/rclpy/node.cpp


  RCPPUTILS_SCOPE_EXIT(
    {
-      fini_names_ret = rcutils_string_array_fini(&node_names);
-      fini_namespaces_ret = rcutils_string_array_fini(&node_namespaces);
-      fini_enclaves_ret = rcutils_string_array_fini(&enclaves);
-      if (RCUTILS_RET_OK != fini_names_ret) {
+      for (size_t idx = 0; idx < node_names.size; ++idx) {
+        if (get_enclaves) {
+          pynode_names_and_namespaces[idx] = py::make_tuple(
+            py::str(node_names.data[idx]),
+            py::str(node_namespaces.data[idx]),
+            py::str(enclaves.data[idx]));
+        } else {
+          pynode_names_and_namespaces[idx] = py::make_tuple(
+            py::str(node_names.data[idx]),
+            py::str(node_namespaces.data[idx]));
+        }
+      }
+


@gbalke this won't have the intended effect. If it throws, it'll throw from a destructor and C++ will terminate the program :)

If I may, you may want to do something like:

RCPPUTILS_SCOPE_EXIT({ // setup cleanup block to be run on scope exit rcutils_ret_t fini_ret = rcutils_string_array_fini(&node_names); if (RCUTILS_RET_OK != fini_ret) { RCUTILS_SAFE_FWRITE_TO_STDERR( "[rclpy|" RCUTILS_STRINGIFY(__FILE__) ":" RCUTILS_STRINGIFY(__LINE__) "]: " "failed to fini node names during error handling: "); RCUTILS_SAFE_FWRITE_TO_STDERR(rcl_get_error_string().str); RCUTILS_SAFE_FWRITE_TO_STDERR("\n"); rcl_reset_error(); } fini_ret = rcutils_string_array_fini(&node_namespaces); if (RCUTILS_RET_OK != fini_ret) { RCUTILS_SAFE_FWRITE_TO_STDERR( "[rclpy|" RCUTILS_STRINGIFY(__FILE__) ":" RCUTILS_STRINGIFY(__LINE__) "]: " "failed to fini node namespaces during error handling: "); RCUTILS_SAFE_FWRITE_TO_STDERR(rcl_get_error_string().str); RCUTILS_SAFE_FWRITE_TO_STDERR("\n"); rcl_reset_error(); } fini_ret = rcutils_string_array_fini(&enclaves); if (RCUTILS_RET_OK != fini_ret) { RCUTILS_SAFE_FWRITE_TO_STDERR( "[rclpy|" RCUTILS_STRINGIFY(__FILE__) ":" RCUTILS_STRINGIFY(__LINE__) "]: " "failed to fini enclaves string array during error handling: "); RCUTILS_SAFE_FWRITE_TO_STDERR(rcl_get_error_string().str); RCUTILS_SAFE_FWRITE_TO_STDERR("\n"); rcl_reset_error(); } }); py::list pynode_names_and_namespaces(node_names.size); for (size_t idx = 0; idx < node_names.size; ++idx) { if (get_enclaves) { pynode_names_and_namespaces[idx] = py::make_tuple( py::str(node_names.data[idx]), py::str(node_namespaces.data[idx]), py::str(enclaves.data[idx])); } else { pynode_names_and_namespaces[idx] = py::make_tuple( py::str(node_names.data[idx]), py::str(node_namespaces.data[idx])); } } return pynode_names_and_namespaces; // and then cleanup code is run automatically

What I guess i don't understand is that the fini calls will happen before the for loop? Or is that the special magic of the SCOPE_EXIT statement? Where that will be called upon completion?

In essence, you're registering a callback with RCCPUTILS_SCOPE_EXIT.

Read the comments on the blocks and now it makes sense 👍. Thanks for walking me through it haha.

Or is that the special magic of the SCOPE_EXIT statement?

If you look under the hood, this RCPPUTILS_SCOPE_EXIT macro instantiates an object that takes your code, wraps it in a lambda function, and executes it on destruction. Using object construction/destruction to manage resource lifecycles is so common in C++ that it has a name. It's the primary idiom when it comes to exception safety.

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

gbalke · 2021-03-23T20:56:17Z

CI up to rclpy

Linux
Linux-aarch64
macOS
Windows

gbalke · 2021-03-24T01:26:25Z

@sloretz lmk if you want to review this before I merge.

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

gbalke · 2021-03-24T20:03:27Z

@ros-pull-request-builder retest this please ❤️

gbalke · 2021-03-24T20:06:15Z

Ci up to rclpy

Linux
Linux-aarch64
macOS
Windows

gbalke · 2021-03-24T20:19:36Z

@ros-pull-request-builder retest this please 💔

gbalke requested review from cottsay and sloretz March 22, 2021 19:21

gbalke self-assigned this Mar 22, 2021

Greg Balke added 4 commits March 22, 2021 12:21

Initial move and completion of minor changes.

b4667f0

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

node_get -> get_node for consistency

caeb041

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

Adding node, namespace, and enclave accessors.

fb71dab

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

Finishing up conversion

865536e

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

gbalke force-pushed the more_node_accessors branch from 65b3947 to 865536e Compare March 22, 2021 19:22

Merge branch 'master' into more_node_accessors

9663b11

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

hidmic reviewed Mar 22, 2021

View reviewed changes

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

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

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

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

Greg Balke added 4 commits March 22, 2021 15:46

Nits

1a08f5d

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

Merge branch 'master' into more_node_accessors

c1cd076

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

Scope handler

f0bd0a7

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

Fixing whitespace error when in paste mode

3415fed

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

hidmic reviewed Mar 23, 2021

View reviewed changes

sloretz mentioned this pull request Mar 23, 2021

Use Pybind11 for rclpy Python Bindings #665

Closed

34 tasks

Scoping adjustments

6a01f3f

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

gbalke requested a review from hidmic March 23, 2021 17:37

hidmic reviewed Mar 23, 2021

View reviewed changes

Fix ordering for cleanup registration

0a4fbd8

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

gbalke force-pushed the more_node_accessors branch from 612b848 to 0a4fbd8 Compare March 23, 2021 20:51

hidmic approved these changes Mar 23, 2021

View reviewed changes

Merge branch 'master' into more_node_accessors

a145f50

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

gbalke merged commit eefcb1c into ros2:master Mar 24, 2021

gbalke deleted the more_node_accessors branch March 24, 2021 20:41

Conversation

gbalke commented Mar 22, 2021

Uh oh!

gbalke commented Mar 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hidmic Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

gbalke Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

hidmic Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gbalke Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

hidmic Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gbalke commented Mar 23, 2021

Uh oh!

gbalke commented Mar 24, 2021

Uh oh!

gbalke commented Mar 24, 2021

Uh oh!

gbalke commented Mar 24, 2021

Uh oh!

gbalke commented Mar 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gbalke Mar 23, 2021 •

edited

Loading

hidmic Mar 23, 2021 •

edited

Loading