Skip to content

Conversation

@minggangw
Copy link
Member

@minggangw minggangw commented Jun 17, 2025

Adds support for passing ROS command-line arguments when creating nodes by extending both the native and JavaScript APIs and verifying behavior with new tests.

  • Introduced helper functions to marshal JS arrays into C-style argv and detect unparsed ROS args
  • Extended the C++ CreateNode binding and the JS Node and createNode wrappers to accept args and a useGlobalArguments flag
  • Added TypeScript and JavaScript tests covering normal remapping, global arguments, and invalid-argument error handling

Fix: #1165

@minggangw minggangw requested a review from Copilot June 17, 2025 03:19

This comment was marked as outdated.

@coveralls
Copy link

coveralls commented Jun 17, 2025

Coverage Status

coverage: 84.726% (+0.01%) from 84.716%
when pulling f91d2b9 on minggangw:fix-1165
into 42785df on RobotWebTools:develop.

@minggangw minggangw requested a review from Copilot June 17, 2025 07:48
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds support for passing ROS command-line arguments when creating nodes by extending both the native and JavaScript APIs and verifying behavior with new tests.

  • Introduced helper functions to marshal JS arrays into C-style argv and detect unparsed ROS args
  • Extended the C++ CreateNode binding and the JS Node and createNode wrappers to accept args and a useGlobalArguments flag
  • Added TypeScript and JavaScript tests covering normal remapping, global arguments, and invalid-argument error handling

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/types/index.test-d.ts Update TypeScript definitions to include args and useGlobalArguments parameters
test/test-node.js Add JS tests for node argument parsing, global args, and error cases
src/rcl_utilities.h Declare AbstractArgsFromNapiArray, FreeArgs, and HasUnparsedROSArgs helpers
src/rcl_utilities.cpp Implement array-to-argv conversion, freeing logic, and unparsed-args detection
src/rcl_node_bindings.cpp Parse argument arrays in CreateNode, add scope guards, and wire up use_global_args
src/rcl_context_bindings.cpp Refactor context init to use helper functions for argv handling
lib/node.js Update JS Node constructor and _init to accept args and useGlobalArguments
index.js Update high-level rcl.createNode wrapper signature and docs
Comments suppressed due to low confidence (3)

lib/node.js:87

  • The options parameter is never passed through to the native createNode binding, so user-supplied NodeOptions are ignored. Consider including options before args when calling rclnodejs.createNode and updating the C++ binding signature accordingly.
this.handle = rclnodejs.createNode(name, namespace, context.handle, args, useGlobalArguments);

test/types/index.test-d.ts:85

  • A test for the default useGlobalArguments behavior (when omitted) would help ensure it actually defaults to true. Consider adding a case that omits the boolean flag.
const nodeWithArgs = rclnodejs.createNode(

src/rcl_node_bindings.cpp:193

  • [nitpick] The error message is generic. Including the original RCL error (e.g. via rcl_get_error_string().str) would make troubleshooting argument parsing failures easier.
Napi::Error::New(env, "failed to parse arguments").ThrowAsJavaScriptException();

reinterpret_cast<rcl_context_t*>(context_handle->ptr());

rcl_node_t* node = reinterpret_cast<rcl_node_t*>(malloc(sizeof(rcl_node_t)));
Napi::Array jsArgv = info[3].As<Napi::Array>();
Copy link

Copilot AI Jun 17, 2025

Choose a reason for hiding this comment

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

Argument indices no longer align with the JS wrapper signature (which also takes options). You should extract options from info[3], then shift jsArgv to info[4] and useGlobalArguments to info[5], with proper type checks and info.Length() validation.

Copilot uses AI. Check for mistakes.
}
free(argv);

FreeArgs(argv, argc);
Copy link

Copilot AI Jun 17, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider using a scope exit guard (e.g. RCPPUTILS_SCOPE_EXIT) for FreeArgs so that argv is always freed even if rcl_init or rcl_logging_configure throw an exception before reaching this line.

Copilot uses AI. Check for mistakes.
@minggangw minggangw merged commit 3c4492b into RobotWebTools:develop Jun 17, 2025
24 of 25 checks passed
minggangw added a commit that referenced this pull request Jun 18, 2025
Adds support for passing ROS command-line arguments when creating nodes by extending both the native and JavaScript APIs and verifying behavior with new tests.

- Introduced helper functions to marshal JS arrays into C-style `argv` and detect unparsed ROS args
- Extended the C++ `CreateNode` binding and the JS `Node` and `createNode` wrappers to accept `args` and a `useGlobalArguments` flag
- Added TypeScript and JavaScript tests covering normal remapping, global arguments, and invalid-argument error handling

Fix: #1165
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.

Support setting arguments for Node

2 participants