Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,10 @@ let rcl = {
* @param {string} [namespace=''] - The namespace used in ROS.
* @param {Context} [context=Context.defaultContext()] - The context to create the node in.
* @param {NodeOptions} [options=NodeOptions.defaultOptions] - The options to configure the new node behavior.
* @param {Array} [args=[]] - The arguments to pass to the node.
* @param {boolean} [useGlobalArguments=true] - If true, the node will use the global arguments
* from the context, otherwise it will only use the arguments
* passed in the args parameter.
* @return {Node} A new instance of the specified node.
* @throws {Error} If the given context is not registered.
* @deprecated since 0.18.0, Use new Node constructor.
Expand All @@ -197,9 +201,18 @@ let rcl = {
nodeName,
namespace = '',
context = Context.defaultContext(),
options = NodeOptions.defaultOptions
options = NodeOptions.defaultOptions,
args = [],
useGlobalArguments = true
) {
return new this.Node(nodeName, namespace, context, options);
return new this.Node(
nodeName,
namespace,
context,
options,
args,
useGlobalArguments
);
},

/**
Expand Down
16 changes: 12 additions & 4 deletions lib/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,24 +66,32 @@ class Node extends rclnodejs.ShadowNode {
nodeName,
namespace = '',
context = Context.defaultContext(),
options = NodeOptions.defaultOptions
options = NodeOptions.defaultOptions,
args = [],
useGlobalArguments = true
) {
super();

if (typeof nodeName !== 'string' || typeof namespace !== 'string') {
throw new TypeError('Invalid argument.');
}

this._init(nodeName, namespace, options, context);
this._init(nodeName, namespace, options, context, args, useGlobalArguments);
debug(
'Finish initializing node, name = %s and namespace = %s.',
nodeName,
namespace
);
}

_init(name, namespace, options, context) {
this.handle = rclnodejs.createNode(name, namespace, context.handle);
_init(name, namespace, options, context, args, useGlobalArguments) {
this.handle = rclnodejs.createNode(
name,
namespace,
context.handle,
args,
useGlobalArguments
);
Object.defineProperty(this, 'handle', {
configurable: false,
writable: false,
Expand Down
21 changes: 4 additions & 17 deletions src/rcl_context_bindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,18 +53,9 @@ Napi::Value Init(const Napi::CallbackInfo& info) {

// Preprocess argc & argv
Napi::Array jsArgv = info[1].As<Napi::Array>();
int argc = jsArgv.Length();
char** argv = nullptr;

if (argc > 0) {
argv = reinterpret_cast<char**>(malloc(argc * sizeof(char*)));
for (int i = 0; i < argc; i++) {
std::string arg = jsArgv.Get(i).As<Napi::String>().Utf8Value();
int len = arg.length() + 1;
argv[i] = reinterpret_cast<char*>(malloc(len * sizeof(char)));
snprintf(argv[i], len, "%s", arg.c_str());
}
}
size_t argc = jsArgv.Length();
char** argv = AbstractArgsFromNapiArray(jsArgv);

// Set up the domain id.
size_t domain_id = RCL_DEFAULT_DOMAIN_ID;
if (info.Length() > 2 && info[2].IsBigInt()) {
Expand All @@ -87,11 +78,7 @@ Napi::Value Init(const Napi::CallbackInfo& info) {
RCL_RET_OK, rcl_logging_configure(&context->global_arguments, &allocator),
rcl_get_error_string().str);

for (int i = 0; i < argc; i++) {
free(argv[i]);
}
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.
return env.Undefined();
}

Expand Down
28 changes: 27 additions & 1 deletion src/rcl_node_bindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
#include <rcl_yaml_param_parser/parser.h>
#include <rcl_yaml_param_parser/types.h>

#include <rcpputils/scope_exit.hpp>
// NOLINTNEXTLINE
#include <string>

#include "macros.h"
Expand Down Expand Up @@ -180,10 +182,34 @@ Napi::Value CreateNode(const Napi::CallbackInfo& info) {
rcl_context_t* context =
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.

The CreateNode binding signature now ignores the original options parameter and shifts jsArgv and useGlobalArguments to indices 3 and 4. This will drop user-specified NodeOptions and misalign arguments. Adjust the binding to read options from info[3], then jsArgv from info[4], and useGlobalArguments from info[5], matching the JS API.

Suggested change
Napi::Array jsArgv = info[3].As<Napi::Array>();
Napi::Object optionsObject = info[3].As<Napi::Object>();
Napi::Array jsArgv = info[4].As<Napi::Array>();

Copilot uses AI. Check for mistakes.
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.
size_t argc = jsArgv.Length();
char** argv = AbstractArgsFromNapiArray(jsArgv);
RCPPUTILS_SCOPE_EXIT({ FreeArgs(argv, argc); });

rcl_arguments_t arguments = rcl_get_zero_initialized_arguments();
rcl_ret_t ret =
rcl_parse_arguments(argc, argv, rcl_get_default_allocator(), &arguments);
if ((ret != RCL_RET_OK) || HasUnparsedROSArgs(arguments)) {
Napi::Error::New(env, "failed to parse arguments")
.ThrowAsJavaScriptException();
return env.Undefined();
}

RCPPUTILS_SCOPE_EXIT({
if (RCL_RET_OK != rcl_arguments_fini(&arguments)) {
Napi::Error::New(env, "failed to fini arguments")
.ThrowAsJavaScriptException();
rcl_reset_error();
}
});
bool use_global_arguments = info[4].As<Napi::Boolean>().Value();
rcl_node_t* node = reinterpret_cast<rcl_node_t*>(malloc(sizeof(rcl_node_t)));
*node = rcl_get_zero_initialized_node();

rcl_node_options_t options = rcl_node_get_default_options();
options.use_global_arguments = use_global_arguments;
options.arguments = arguments;
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.

The argv buffer allocated by AbstractArgsFromNapiArray is not freed on the successful path, resulting in a memory leak. Call FreeArgs(argv, argc) after rcl_node_init (or use a scope guard) to release the allocated memory.

Copilot uses AI. Check for mistakes.

THROW_ERROR_IF_NOT_EQUAL(RCL_RET_OK,
rcl_node_init(node, node_name.c_str(),
Expand Down
31 changes: 31 additions & 0 deletions src/rcl_utilities.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include <rmw/topic_endpoint_info.h>
#include <uv.h>

#include <cstdio>
#include <memory>
#include <string>

Expand Down Expand Up @@ -260,4 +261,34 @@ Napi::Array ConvertToJSTopicEndpointInfoList(
return list;
}

char** AbstractArgsFromNapiArray(const Napi::Array& jsArgv) {
size_t argc = jsArgv.Length();
char** argv = nullptr;

if (argc > 0) {
argv = reinterpret_cast<char**>(malloc(argc * sizeof(char*)));
for (size_t i = 0; i < argc; i++) {
std::string arg = jsArgv.Get(i).As<Napi::String>().Utf8Value();
int len = arg.length() + 1;
argv[i] = reinterpret_cast<char*>(malloc(len * sizeof(char)));
snprintf(argv[i], len, "%s", arg.c_str());
}
}
return argv;
}

void FreeArgs(char** argv, size_t argc) {
if (argv) {
for (size_t i = 0; i < argc; i++) {
free(argv[i]);
}
free(argv);
}
}

bool HasUnparsedROSArgs(const rcl_arguments_t& rcl_args) {
int unparsed_ros_args_count = rcl_arguments_get_count_unparsed_ros(&rcl_args);
return unparsed_ros_args_count != 0;
}

} // namespace rclnodejs
7 changes: 7 additions & 0 deletions src/rcl_utilities.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,13 @@ Napi::Array ConvertToJSTopicEndpointInfoList(

Napi::Value ConvertToQoS(Napi::Env env, const rmw_qos_profile_t* qos_profile);

// `AbstractArgsFromNapiArray` and `FreeArgs` must be called in pairs.
char** AbstractArgsFromNapiArray(const Napi::Array& jsArgv);
// `AbstractArgsFromNapiArray` and `FreeArgs` must be called in pairs.
void FreeArgs(char** argv, size_t argc);

bool HasUnparsedROSArgs(const rcl_arguments_t& rcl_args);

} // namespace rclnodejs

#endif // SRC_RCL_UTILITIES_H_
82 changes: 82 additions & 0 deletions test/test-node.js
Original file line number Diff line number Diff line change
Expand Up @@ -572,3 +572,85 @@ describe('Test the node with no handles attached when initializing', function ()
assert.notStrictEqual(node.getRMWImplementationIdentifier().length, 0);
});
});

describe('Node arguments', function () {
this.timeout(60 * 1000);

it('Test node arguments', async function () {
await rclnodejs.init();
var node = rclnodejs.createNode(
'publisher',
'/topic_getter',
Context.defaultContext(),
NodeOptions.defaultOptions,
['--ros-args', '-r', '__ns:=/foo/bar']
);
assert.deepStrictEqual(node.namespace(), '/foo/bar');
node.destroy();
rclnodejs.shutdown();
});

it('Test node global arguments', async function () {
await rclnodejs.init(Context.defaultContext(), [
'process_name',
'--ros-args',
'-r',
'__node:=global_node_name',
]);
const node1 = rclnodejs.createNode(
'publisher',
'/topic_getter',
Context.defaultContext(),
NodeOptions.defaultOptions,
['--ros-args', '-r', '__ns:=/foo/bar']
);

const node2 = rclnodejs.createNode(
'my_node',
'/topic_getter',
Context.defaultContext(),
NodeOptions.defaultOptions,
[],
false
);

assert.deepStrictEqual(node1.name(), 'global_node_name');
assert.deepStrictEqual(node2.name(), 'my_node');
node1.destroy();
node2.destroy();
rclnodejs.shutdown();
});

it('Test node invalid arguments', async function () {
await rclnodejs.init();
assert.throws(
() => {
rclnodejs.createNode(
'invalid_node1',
'/topic1',
Context.defaultContext(),
NodeOptions.defaultOptions,
['--ros-args', '-r', 'not-a-remap']
);
},
Error,
/failed to parse arguments/
);

assert.throws(
() => {
rclnodejs.createNode(
'invalid_node2',
'/topic2',
Context.defaultContext(),
NodeOptions.defaultOptions,
['--ros-args', '--my-custom-flag']
);
},
Error,
/failed to parse arguments/
);

rclnodejs.shutdown();
});
});
9 changes: 9 additions & 0 deletions test/types/index.test-d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,15 @@ expectType<rclnodejs.Options<string | rclnodejs.QoS>>(
);
expectType<string>(node.getFullyQualifiedName());
expectType<string>(node.getRMWImplementationIdentifier());
const nodeWithArgs = rclnodejs.createNode(
NODE_NAME,
'topic',
rclnodejs.Context.defaultContext(),
rclnodejs.NodeOptions.defaultOptions,
['--ros-args', '-r', '__ns:=/foo/bar'],
false
);
expectType<rclnodejs.Node>(nodeWithArgs);

// ---- LifecycleNode ----
const lifecycleNode = rclnodejs.createLifecycleNode(LIFECYCLE_NODE_NAME);
Expand Down
7 changes: 6 additions & 1 deletion types/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,19 @@ declare module 'rclnodejs' {
* @param namespace - The namespace used in ROS, default is an empty string.
* @param context - The context, default is Context.defaultContext().
* @param options - The node options, default is NodeOptions.defaultOptions.
* @param args - The arguments to be passed to the node, default is an empty array.
* @param useGlobalArguments - If true, the node will use global arguments, default is true.
* If false, the node will not use global arguments.
* @returns The new Node instance.
* @deprecated since 0.18.0, Use new Node constructor.
*/
function createNode(
nodeName: string,
namespace?: string,
context?: Context,
options?: NodeOptions
options?: NodeOptions,
args?: string[],
useGlobalArguments?: boolean
): Node;

/**
Expand Down
Loading