Skip to content

Pybind11-ify rclpy_get_node_parameters#718

Merged
sloretz merged 10 commits intomasterfrom
pybind11_get_node_parameters
Mar 24, 2021
Merged

Pybind11-ify rclpy_get_node_parameters#718
sloretz merged 10 commits intomasterfrom
pybind11_get_node_parameters

Conversation

@sloretz
Copy link
Copy Markdown
Contributor

@sloretz sloretz commented Mar 17, 2021

Part of #665

@sloretz sloretz self-assigned this Mar 17, 2021
@sloretz sloretz mentioned this pull request Mar 17, 2021
Closed
34 tasks
@sloretz sloretz force-pushed the pybind11_get_node_parameters branch 2 times, most recently from ea81742 to 6115c35 Compare March 22, 2021 18:36
sloretz added 4 commits March 22, 2021 16:38
@sloretz
Partial conversion of rclpy_get_node_parameters
1883e26 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Convert slightly more
e323dee 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Clean up headers for cpplint
05c04e9 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Convert a tiny bit more
a269aef 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz force-pushed the pybind11_get_node_parameters branch from 617ea3a to a269aef Compare March 22, 2021 23:38
sloretz added 2 commits March 22, 2021 17:02
@sloretz
Convert a little more to pybind11
83458f6 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Complete translation
caa33bc 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz marked this pull request as ready for review March 23, 2021 16:47
@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Mar 23, 2021

CI (build: --packages-up-to rclpy test: --packages-select rclpy)

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

@sloretz sloretz requested a review from hidmic March 23, 2021 16:55
hidmic
hidmic approved these changes Mar 23, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM but for some minor comments !

rclpy/src/rclpy/node.cpp Outdated
if (!pynode_params.contains(pynode_name)) {
pynode_params[pynode_name] = py::dict();
}
auto parameter_dict = pynode_params[pynode_name].cast<py::dict>();
Copy link
Copy Markdown
Contributor

@hidmic hidmic Mar 23, 2021

Choose a reason for hiding this comment

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

@sloretz nit^N: Python object wrappers can be cast implicitly:

Suggested change
auto parameter_dict = pynode_params[pynode_name].cast<py::dict>();
py::dict parameter_dict = pynode_params[pynode_name];

Copy link
Copy Markdown
Contributor Author

@sloretz sloretz Mar 23, 2021

Choose a reason for hiding this comment

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

Implicit cast in 3ed7cb8

rclpy/src/rclpy/node.cpp Outdated
auto parameter_dict = pynode_params[pynode_name].cast<py::dict>();

rcl_node_params_t node_params = params->params[i];
for (size_t ii = 0; ii < node_params.num_params; ++ii) {
Copy link
Copy Markdown
Contributor

@hidmic hidmic Mar 23, 2021

Choose a reason for hiding this comment

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

@sloretz would you consider using j instead of ii ? The latter is error prone.

Copy link
Copy Markdown
Contributor Author

@sloretz sloretz Mar 23, 2021

Choose a reason for hiding this comment

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

j instead of ii in 18197c1

rclpy/src/rclpy/node.hpp Outdated

/// Get a list of parameters for the current node
/**
* On failure, an exception is raised and NULL is returned if:
Copy link
Copy Markdown
Contributor

@hidmic hidmic Mar 23, 2021

Choose a reason for hiding this comment

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

@sloretz safe to delete?

Copy link
Copy Markdown
Contributor Author

@sloretz sloretz Mar 23, 2021

Choose a reason for hiding this comment

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

Deleted this text in 33c40a3

rclpy/src/rclpy/node.cpp Outdated
{
rcl_params_t * params = nullptr;
if (RCL_RET_OK != rcl_arguments_get_param_overrides(args, &params)) {
throw RCLError("failed to get parameter overrrides");
Copy link
Copy Markdown
Contributor

@hidmic hidmic Mar 23, 2021

Choose a reason for hiding this comment

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

@sloretz typo:

Suggested change
throw RCLError("failed to get parameter overrrides");
throw RCLError("failed to get parameter overrides");

Copy link
Copy Markdown
Contributor Author

@sloretz sloretz Mar 23, 2021

Choose a reason for hiding this comment

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

overrrides -> overrides in be8fa76

rclpy/src/rclpy/node.cpp Outdated
* \param[in] pyparameter_cls The rclpy.parameter.Parameter class object.
* \param[in] node_capsule Capsule pointing to the node handle
* \return A dict mapping parameter names to rclpy.parameter.Parameter (may be empty).
*/
Copy link
Copy Markdown
Contributor

@hidmic hidmic Mar 23, 2021

Choose a reason for hiding this comment

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

@sloretz nit: isn't this docblock duplicated in the header file?

Copy link
Copy Markdown
Contributor Author

@sloretz sloretz Mar 23, 2021

Choose a reason for hiding this comment

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

Removed duplicate docblock in 33c40a3

rclpy/src/rclpy/node.hpp Outdated
* On failure, an exception is raised and NULL is returned if:
*
* Raises ValueError if the argument is not a node handle.
* Raises RCLError if the parameters file fails to parse
Copy link
Copy Markdown
Contributor

@hidmic hidmic Mar 23, 2021

Choose a reason for hiding this comment

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

@sloretz meta: I think there are quite a few more failure modes.

Copy link
Copy Markdown
Contributor Author

@sloretz sloretz Mar 23, 2021

Choose a reason for hiding this comment

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

Documented more exceptions in 33c40a3

sloretz added 4 commits March 23, 2021 14:35
@sloretz
Implicit cast
3ed7cb8 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
ii -> j
18197c1 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Update docblock documentation
33c40a3 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
overrrides -> overrides
be8fa76 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Mar 23, 2021

CI with feedback addressed (build: --packages-up-to rclpy test: --packages-select rclpy)

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

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Mar 23, 2021

CI LGTM, @hidmic may I ask you to double check that the changes address your feedback?

hidmic
hidmic approved these changes Mar 24, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM !

@sloretz sloretz merged commit f68ace5 into master Mar 24, 2021
@delete-merged-branch delete-merged-branch bot deleted the pybind11_get_node_parameters branch March 24, 2021 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hidmic hidmic hidmic approved these changes

Assignees

@sloretz sloretz

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sloretz @hidmic