-
Notifications
You must be signed in to change notification settings - Fork 85
Add QoS overriding options for publishers and subscriptions #1468
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -48,6 +48,10 @@ const Service = require('./service.js'); | |||||||||||||||||
| const Subscription = require('./subscription.js'); | ||||||||||||||||||
| const ObservableSubscription = require('./observable_subscription.js'); | ||||||||||||||||||
| const MessageInfo = require('./message_info.js'); | ||||||||||||||||||
| const { | ||||||||||||||||||
| declareQosParameters, | ||||||||||||||||||
| _resolveQoS, | ||||||||||||||||||
| } = require('./qos_overriding_options.js'); | ||||||||||||||||||
| const TimeSource = require('./time_source.js'); | ||||||||||||||||||
| const Timer = require('./timer.js'); | ||||||||||||||||||
| const TypeDescriptionService = require('./type_description_service.js'); | ||||||||||||||||||
|
|
@@ -752,6 +756,21 @@ class Node extends rclnodejs.ShadowNode { | |||||||||||||||||
| ); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // Apply QoS overriding options if provided | ||||||||||||||||||
| if (options.qosOverridingOptions) { | ||||||||||||||||||
| const resolvedTopic = this.resolveTopicName(topic); | ||||||||||||||||||
| if (typeof options.qos === 'string' || !(options.qos instanceof QoS)) { | ||||||||||||||||||
| options.qos = _resolveQoS(options.qos); | ||||||||||||||||||
|
Comment on lines
+766
to
+767
|
||||||||||||||||||
| if (typeof options.qos === 'string' || !(options.qos instanceof QoS)) { | |
| options.qos = _resolveQoS(options.qos); | |
| // Only resolve QoS profile *strings* (and undefined → default); | |
| // for plain objects, construct a QoS instance so their fields are preserved. | |
| if (typeof options.qos === 'string' || typeof options.qos === 'undefined') { | |
| options.qos = _resolveQoS(options.qos); | |
| } else if (options.qos && !(options.qos instanceof QoS)) { | |
| options.qos = new QoS(options.qos); |
Copilot
AI
Apr 2, 2026
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 publisher creation path now supports options.qosOverridingOptions, but the JSDoc for createPublisher/_createPublisher options doesn't mention this new option or the parameter naming convention. Since this is a public API surface, please update the relevant docstring to document qosOverridingOptions and how/when overrides are applied (including that QoS profile strings will be resolved to a mutable QoS object).
Copilot
AI
Apr 2, 2026
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.
Same issue as in _createPublisher(): if qosOverridingOptions is set and the user passes a plain-object QoS (supported by the native layer), this condition will replace it with _resolveQoS(...) (default profile), silently ignoring the caller’s provided QoS fields. Limit _resolveQoS to profile strings, and handle non-QoS objects explicitly (convert or throw) so overrides apply to the intended base QoS.
| if (typeof options.qos === 'string' || !(options.qos instanceof QoS)) { | |
| options.qos = _resolveQoS(options.qos); | |
| if (typeof options.qos === 'string') { | |
| // Resolve QoS profile strings to QoS instances | |
| options.qos = _resolveQoS(options.qos); | |
| } else if (options.qos && !(options.qos instanceof QoS)) { | |
| // Convert plain-object or non-QoS QoS values to QoS instances | |
| options.qos = new QoS(options.qos); |
Copilot
AI
Apr 2, 2026
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.
Same as publisher: createSubscription now accepts options.qosOverridingOptions, but the createSubscription JSDoc options list doesn't document it. Please add it to the documentation so consumers discover the feature and understand the parameter naming/override behavior.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,306 @@ | ||||||
| // Copyright (c) 2026 The Robot Web Tools Contributors. All rights reserved. | ||||||
| // | ||||||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||||||
| // you may not use this file except in compliance with the License. | ||||||
| // You may obtain a copy of the License at | ||||||
| // | ||||||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||||||
| // | ||||||
| // Unless required by applicable law or agreed to in writing, software | ||||||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||||||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||
| // See the License for the specific language governing permissions and | ||||||
| // limitations under the License. | ||||||
|
|
||||||
| 'use strict'; | ||||||
|
|
||||||
| const QoS = require('./qos.js'); | ||||||
| const { | ||||||
| Parameter, | ||||||
| ParameterType, | ||||||
| ParameterDescriptor, | ||||||
| } = require('./parameter.js'); | ||||||
|
|
||||||
| /** | ||||||
| * Enum of overridable QoS policy kinds. | ||||||
| * Each value corresponds to a QoS property on the {@link QoS} class. | ||||||
| * @enum {number} | ||||||
| */ | ||||||
| const QoSPolicyKind = Object.freeze({ | ||||||
| HISTORY: 1, | ||||||
| DEPTH: 2, | ||||||
| RELIABILITY: 3, | ||||||
| DURABILITY: 4, | ||||||
| LIVELINESS: 5, | ||||||
| AVOID_ROS_NAMESPACE_CONVENTIONS: 6, | ||||||
| }); | ||||||
|
|
||||||
| // Maps QoSPolicyKind -> { qosProp, paramType, toParam, fromParam } | ||||||
| const POLICY_MAP = { | ||||||
| [QoSPolicyKind.HISTORY]: { | ||||||
| qosProp: 'history', | ||||||
| enumObj: QoS.HistoryPolicy, | ||||||
| paramType: ParameterType.PARAMETER_STRING, | ||||||
| toParam: (val, enumObj) => _enumToString(val, enumObj), | ||||||
| fromParam: (val, enumObj) => _stringToEnum(val, enumObj), | ||||||
| }, | ||||||
| [QoSPolicyKind.DEPTH]: { | ||||||
| qosProp: 'depth', | ||||||
| paramType: ParameterType.PARAMETER_INTEGER, | ||||||
| toParam: (val) => val, | ||||||
|
||||||
| toParam: (val) => val, | |
| toParam: (val) => BigInt(val), |
Copilot
AI
Apr 2, 2026
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 parameter suffix is derived from mapping.qosProp. For AVOID_ROS_NAMESPACE_CONVENTIONS this yields a camelCase parameter name (...avoidRosNameSpaceConventions), unlike the other policy parameters (history, depth, ...). Since these parameter names are user-facing/stable, consider decoupling the parameter key from the QoS property name (e.g., use a consistent snake_case key like avoid_ros_namespace_conventions) while still mapping back to qos.avoidRosNameSpaceConventions.
Copilot
AI
Apr 2, 2026
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.
_resolveQoS() currently collapses several supported QoS profile strings into the same hard-coded defaults. In particular, QoS.profileSystemDefault should preserve "system default" semantics (history/reliability/durability/liveliness = SYSTEM_DEFAULT, depth = 0), and profiles like profileParameters/profileParameterEvents/profileActionStatusDefault are not handled at all (native layer supports them). Please add explicit cases for all QoS.profile* strings so using qosOverridingOptions doesn't silently change the effective QoS when a profile string is provided.
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.
index.js now exports QoSPolicyKind and QoSOverridingOptions, but the published TypeScript declarations (types/*.d.ts) don't currently declare these exports or the new
qosOverridingOptionsoption on publisher/subscription creation. Please update the typings (e.g., types/index.d.ts and the Node publisher/subscription option types) to keep TS consumers in sync with the JS API.