-
Notifications
You must be signed in to change notification settings - Fork 79
Support to get the RMW implementation identifier #1148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support to get the RMW implementation identifier #1148
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request adds support for retrieving the RMW implementation identifier through the Node API.
- Implements a new method in the C++ bindings to retrieve the identifier.
- Adds a corresponding method in the Node class.
- Introduces unit tests in both TypeScript and JavaScript to verify the new functionality.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/types/index.test-d.ts | Adds a type assertion for getRMWImplementationIdentifier() in TS tests. |
| test/test-node.js | Adds a JavaScript test to ensure getRMWImplementationIdentifier() returns a value. |
| src/rcl_node_bindings.cpp | Implements and exports the new binding function for retrieving the identifier. |
| lib/node.js | Adds a wrapper method in the Node class to call the new binding function. |
95dcfc6 to
891804e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for retrieving the RMW implementation identifier via the Node API.
- Implements a new C++ binding to call
rmw_get_implementation_identifier() - Exposes an instance method
getRMWImplementationIdentifier()on the JavaScriptNodeclass - Adds corresponding unit tests in TypeScript and JavaScript
- Updates build (linking
rmw_fastrtps_cpp) and CI to use the FastRTPS RMW implementation
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/types/index.test-d.ts | Added a TypeScript type assertion for node.getRMWImplementationIdentifier() |
| test/test-node.js | Added a JS unit test to verify the new RMW identifier method |
| src/rcl_node_bindings.cpp | Added GetRMWImplementationIdentifier binding and exported it |
| lib/node.js | Introduced Node.prototype.getRMWImplementationIdentifier() with JSDoc |
| binding.gyp | Linked against -lrmw_fastrtps_cpp |
| .github/workflows/windows-build-and-test.yml | Set RMW_IMPLEMENTATION=rmw_fastrtps_cpp before Windows build and test |
Comments suppressed due to low confidence (2)
src/rcl_node_bindings.cpp:449
- Missing include for
rmw_get_implementation_identifier(); please add#include <rmw/get_implementation_identifier.h>to ensure the symbol is declared.
Napi::Value GetRMWImplementationIdentifier(const Napi::CallbackInfo& info) {
test/test-node.js:570
- [nitpick] This test is placed outside the existing
describeblock; consider grouping it under a relevantdescribefor consistent test structure.
it('Get RMW identifier', function () {
binding.gyp
Outdated
| '-lrcl_yaml_param_parser', | ||
| '-lrcpputils', | ||
| '-lrmw', | ||
| '-lrmw_fastrtps_cpp', |
Copilot
AI
May 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Hardcoding the FastRTPS library couples the build to one RMW implementation; consider linking only the generic rmw library or making this conditional based on the RMW_IMPLEMENTATION variable.
| '-lrmw_fastrtps_cpp', | |
| '<!(node -p "process.env.RMW_IMPLEMENTATION === \'fastrtps\' ? \'-lrmw_fastrtps_cpp\' : \'\'")', |
|
|
||
| /** | ||
| * Get the RMW implementation identifier | ||
| * @returns {string} - The RMW implementation identifier. |
Copilot
AI
May 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] In the JSDoc comment, update the @returns tag to follow standard format: @returns {string} The RMW implementation identifier. (remove the hyphen).
| * @returns {string} - The RMW implementation identifier. | |
| * @returns {string} The RMW implementation identifier. |
This PR adds support for retrieving the RMW implementation identifier via the Node API. - Implements a new C++ binding to call `rmw_get_implementation_identifier()` - Exposes an instance method `getRMWImplementationIdentifier()` on the JavaScript `Node` class - Adds corresponding unit tests in TypeScript and JavaScript - Updates build (linking `rmw_fastrtps_cpp`) and CI to use the FastRTPS RMW implementation Fix: #1149, #1153
This PR adds support for retrieving the RMW implementation identifier via the Node API.
rmw_get_implementation_identifier()getRMWImplementationIdentifier()on the JavaScriptNodeclassrmw_fastrtps_cpp) and CI to use the FastRTPS RMW implementationFix: #1149, #1153