Support type description service #1146
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for the ROS 2 Type Description Service, including native bindings, JavaScript API, test coverage, build integration, and an option to start the service automatically.
- Introduces C++ N-API bindings for the type description service and integrates them in the module initialization.
- Implements a JavaScript
TypeDescriptionServiceclass, updatesNodeOptions/Nodeto optionally start the service, and adds tests. - Updates
binding.gypto include the new source and extends test files to handle distro version differences.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/rcl_type_description_service_bindings.h | New header for N-API bindings of the type description service. |
| src/rcl_type_description_service_bindings.cpp | Implements init and request handling, with JS exceptions. |
| src/addon.cpp | Conditionally includes and initializes the new bindings. |
| lib/type_description_service.js | Exposes a JS class to start and handle the type description service. |
| lib/node_options.js | Adds startTypeDescriptionService option with getter/setter. |
| lib/node.js | Integrates the service into Node based on distro and options. |
| binding.gyp | Adds the new C++ source under the post-Humble condition. |
| test/test-type-description-service.js | New test for TypeDescriptionService. |
| test/test-extra-destroy-methods.js | Updates existing test to account for extra service in newer distros. |
Comments suppressed due to low confidence (2)
src/rcl_type_description_service_bindings.cpp:33
- After throwing the exception, the function continues without freeing the allocated
servicepointer. Consider freeingserviceand returning immediately to prevent memory leaks and invalid state.
Napi::Error::New(env, "Failed to initialize type description service").ThrowAsJavaScriptException();
lib/node.js:153
- DistroUtils is used here but not imported; add
const DistroUtils = require('./distro.js');at the top to avoid a runtime ReferenceError.
if (
test/test-extra-destroy-methods.js
Outdated
| if (DistroUtils.getDistroId() <= DistroUtils.getDistroId('humble')) { | ||
| assert.deepStrictEqual(node._services.length, 7); | ||
| } else { | ||
| assert.deepStrictEqual(node._services.length, 8); | ||
| } |
There was a problem hiding this comment.
[nitpick] Using hardcoded expected counts (7 and 8) for _services.length may break if other services change. Consider deriving the expected count dynamically or add a comment explaining why these specific values are used.
| if (DistroUtils.getDistroId() <= DistroUtils.getDistroId('humble')) { | |
| assert.deepStrictEqual(node._services.length, 7); | |
| } else { | |
| assert.deepStrictEqual(node._services.length, 8); | |
| } | |
| const initialServiceCount = node._services.length; | |
| node.createService(AddTwoInts, 'add_two_ints', () => {}); | |
| assert.deepStrictEqual(node._services.length, initialServiceCount + 1); |
Copilot uses AI. Check for mistakes.
| const topicType = 'std_msgs/msg/String'; | ||
| node.createPublisher(topicType, topic); | ||
| const infos = node.getPublishersInfoByTopic( | ||
| '/test_get_type_description_publisher', |
There was a problem hiding this comment.
The topic name passed to getPublishersInfoByTopic includes a leading slash ('/test_get_type_description_publisher') while the publisher was created with a relative name. Verify this matches exactly, or use the same string to avoid mismatches in test setup.
| '/test_get_type_description_publisher', | |
| topic, |
Copilot uses AI. Check for mistakes.
| ], | ||
| [ | ||
| # After Humble, e.g., Jazzy, Kilted. | ||
| 'ros_version > 2205', { |
There was a problem hiding this comment.
The GYP condition uses ros_version while the code uses ROS_VERSION. Ensure the variable names and casing align so the new source is correctly included during build.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for the ROS 2 Type Description Service by introducing native C++ N-API bindings, a new JavaScript API, and test coverage while integrating the service into the node lifecycle.
- Introduces C++ bindings for the type description service and a new TypeDescriptionService class in JavaScript.
- Updates NodeOptions and Node to optionally start the service, with corresponding test updates and build configuration changes.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/test-type-description-service.js | New test suite for verifying type description service behavior |
| test/test-extra-destroy-methods.js | Updated expectations for service destruction methods |
| src/rcl_type_description_service_bindings.h & .cpp | Added new C++ service bindings for type description service |
| src/addon.cpp | Integrated new service binding into module initialization |
| lib/type_description_service.js | New JS class implementation for type description service |
| lib/node_options.js | Added configuration option to start the type description service |
| lib/node.js | Modified to optionally initialize and start the new service |
| binding.gyp | Extended to compile new service binding source code |
Comments suppressed due to low confidence (1)
lib/node.js:153
- DistroUtils is used here but has not been imported in this file; consider adding 'const DistroUtils = require('./distro.js');' at the top of the file to avoid runtime errors.
if (DistroUtils.getDistroId() >= DistroUtils.getDistroId('jazzy') && options.startTypeDescriptionService) {
This PR adds support for the ROS 2 Type Description Service, including native bindings, JavaScript API, test coverage, build integration, and an option to start the service automatically. - Introduces C++ N-API bindings for the type description service and integrates them in the module initialization. - Implements a JavaScript `TypeDescriptionService` class, updates `NodeOptions`/`Node` to optionally start the service, and adds tests. - Updates `binding.gyp` to include the new source and extends test files to handle distro version differences. See doc: https://docs.ros.org/en/jazzy/Releases/Release-Jazzy-Jalisco.html#add-get-type-description-service Fix: #1143
2 participants
This PR adds support for the ROS 2 Type Description Service, including native bindings, JavaScript API, test coverage, build integration, and an option to start the service automatically.
TypeDescriptionServiceclass, updatesNodeOptions/Nodeto optionally start the service, and adds tests.binding.gypto include the new source and extends test files to handle distro version differences.See doc: https://docs.ros.org/en/jazzy/Releases/Release-Jazzy-Jalisco.html#add-get-type-description-service
Fix: #1143