Conversation
Signed-off-by: Greg Balke <greg@openrobotics.org>
Signed-off-by: Greg Balke <greg@openrobotics.org>
rclpy/src/rclpy/node.cpp
Outdated
|
|
||
| const char * node_name = rcl_node_get_name(node); | ||
| if (!node_name) { | ||
| return py::none(); |
There was a problem hiding this comment.
Huh, I see this is what the code was doing already, but it seems incorrect. It looks like when rcl_node_get_name fails it sets the error state, so returning None means the error state is still set. That's not a big problem, AFAIK it means it will print extra error messages the next time something tries to set the error state again.
I would recommend changing this API to throw RCLError instead of returning None. That means the return type could be const char * too (IIUC pybind11 will handle the conversion to a python string from that return type).
rclpy/src/rclpy/node.cpp
Outdated
|
|
||
| const char * node_namespace = rcl_node_get_namespace(node); | ||
| if (!node_namespace) { | ||
| return py::none(); |
There was a problem hiding this comment.
Same comment about making this API throw RCLError here.
rclpy/src/rclpy/node.hpp
Outdated
|
|
||
| #include <pybind11/pybind11.h> | ||
|
|
||
| #include <string> |
There was a problem hiding this comment.
I forgot to clean up headers after the copy/pasting. Sorry about that!
rclpy/src/rclpy/node.cpp
Outdated
|
|
||
| #include "rclpy_common/exceptions.hpp" | ||
|
|
||
| #include "names.hpp" |
There was a problem hiding this comment.
I think this should be including "node.hpp"
rclpy/src/rclpy/node.cpp
Outdated
| #include <rmw/error_handling.h> | ||
| #include <rmw/validate_full_topic_name.h> | ||
| #include <rmw/validate_namespace.h> | ||
| #include <rmw/validate_node_name.h> |
There was a problem hiding this comment.
It seems like there are a lot of unused headers here. I see reason for:
- pybind11/pybind11.h for all the
py::stuff - rcl/node.h for
rcl_node_t,rcl_node_get_name(),rcl_node_get_namespace() - rclpy_common/handle.h for the `rclpy_handle_get_pointer_from_capsule
And if you change the code to throw RCLError instead: "rclpy_common/exceptions.hpp"
But if you don't change it and instead just want to clear the error state: "rcl/error_handling.h"
I'm pretty sure the rest are unused.
Signed-off-by: Greg Balke <greg@openrobotics.org>
Signed-off-by: Greg Balke <greg@openrobotics.org>
Signed-off-by: Greg Balke <greg@openrobotics.org>
hidmic
left a comment
There was a problem hiding this comment.
Overall LGTM pending green CI
rclpy/src/rclpy/node.hpp
Outdated
|
|
||
| #include <pybind11/pybind11.h> | ||
|
|
||
| #include <string> |
Signed-off-by: Greg Balke <greg@openrobotics.org>
|
CI up to
|
Part of #665.