[Kilted] Support to configure introspection for action client/server #1128
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for action introspection by introducing configuration functions and tests in both the action client and server modules, as well as corresponding C++ bindings.
- Added utility functions and tests to manage configuration of introspection for action clients and servers.
- Introduced new C++ functions guarded by ROS_VERSION checks to support introspection configuration.
- Updated both JavaScript and TypeScript test files to verify the new introspection functionality.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/utils.js | Added utility function to check if introspection is supported |
| test/types/index.test-d.ts | Added test assertions for client/server configuration introspection |
| test/test-action-server.js | Added test case for action server introspection configuration |
| test/test-action-client.js | Added test case for action client introspection configuration |
| src/rcl_action_server_bindings.cpp | Added C++ binding for configuring introspection on action servers |
| src/rcl_action_client_bindings.cpp | Added C++ binding for configuring introspection on action clients |
| lib/action/server.js | Added introspection configuration function in ActionServer |
| lib/action/client.js | Added introspection configuration function in ActionClient |
lib/action/server.js
Outdated
| configureIntrospection(clock, qos, introspectionState) { | ||
| if (DistroUtils.getDistroId() <= DistroUtils.getDistroId('jazzy')) { | ||
| console.warn( | ||
| 'Service introspection is not supported by this versionof ROS 2' |
There was a problem hiding this comment.
There is a missing space in the warning message: 'this versionof ROS 2' should be 'this version of ROS 2'.
| 'Service introspection is not supported by this versionof ROS 2' | |
| 'Service introspection is not supported by this version of ROS 2' |
lib/action/client.js
Outdated
| configureIntrospection(clock, qos, introspectionState) { | ||
| if (DistroUtils.getDistroId() <= DistroUtils.getDistroId('jazzy')) { | ||
| console.warn( | ||
| 'Service introspection is not supported by this versionof ROS 2' |
There was a problem hiding this comment.
There is a missing space in the warning message: 'this versionof ROS 2' should be 'this version of ROS 2'.
| 'Service introspection is not supported by this versionof ROS 2' | |
| 'Service introspection is not supported by this version of ROS 2' |
There was a problem hiding this comment.
Pull Request Overview
This PR extends action client/server functionality by adding introspection support across the codebase.
- Added isActionIntrospectionSupported helper and tests for introspection support.
- Extended C++ bindings and updated JavaScript modules for both action client and server to allow configuring introspection.
- Updated TypeScript tests to verify the new introspection configuration functions.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/utils.js | Added isActionIntrospectionSupported and imported DistroUtils |
| test/types/index.test-d.ts | Added type tests for introspection configuration functions |
| test/test-action-server.js | Added a test case for action server introspection configuration |
| test/test-action-client.js | Added a test case for action client introspection configuration |
| src/rcl_action_server_bindings.cpp | Introduced ConfigureActionServerIntrospection with conditional inclusion |
| src/rcl_action_client_bindings.cpp | Introduced ConfigureActionClientIntrospection with conditional inclusion |
| lib/action/server.js | Added configureIntrospection method for ActionServer |
| lib/action/client.js | Added configureIntrospection method for ActionClient |
src/rcl_action_server_bindings.cpp
Outdated
|
|
||
| #include <rcl/error_handling.h> | ||
| #include <rcl/rcl.h> | ||
| #include <rcl_action/action_client.h> |
There was a problem hiding this comment.
The inclusion of 'rcl_action/action_client.h' in the server bindings file appears unnecessary since only server functionality is used; consider removing it to reduce potential confusion and maintain clean separation.
| #include <rcl_action/action_client.h> |
| 'use strict'; | ||
|
|
||
| const assert = require('assert'); | ||
| const { DistroUtils } = require('../index.js'); |
There was a problem hiding this comment.
[nitpick] For consistency with production code, consider importing DistroUtils directly from '../distro.js' instead of '../index.js' to avoid any potential circular dependency issues.
| const { DistroUtils } = require('../index.js'); | |
| const { DistroUtils } = require('../distro.js'); |
minggangw
changed the title
2 participants
This PR adds support for action introspection by introducing configuration functions and tests in both the action client and server modules, as well as corresponding C++ bindings.
Fix: #1129