Skip to content

fix(rmw-zenoh-rs): stop double-declaring parameter services - #358

Merged
YuanYuYuan merged 2 commits into
mainfrom
fix/rmw-zenoh-rs-double-param-services
Sep 23, 2026
Merged

YuanYuYuan merged 2 commits into
mainfrom
fix/rmw-zenoh-rs-double-param-services

Conversation

@YuanYuYuan

Copy link
Copy Markdown
Collaborator

Summary

Every rmw-zenoh-rs node declares its own six ROS 2 parameter services on top of the ones rclcpp already provides for it. Two Queryables answer the same service names with two independent implementations. This is a correctness concern, not only extra load: a real ros2 param get/set call can be answered by either one.

Root cause

rclcpp::Node's constructor already sets up its own generic parameter-service machinery (describe_parameters, get_parameters, get_parameter_types, list_parameters, set_parameters, set_parameters_atomically, plus /parameter_events) over rmw_create_service, unless the application opts out with NodeOptions::start_parameter_services(false).

NodeImpl::new() in rmw-zenoh-rs never told hiroz's ZContextBuilder to skip its own internal ParameterService, which is enabled by default. So every node ended up with both sets declared under identical names.

Fix

Add .without_parameters() to the builder chain in NodeImpl::new(). rclcpp's own parameter services are unaffected — they never depended on hiroz's internal one, which only matters when hiroz is driven through its native API rather than as an RMW backend under rclcpp.

What fails without this

before this PR with this PR
NodeImpl::new builds a node declares 6 parameter-service Queryables of its own, on top of whatever rclcpp adds declares none
no_parameter_services.rs (added here) FAILED: lists all 6 duplicated service names ok

The new test builds a NodeImpl directly against an in-process router (no rclcpp process needed — NodeImpl::new is plain Rust) and reads the discovery graph after a settle window. Reverting only the .without_parameters() line reproduces the failure with the exact service names; restoring it passes.

Breaking Changes

None. rmw-zenoh-rs nodes stop declaring redundant services; rclcpp's own parameter services are unaffected.

Checklist

  • Added a test: crates/rmw-zenoh-rs/tests/no_parameter_services.rs. Verified both directions (fails without the fix, passes with it) in the ros-jazzy devshell — rmw-zenoh-rs cannot build under a plain Rust devshell (its build.rs needs ROS headers).
  • Ran ./scripts/check-local.sh successfully. Not run as one command: rmw-zenoh-rs is outside the pure-Rust workspace subset that script covers on this environment. cargo test -p rmw-zenoh-rs --test no_parameter_services was run directly, both directions, above.

rclcpp::Node's constructor already sets up its own generic parameter-
service machinery via rmw_create_service (the standard six services:
describe_parameters, get_parameters, get_parameter_types,
list_parameters, set_parameters, set_parameters_atomically), unless the
application opts out with NodeOptions::start_parameter_services(false).
NodeImpl::new() never called .without_parameters() on hiroz's
ZContextBuilder, so every rmw-zenoh-rs node also got hiroz's own
built-in ParameterService on top: two Queryables answering the same six
service names, backed by two independent implementations. Beyond the
extra router load, this is a correctness concern -- two declarers of
the same service key are both eligible to answer a real
`ros2 param get/set` call, which is undefined/racy.

Add .without_parameters() to NodeImpl::new()'s builder chain. rclcpp's
own generic parameter services still work; they never depended on
hiroz's internal ParameterService, which is only useful when hiroz is
driven through its own native API rather than as an RMW backend under
rclcpp.
NodeImpl::new is plain Rust and needs no live rclcpp process to build:
open an in-process router, build a node through it, and read the
service names in the discovery graph after a settle window. Asserts
none of them belong to this node -- the absence the fix establishes.
@YuanYuYuan
YuanYuYuan force-pushed the fix/rmw-zenoh-rs-double-param-services branch from c7237c1 to 8ab163a Compare September 23, 2026 07:45
@YuanYuYuan
YuanYuYuan merged commit 037bfb5 into main Sep 23, 2026
31 checks passed
@YuanYuYuan
YuanYuYuan deleted the fix/rmw-zenoh-rs-double-param-services branch September 23, 2026 08:15
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.

1 participant