Skip to content

Conversation

minggangw
Copy link
Member

@minggangw minggangw commented May 23, 2025

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

@minggangw minggangw requested a review from Copilot May 23, 2025 10:30
Copy link

@Copilot 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

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.

@coveralls
Copy link

coveralls commented May 23, 2025

Coverage Status

coverage: 84.823% (+0.005%) from 84.818%
when pulling 206dd34 on minggangw:get-rmw-identifier
into 2b5711b on RobotWebTools:develop.

@minggangw minggangw changed the base branch from develop to jazzy May 23, 2025 13:21
@minggangw minggangw changed the base branch from jazzy to kilted May 23, 2025 13:21
@minggangw minggangw changed the base branch from kilted to develop May 23, 2025 13:21
@minggangw minggangw force-pushed the get-rmw-identifier branch from 95dcfc6 to 891804e Compare May 26, 2025 10:40
@minggangw minggangw requested a review from Copilot May 27, 2025 03:21
Copy link

@Copilot 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

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

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 describe block; consider grouping it under a relevant describe for consistent test structure.
  it('Get RMW identifier', function () {

binding.gyp Outdated
'-lrcl_yaml_param_parser',
'-lrcpputils',
'-lrmw',
'-lrmw_fastrtps_cpp',
Copy link

Copilot AI May 27, 2025

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.

Suggested change
'-lrmw_fastrtps_cpp',
'<!(node -p "process.env.RMW_IMPLEMENTATION === \'fastrtps\' ? \'-lrmw_fastrtps_cpp\' : \'\'")',

Copilot uses AI. Check for mistakes.


/**
* Get the RMW implementation identifier
* @returns {string} - The RMW implementation identifier.
Copy link

Copilot AI May 27, 2025

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).

Suggested change
* @returns {string} - The RMW implementation identifier.
* @returns {string} The RMW implementation identifier.

Copilot uses AI. Check for mistakes.

@minggangw minggangw merged commit f2dd1f0 into RobotWebTools:develop May 27, 2025
25 of 26 checks passed
minggangw added a commit that referenced this pull request Jun 6, 2025
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
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 to get RMW identifier

2 participants