-
Notifications
You must be signed in to change notification settings - Fork 81
Add getServersInfoByService and getClientsInfoByService for node #1335
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
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 PR adds two new methods to the Node class for querying service endpoint information: getClientsInfoByService and getServersInfoByService. These methods mirror the existing getPublishersInfoByTopic and getSubscriptionsInfoByTopic functionality but for ROS services instead of topics.
- Adds
getClientsInfoByService()andgetServersInfoByService()methods to query service endpoint information - Implements C++ bindings using new ROS 2 service endpoint info APIs
- Includes TypeScript type definitions and test coverage for the new methods
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| types/node.d.ts | Adds TypeScript type definitions for the two new service info query methods |
| test/types/index.test-d.ts | Adds type tests for the new methods |
| test/test-graph.js | Adds integration tests for querying clients and servers info by service name |
| src/rcl_utilities.h | Declares the new ConvertToJSServiceEndpointInfoList conversion function |
| src/rcl_utilities.cpp | Implements conversion from RMW service endpoint info structures to JavaScript objects |
| src/rcl_graph_bindings.cpp | Implements C++ bindings for the service info query functions and exports them |
| lib/node.js | Implements the JavaScript API methods with validation and version checking |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Napi::Value GetInfoByService( | ||
| Napi::Env env, rcl_node_t* node, const char* service_name, bool no_mangle, | ||
| const char* type, rcl_get_info_by_service_func_t rcl_get_info_by_service) { | ||
| rcutils_allocator_t allocator = rcutils_get_default_allocator(); | ||
| rcl_service_endpoint_info_array_t info_array = | ||
| rcl_get_zero_initialized_service_endpoint_info_array(); | ||
|
|
||
| RCPPUTILS_SCOPE_EXIT({ | ||
| rcl_ret_t fini_ret = | ||
| rcl_service_endpoint_info_array_fini(&info_array, &allocator); | ||
| if (RCL_RET_OK != fini_ret) { | ||
| Napi::Error::New(env, rcl_get_error_string().str) | ||
| .ThrowAsJavaScriptException(); | ||
| rcl_reset_error(); | ||
| } | ||
| }); | ||
|
|
||
| rcl_ret_t ret = rcl_get_info_by_service(node, &allocator, service_name, | ||
| no_mangle, &info_array); | ||
| if (RCL_RET_OK != ret) { | ||
| if (RCL_RET_UNSUPPORTED == ret) { | ||
| Napi::Error::New( | ||
| env, std::string("Failed to get information by service for ") + type + | ||
| ": function not supported by RMW_IMPLEMENTATION") | ||
| .ThrowAsJavaScriptException(); | ||
| return env.Undefined(); | ||
| } | ||
| Napi::Error::New( | ||
| env, std::string("Failed to get information by service for ") + type) | ||
| .ThrowAsJavaScriptException(); | ||
| return env.Undefined(); | ||
| } | ||
|
|
||
| return ConvertToJSServiceEndpointInfoList(env, &info_array); | ||
| } |
Copilot
AI
Dec 1, 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.
The GetInfoByService function uses types and functions (e.g., rcl_service_endpoint_info_array_t, rcl_get_zero_initialized_service_endpoint_info_array(), rcl_service_endpoint_info_array_fini()) that only exist in ROS versions > 2405. This entire function should be guarded with #if ROS_VERSION > 2405 and #endif to prevent compilation errors on older ROS versions.
| * @returns {Array} - list of clients | ||
| */ | ||
| getClientsInfoByService(service, noDemangle = false) { | ||
| if (DistroUtils.getDistroId() < DistroUtils.DistroId.ROLLING) { |
Copilot
AI
Dec 1, 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.
The version check is inconsistent with the C++ code. The C++ code uses #if ROS_VERSION > 2405 (greater than JAZZY), which includes KILTED (2505) and ROLLING (5000). However, this JavaScript check < DistroUtils.DistroId.ROLLING only includes ROLLING and excludes KILTED, causing a mismatch.
For KILTED (2505):
- C++ code will compile the functions (2505 > 2405 = true)
- JavaScript code will return null (2505 < 5000 = true)
The check should be <= DistroUtils.DistroId.JAZZY to match the C++ version check.
| if (DistroUtils.getDistroId() < DistroUtils.DistroId.ROLLING) { | |
| if (DistroUtils.getDistroId() <= DistroUtils.DistroId.JAZZY) { |
| * @returns {Array} - list of servers | ||
| */ | ||
| getServersInfoByService(service, noDemangle = false) { | ||
| if (DistroUtils.getDistroId() < DistroUtils.DistroId.ROLLING) { |
Copilot
AI
Dec 1, 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.
The version check is inconsistent with the C++ code. The C++ code uses #if ROS_VERSION > 2405 (greater than JAZZY), which includes KILTED (2505) and ROLLING (5000). However, this JavaScript check < DistroUtils.DistroId.ROLLING only includes ROLLING and excludes KILTED, causing a mismatch.
For KILTED (2505):
- C++ code will compile the functions (2505 > 2405 = true)
- JavaScript code will return null (2505 < 5000 = true)
The check should be <= DistroUtils.DistroId.JAZZY to match the C++ version check.
| rclnodejs.DistroUtils.getDistroId() < | ||
| rclnodejs.DistroUtils.DistroId.ROLLING |
Copilot
AI
Dec 1, 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.
The version check is inconsistent with the C++ code. The C++ code uses #if ROS_VERSION > 2405 (greater than JAZZY), which includes KILTED (2505) and ROLLING (5000). However, this test skip condition < DistroUtils.DistroId.ROLLING only includes ROLLING and excludes KILTED.
For KILTED (2505):
- C++ code will compile the functions (2505 > 2405 = true)
- Test will be skipped (2505 < 5000 = true)
The check should be <= DistroUtils.DistroId.JAZZY to match the C++ version check.
| rclnodejs.DistroUtils.getDistroId() < | |
| rclnodejs.DistroUtils.DistroId.ROLLING | |
| rclnodejs.DistroUtils.getDistroId() <= | |
| rclnodejs.DistroUtils.DistroId.JAZZY |
| rclnodejs.DistroUtils.getDistroId() < | ||
| rclnodejs.DistroUtils.DistroId.ROLLING |
Copilot
AI
Dec 1, 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.
The version check is inconsistent with the C++ code. The C++ code uses #if ROS_VERSION > 2405 (greater than JAZZY), which includes KILTED (2505) and ROLLING (5000). However, this test skip condition < DistroUtils.DistroId.ROLLING only includes ROLLING and excludes KILTED.
For KILTED (2505):
- C++ code will compile the functions (2505 > 2405 = true)
- Test will be skipped (2505 < 5000 = true)
The check should be <= DistroUtils.DistroId.JAZZY to match the C++ version check.
|
|
||
| typedef rcl_ret_t (*rcl_get_info_by_service_func_t)( | ||
| const rcl_node_t* node, rcutils_allocator_t* allocator, | ||
| const char* service_name, bool no_mangle, | ||
| rcl_service_endpoint_info_array_t* info_array); |
Copilot
AI
Dec 1, 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.
The typedef for rcl_get_info_by_service_func_t uses types that only exist in ROS versions > 2405. This typedef should be guarded with #if ROS_VERSION > 2405 to prevent compilation errors on older ROS versions.
| typedef rcl_ret_t (*rcl_get_info_by_service_func_t)( | |
| const rcl_node_t* node, rcutils_allocator_t* allocator, | |
| const char* service_name, bool no_mangle, | |
| rcl_service_endpoint_info_array_t* info_array); | |
| #if ROS_VERSION > 2405 | |
| typedef rcl_ret_t (*rcl_get_info_by_service_func_t)( | |
| const rcl_node_t* node, rcutils_allocator_t* allocator, | |
| const char* service_name, bool no_mangle, | |
| rcl_service_endpoint_info_array_t* info_array); | |
| #endif |
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
Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This PR adds two new methods to the Node class for querying service endpoint information:
getClientsInfoByServiceandgetServersInfoByService. These methods mirror the existinggetPublishersInfoByTopicandgetSubscriptionsInfoByTopicfunctionality but for ROS services instead of topics.getClientsInfoByService()andgetServersInfoByService()methods to query service endpoint informationFix: #1330