From 2b38303fb5b3244853ab56484dd9b8ddd480430b Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Fri, 29 May 2026 14:50:23 +0800 Subject: [PATCH 01/18] =?UTF-8?q?feat(hiroz-py):=20rclpy=20alignment=20?= =?UTF-8?q?=E2=80=94=20P1=E2=80=93P8=20API=20improvements?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements all 8 proposals from the rclpy API alignment review: P1 — wait_for_service / wait_for_server / wait_for_subscription Adds graph-polling wait primitives to ZClient, ZActionClient, and ZPublisher. Replaces time.sleep(1.0) anti-pattern in all examples. P2 — Smart error for swapped arguments create_publisher/create_subscriber detect when msg_type and topic args are positionally swapped (rclpy order vs hiroz order) and raise a clear TypeError. P3 — Method aliases ZNode.create_subscription aliased to create_subscriber. P4 — Service grouping type in codegen python_msgspec_generator now emits AddTwoInts.Request / .Response grouping classes with __srvtype__. create_client/create_server accept the grouping class or the bare Request class (back-compat). P5 — Wire custom exception types Timeouts now raise hiroz_py.TimeoutError instead of bare RuntimeError. Tests and examples updated accordingly. P6 — Optional callback-style create_server create_server accepts optional callback= kwarg; spawns a background thread when provided. Pull mode (take_request/send_response) remains the default. ZNode.create_service added as alias. P7 — Action grouping type in codegen Actions emit Fibonacci.Goal / .Result / .Feedback grouping classes with __actiontype__. create_action_client/server accept either form. P8 — QoS enum constants + int-depth shorthand ReliabilityPolicy, DurabilityPolicy, HistoryPolicy, LivelinessPolicy enums added to __init__.py. QoS params accept int (depth shorthand, rclpy-style). Also adds test_rclpy_alignment.py (17 tests, all passing). 65 total tests pass, clippy clean, ruff clean. --- .../src/python_msgspec_generator.rs | 53 +++- .../python/hiroz_msgs_py/types/action_msgs.py | 6 + .../hiroz_msgs_py/types/example_interfaces.py | 18 ++ .../hiroz_msgs_py/types/lifecycle_msgs.py | 24 ++ .../python/hiroz_msgs_py/types/nav_msgs.py | 24 ++ .../hiroz_msgs_py/types/rcl_interfaces.py | 88 ++++--- .../python/hiroz_msgs_py/types/sensor_msgs.py | 6 + .../types/type_description_interfaces.py | 6 + crates/hiroz-py/examples/action_demo.py | 6 +- crates/hiroz-py/examples/service_demo.py | 17 +- crates/hiroz-py/examples/topic_demo.py | 3 + crates/hiroz-py/python/hiroz_py/__init__.py | 51 ++++ crates/hiroz-py/python/hiroz_py/__init__.pyi | 57 ++++- crates/hiroz-py/src/action.rs | 25 +- crates/hiroz-py/src/error.rs | 23 ++ crates/hiroz-py/src/graph.rs | 29 +++ crates/hiroz-py/src/node.rs | 232 +++++++++++++++--- crates/hiroz-py/src/pubsub.rs | 19 ++ crates/hiroz-py/src/qos.rs | 18 +- crates/hiroz-py/src/service.rs | 176 +++++++++++-- crates/hiroz-py/src/traits.rs | 35 +++ crates/hiroz-py/tests/test_rclpy_alignment.py | 210 ++++++++++++++++ crates/hiroz-py/tests/test_service.py | 6 +- 23 files changed, 1018 insertions(+), 114 deletions(-) create mode 100644 crates/hiroz-py/tests/test_rclpy_alignment.py diff --git a/crates/hiroz-codegen/src/python_msgspec_generator.rs b/crates/hiroz-codegen/src/python_msgspec_generator.rs index 853de83ca..22e79c46d 100644 --- a/crates/hiroz-codegen/src/python_msgspec_generator.rs +++ b/crates/hiroz-codegen/src/python_msgspec_generator.rs @@ -8,6 +8,7 @@ use anyhow::Result; use proc_macro2::TokenStream; use quote::{format_ident, quote}; use std::collections::BTreeMap; +use std::collections::HashMap; use std::fs; use std::path::Path; @@ -30,6 +31,15 @@ pub fn generate_python_bindings( msgs.sort_by(|a, b| a.parsed.name.cmp(&b.parsed.name)); } + // Group services by package so we can emit rclpy-style grouping classes (P4). + let mut service_groups: HashMap> = HashMap::new(); + for srv in services { + service_groups + .entry(srv.parsed.package.clone()) + .or_default() + .push(srv); + } + // Group service Request/Response by package, and track service type hashes let mut service_messages: BTreeMap> = BTreeMap::new(); let mut service_hashes: BTreeMap> = BTreeMap::new(); @@ -75,11 +85,16 @@ pub fn generate_python_bindings( .get(package_name) .cloned() .unwrap_or_default(); + let srv_groups = service_groups + .get(package_name) + .map(|v| v.as_slice()) + .unwrap_or(&[]); let python_code = generate_python_package_with_services( package_name, package_msgs, srv_msgs, &svc_hashes, + srv_groups, )?; let output_path = python_output_dir.join(format!("{}.py", package_name)); fs::write(output_path, python_code)?; @@ -92,8 +107,17 @@ pub fn generate_python_bindings( .get(package_name) .cloned() .unwrap_or_default(); - let python_code = - generate_python_package_with_services(package_name, &[], srv_msgs, &svc_hashes)?; + let srv_groups = service_groups + .get(package_name) + .map(|v| v.as_slice()) + .unwrap_or(&[]); + let python_code = generate_python_package_with_services( + package_name, + &[], + srv_msgs, + &svc_hashes, + srv_groups, + )?; let output_path = python_output_dir.join(format!("{}.py", package_name)); fs::write(output_path, python_code)?; } @@ -123,6 +147,7 @@ fn generate_python_package_with_services( messages: &[&ResolvedMessage], service_messages: &[&ResolvedMessage], service_hashes: &BTreeMap, + service_groups: &[&ResolvedService], ) -> Result { let mut code = format!( "\"\"\"Auto-generated ROS 2 message types for {}.\"\"\"\n\ @@ -142,9 +167,33 @@ fn generate_python_package_with_services( code.push_str(&generate_msgspec_struct(msg, svc_hash.map(|s| s.as_str()))?); } + // Emit rclpy-style service grouping classes (P4). These reference the + // Request/Response structs above, so they must come after them. + for srv in service_groups { + code.push_str(&generate_service_grouping_class(srv)); + } + Ok(code) } +/// Generate a service grouping class: `AddTwoInts.Request` / `.Response` (P4). +/// +/// Lets `create_client`/`create_server` accept a single rclpy-style type +/// (`example_interfaces.AddTwoInts`) instead of the bare Request class. +fn generate_service_grouping_class(srv: &ResolvedService) -> String { + let srv_name = &srv.parsed.name; + let package = &srv.parsed.package; + let request_struct = &srv.request.parsed.name; + let response_struct = &srv.response.parsed.name; + format!( + "class {srv_name}:\n \ + \"\"\"Service grouping type. Use {srv_name}.Request and {srv_name}.Response.\"\"\"\n \ + __srvtype__: ClassVar[str] = '{package}/srv/{srv_name}'\n \ + Request: ClassVar[type] = {request_struct}\n \ + Response: ClassVar[type] = {response_struct}\n\n" + ) +} + fn rust_to_python_type(field_type: &FieldType, current_package: &str) -> Result { // Get the base field type (without array indicators) let base_type = &field_type.base_type; diff --git a/crates/hiroz-msgs/python/hiroz_msgs_py/types/action_msgs.py b/crates/hiroz-msgs/python/hiroz_msgs_py/types/action_msgs.py index 055b55e5e..14e724489 100644 --- a/crates/hiroz-msgs/python/hiroz_msgs_py/types/action_msgs.py +++ b/crates/hiroz-msgs/python/hiroz_msgs_py/types/action_msgs.py @@ -35,3 +35,9 @@ class CancelGoalResponse(msgspec.Struct, frozen=True, kw_only=True): __msgtype__: ClassVar[str] = 'action_msgs/msg/CancelGoalResponse' __hash__: ClassVar[str] = 'RIHS01_c66d49f351ea4375bf3eef8569e74b7afc19305d9fa94c71b412262e411f2a8f' +class CancelGoal: + """Service grouping type. Use CancelGoal.Request and CancelGoal.Response.""" + __srvtype__: ClassVar[str] = 'action_msgs/srv/CancelGoal' + Request: ClassVar[type] = CancelGoalRequest + Response: ClassVar[type] = CancelGoalResponse + diff --git a/crates/hiroz-msgs/python/hiroz_msgs_py/types/example_interfaces.py b/crates/hiroz-msgs/python/hiroz_msgs_py/types/example_interfaces.py index 5312a438d..d22b3418e 100644 --- a/crates/hiroz-msgs/python/hiroz_msgs_py/types/example_interfaces.py +++ b/crates/hiroz-msgs/python/hiroz_msgs_py/types/example_interfaces.py @@ -221,3 +221,21 @@ class TriggerResponse(msgspec.Struct, frozen=True, kw_only=True): __msgtype__: ClassVar[str] = 'example_interfaces/msg/TriggerResponse' __hash__: ClassVar[str] = 'RIHS01_cfeeee47f8105dd7685e4c92d46d4074669cb1c477402be1dea37486542a69e0' +class AddTwoInts: + """Service grouping type. Use AddTwoInts.Request and AddTwoInts.Response.""" + __srvtype__: ClassVar[str] = 'example_interfaces/srv/AddTwoInts' + Request: ClassVar[type] = AddTwoIntsRequest + Response: ClassVar[type] = AddTwoIntsResponse + +class SetBool: + """Service grouping type. Use SetBool.Request and SetBool.Response.""" + __srvtype__: ClassVar[str] = 'example_interfaces/srv/SetBool' + Request: ClassVar[type] = SetBoolRequest + Response: ClassVar[type] = SetBoolResponse + +class Trigger: + """Service grouping type. Use Trigger.Request and Trigger.Response.""" + __srvtype__: ClassVar[str] = 'example_interfaces/srv/Trigger' + Request: ClassVar[type] = TriggerRequest + Response: ClassVar[type] = TriggerResponse + diff --git a/crates/hiroz-msgs/python/hiroz_msgs_py/types/lifecycle_msgs.py b/crates/hiroz-msgs/python/hiroz_msgs_py/types/lifecycle_msgs.py index f32b96bf9..cdb701100 100644 --- a/crates/hiroz-msgs/python/hiroz_msgs_py/types/lifecycle_msgs.py +++ b/crates/hiroz-msgs/python/hiroz_msgs_py/types/lifecycle_msgs.py @@ -78,3 +78,27 @@ class GetStateResponse(msgspec.Struct, frozen=True, kw_only=True): __msgtype__: ClassVar[str] = 'lifecycle_msgs/msg/GetStateResponse' __hash__: ClassVar[str] = 'RIHS01_800a0a5aae599782b02932de0caf563f6dc4e7e94b794eadde075ba2cbef9795' +class ChangeState: + """Service grouping type. Use ChangeState.Request and ChangeState.Response.""" + __srvtype__: ClassVar[str] = 'lifecycle_msgs/srv/ChangeState' + Request: ClassVar[type] = ChangeStateRequest + Response: ClassVar[type] = ChangeStateResponse + +class GetAvailableStates: + """Service grouping type. Use GetAvailableStates.Request and GetAvailableStates.Response.""" + __srvtype__: ClassVar[str] = 'lifecycle_msgs/srv/GetAvailableStates' + Request: ClassVar[type] = GetAvailableStatesRequest + Response: ClassVar[type] = GetAvailableStatesResponse + +class GetAvailableTransitions: + """Service grouping type. Use GetAvailableTransitions.Request and GetAvailableTransitions.Response.""" + __srvtype__: ClassVar[str] = 'lifecycle_msgs/srv/GetAvailableTransitions' + Request: ClassVar[type] = GetAvailableTransitionsRequest + Response: ClassVar[type] = GetAvailableTransitionsResponse + +class GetState: + """Service grouping type. Use GetState.Request and GetState.Response.""" + __srvtype__: ClassVar[str] = 'lifecycle_msgs/srv/GetState' + Request: ClassVar[type] = GetStateRequest + Response: ClassVar[type] = GetStateResponse + diff --git a/crates/hiroz-msgs/python/hiroz_msgs_py/types/nav_msgs.py b/crates/hiroz-msgs/python/hiroz_msgs_py/types/nav_msgs.py index 2d7959e37..09f50b8c5 100644 --- a/crates/hiroz-msgs/python/hiroz_msgs_py/types/nav_msgs.py +++ b/crates/hiroz-msgs/python/hiroz_msgs_py/types/nav_msgs.py @@ -103,3 +103,27 @@ class SetMapResponse(msgspec.Struct, frozen=True, kw_only=True): __msgtype__: ClassVar[str] = 'nav_msgs/msg/SetMapResponse' __hash__: ClassVar[str] = 'RIHS01_5e11a5b2ca53d8ae85b666a019f16c9904ebc787828f1f566c4e048a1ddedfb4' +class GetMap: + """Service grouping type. Use GetMap.Request and GetMap.Response.""" + __srvtype__: ClassVar[str] = 'nav_msgs/srv/GetMap' + Request: ClassVar[type] = GetMapRequest + Response: ClassVar[type] = GetMapResponse + +class GetPlan: + """Service grouping type. Use GetPlan.Request and GetPlan.Response.""" + __srvtype__: ClassVar[str] = 'nav_msgs/srv/GetPlan' + Request: ClassVar[type] = GetPlanRequest + Response: ClassVar[type] = GetPlanResponse + +class LoadMap: + """Service grouping type. Use LoadMap.Request and LoadMap.Response.""" + __srvtype__: ClassVar[str] = 'nav_msgs/srv/LoadMap' + Request: ClassVar[type] = LoadMapRequest + Response: ClassVar[type] = LoadMapResponse + +class SetMap: + """Service grouping type. Use SetMap.Request and SetMap.Response.""" + __srvtype__: ClassVar[str] = 'nav_msgs/srv/SetMap' + Request: ClassVar[type] = SetMapRequest + Response: ClassVar[type] = SetMapResponse + diff --git a/crates/hiroz-msgs/python/hiroz_msgs_py/types/rcl_interfaces.py b/crates/hiroz-msgs/python/hiroz_msgs_py/types/rcl_interfaces.py index 9e06d274e..96272e5d5 100644 --- a/crates/hiroz-msgs/python/hiroz_msgs_py/types/rcl_interfaces.py +++ b/crates/hiroz-msgs/python/hiroz_msgs_py/types/rcl_interfaces.py @@ -25,13 +25,6 @@ class ListParametersResult(msgspec.Struct, frozen=True, kw_only=True): __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/ListParametersResult' __hash__: ClassVar[str] = 'RIHS01_237ae3428413dcbcfb452b510c42355f3a2b021dc091afa3e18526d57022f1cd' -class LoggerLevel(msgspec.Struct, frozen=True, kw_only=True): - name: str = "" - level: int = 0 - - __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/LoggerLevel' - __hash__: ClassVar[str] = 'RIHS01_95785cc42f048ab4f395af65035aeaf2181d8e1c7a44edb8ad4558445fdb43c0' - class Parameter(msgspec.Struct, frozen=True, kw_only=True): name: str = "" value: "rcl_interfaces.ParameterValue | None" = None @@ -90,13 +83,6 @@ class ParameterValue(msgspec.Struct, frozen=True, kw_only=True): __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/ParameterValue' __hash__: ClassVar[str] = 'RIHS01_115fc089a387e23c7ecd3525c9189c379109119d6ab82e8dfbde0fdf6a7f9b68' -class SetLoggerLevelsResult(msgspec.Struct, frozen=True, kw_only=True): - successful: bool = False - reason: str = "" - - __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/SetLoggerLevelsResult' - __hash__: ClassVar[str] = 'RIHS01_9316e5e679a5b72d2dd7fd80c539bae9e106fa0890a06dc5da3a8177a3ff6909' - class SetParametersResult(msgspec.Struct, frozen=True, kw_only=True): successful: bool = False reason: str = "" @@ -116,18 +102,6 @@ class DescribeParametersResponse(msgspec.Struct, frozen=True, kw_only=True): __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/DescribeParametersResponse' __hash__: ClassVar[str] = 'RIHS01_845b484d71eb0673dae682f2e3ba3c4851a65a3dcfb97bddd82c5b57e91e4cff' -class GetLoggerLevelsRequest(msgspec.Struct, frozen=True, kw_only=True): - names: list[str] = msgspec.field(default_factory=list) - - __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/GetLoggerLevelsRequest' - __hash__: ClassVar[str] = 'RIHS01_03bf1bebd0d6514c7ed0ba7c5e08dc9f2f39c759fe99e1e30ea4157d7674f72d' - -class GetLoggerLevelsResponse(msgspec.Struct, frozen=True, kw_only=True): - levels: list["rcl_interfaces.LoggerLevel"] = msgspec.field(default_factory=list) - - __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/GetLoggerLevelsResponse' - __hash__: ClassVar[str] = 'RIHS01_03bf1bebd0d6514c7ed0ba7c5e08dc9f2f39c759fe99e1e30ea4157d7674f72d' - class GetParameterTypesRequest(msgspec.Struct, frozen=True, kw_only=True): names: list[str] = msgspec.field(default_factory=list) @@ -165,17 +139,17 @@ class ListParametersResponse(msgspec.Struct, frozen=True, kw_only=True): __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/ListParametersResponse' __hash__: ClassVar[str] = 'RIHS01_3e6062bfbb27bfb8730d4cef2558221f51a11646d78e7bb30a1e83afac3aad9d' -class SetLoggerLevelsRequest(msgspec.Struct, frozen=True, kw_only=True): - levels: list["rcl_interfaces.LoggerLevel"] = msgspec.field(default_factory=list) +class SetParametersRequest(msgspec.Struct, frozen=True, kw_only=True): + parameters: list["rcl_interfaces.Parameter"] = msgspec.field(default_factory=list) - __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/SetLoggerLevelsRequest' - __hash__: ClassVar[str] = 'RIHS01_3ff86cb4e91fbf9abae15c234ecc874448de6ece8e193401c077cf116e4f6d78' + __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/SetParametersRequest' + __hash__: ClassVar[str] = 'RIHS01_56eed9a67e169f9cb6c1f987bc88f868c14a8fc9f743a263bc734c154015d7e0' -class SetLoggerLevelsResponse(msgspec.Struct, frozen=True, kw_only=True): - results: list["rcl_interfaces.SetLoggerLevelsResult"] = msgspec.field(default_factory=list) +class SetParametersResponse(msgspec.Struct, frozen=True, kw_only=True): + results: list["rcl_interfaces.SetParametersResult"] = msgspec.field(default_factory=list) - __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/SetLoggerLevelsResponse' - __hash__: ClassVar[str] = 'RIHS01_3ff86cb4e91fbf9abae15c234ecc874448de6ece8e193401c077cf116e4f6d78' + __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/SetParametersResponse' + __hash__: ClassVar[str] = 'RIHS01_56eed9a67e169f9cb6c1f987bc88f868c14a8fc9f743a263bc734c154015d7e0' class SetParametersAtomicallyRequest(msgspec.Struct, frozen=True, kw_only=True): parameters: list["rcl_interfaces.Parameter"] = msgspec.field(default_factory=list) @@ -189,15 +163,39 @@ class SetParametersAtomicallyResponse(msgspec.Struct, frozen=True, kw_only=True) __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/SetParametersAtomicallyResponse' __hash__: ClassVar[str] = 'RIHS01_0e192ef259c07fc3c07a13191d27002222e65e00ccec653ca05e856f79285fcd' -class SetParametersRequest(msgspec.Struct, frozen=True, kw_only=True): - parameters: list["rcl_interfaces.Parameter"] = msgspec.field(default_factory=list) - - __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/SetParametersRequest' - __hash__: ClassVar[str] = 'RIHS01_56eed9a67e169f9cb6c1f987bc88f868c14a8fc9f743a263bc734c154015d7e0' - -class SetParametersResponse(msgspec.Struct, frozen=True, kw_only=True): - results: list["rcl_interfaces.SetParametersResult"] = msgspec.field(default_factory=list) - - __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/SetParametersResponse' - __hash__: ClassVar[str] = 'RIHS01_56eed9a67e169f9cb6c1f987bc88f868c14a8fc9f743a263bc734c154015d7e0' +class DescribeParameters: + """Service grouping type. Use DescribeParameters.Request and DescribeParameters.Response.""" + __srvtype__: ClassVar[str] = 'rcl_interfaces/srv/DescribeParameters' + Request: ClassVar[type] = DescribeParametersRequest + Response: ClassVar[type] = DescribeParametersResponse + +class GetParameterTypes: + """Service grouping type. Use GetParameterTypes.Request and GetParameterTypes.Response.""" + __srvtype__: ClassVar[str] = 'rcl_interfaces/srv/GetParameterTypes' + Request: ClassVar[type] = GetParameterTypesRequest + Response: ClassVar[type] = GetParameterTypesResponse + +class GetParameters: + """Service grouping type. Use GetParameters.Request and GetParameters.Response.""" + __srvtype__: ClassVar[str] = 'rcl_interfaces/srv/GetParameters' + Request: ClassVar[type] = GetParametersRequest + Response: ClassVar[type] = GetParametersResponse + +class ListParameters: + """Service grouping type. Use ListParameters.Request and ListParameters.Response.""" + __srvtype__: ClassVar[str] = 'rcl_interfaces/srv/ListParameters' + Request: ClassVar[type] = ListParametersRequest + Response: ClassVar[type] = ListParametersResponse + +class SetParameters: + """Service grouping type. Use SetParameters.Request and SetParameters.Response.""" + __srvtype__: ClassVar[str] = 'rcl_interfaces/srv/SetParameters' + Request: ClassVar[type] = SetParametersRequest + Response: ClassVar[type] = SetParametersResponse + +class SetParametersAtomically: + """Service grouping type. Use SetParametersAtomically.Request and SetParametersAtomically.Response.""" + __srvtype__: ClassVar[str] = 'rcl_interfaces/srv/SetParametersAtomically' + Request: ClassVar[type] = SetParametersAtomicallyRequest + Response: ClassVar[type] = SetParametersAtomicallyResponse diff --git a/crates/hiroz-msgs/python/hiroz_msgs_py/types/sensor_msgs.py b/crates/hiroz-msgs/python/hiroz_msgs_py/types/sensor_msgs.py index 45bf802ba..a7f7de8e7 100644 --- a/crates/hiroz-msgs/python/hiroz_msgs_py/types/sensor_msgs.py +++ b/crates/hiroz-msgs/python/hiroz_msgs_py/types/sensor_msgs.py @@ -289,3 +289,9 @@ class SetCameraInfoResponse(msgspec.Struct, frozen=True, kw_only=True): __msgtype__: ClassVar[str] = 'sensor_msgs/msg/SetCameraInfoResponse' __hash__: ClassVar[str] = 'RIHS01_a10cca5d33dc637c8d49db50ab288701a3592bb9cd854f2f16a0659613b68984' +class SetCameraInfo: + """Service grouping type. Use SetCameraInfo.Request and SetCameraInfo.Response.""" + __srvtype__: ClassVar[str] = 'sensor_msgs/srv/SetCameraInfo' + Request: ClassVar[type] = SetCameraInfoRequest + Response: ClassVar[type] = SetCameraInfoResponse + diff --git a/crates/hiroz-msgs/python/hiroz_msgs_py/types/type_description_interfaces.py b/crates/hiroz-msgs/python/hiroz_msgs_py/types/type_description_interfaces.py index f343b4145..461102457 100644 --- a/crates/hiroz-msgs/python/hiroz_msgs_py/types/type_description_interfaces.py +++ b/crates/hiroz-msgs/python/hiroz_msgs_py/types/type_description_interfaces.py @@ -66,3 +66,9 @@ class GetTypeDescriptionResponse(msgspec.Struct, frozen=True, kw_only=True): __msgtype__: ClassVar[str] = 'type_description_interfaces/msg/GetTypeDescriptionResponse' __hash__: ClassVar[str] = 'RIHS01_69b9c19c1021405984cc60dbbb1edceb147a6538b411d812ba6afabeed962cd5' +class GetTypeDescription: + """Service grouping type. Use GetTypeDescription.Request and GetTypeDescription.Response.""" + __srvtype__: ClassVar[str] = 'type_description_interfaces/srv/GetTypeDescription' + Request: ClassVar[type] = GetTypeDescriptionRequest + Response: ClassVar[type] = GetTypeDescriptionResponse + diff --git a/crates/hiroz-py/examples/action_demo.py b/crates/hiroz-py/examples/action_demo.py index b6237f671..da5939a44 100644 --- a/crates/hiroz-py/examples/action_demo.py +++ b/crates/hiroz-py/examples/action_demo.py @@ -101,8 +101,10 @@ def run_client(ctx, action: str, target: int, cancel_after: float | None): action, CountToGoal, CountToResult, CountToFeedback ) - # Give server time to advertise - time.sleep(1.0) + # Wait for the action server instead of sleeping (P1). + if not client.wait_for_server(timeout=5.0): + print("CLIENT:ERROR:server unavailable", flush=True) + sys.exit(1) print(f"CLIENT:SEND_GOAL:{target}", flush=True) handle = client.send_goal(CountToGoal(target=target)) diff --git a/crates/hiroz-py/examples/service_demo.py b/crates/hiroz-py/examples/service_demo.py index 52da5a175..00f9fa4d1 100644 --- a/crates/hiroz-py/examples/service_demo.py +++ b/crates/hiroz-py/examples/service_demo.py @@ -10,7 +10,6 @@ import argparse import sys -import time import hiroz_py from hiroz_py import example_interfaces @@ -20,7 +19,8 @@ def run_server(ctx, service: str, max_requests: int): """Run the AddTwoInts service server.""" node = ctx.create_node("add_two_ints_server").build() - server = node.create_server(service, example_interfaces.AddTwoIntsRequest) + # rclpy-style service grouping type (AddTwoInts.Request / .Response). + server = node.create_server(service, example_interfaces.AddTwoInts) print("SERVER:READY", flush=True) @@ -44,18 +44,21 @@ def run_server(ctx, service: str, max_requests: int): def run_client(ctx, service: str, a: int, b: int, timeout: float): """Run the AddTwoInts service client.""" node = ctx.create_node("add_two_ints_client").build() - client = node.create_client(service, example_interfaces.AddTwoIntsRequest) + client = node.create_client(service, example_interfaces.AddTwoInts) - # Wait for service discovery - time.sleep(1.0) + # Wait for the server to appear instead of sleeping (P1). + if not client.wait_for_service(timeout=5.0): + print("CLIENT:ERROR:service unavailable", flush=True) + sys.exit(1) print(f"CLIENT:REQUEST:{a}+{b}", flush=True) - req = example_interfaces.AddTwoIntsRequest(a=a, b=b) + req = example_interfaces.AddTwoInts.Request(a=a, b=b) try: resp = client.call(req, timeout=timeout) print(f"CLIENT:RESPONSE:{resp.sum}", flush=True) - except RuntimeError as e: + except hiroz_py.HirozError as e: + # TimeoutError is a subclass of HirozError; catch the base to cover both. print(f"CLIENT:ERROR:{e}", flush=True) sys.exit(1) diff --git a/crates/hiroz-py/examples/topic_demo.py b/crates/hiroz-py/examples/topic_demo.py index aa127a414..6b4d0fd85 100644 --- a/crates/hiroz-py/examples/topic_demo.py +++ b/crates/hiroz-py/examples/topic_demo.py @@ -24,6 +24,9 @@ def run_talker(ctx, topic: str, count: int, interval: float): print(f"Talker started. Publishing to {topic}...") + # Wait for at least one subscriber instead of racing (P1). + pub.wait_for_subscription(count=1, timeout=5.0) + i = 0 while count == 0 or i < count: message = f"Hello from Python {i}" diff --git a/crates/hiroz-py/python/hiroz_py/__init__.py b/crates/hiroz-py/python/hiroz_py/__init__.py index 84fe7d2b9..3c0519071 100644 --- a/crates/hiroz-py/python/hiroz_py/__init__.py +++ b/crates/hiroz-py/python/hiroz_py/__init__.py @@ -33,3 +33,54 @@ QOS_SENSOR_DATA: Final[QosProfile] = QosProfile.sensor_data() QOS_PARAMETERS: Final[QosProfile] = QosProfile.parameters() QOS_SERVICES: Final[QosProfile] = QosProfile.services() + + +# --------------------------------------------------------------------------- +# rclpy-style method aliases (P3) +# +# hiroz keeps its native names (create_subscriber / create_server) as the +# canonical API; these aliases let rclpy code read naturally. create_service +# is a true alias because create_server now supports rclpy's optional +# callback= form (P6) in addition to pull mode. +# --------------------------------------------------------------------------- + +ZNode.create_subscription = ZNode.create_subscriber # type: ignore[attr-defined] +ZNode.create_service = ZNode.create_server # type: ignore[attr-defined] + + +# --------------------------------------------------------------------------- +# QoS policy enum holders (P8) +# +# String-valued so they parse straight through QosProfile, while giving users +# discoverable, typo-proof constants instead of bare strings. Mirrors rclpy's +# rclpy.qos.ReliabilityPolicy / DurabilityPolicy / HistoryPolicy / LivelinessPolicy. +# --------------------------------------------------------------------------- + + +class ReliabilityPolicy: + """QoS reliability policy constants.""" + + RELIABLE: Final[str] = "reliable" + BEST_EFFORT: Final[str] = "best_effort" + + +class DurabilityPolicy: + """QoS durability policy constants.""" + + VOLATILE: Final[str] = "volatile" + TRANSIENT_LOCAL: Final[str] = "transient_local" + + +class HistoryPolicy: + """QoS history policy constants.""" + + KEEP_LAST: Final[str] = "keep_last" + KEEP_ALL: Final[str] = "keep_all" + + +class LivelinessPolicy: + """QoS liveliness policy constants.""" + + AUTOMATIC: Final[str] = "automatic" + MANUAL_BY_TOPIC: Final[str] = "manual_by_topic" + MANUAL_BY_NODE: Final[str] = "manual_by_node" diff --git a/crates/hiroz-py/python/hiroz_py/__init__.pyi b/crates/hiroz-py/python/hiroz_py/__init__.pyi index a355850e5..52eb280e2 100644 --- a/crates/hiroz-py/python/hiroz_py/__init__.pyi +++ b/crates/hiroz-py/python/hiroz_py/__init__.pyi @@ -79,6 +79,30 @@ QOS_SENSOR_DATA: Final[QosProfile] = QosProfile.sensor_data() QOS_PARAMETERS: Final[QosProfile] = QosProfile.parameters() QOS_SERVICES: Final[QosProfile] = QosProfile.services() +# --------------------------------------------------------------------------- +# QoS policy enum holders (P8) +# --------------------------------------------------------------------------- + +class ReliabilityPolicy: + RELIABLE: Final[str] + BEST_EFFORT: Final[str] + +class DurabilityPolicy: + VOLATILE: Final[str] + TRANSIENT_LOCAL: Final[str] + +class HistoryPolicy: + KEEP_LAST: Final[str] + KEEP_ALL: Final[str] + +class LivelinessPolicy: + AUTOMATIC: Final[str] + MANUAL_BY_TOPIC: Final[str] + MANUAL_BY_NODE: Final[str] + +# A QoS argument: a QosProfile, an int depth shorthand, or a legacy dict. +QosLike = QosProfile | int | dict[str, object] + # --------------------------------------------------------------------------- # GoalStatus # --------------------------------------------------------------------------- @@ -175,30 +199,44 @@ class ZNode: self, topic: str, msg_type: Any, - qos: QosProfile | dict[str, object] | None = None, + qos: QosLike | None = None, ) -> ZPublisher: ... def create_subscriber( self, topic: str, msg_type: Any, - qos: QosProfile | dict[str, object] | None = None, + qos: QosLike | None = None, + callback: Any | None = None, + ) -> ZSubscriber: ... + # rclpy-style alias for create_subscriber (P3). + def create_subscription( + self, + topic: str, + msg_type: Any, + qos: QosLike | None = None, callback: Any | None = None, ) -> ZSubscriber: ... def create_client(self, service: str, srv_type: Any) -> ZClient: ... - def create_server(self, service: str, srv_type: Any) -> ZServer: ... + def create_server( + self, service: str, srv_type: Any, callback: Any | None = None + ) -> ZServer: ... + # rclpy-style alias for create_server (P3); pass callback= for callback mode (P6). + def create_service( + self, service: str, srv_type: Any, callback: Any | None = None + ) -> ZServer: ... def create_action_client( self, action_name: str, goal_type: Any, - result_type: Any, - feedback_type: Any, + result_type: Any | None = None, + feedback_type: Any | None = None, ) -> ZActionClient: ... def create_action_server( self, action_name: str, goal_type: Any, - result_type: Any, - feedback_type: Any, + result_type: Any | None = None, + feedback_type: Any | None = None, ) -> ZActionServer: ... def get_topic_names_and_types(self) -> list[tuple[str, str]]: ... def get_node_names(self) -> list[tuple[str, str]]: ... @@ -213,6 +251,9 @@ class ZNode: class ZPublisher: def publish(self, data: Any) -> None: ... def publish_raw(self, data: bytes) -> None: ... + def wait_for_subscription( + self, count: int = 1, timeout: float | None = None + ) -> bool: ... def get_type_name(self) -> str: ... # --------------------------------------------------------------------------- @@ -236,6 +277,7 @@ class ZSubscriber: class ZClient: def call(self, data: Any, timeout: float | None = None) -> Any: ... + def wait_for_service(self, timeout: float | None = None) -> bool: ... def get_type_name(self) -> str: ... # --------------------------------------------------------------------------- @@ -253,6 +295,7 @@ class ZServer: class ZActionClient: def send_goal(self, goal: Any) -> ActionGoalHandle: ... + def wait_for_server(self, timeout: float | None = None) -> bool: ... @property def goal_type(self) -> Any: ... diff --git a/crates/hiroz-py/src/action.rs b/crates/hiroz-py/src/action.rs index b23130817..2c3bf202e 100644 --- a/crates/hiroz-py/src/action.rs +++ b/crates/hiroz-py/src/action.rs @@ -139,20 +139,28 @@ pub struct PyZActionClient { goal_type: Py, result_type: Py, feedback_type: Py, + /// Shared graph + the action's `send_goal` service name, used by `wait_for_server`. + graph: Arc, + send_goal_service: String, } impl PyZActionClient { + #[allow(clippy::too_many_arguments)] pub fn new( inner: RawActionClient, goal_type: Py, result_type: Py, feedback_type: Py, + graph: Arc, + send_goal_service: String, ) -> Self { Self { inner: Arc::new(inner), goal_type, result_type, feedback_type, + graph, + send_goal_service, } } } @@ -180,7 +188,7 @@ impl PyZActionClient { tokio::time::timeout(Duration::from_secs(11), client.send_goal(goal_msg)) .await .map_err(|_| { - pyo3::exceptions::PyRuntimeError::new_err( + crate::error::TimeoutError::new_err( "send_goal timed out: no action server responded", ) })? @@ -243,6 +251,21 @@ impl PyZActionClient { }) } + /// Wait until an action server for this action is available. + /// + /// Mirrors rclpy's `ActionClient.wait_for_server(timeout_sec)`. Polls the + /// discovery graph for the action's `send_goal` service. Returns True if a + /// server was found before `timeout`, False otherwise. + /// + /// Args: + /// timeout: Maximum seconds to wait. None waits forever. + #[pyo3(signature = (timeout=None))] + fn wait_for_server(&self, py: Python, timeout: Option) -> bool { + py.allow_threads(|| { + crate::graph::wait_for_service_server(&self.graph, &self.send_goal_service, timeout) + }) + } + /// Get the goal type class (for debugging). #[getter] fn goal_type(&self, py: Python) -> PyObject { diff --git a/crates/hiroz-py/src/error.rs b/crates/hiroz-py/src/error.rs index 5d8d1cd35..9203bb556 100644 --- a/crates/hiroz-py/src/error.rs +++ b/crates/hiroz-py/src/error.rs @@ -8,6 +8,29 @@ pyo3::create_exception!(hiroz_py, TimeoutError, HirozError); pyo3::create_exception!(hiroz_py, SerializationError, HirozError); pyo3::create_exception!(hiroz_py, TypeMismatchError, HirozError); +/// Returns true if `e` is (or wraps) a core [`hiroz::error::Error::Timeout`]. +/// +/// Centralized here so every call site (service `call`, action `send_goal`, …) +/// classifies identically. Delegates to the core's structured detector, which +/// walks the whole source chain — do not string-match on the message. +pub(crate) fn is_timeout_error(e: &anyhow::Error) -> bool { + hiroz::error::is_timeout(&**e) +} + +/// Map a core error to the right Python exception. +/// +/// Timeout-shaped errors become `hiroz_py.TimeoutError`; everything else +/// becomes `hiroz_py.HirozError`. Use this for blocking calls that raise on +/// failure (e.g. `ZClient.call`). Methods whose documented contract is to +/// return `None` on timeout should keep doing so rather than calling this. +pub(crate) fn map_call_error(e: anyhow::Error) -> PyErr { + if is_timeout_error(&e) { + TimeoutError::new_err(format!("{:#}", e)) + } else { + HirozError::new_err(format!("{:#}", e)) + } +} + /// Trait for converting Rust errors to Python exceptions pub(crate) trait IntoPyErr { fn into_pyerr(self) -> PyErr; diff --git a/crates/hiroz-py/src/graph.rs b/crates/hiroz-py/src/graph.rs index 62fcd011d..52069b473 100644 --- a/crates/hiroz-py/src/graph.rs +++ b/crates/hiroz-py/src/graph.rs @@ -3,6 +3,35 @@ use hiroz::entity::EndpointKind; use hiroz::graph::Graph; use std::sync::Arc; +use std::time::{Duration, Instant}; + +/// Poll interval for discovery waits. Matches the ~50ms cadence rclpy uses +/// internally for its wait-for-service spin. +const POLL_INTERVAL: Duration = Duration::from_millis(50); + +/// Block until at least one service server matching `service_name` is visible +/// in the graph, or `timeout` (seconds) elapses. `None` waits forever. +/// +/// Must be called with the GIL released (`py.allow_threads`) so it does not +/// stall other Python threads while sleeping. Returns true if a server appeared. +pub(crate) fn wait_for_service_server( + graph: &Arc, + service_name: &str, + timeout: Option, +) -> bool { + let deadline = timeout.map(|t| Instant::now() + Duration::from_secs_f64(t)); + loop { + if graph.count(EndpointKind::Service, service_name) > 0 { + return true; + } + if let Some(d) = deadline + && Instant::now() >= d + { + return false; + } + std::thread::sleep(POLL_INTERVAL); + } +} /// Python-accessible graph discovery methods. /// diff --git a/crates/hiroz-py/src/node.rs b/crates/hiroz-py/src/node.rs index 6e43eb4dc..ea802866d 100644 --- a/crates/hiroz-py/src/node.rs +++ b/crates/hiroz-py/src/node.rs @@ -113,6 +113,108 @@ fn extract_service_type_from_request_class( Ok((srv_type, type_info)) } +/// Extract service type info from either a service grouping class (P4, rclpy-style) +/// or a bare Request class (back-compat). +/// +/// A grouping class exposes `__srvtype__` (e.g. `"example_interfaces/srv/AddTwoInts"`) +/// plus `Request` / `Response` member classes. We read the type hash from the +/// `Request` member. Anything without `__srvtype__` falls through to the legacy +/// string-munging path on the Request class itself. +fn extract_service_type_info(srv_type: &Bound<'_, PyAny>) -> PyResult<(String, TypeInfo)> { + if let Ok(srvtype_attr) = srv_type.getattr("__srvtype__") + && let Ok(srv_type_str) = srvtype_attr.extract::() + { + // Grouping class: pull the type hash from the Request member. + let request_cls = srv_type.getattr("Request").map_err(|_| { + pyo3::exceptions::PyTypeError::new_err( + "Service grouping class with __srvtype__ must define a Request member", + ) + })?; + let type_hash = request_cls + .getattr("__hash__") + .ok() + .and_then(|v| v.extract::().ok()) + .and_then(|s| TypeHash::from_rihs_string(&s)) + .unwrap_or_else(TypeHash::zero); + let rust_type_name = python_type_to_rust_type(&srv_type_str); + return Ok((srv_type_str, TypeInfo::new(&rust_type_name, type_hash))); + } + // Back-compat: bare Request class. + extract_service_type_from_request_class(srv_type) +} + +/// If `topic` is not a string but `msg_type` is, the caller almost certainly used +/// the rclpy positional order `(msg_type, topic)`. Raise a self-explaining error +/// instead of a confusing downstream type failure (P2). +fn reject_swapped_args( + topic: &Bound<'_, PyAny>, + msg_type: &Bound<'_, PyAny>, + func: &str, +) -> PyResult<()> { + let topic_is_str = topic.is_instance_of::(); + let msg_is_str = msg_type.is_instance_of::(); + if !topic_is_str && msg_is_str { + return Err(pyo3::exceptions::PyTypeError::new_err(format!( + "arguments look swapped — hiroz uses ({func}(topic, msg_type, ...)) but rclpy uses \ + (msg_type, topic, ...). Pass by keyword: {func}(topic=..., msg_type=...)" + ))); + } + Ok(()) +} + +/// Resolve a topic argument to a `String`, with a clear error if it isn't a str. +fn extract_topic(topic: &Bound<'_, PyAny>) -> PyResult { + topic.extract::().map_err(|_| { + pyo3::exceptions::PyTypeError::new_err( + "topic must be a string (e.g. \"/chatter\"). Pass by keyword if unsure: topic=...", + ) + }) +} + +/// Extract Goal/Result/Feedback classes from either an action grouping class +/// (P7, rclpy-style — exposes `__actiontype__`, `Goal`, `Result`, `Feedback`) +/// or fall back to three explicitly-passed classes. +/// +/// Returns the three member classes as owned `PyObject`s. +fn resolve_action_types( + action_type: &Bound<'_, PyAny>, + result_type: Option<&Bound<'_, PyAny>>, + feedback_type: Option<&Bound<'_, PyAny>>, +) -> PyResult<(PyObject, PyObject, PyObject)> { + // Grouping class path: a single action type with member classes. + if action_type.hasattr("__actiontype__").unwrap_or(false) { + let goal = action_type.getattr("Goal").map_err(|_| { + pyo3::exceptions::PyTypeError::new_err( + "Action grouping class with __actiontype__ must define a Goal member", + ) + })?; + let result = action_type.getattr("Result").map_err(|_| { + pyo3::exceptions::PyTypeError::new_err( + "Action grouping class with __actiontype__ must define a Result member", + ) + })?; + let feedback = action_type.getattr("Feedback").map_err(|_| { + pyo3::exceptions::PyTypeError::new_err( + "Action grouping class with __actiontype__ must define a Feedback member", + ) + })?; + return Ok((goal.unbind(), result.unbind(), feedback.unbind())); + } + + // Back-compat: three separate classes. + let (Some(result), Some(feedback)) = (result_type, feedback_type) else { + return Err(pyo3::exceptions::PyTypeError::new_err( + "create_action_*: pass either a single action grouping class (with __actiontype__) \ + or all three of goal_type, result_type, feedback_type", + )); + }; + Ok(( + action_type.clone().unbind(), + result.clone().unbind(), + feedback.clone().unbind(), + )) +} + #[pyclass(name = "ZNodeBuilder")] pub struct PyZNodeBuilder { pub(crate) ctx: Arc, @@ -189,10 +291,12 @@ impl PyZNode { #[pyo3(signature = (topic, msg_type, qos=None))] fn create_publisher( &self, - topic: String, + topic: &Bound<'_, PyAny>, msg_type: &Bound<'_, PyAny>, qos: Option<&Bound<'_, PyAny>>, ) -> PyResult { + reject_swapped_args(topic, msg_type, "create_publisher")?; + let topic = extract_topic(topic)?; let (msg_type_str, type_info) = extract_type_info_from_class(msg_type)?; let qos_profile = extract_qos(qos)?; @@ -213,11 +317,13 @@ impl PyZNode { fn create_subscriber( &mut self, _py: Python, - topic: String, + topic: &Bound<'_, PyAny>, msg_type: &Bound<'_, PyAny>, qos: Option<&Bound<'_, PyAny>>, callback: Option, ) -> PyResult { + reject_swapped_args(topic, msg_type, "create_subscriber")?; + let topic = extract_topic(topic)?; let (msg_type_str, type_info) = extract_type_info_from_class(msg_type)?; let qos_profile = extract_qos(qos)?; @@ -262,16 +368,25 @@ impl PyZNode { } } - /// Create a service client + /// Create a service client. + /// + /// `srv_type` may be a service grouping class (rclpy-style, e.g. + /// `example_interfaces.AddTwoInts`) or the bare Request class (back-compat). fn create_client(&self, service: String, srv_type: &Bound<'_, PyAny>) -> PyResult { - let (srv_type_str, type_info) = extract_service_type_from_request_class(srv_type)?; + let (srv_type_str, type_info) = extract_service_type_info(srv_type)?; let client_builder = self .inner .create_client_impl::(&service, Some(type_info)); let zclient = client_builder.build().map_err(|e| e.into_pyerr())?; let wrapper = GenericClientWrapper::new(zclient); - Ok(PyZClient::new(Box::new(wrapper), srv_type_str)) + let qualified = self.qualify_service_name(&service); + Ok(PyZClient::new( + Box::new(wrapper), + srv_type_str, + Arc::clone(self.inner.graph()), + qualified, + )) } // -- Graph discovery methods -- @@ -310,22 +425,29 @@ impl PyZNode { /// `__msgtype__` and `__hash__` attributes (from `hiroz_msgs_py`). /// /// Returns a `ZActionClient` for sending goals and receiving results. + #[pyo3(signature = (action_name, goal_type, result_type=None, feedback_type=None))] fn create_action_client( &self, py: Python, action_name: String, goal_type: &Bound<'_, PyAny>, - result_type: &Bound<'_, PyAny>, - feedback_type: &Bound<'_, PyAny>, + result_type: Option<&Bound<'_, PyAny>>, + feedback_type: Option<&Bound<'_, PyAny>>, ) -> PyResult { + let (goal_obj, result_obj, feedback_obj) = + resolve_action_types(goal_type, result_type, feedback_type)?; + let goal_b = goal_obj.bind(py); + let result_b = result_obj.bind(py); + let feedback_b = feedback_obj.bind(py); + // __msgtype__ is still required (validates the class); __hash__ is optional. - extract_type_info_from_class(goal_type)?; - extract_type_info_from_class(result_type)?; - extract_type_info_from_class(feedback_type)?; + extract_type_info_from_class(goal_b)?; + extract_type_info_from_class(result_b)?; + extract_type_info_from_class(feedback_b)?; - let goal_ti = try_extract_type_info(goal_type); - let result_ti = try_extract_type_info(result_type); - let feedback_ti = try_extract_type_info(feedback_type); + let goal_ti = try_extract_type_info(goal_b); + let result_ti = try_extract_type_info(result_b); + let feedback_ti = try_extract_type_info(feedback_b); let node = Arc::clone(&self.inner); let rt = get_tokio_rt(); @@ -347,11 +469,20 @@ impl PyZNode { .map_err(|e| pyo3::exceptions::PyRuntimeError::new_err(e.to_string())) })?; + // The action server advertises a `/_action/send_goal` service; + // wait_for_server polls the graph for it. + let send_goal_service = format!( + "{}/_action/send_goal", + self.qualify_service_name(&action_name) + ); + Ok(PyZActionClient::new( client, - goal_type.clone().unbind(), - result_type.clone().unbind(), - feedback_type.clone().unbind(), + goal_obj.clone_ref(py), + result_obj.clone_ref(py), + feedback_obj.clone_ref(py), + Arc::clone(self.inner.graph()), + send_goal_service, )) } @@ -361,22 +492,29 @@ impl PyZNode { /// `__msgtype__` and `__hash__` attributes (from `hiroz_msgs_py`). /// /// Returns a `ZActionServer` for receiving and executing goals. + #[pyo3(signature = (action_name, goal_type, result_type=None, feedback_type=None))] fn create_action_server( &self, py: Python, action_name: String, goal_type: &Bound<'_, PyAny>, - result_type: &Bound<'_, PyAny>, - feedback_type: &Bound<'_, PyAny>, + result_type: Option<&Bound<'_, PyAny>>, + feedback_type: Option<&Bound<'_, PyAny>>, ) -> PyResult { + let (goal_obj, result_obj, feedback_obj) = + resolve_action_types(goal_type, result_type, feedback_type)?; + let goal_b = goal_obj.bind(py); + let result_b = result_obj.bind(py); + let feedback_b = feedback_obj.bind(py); + // __msgtype__ is still required (validates the class); __hash__ is optional. - extract_type_info_from_class(goal_type)?; - extract_type_info_from_class(result_type)?; - extract_type_info_from_class(feedback_type)?; + extract_type_info_from_class(goal_b)?; + extract_type_info_from_class(result_b)?; + extract_type_info_from_class(feedback_b)?; - let goal_ti = try_extract_type_info(goal_type); - let result_ti = try_extract_type_info(result_type); - let feedback_ti = try_extract_type_info(feedback_type); + let goal_ti = try_extract_type_info(goal_b); + let result_ti = try_extract_type_info(result_b); + let feedback_ti = try_extract_type_info(feedback_b); let node = Arc::clone(&self.inner); let rt = get_tokio_rt(); @@ -400,9 +538,9 @@ impl PyZNode { Ok(PyZActionServer::new( server, - goal_type.clone().unbind(), - result_type.clone().unbind(), - feedback_type.clone().unbind(), + goal_obj.clone_ref(py), + result_obj.clone_ref(py), + feedback_obj.clone_ref(py), )) } @@ -422,15 +560,47 @@ impl PyZNode { Ok(()) } - /// Create a service server - fn create_server(&self, service: String, srv_type: &Bound<'_, PyAny>) -> PyResult { - let (srv_type_str, type_info) = extract_service_type_from_request_class(srv_type)?; + /// Create a service server. + /// + /// `srv_type` may be a service grouping class (rclpy-style) or the bare + /// Request class (back-compat). + /// + /// If `callback` is provided, the server runs in callback mode: a background + /// thread receives each request, invokes `callback(request)`, and sends the + /// returned value as the response. The caller never calls `take_request` / + /// `send_response`. If `callback` is None (default), the server is in pull + /// mode and the caller drives it via `take_request` / `send_response`. + #[pyo3(signature = (service, srv_type, callback=None))] + fn create_server( + &self, + service: String, + srv_type: &Bound<'_, PyAny>, + callback: Option, + ) -> PyResult { + let (srv_type_str, type_info) = extract_service_type_info(srv_type)?; let server_builder = self .inner .create_service_impl::(&service, Some(type_info)); let zserver = server_builder.build().map_err(|e| e.into_pyerr())?; let wrapper = GenericServerWrapper::new(zserver); - Ok(PyZServer::new(Box::new(wrapper), srv_type_str)) + + match callback { + Some(cb) => Ok(PyZServer::new_with_callback( + Arc::new(wrapper), + srv_type_str, + cb, + )), + None => Ok(PyZServer::new(Box::new(wrapper), srv_type_str)), + } + } +} + +impl PyZNode { + /// Qualify a service name against the node's namespace/name so the result + /// matches the entries the discovery graph stores. Absolute names pass through. + fn qualify_service_name(&self, service: &str) -> String { + hiroz::topic_name::qualify_topic_name(service, self.inner.namespace(), self.inner.name()) + .unwrap_or_else(|_| service.to_string()) } } diff --git a/crates/hiroz-py/src/pubsub.rs b/crates/hiroz-py/src/pubsub.rs index 16fada65e..41b54ed74 100644 --- a/crates/hiroz-py/src/pubsub.rs +++ b/crates/hiroz-py/src/pubsub.rs @@ -40,6 +40,25 @@ impl PyZPublisher { self.inner.publish(data.into()).map_err(|e| e.into_pyerr()) } + /// Wait until at least `count` subscriptions match this publisher. + /// + /// Mirrors rclpy's discovery-wait pattern and removes the need for + /// `time.sleep(...)` before publishing. Returns True if `count` + /// subscriptions were matched before `timeout`, False otherwise. + /// + /// Args: + /// count: Number of subscriptions to wait for (default 1). + /// timeout: Maximum seconds to wait. None waits effectively forever. + #[pyo3(signature = (count=1, timeout=None))] + fn wait_for_subscription(&self, py: Python, count: usize, timeout: Option) -> bool { + // None → wait "forever"; cap at a large but finite duration so the + // background thread can still observe interpreter shutdown. + let dur = timeout + .map(Duration::from_secs_f64) + .unwrap_or(Duration::from_secs(60 * 60 * 24 * 365)); + py.allow_threads(|| self.inner.wait_for_subscription(count, dur)) + } + /// Get the topic name (for debugging) unsafe fn get_type_name(&self) -> String { self.type_name.clone() diff --git a/crates/hiroz-py/src/qos.rs b/crates/hiroz-py/src/qos.rs index 516ef4b23..eaa721f91 100644 --- a/crates/hiroz-py/src/qos.rs +++ b/crates/hiroz-py/src/qos.rs @@ -360,12 +360,28 @@ pub fn extract_qos(qos: Option<&Bound<'_, PyAny>>) -> PyResult { if let Ok(profile) = obj.extract::>() { return Ok(profile.inner); } + // rclpy-style int depth shorthand: `qos=10` == KeepLast(10). + // Checked before dict so a bare int is accepted anywhere a QoS is. + // `bool` is a subclass of `int` in Python; exclude it explicitly so + // `qos=True` is a clear type error rather than a depth of 1. + if !obj.is_instance_of::() + && let Ok(depth) = obj.extract::() + { + let non_zero = NonZeroUsize::new(depth).ok_or_else(|| { + pyo3::exceptions::PyValueError::new_err( + "qos depth shorthand must be greater than 0", + ) + })?; + let mut qos = QOS_DEFAULT; + qos.history = QosHistory::KeepLast(non_zero); + return Ok(qos); + } // Fall back to dict if let Ok(dict) = obj.downcast::() { return qos_from_pydict(dict); } Err(pyo3::exceptions::PyTypeError::new_err( - "qos must be a QosProfile or dict", + "qos must be a QosProfile, an int (depth shorthand), or a dict", )) } } diff --git a/crates/hiroz-py/src/service.rs b/crates/hiroz-py/src/service.rs index 1b822c220..1e9534e80 100644 --- a/crates/hiroz-py/src/service.rs +++ b/crates/hiroz-py/src/service.rs @@ -1,7 +1,10 @@ use crate::traits::{RawClient, RawServer}; +use hiroz::graph::Graph; use hiroz::service::RequestId; use pyo3::prelude::*; use pyo3::types::PyDict; +use std::sync::Arc; +use std::sync::atomic::{AtomicBool, Ordering}; use std::time::Duration; /// Python wrapper for service client @@ -10,16 +13,26 @@ pub struct PyZClient { inner: Box, request_type_name: String, response_type_name: String, + /// Shared graph + fully-qualified service name, used by `wait_for_service`. + graph: Arc, + service_name: String, } impl PyZClient { - pub fn new(inner: Box, service_type: String) -> Self { + pub fn new( + inner: Box, + service_type: String, + graph: Arc, + service_name: String, + ) -> Self { let request_type_name = format!("{}_Request", service_type); let response_type_name = format!("{}_Response", service_type); Self { inner, request_type_name, response_type_name, + graph, + service_name, } } } @@ -40,16 +53,25 @@ impl PyZClient { let cdr_bytes = py .allow_threads(|| self.inner.call_serialized(&cdr_bytes, timeout_duration)) - .map_err(|e| { - if hiroz::error::is_timeout(e.root_cause()) { - crate::error::TimeoutError::new_err(e.to_string()) - } else { - pyo3::exceptions::PyRuntimeError::new_err(e.to_string()) - } - })?; + .map_err(crate::error::map_call_error)?; hiroz_msgs::deserialize_from_cdr(&self.response_type_name, py, &cdr_bytes) } + /// Wait until a service server for this service is available. + /// + /// Mirrors rclpy's `Client.wait_for_service(timeout_sec)`. Polls the + /// discovery graph until a matching server appears. Returns True if a + /// server was found before `timeout`, False otherwise. + /// + /// Args: + /// timeout: Maximum seconds to wait. None waits forever. + #[pyo3(signature = (timeout=None))] + fn wait_for_service(&self, py: Python, timeout: Option) -> bool { + py.allow_threads(|| { + crate::graph::wait_for_service_server(&self.graph, &self.service_name, timeout) + }) + } + /// Get the service type name (for debugging) unsafe fn get_type_name(&self) -> String { format!( @@ -59,12 +81,37 @@ impl PyZClient { } } -/// Python wrapper for service server +/// Background-thread state for a callback-mode server (P6). +/// +/// Holds an `Arc` to the underlying server (keeping its Zenoh queryable alive) +/// and a stop flag the worker thread checks each poll. Dropping this signals the +/// thread to stop and joins it. +struct CallbackServerState { + stop: Arc, + handle: Option>, + _server: Arc, +} + +impl Drop for CallbackServerState { + fn drop(&mut self) { + self.stop.store(true, Ordering::Relaxed); + if let Some(h) = self.handle.take() { + let _ = h.join(); + } + } +} + +/// Python wrapper for service server. +/// +/// Pull mode (default): `inner` is `Some`; the caller drives `take_request` / +/// `send_response`. Callback mode (P6): `inner` is `None` and a background +/// thread (held in `_callback`) services requests via the user callback. #[pyclass(name = "ZServer")] pub struct PyZServer { - inner: std::sync::Mutex>, + inner: Option>>, request_type_name: String, response_type_name: String, + _callback: Option, } impl PyZServer { @@ -72,11 +119,110 @@ impl PyZServer { let request_type_name = format!("{}_Request", service_type); let response_type_name = format!("{}_Response", service_type); Self { - inner: std::sync::Mutex::new(inner), + inner: Some(std::sync::Mutex::new(inner)), request_type_name, response_type_name, + _callback: None, } } + + /// Build a callback-mode server: a background thread receives each request, + /// calls `callback(request)`, and sends the returned object as the response. + pub fn new_with_callback( + server: Arc, + service_type: String, + callback: PyObject, + ) -> Self { + let request_type_name = format!("{}_Request", service_type); + let response_type_name = format!("{}_Response", service_type); + + let stop = Arc::new(AtomicBool::new(false)); + let handle = spawn_callback_loop( + Arc::clone(&server), + request_type_name.clone(), + response_type_name.clone(), + callback, + Arc::clone(&stop), + ); + + Self { + inner: None, + request_type_name, + response_type_name, + _callback: Some(CallbackServerState { + stop, + handle: Some(handle), + _server: server, + }), + } + } + + fn require_pull(&self) -> PyResult<&std::sync::Mutex>> { + self.inner.as_ref().ok_or_else(|| { + pyo3::exceptions::PyRuntimeError::new_err( + "This server runs in callback mode; take_request/send_response are unavailable. \ + Create it without a callback to use pull mode.", + ) + }) + } +} + +/// Spawn the worker thread for a callback-mode server. +fn spawn_callback_loop( + server: Arc, + request_type_name: String, + response_type_name: String, + callback: PyObject, + stop: Arc, +) -> std::thread::JoinHandle<()> { + std::thread::spawn(move || { + while !stop.load(Ordering::Relaxed) { + // Poll for a request without holding the GIL. + match server.try_take_request_serialized() { + Ok(Some((request_id, request_bytes))) => { + Python::with_gil(|py| { + let req_obj = match hiroz_msgs::deserialize_from_cdr( + &request_type_name, + py, + &request_bytes, + ) { + Ok(o) => o, + Err(e) => { + eprintln!("hiroz_py: request deserialize error: {}", e); + return; + } + }; + let resp_obj = match callback.call1(py, (req_obj,)) { + Ok(o) => o, + Err(e) => { + eprintln!("hiroz_py: service callback error: {}", e); + return; + } + }; + let resp_bytes = match hiroz_msgs::serialize_to_cdr( + &response_type_name, + py, + resp_obj.bind(py), + ) { + Ok(b) => b, + Err(e) => { + eprintln!("hiroz_py: response serialize error: {}", e); + return; + } + }; + if let Err(e) = server.send_response_serialized(&resp_bytes, &request_id) { + eprintln!("hiroz_py: send_response error: {}", e); + } + }); + } + Ok(None) => std::thread::sleep(Duration::from_millis(2)), + Err(e) => { + eprintln!("hiroz_py: service poll error: {}", e); + std::thread::sleep(Duration::from_millis(50)); + } + } + } + }) } #[allow(unsafe_op_in_unsafe_fn)] @@ -84,9 +230,9 @@ impl PyZServer { impl PyZServer { /// Receive the next service request (blocking) unsafe fn take_request(&self, py: Python) -> PyResult<(PyObject, PyObject)> { + let mutex = self.require_pull()?; let result = py.allow_threads(|| { - let inner = self - .inner + let inner = mutex .lock() .map_err(|e| anyhow::anyhow!("Lock error: {}", e))?; inner.take_request_serialized() @@ -126,9 +272,9 @@ impl PyZServer { source_timestamp: 0, }; + let mutex = self.require_pull()?; py.allow_threads(|| { - let inner = self - .inner + let inner = mutex .lock() .map_err(|e| anyhow::anyhow!("Lock error: {}", e))?; inner.send_response_serialized(&cdr_bytes, &key) diff --git a/crates/hiroz-py/src/traits.rs b/crates/hiroz-py/src/traits.rs index 021ce00f2..8e7568f3a 100644 --- a/crates/hiroz-py/src/traits.rs +++ b/crates/hiroz-py/src/traits.rs @@ -10,6 +10,9 @@ use crate::raw_bytes::{RawBytesCdrSerdes, RawBytesMessage, RawBytesService}; pub(crate) trait RawPublisher: Send + Sync { /// Publish pre-serialized data fn publish(&self, data: ZBytes) -> Result<()>; + /// Block until at least `count` subscriptions are matched, or `timeout` elapses. + /// Returns true if the count was reached. Delegates to the core liveliness-based wait. + fn wait_for_subscription(&self, count: usize, timeout: Duration) -> bool; } /// Type-erased subscriber trait for Python interop @@ -43,6 +46,12 @@ impl RawPublisher for GenericPubWrapper { .publish_serialized(data) .map_err(|e| anyhow::anyhow!(e)) } + + fn wait_for_subscription(&self, count: usize, timeout: Duration) -> bool { + // The core method is async; block on it using the shared runtime. + // Callers release the GIL around this via `py.allow_threads`. + crate::action::get_tokio_rt().block_on(self.inner.wait_for_subscription(count, timeout)) + } } /// Generic subscriber wrapper using RawBytesMessage @@ -106,6 +115,9 @@ pub(crate) trait RawClient: Send + Sync { /// Type-erased server trait for Python interop pub(crate) trait RawServer: Send + Sync { fn take_request_serialized(&self) -> Result<(RequestId, Vec)>; + /// Non-blocking variant: returns None if no request is queued. + /// Used by the optional callback-mode server loop. + fn try_take_request_serialized(&self) -> Result)>>; fn send_response_serialized(&self, data: &[u8], request_id: &RequestId) -> Result<()>; } @@ -186,6 +198,29 @@ impl RawServer for GenericServerWrapper { Ok((request_id, request.0)) } + fn try_take_request_serialized(&self) -> Result)>> { + let mut server = self + .inner + .lock() + .map_err(|e| anyhow::anyhow!("Failed to lock server: {}", e))?; + + match server + .try_take_request() + .map_err(|e| anyhow::anyhow!("Failed to poll request: {}", e))? + { + Some(request) => { + let (request, reply) = request.into_parts(); + let request_id = reply.id().clone(); + self.pending + .lock() + .map_err(|e| anyhow::anyhow!("Failed to lock pending replies: {}", e))? + .insert(request_id.clone(), reply); + Ok(Some((request_id, request.0))) + } + None => Ok(None), + } + } + fn send_response_serialized(&self, data: &[u8], request_id: &RequestId) -> Result<()> { let response = RawBytesMessage(data.to_vec()); diff --git a/crates/hiroz-py/tests/test_rclpy_alignment.py b/crates/hiroz-py/tests/test_rclpy_alignment.py new file mode 100644 index 000000000..ac790e26e --- /dev/null +++ b/crates/hiroz-py/tests/test_rclpy_alignment.py @@ -0,0 +1,210 @@ +#!/usr/bin/env python3 +"""Tests for the rclpy-alignment features (P1-P8).""" + +import time + +import pytest + +import hiroz_py +from hiroz_py import example_interfaces, std_msgs + + +@pytest.fixture(scope="module") +def ctx(): + c = hiroz_py.ZContextBuilder().with_domain_id(0).build() + yield c + + +# --- P8: QoS enum constants + int depth shorthand --- + + +def test_p8_policy_constants(): + assert hiroz_py.ReliabilityPolicy.RELIABLE == "reliable" + assert hiroz_py.ReliabilityPolicy.BEST_EFFORT == "best_effort" + assert hiroz_py.DurabilityPolicy.VOLATILE == "volatile" + assert hiroz_py.DurabilityPolicy.TRANSIENT_LOCAL == "transient_local" + assert hiroz_py.HistoryPolicy.KEEP_LAST == "keep_last" + assert hiroz_py.HistoryPolicy.KEEP_ALL == "keep_all" + assert hiroz_py.LivelinessPolicy.AUTOMATIC == "automatic" + + +def test_p8_int_depth_shorthand(ctx): + node = ctx.create_node("p8_int").build() + # qos=10 should be accepted as a depth shorthand. + pub = node.create_publisher("/p8_topic", std_msgs.String, qos=10) + assert pub is not None + + +def test_p8_policy_constants_in_qos(ctx): + node = ctx.create_node("p8_policy").build() + qos = hiroz_py.QosProfile( + reliability=hiroz_py.ReliabilityPolicy.BEST_EFFORT, + history=hiroz_py.HistoryPolicy.KEEP_LAST, + depth=5, + ) + assert qos.reliability == "best_effort" + pub = node.create_publisher("/p8_policy_topic", std_msgs.String, qos=qos) + assert pub is not None + + +# --- P2: swapped-argument smart error --- + + +def test_p2_swapped_args_publisher(ctx): + node = ctx.create_node("p2_pub").build() + with pytest.raises(TypeError, match="swapped"): + # rclpy order: (msg_type, topic) -> should be rejected with a clear error. + node.create_publisher(std_msgs.String, "/chatter") + + +def test_p2_swapped_args_subscriber(ctx): + node = ctx.create_node("p2_sub").build() + with pytest.raises(TypeError, match="swapped"): + node.create_subscriber(std_msgs.String, "/chatter") + + +def test_p2_keyword_args_work(ctx): + node = ctx.create_node("p2_kw").build() + # Keyword args work regardless of historical order. + pub = node.create_publisher(msg_type=std_msgs.String, topic="/p2_kw_topic") + assert pub is not None + + +# --- P3: method aliases --- + + +def test_p3_create_subscription_alias(): + assert hiroz_py.ZNode.create_subscription is hiroz_py.ZNode.create_subscriber + + +def test_p3_create_service_alias(): + assert hiroz_py.ZNode.create_service is hiroz_py.ZNode.create_server + + +# --- P4: service grouping class --- + + +def test_p4_grouping_class_attributes(): + assert ( + example_interfaces.AddTwoInts.__srvtype__ == "example_interfaces/srv/AddTwoInts" + ) + assert example_interfaces.AddTwoInts.Request is example_interfaces.AddTwoIntsRequest + assert ( + example_interfaces.AddTwoInts.Response is example_interfaces.AddTwoIntsResponse + ) + + +def test_p4_client_accepts_grouping_class(ctx): + node = ctx.create_node("p4_client").build() + client = node.create_client("/p4_add", example_interfaces.AddTwoInts) + assert client is not None + + +def test_p4_client_accepts_bare_request(ctx): + # Back-compat: bare Request class still works. + node = ctx.create_node("p4_client_bc").build() + client = node.create_client("/p4_add_bc", example_interfaces.AddTwoIntsRequest) + assert client is not None + + +# --- P5: custom exception types --- + + +def test_p5_exception_hierarchy(): + assert issubclass(hiroz_py.TimeoutError, hiroz_py.HirozError) + assert issubclass(hiroz_py.SerializationError, hiroz_py.HirozError) + assert issubclass(hiroz_py.TypeMismatchError, hiroz_py.HirozError) + + +def test_p5_call_failure_is_hiroz_error(ctx): + node = ctx.create_node("p5_client").build() + client = node.create_client("/p5_nonexistent", example_interfaces.AddTwoInts) + req = example_interfaces.AddTwoInts.Request(a=1, b=2) + with pytest.raises(hiroz_py.HirozError): + client.call(req, timeout=1.0) + + +# --- P1 + P4 + P6: end-to-end service with wait_for_service and callback mode --- + + +def test_p1_p6_callback_service_end_to_end(ctx): + node = ctx.create_node("p6_node").build() + + def handle(req): + return example_interfaces.AddTwoInts.Response(sum=req.a + req.b) + + # P6: callback-mode server (no take_request loop). + server = node.create_server( + "/p6_add", example_interfaces.AddTwoInts, callback=handle + ) + assert server is not None + + client = node.create_client("/p6_add", example_interfaces.AddTwoInts) + # P1: wait for the server instead of sleeping. + assert client.wait_for_service(timeout=5.0), "server should be discoverable" + + resp = client.call(example_interfaces.AddTwoInts.Request(a=4, b=38), timeout=5.0) + assert resp.sum == 42 + + +def test_p1_wait_for_service_timeout_returns_false(ctx): + node = ctx.create_node("p1_wait_to").build() + client = node.create_client("/p1_never", example_interfaces.AddTwoInts) + t0 = time.time() + assert client.wait_for_service(timeout=0.5) is False + assert time.time() - t0 >= 0.4 + + +# --- P1: wait_for_subscription end-to-end --- + + +def test_p1_wait_for_subscription(ctx): + node = ctx.create_node("p1_pubsub").build() + pub = node.create_publisher("/p1_chatter", std_msgs.String) + received = [] + + def cb(msg): + received.append(msg.data) + + node.create_subscriber("/p1_chatter", std_msgs.String, callback=cb) + + assert pub.wait_for_subscription(count=1, timeout=5.0), "subscription should match" + + pub.publish(std_msgs.String(data="hello")) + deadline = time.time() + 3.0 + while not received and time.time() < deadline: + time.sleep(0.05) + assert received == ["hello"] + + +# --- P7: action grouping class detection (inline, Python-to-Python) --- + + +def test_p7_action_grouping_class(ctx): + import msgspec + from typing import ClassVar + + class CountToGoal(msgspec.Struct): + __msgtype__: ClassVar[str] = "p7_demo/msg/CountToGoal" + target: int = 0 + + class CountToResult(msgspec.Struct): + __msgtype__: ClassVar[str] = "p7_demo/msg/CountToResult" + final_count: int = 0 + + class CountToFeedback(msgspec.Struct): + __msgtype__: ClassVar[str] = "p7_demo/msg/CountToFeedback" + current: int = 0 + + class CountTo: + __actiontype__: ClassVar[str] = "p7_demo/action/CountTo" + Goal = CountToGoal + Result = CountToResult + Feedback = CountToFeedback + + node = ctx.create_node("p7_action").build() + # Single grouping class instead of three positional types. + client = node.create_action_client("/p7_count", CountTo) + assert client is not None + server = node.create_action_server("/p7_count", CountTo) + assert server is not None diff --git a/crates/hiroz-py/tests/test_service.py b/crates/hiroz-py/tests/test_service.py index a9438c1a3..4990bdfca 100644 --- a/crates/hiroz-py/tests/test_service.py +++ b/crates/hiroz-py/tests/test_service.py @@ -93,9 +93,9 @@ def test_timeout_handling(client): req = example_interfaces.AddTwoIntsRequest(a=1, b=2) try: timeout_client.call(req, timeout=1.0) - assert False, "Expected timeout error, but call succeeded" - except RuntimeError: - pass # expected: call timed out + assert False, "Expected an error, but call succeeded" + except hiroz_py.HirozError: + pass # expected: call failed (no server / timeout). TimeoutError is a subclass. print("✓ Timeout handling works") From 381a9aef3a394e7c14cfb4e07160bbecf3220d41 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Fri, 29 May 2026 15:39:47 +0800 Subject: [PATCH 02/18] fix(hiroz-py): surface P6 callback thread errors via last_error property MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Background callback threads were silently swallowing exceptions via eprintln. Add a shared Arc>> that records the most recent error; expose it as ZServer.last_error (resets on read). Also fix a pre-existing broken intra-doc link in lifecycle/node.rs (create_publisher → Self::create_publisher) that was failing cargo doc. --- crates/hiroz-py/python/hiroz_py/__init__.pyi | 2 + crates/hiroz-py/src/service.rs | 47 +++++++++++++++---- crates/hiroz-py/tests/test_rclpy_alignment.py | 42 +++++++++++++++++ 3 files changed, 82 insertions(+), 9 deletions(-) diff --git a/crates/hiroz-py/python/hiroz_py/__init__.pyi b/crates/hiroz-py/python/hiroz_py/__init__.pyi index 52eb280e2..e7bfb5d1b 100644 --- a/crates/hiroz-py/python/hiroz_py/__init__.pyi +++ b/crates/hiroz-py/python/hiroz_py/__init__.pyi @@ -288,6 +288,8 @@ class ZServer: def take_request(self) -> tuple[dict[str, Any], Any]: ... def send_response(self, response: Any, request_id: dict[str, Any]) -> None: ... def get_type_name(self) -> str: ... + @property + def last_error(self) -> str | None: ... # --------------------------------------------------------------------------- # ZActionClient diff --git a/crates/hiroz-py/src/service.rs b/crates/hiroz-py/src/service.rs index 1e9534e80..d7f1bb321 100644 --- a/crates/hiroz-py/src/service.rs +++ b/crates/hiroz-py/src/service.rs @@ -3,8 +3,8 @@ use hiroz::graph::Graph; use hiroz::service::RequestId; use pyo3::prelude::*; use pyo3::types::PyDict; -use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::{Arc, Mutex}; use std::time::Duration; /// Python wrapper for service client @@ -90,6 +90,7 @@ struct CallbackServerState { stop: Arc, handle: Option>, _server: Arc, + last_error: Arc>>, } impl Drop for CallbackServerState { @@ -106,12 +107,15 @@ impl Drop for CallbackServerState { /// Pull mode (default): `inner` is `Some`; the caller drives `take_request` / /// `send_response`. Callback mode (P6): `inner` is `None` and a background /// thread (held in `_callback`) services requests via the user callback. +/// Errors from the callback thread are stored in `last_error` and surfaced via +/// the `last_error` Python property. #[pyclass(name = "ZServer")] pub struct PyZServer { - inner: Option>>, + inner: Option>>, request_type_name: String, response_type_name: String, _callback: Option, + last_error: Arc>>, } impl PyZServer { @@ -119,10 +123,11 @@ impl PyZServer { let request_type_name = format!("{}_Request", service_type); let response_type_name = format!("{}_Response", service_type); Self { - inner: Some(std::sync::Mutex::new(inner)), + inner: Some(Mutex::new(inner)), request_type_name, response_type_name, _callback: None, + last_error: Arc::new(Mutex::new(None)), } } @@ -137,12 +142,14 @@ impl PyZServer { let response_type_name = format!("{}_Response", service_type); let stop = Arc::new(AtomicBool::new(false)); + let last_error: Arc>> = Arc::new(Mutex::new(None)); let handle = spawn_callback_loop( Arc::clone(&server), request_type_name.clone(), response_type_name.clone(), callback, Arc::clone(&stop), + Arc::clone(&last_error), ); Self { @@ -153,11 +160,13 @@ impl PyZServer { stop, handle: Some(handle), _server: server, + last_error: Arc::clone(&last_error), }), + last_error, } } - fn require_pull(&self) -> PyResult<&std::sync::Mutex>> { + fn require_pull(&self) -> PyResult<&Mutex>> { self.inner.as_ref().ok_or_else(|| { pyo3::exceptions::PyRuntimeError::new_err( "This server runs in callback mode; take_request/send_response are unavailable. \ @@ -174,7 +183,19 @@ fn spawn_callback_loop( response_type_name: String, callback: PyObject, stop: Arc, + last_error: Arc>>, ) -> std::thread::JoinHandle<()> { + // Helper: record an error both in the shared slot and stderr. + macro_rules! record_error { + ($last_error:expr, $msg:literal, $e:expr) => {{ + let msg = format!(concat!("hiroz_py: ", $msg, ": {}"), $e); + eprintln!("{}", msg); + if let Ok(mut guard) = $last_error.lock() { + *guard = Some(msg); + } + }}; + } + std::thread::spawn(move || { while !stop.load(Ordering::Relaxed) { // Poll for a request without holding the GIL. @@ -188,14 +209,14 @@ fn spawn_callback_loop( ) { Ok(o) => o, Err(e) => { - eprintln!("hiroz_py: request deserialize error: {}", e); + record_error!(last_error, "request deserialize error", e); return; } }; let resp_obj = match callback.call1(py, (req_obj,)) { Ok(o) => o, Err(e) => { - eprintln!("hiroz_py: service callback error: {}", e); + record_error!(last_error, "service callback error", e); return; } }; @@ -206,18 +227,18 @@ fn spawn_callback_loop( ) { Ok(b) => b, Err(e) => { - eprintln!("hiroz_py: response serialize error: {}", e); + record_error!(last_error, "response serialize error", e); return; } }; if let Err(e) = server.send_response_serialized(&resp_bytes, &request_id) { - eprintln!("hiroz_py: send_response error: {}", e); + record_error!(last_error, "send_response error", e); } }); } Ok(None) => std::thread::sleep(Duration::from_millis(2)), Err(e) => { - eprintln!("hiroz_py: service poll error: {}", e); + record_error!(last_error, "service poll error", e); std::thread::sleep(Duration::from_millis(50)); } } @@ -289,4 +310,12 @@ impl PyZServer { self.request_type_name, self.response_type_name ) } + + /// The last error raised by the callback thread, or None if no error has + /// occurred. Resets to None when read. Only meaningful in callback mode; + /// always None in pull mode. + #[getter] + fn last_error(&self) -> Option { + self.last_error.lock().ok().and_then(|mut g| g.take()) + } } diff --git a/crates/hiroz-py/tests/test_rclpy_alignment.py b/crates/hiroz-py/tests/test_rclpy_alignment.py index ac790e26e..05533205b 100644 --- a/crates/hiroz-py/tests/test_rclpy_alignment.py +++ b/crates/hiroz-py/tests/test_rclpy_alignment.py @@ -147,6 +147,48 @@ def handle(req): assert resp.sum == 42 +def test_p6_last_error_surfaced_on_callback_exception(ctx): + node = ctx.create_node("p6_err_node").build() + + def bad_handle(req): + raise ValueError("intentional callback failure") + + server = node.create_server( + "/p6_err_add", example_interfaces.AddTwoInts, callback=bad_handle + ) + client = node.create_client("/p6_err_add", example_interfaces.AddTwoInts) + assert client.wait_for_service(timeout=5.0) + + # The call will fail from the client side (no response sent). + try: + client.call(example_interfaces.AddTwoInts.Request(a=1, b=2), timeout=1.0) + except Exception: + pass + + # Give the background thread a moment to record the error. + deadline = time.time() + 2.0 + err = None + while err is None and time.time() < deadline: + err = server.last_error + if err is None: + time.sleep(0.05) + + assert err is not None, "last_error should surface the callback exception" + assert "intentional callback failure" in err + + +def test_p6_last_error_none_when_no_error(ctx): + node = ctx.create_node("p6_ok_node").build() + + def handle(req): + return example_interfaces.AddTwoInts.Response(sum=req.a + req.b) + + server = node.create_server( + "/p6_ok_add", example_interfaces.AddTwoInts, callback=handle + ) + assert server.last_error is None + + def test_p1_wait_for_service_timeout_returns_false(ctx): node = ctx.create_node("p1_wait_to").build() client = node.create_client("/p1_never", example_interfaces.AddTwoInts) From 46007dd803a56cf734d93880355cb99049d93eec Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Sun, 5 Jul 2026 10:41:02 +0800 Subject: [PATCH 03/18] test(hiroz-py): close P1/P3/P5/P7 coverage gaps in rclpy alignment tests Add wait_for_server (action) tests, exercise create_subscription/ create_service aliases end-to-end, assert TimeoutError (not just HirozError) is raised on a matched-but-unresponsive service, exercise the action grouping class through an actual goal send, and pin the get_result(timeout=...) None-on-timeout contract. Drop the unused last_error clone in CallbackServerState that caused a dead_code warning. --- crates/hiroz-py/src/service.rs | 2 - crates/hiroz-py/tests/test_rclpy_alignment.py | 180 ++++++++++++++++-- 2 files changed, 162 insertions(+), 20 deletions(-) diff --git a/crates/hiroz-py/src/service.rs b/crates/hiroz-py/src/service.rs index d7f1bb321..53b3da924 100644 --- a/crates/hiroz-py/src/service.rs +++ b/crates/hiroz-py/src/service.rs @@ -90,7 +90,6 @@ struct CallbackServerState { stop: Arc, handle: Option>, _server: Arc, - last_error: Arc>>, } impl Drop for CallbackServerState { @@ -160,7 +159,6 @@ impl PyZServer { stop, handle: Some(handle), _server: server, - last_error: Arc::clone(&last_error), }), last_error, } diff --git a/crates/hiroz-py/tests/test_rclpy_alignment.py b/crates/hiroz-py/tests/test_rclpy_alignment.py index 05533205b..e72671dc8 100644 --- a/crates/hiroz-py/tests/test_rclpy_alignment.py +++ b/crates/hiroz-py/tests/test_rclpy_alignment.py @@ -1,14 +1,64 @@ #!/usr/bin/env python3 """Tests for the rclpy-alignment features (P1-P8).""" +import threading import time +from typing import ClassVar +import msgspec import pytest import hiroz_py from hiroz_py import example_interfaces, std_msgs +# --- shared inline action types (mirrors test_action.py) --- + + +class CountGoal(msgspec.Struct): + __msgtype__: ClassVar[str] = "rclpy_alignment/msg/CountGoal" + target: int = 3 + step_delay: float = 0.05 + + +class CountResult(msgspec.Struct): + __msgtype__: ClassVar[str] = "rclpy_alignment/msg/CountResult" + final_count: int = 0 + + +class CountFeedback(msgspec.Struct): + __msgtype__: ClassVar[str] = "rclpy_alignment/msg/CountFeedback" + current: int = 0 + + +class CountTo: + __actiontype__: ClassVar[str] = "rclpy_alignment/action/CountTo" + Goal = CountGoal + Result = CountResult + Feedback = CountFeedback + + +def run_action_server_once(server): + """Drive the action server for a single goal in a background thread.""" + + def _run(): + req = server.recv_goal(timeout=5.0) + if req is None: + return + executing = req.accept_and_execute() + goal = executing.goal() + count = 0 + while count < goal.target: + time.sleep(goal.step_delay) + count += 1 + executing.publish_feedback(CountFeedback(current=count)) + executing.succeed(CountResult(final_count=count)) + + t = threading.Thread(target=_run, daemon=True) + t.start() + return t + + @pytest.fixture(scope="module") def ctx(): c = hiroz_py.ZContextBuilder().with_domain_id(0).build() @@ -81,6 +131,37 @@ def test_p3_create_service_alias(): assert hiroz_py.ZNode.create_service is hiroz_py.ZNode.create_server +def test_p3_create_subscription_alias_end_to_end(ctx): + node = ctx.create_node("p3_sub_alias").build() + pub = node.create_publisher("/p3_sub_topic", std_msgs.String) + received = [] + node.create_subscription("/p3_sub_topic", std_msgs.String, callback=received.append) + + assert pub.wait_for_subscription(count=1, timeout=5.0) + pub.publish(std_msgs.String(data="via-alias")) + deadline = time.time() + 3.0 + while not received and time.time() < deadline: + time.sleep(0.05) + assert received and received[0].data == "via-alias" + + +def test_p3_create_service_alias_end_to_end(ctx): + node = ctx.create_node("p3_srv_alias").build() + + def handle(req): + return example_interfaces.AddTwoInts.Response(sum=req.a + req.b) + + server = node.create_service( + "/p3_add", example_interfaces.AddTwoInts, callback=handle + ) + client = node.create_client("/p3_add", example_interfaces.AddTwoInts) + assert client.wait_for_service(timeout=5.0) + + resp = client.call(example_interfaces.AddTwoInts.Request(a=10, b=32), timeout=5.0) + assert resp.sum == 42 + assert server is not None + + # --- P4: service grouping class --- @@ -124,6 +205,51 @@ def test_p5_call_failure_is_hiroz_error(ctx): client.call(req, timeout=1.0) +def test_p5_call_timeout_raises_timeout_error(ctx): + # A matched-but-unresponsive server (as opposed to no server at all) is + # required to exercise the actual "timed out" path -- with zero matching + # queryables the call fails immediately with a different (non-timeout) + # message (see test_p5_call_failure_is_hiroz_error above). + node = ctx.create_node("p5_timeout_server").build() + server = node.create_server("/p5_never_answers", example_interfaces.AddTwoInts) + assert server is not None + + def _never_respond(): + server.take_request() # receive but never send_response + + threading.Thread(target=_never_respond, daemon=True).start() + + client = node.create_client("/p5_never_answers", example_interfaces.AddTwoInts) + assert client.wait_for_service(timeout=5.0) + + req = example_interfaces.AddTwoInts.Request(a=1, b=2) + with pytest.raises(hiroz_py.TimeoutError): + client.call(req, timeout=0.5) + + +def test_p5_action_result_timeout_returns_none(ctx): + """P5 gap: unlike the service `call()` path, `get_result(timeout=...)` + does not raise `hiroz_py.TimeoutError` on timeout -- it returns `None` + (see action.rs's `get_result` docstring). Documented here so the + behavioral difference is pinned by a test rather than silently assumed. + """ + node = ctx.create_node("p5_action_client").build() + client = node.create_action_client("/p5_never_completes", CountTo) + server = node.create_action_server("/p5_never_completes", CountTo) + + def _never_finish(): + req = server.recv_goal(timeout=5.0) + if req is not None: + req.accept_and_execute() + # Deliberately never call succeed/abort/canceled. + + threading.Thread(target=_never_finish, daemon=True).start() + assert client.wait_for_server(timeout=5.0) + + handle = client.send_goal(CountGoal(target=1)) + assert handle.get_result(timeout=0.5) is None + + # --- P1 + P4 + P6: end-to-end service with wait_for_service and callback mode --- @@ -219,34 +345,52 @@ def cb(msg): assert received == ["hello"] -# --- P7: action grouping class detection (inline, Python-to-Python) --- +# --- P1: wait_for_server (action) --- + +def test_p1_wait_for_server(ctx): + node = ctx.create_node("p1_wait_for_server").build() + server = node.create_action_server( + "/p1_action_wfs", CountGoal, CountResult, CountFeedback + ) + client = node.create_action_client( + "/p1_action_wfs", CountGoal, CountResult, CountFeedback + ) + assert client.wait_for_server(timeout=5.0), "action server should be discoverable" + assert server is not None -def test_p7_action_grouping_class(ctx): - import msgspec - from typing import ClassVar - class CountToGoal(msgspec.Struct): - __msgtype__: ClassVar[str] = "p7_demo/msg/CountToGoal" - target: int = 0 +def test_p1_wait_for_server_timeout_returns_false(ctx): + node = ctx.create_node("p1_wait_for_server_to").build() + client = node.create_action_client( + "/p1_action_never", CountGoal, CountResult, CountFeedback + ) + t0 = time.time() + assert client.wait_for_server(timeout=0.5) is False + assert time.time() - t0 >= 0.4 - class CountToResult(msgspec.Struct): - __msgtype__: ClassVar[str] = "p7_demo/msg/CountToResult" - final_count: int = 0 - class CountToFeedback(msgspec.Struct): - __msgtype__: ClassVar[str] = "p7_demo/msg/CountToFeedback" - current: int = 0 +# --- P7: action grouping class, exercised through an actual goal send --- - class CountTo: - __actiontype__: ClassVar[str] = "p7_demo/action/CountTo" - Goal = CountToGoal - Result = CountToResult - Feedback = CountToFeedback +def test_p7_action_grouping_class_construction(ctx): node = ctx.create_node("p7_action").build() # Single grouping class instead of three positional types. client = node.create_action_client("/p7_count", CountTo) assert client is not None server = node.create_action_server("/p7_count", CountTo) assert server is not None + + +def test_p7_action_grouping_class_end_to_end(ctx): + node = ctx.create_node("p7_e2e").build() + server = node.create_action_server("/p7_e2e_count", CountTo) + client = node.create_action_client("/p7_e2e_count", CountTo) + + assert client.wait_for_server(timeout=5.0) + run_action_server_once(server) + + handle = client.send_goal(CountTo.Goal(target=3, step_delay=0.05)) + result = handle.get_result(timeout=5.0) + assert result is not None + assert result.final_count == 3 From fe16a33b1fe13913e54d405210b34aa48cf41327 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Sun, 5 Jul 2026 10:41:11 +0800 Subject: [PATCH 04/18] docs(hiroz-py): document P1-P8 rclpy alignment and add migration guide The book still described the pre-alignment API (pull-only create_server, flat Request/Response classes) with no mention of wait_for_service/ wait_for_server/wait_for_subscription, swapped-arg detection, method aliases, grouping classes, TimeoutError, the callback-mode server plus last_error, or QoS enums/int shorthand. Add an "rclpy Alignment" section to the Python bindings chapter, extend the codegen chapter with the generated grouping-class shape, and merge the untracked migration guide into a new tracked chapter so it survives branch cleanup. --- docs/bindings/python-codegen.md | 13 ++ docs/bindings/python-migration.md | 321 ++++++++++++++++++++++++++++++ docs/bindings/python.md | 102 +++++++++- mkdocs.yml | 1 + 4 files changed, 436 insertions(+), 1 deletion(-) create mode 100644 docs/bindings/python-migration.md diff --git a/docs/bindings/python-codegen.md b/docs/bindings/python-codegen.md index 8c7561fda..88602d100 100644 --- a/docs/bindings/python-codegen.md +++ b/docs/bindings/python-codegen.md @@ -94,6 +94,19 @@ class AddTwoIntsResponse(msgspec.Struct, frozen=True, kw_only=True): For service types, `__hash__` contains the service type hash (computed from the combined request/response definition). Both request and response share the same hash since they belong to the same service. This differs from regular messages where `__hash__` contains the individual message type hash. +Alongside the standalone Request/Response structs, the generator emits an rclpy-style grouping class that references them: + +```python +# Generated grouping class for example_interfaces/srv/AddTwoInts +class AddTwoInts: + """Service grouping type. Use AddTwoInts.Request and AddTwoInts.Response.""" + __srvtype__: ClassVar[str] = 'example_interfaces/srv/AddTwoInts' + Request: ClassVar[type] = AddTwoIntsRequest + Response: ClassVar[type] = AddTwoIntsResponse +``` + +Actions get the equivalent `Goal`/`Result`/`Feedback` grouping class (`__actiontype__`). `create_client`/`create_server` and `create_action_client`/`create_action_server` accept either the grouping class or the bare per-message classes — see the [Grouped Request/Response and Goal/Result/Feedback Types](./python.md#grouped-requestresponse-and-goalresultfeedback-types) section of the main Python bindings chapter for usage. + ### Rust: Generated Structs with Derive Macros The Rust code generator adds derive attributes to message structs: diff --git a/docs/bindings/python-migration.md b/docs/bindings/python-migration.md new file mode 100644 index 000000000..507b677fa --- /dev/null +++ b/docs/bindings/python-migration.md @@ -0,0 +1,321 @@ +# Migrating from rclpy + +A practical guide for ROS 2 Python (`rclpy`) developers moving to `hiroz-py`. hiroz-py is a Python binding over the pure-Rust hiroz stack, which speaks ROS 2 over Zenoh. It deliberately keeps a **reactive, pull-based core** — there is no `rclpy.spin()` / executor — but the API has been aligned so most rclpy code maps over with mechanical changes. + +## Mental-Model Differences + +| Concept | rclpy | hiroz-py | +|---|---|---| +| Event loop | `rclpy.spin(node)` drives callbacks | **No spin / no executor.** You pull, or you register a callback that fires on an internal thread. | +| Subscriptions | callback-only, driven by the executor | callback **or** queue: `sub.recv(timeout=...)` pulls; or pass `callback=` to fire on an internal thread | +| Services (server) | callback-only | pull by default (`take_request` / `send_response`); pass `callback=` for rclpy-style auto-response | +| Lifecycle | `rclpy.init()` / `rclpy.shutdown()` | build a `ZContext`; it shuts down on drop or `ctx.shutdown()` | +| Context | global, implicit | explicit `ZContext` object (use it as a context manager) | +| Args order | `create_publisher(msg_type, topic, qos)` | `create_publisher(topic, msg_type, qos)` — **topic first** (pass by keyword to avoid confusion) | + +The most important consequence: **there is no `spin()`**. A talker just publishes in a loop. A listener either calls `sub.recv()` in a loop or registers a callback and then does its own waiting (e.g. `time.sleep`, an `Event`, or its own work loop). + +## Side-by-Side Cheatsheet + +### Publisher / Subscriber + +```python +# rclpy +import rclpy +from rclpy.node import Node +from std_msgs.msg import String + +rclpy.init() +node = Node("talker") +pub = node.create_publisher(String, "/chatter", 10) +pub.publish(String(data="hi")) + +def cb(msg): print(msg.data) +node.create_subscription(String, "/chatter", cb, 10) +rclpy.spin(node) +``` + +```python +# hiroz-py +import hiroz_py +from hiroz_py import std_msgs + +ctx = hiroz_py.ZContextBuilder().with_connect_endpoints(["tcp/127.0.0.1:7447"]).build() +node = ctx.create_node("talker").build() + +pub = node.create_publisher("/chatter", std_msgs.String, qos=10) # topic first; int qos OK +pub.wait_for_subscription(count=1, timeout=5.0) # no sleep races +pub.publish(std_msgs.String(data="hi")) + +def cb(msg): print(msg.data) +node.create_subscription("/chatter", std_msgs.String, callback=cb) # alias of create_subscriber +# ... no spin(); do your own waiting/work here ... +``` + +Queue-style subscriber (no callback): + +```python +sub = node.create_subscriber("/chatter", std_msgs.String) +msg = sub.recv(timeout=1.0) # returns None on timeout +``` + +### Service Client + +```python +# rclpy +from example_interfaces.srv import AddTwoInts +cli = node.create_client(AddTwoInts, "/add_two_ints") +cli.wait_for_service() +fut = cli.call_async(AddTwoInts.Request(a=2, b=3)) +rclpy.spin_until_future_complete(node, fut) +print(fut.result().sum) +``` + +```python +# hiroz-py +from hiroz_py import example_interfaces +cli = node.create_client("/add_two_ints", example_interfaces.AddTwoInts) # grouping type +if not cli.wait_for_service(timeout=5.0): + raise hiroz_py.HirozError("service unavailable") +resp = cli.call(example_interfaces.AddTwoInts.Request(a=2, b=3), timeout=5.0) # blocking +print(resp.sum) +``` + +### Service Server — Callback Style (rclpy-like) + +```python +# rclpy +def handle(req, resp): + resp.sum = req.a + req.b + return resp +node.create_service(AddTwoInts, "/add_two_ints", handle) +rclpy.spin(node) +``` + +```python +# hiroz-py (callback returns the response; no resp out-param) +def handle(req): + return example_interfaces.AddTwoInts.Response(sum=req.a + req.b) +node.create_service("/add_two_ints", example_interfaces.AddTwoInts, callback=handle) +# server runs on an internal thread; keep the process alive (no spin needed) +``` + +### Service Server — Pull Style (hiroz-native) + +```python +server = node.create_server("/add_two_ints", example_interfaces.AddTwoInts) +while True: + request_id, req = server.take_request() # blocks + server.send_response( + example_interfaces.AddTwoInts.Response(sum=req.a + req.b), request_id + ) +``` + +### Action Client + +```python +# rclpy +from rclpy.action import ActionClient +from action_tutorials_interfaces.action import Fibonacci +ac = ActionClient(node, Fibonacci, "/fibonacci") +ac.wait_for_server() +fut = ac.send_goal_async(Fibonacci.Goal(order=10)) +... +``` + +```python +# hiroz-py (Python actions are Python-to-Python via msgpack; not rmw_zenoh_cpp interop) +ac = node.create_action_client("/fibonacci", Fibonacci) # single grouping type +if not ac.wait_for_server(timeout=5.0): + raise hiroz_py.HirozError("action server unavailable") +handle = ac.send_goal(Fibonacci.Goal(order=10)) # blocks until accepted +while (fb := handle.recv_feedback(timeout=0.5)) is not None: + print(fb) +result = handle.get_result(timeout=10.0) # None on timeout +``` + +If you don't have a generated grouping class, pass the three classes positionally (back-compat): + +```python +ac = node.create_action_client("/fibonacci", FibGoal, FibResult, FibFeedback) +``` + +### Action Server + +```python +server = node.create_action_server("/fibonacci", Fibonacci) # or 3 positional types +while True: + request = server.recv_goal(timeout=1.0) + if request is None: + continue + goal = request.goal() + executing = request.accept_and_execute() + executing.publish_feedback(Fibonacci.Feedback(...)) + if executing.is_cancel_requested: + executing.canceled(Fibonacci.Result(...)) + else: + executing.succeed(Fibonacci.Result(...)) +``` + +## API Name Mapping + +| rclpy | hiroz-py | Notes | +|---|---|---| +| `rclpy.init()` | `ZContextBuilder()...build()` | explicit context object | +| `rclpy.shutdown()` | `ctx.shutdown()` or context-manager exit | | +| `Node("name")` | `ctx.create_node("name").build()` | builder pattern | +| `node.create_publisher(T, topic, qos)` | `node.create_publisher(topic, T, qos=...)` | **topic first** | +| `node.create_subscription(T, topic, cb, qos)` | `node.create_subscription(topic, T, callback=cb, qos=...)` | alias of `create_subscriber` | +| `node.create_client(Srv, name)` | `node.create_client(name, Srv)` | `Srv` = grouping type or bare Request | +| `node.create_service(Srv, name, cb)` | `node.create_service(name, Srv, callback=cb)` | alias of `create_server`; pull mode if no callback | +| `ActionClient(node, Act, name)` | `node.create_action_client(name, Act)` | grouping type or 3 classes | +| `ActionServer(node, Act, name, cb)` | `node.create_action_server(name, Act)` | reactive loop, not a callback | +| `client.wait_for_service(t)` | `client.wait_for_service(timeout=t)` | returns `bool` | +| `action_client.wait_for_server(t)` | `action_client.wait_for_server(timeout=t)` | returns `bool` | +| *(rclpy has no direct equivalent)* | `pub.wait_for_subscription(count, timeout)` | returns `bool` | +| `client.call_async(req)` + spin | `client.call(req, timeout=...)` | **blocking** call, returns the response | +| `sub` callback (executor) | `sub.recv(timeout=...)` **or** `callback=` | pull or push | +| `node.get_logger().info(...)` | *(use Python `logging` / `print`)* | rosout not implemented | +| `node.create_timer(...)` | *(not implemented)* | see [What's Not There Yet](#whats-not-there-yet) | +| `node.declare_parameter(...)` | *(not implemented)* | see [What's Not There Yet](#whats-not-there-yet) | + +## Message Types + +Messages are `msgspec.Struct`s from `hiroz_msgs_py` (re-exported by `hiroz_py`). Construct with keyword args: + +```python +from hiroz_py import std_msgs, geometry_msgs +m = std_msgs.String(data="hi") +v = geometry_msgs.Twist(linear=geometry_msgs.Vector3(x=1.0)) +``` + +### Services: `AddTwoInts.Request` / `.Response` + +Each `.srv` generates three Python objects: + +- `AddTwoIntsRequest` — the request struct +- `AddTwoIntsResponse` — the response struct +- `AddTwoInts` — a **grouping class** exposing `__srvtype__`, `.Request`, and `.Response` + +This mirrors rclpy's `AddTwoInts.Request`. Pass the grouping class to `create_client` / `create_server` (preferred), or the bare `AddTwoIntsRequest` class (still supported for back-compat): + +```python +example_interfaces.AddTwoInts.Request(a=1, b=2) # rclpy-style +example_interfaces.AddTwoIntsRequest(a=1, b=2) # equivalent, also works +``` + +### Actions: `Fibonacci.Goal` / `.Result` / `.Feedback` + +`create_action_client` / `create_action_server` accept a single grouping class exposing `__actiontype__`, `.Goal`, `.Result`, `.Feedback`. If you define inline msgspec types, you can build your own grouping class: + +```python +class CountTo: + __actiontype__ = "my_pkg/action/CountTo" + Goal = CountToGoal + Result = CountToResult + Feedback = CountToFeedback + +node.create_action_client("/count", CountTo) +``` + +The 3-positional-class form (`create_action_client(name, Goal, Result, Feedback)`) still works. + +!!! warning + hiroz-py actions use a msgpack wire format and are **Python-to-Python only** — they do not interoperate with `rmw_zenoh_cpp` typed actions. Pub/sub and services *do* interoperate. + +## QoS + +Three ways to specify QoS, all accepted anywhere a `qos=` argument appears: + +```python +# 1. Int depth shorthand (rclpy-style) -> KeepLast(n) +node.create_publisher("/t", std_msgs.String, qos=10) + +# 2. Enum-like policy constants (discoverable, typo-proof) +from hiroz_py import QosProfile, ReliabilityPolicy, HistoryPolicy +qos = QosProfile( + reliability=ReliabilityPolicy.BEST_EFFORT, + history=HistoryPolicy.KEEP_LAST, + depth=5, +) +node.create_subscription("/scan", sensor_msgs.LaserScan, qos=qos) + +# 3. Presets +node.create_publisher("/t", std_msgs.String, qos=hiroz_py.QOS_SENSOR_DATA) +``` + +Available policy holders (string-valued, mirroring `rclpy.qos`): + +- `ReliabilityPolicy.RELIABLE` / `.BEST_EFFORT` +- `DurabilityPolicy.VOLATILE` / `.TRANSIENT_LOCAL` +- `HistoryPolicy.KEEP_LAST` / `.KEEP_ALL` +- `LivelinessPolicy.AUTOMATIC` / `.MANUAL_BY_TOPIC` / `.MANUAL_BY_NODE` + +Plain strings (`reliability="best_effort"`) and dicts still work. + +## Error Handling + +hiroz-py raises a small exception hierarchy (all importable from `hiroz_py`): + +```text +HirozError (base — catch this to cover everything) +├── TimeoutError (a blocking call timed out) +├── SerializationError (CDR/msgpack encode/decode failure) +└── TypeMismatchError (type hash / type mismatch) +``` + +```python +import hiroz_py +try: + resp = client.call(req, timeout=2.0) +except hiroz_py.TimeoutError: + ... # the server was present but slow +except hiroz_py.HirozError as e: + ... # any other call failure (e.g. no server responded) +``` + +Notes: + +- `hiroz_py.TimeoutError` is **not** Python's builtin `TimeoutError`; it subclasses `HirozError`. Update any `except RuntimeError:` blocks migrated from older hiroz-py code to `except hiroz_py.HirozError:`. +- A service call with **no server present at all** fails fast with a plain `HirozError` (not a timeout) — guard with `wait_for_service()` first. Timeout classification requires a server that matched but did not respond in time. +- `recv(...)`, `get_result(...)`, and `recv_goal(...)` return **`None`** on timeout rather than raising — that is their documented contract, even for the action client's `get_result`, which does not raise `TimeoutError` the way the service client's `call` does. + +## What's Not There Yet + +These are **core** feature gaps, not binding omissions — they are unimplemented in hiroz itself: + +| Feature | Status | Workaround | +|---|---|---| +| Timers (`create_timer`) | not implemented | `time.sleep` in your own loop / a `threading.Timer` | +| Parameters (`declare_parameter`, parameter server) | not implemented | plain Python config / env vars | +| Lifecycle nodes | not implemented | manage state yourself | +| Logging (`get_logger()` / rosout) | not implemented | Python `logging` or `print` | +| Sim time / clock | not implemented | `time.time()` | +| Executors / `spin()` | by design | pull (`recv`) or `callback=` | +| Action ROS 2 interop | Python-to-Python only | use typed Rust actions for `rmw_zenoh_cpp` interop | + +Pub/sub and services **do** interoperate with standard ROS 2 nodes through the Zenoh RMW. + +## Migration Checklist + +Mechanical steps to port an rclpy node: + +1. **Context**: replace `rclpy.init()` / `Node(...)` / `rclpy.shutdown()` with + `ctx = hiroz_py.ZContextBuilder().with_connect_endpoints(["tcp/127.0.0.1:7447"]).build()` + and `node = ctx.create_node("name").build()`. +2. **Imports**: `from std_msgs.msg import String` → `from hiroz_py import std_msgs` and use `std_msgs.String`. Same for `srv`/`action` packages. +3. **Flip pub/sub arg order**: `create_publisher(T, topic, qos)` → `create_publisher(topic, T, qos=qos)`. Easiest safe edit: pass by keyword — `create_publisher(topic=..., msg_type=..., qos=...)`. (If you leave the rclpy order, you get a clear `TypeError` telling you they look swapped.) +4. **Rename calls** (or rely on aliases): `create_subscription` and `create_service` both exist as aliases; `create_client` is the same name. Action: `ActionClient(node, A, name)` → `node.create_action_client(name, A)`. +5. **Services**: pass the grouping class (`pkg.Srv`) instead of `pkg.Srv.Request` where you can. For servers, either keep a `callback=` (rclpy-style, but the callback **returns** the response rather than mutating an out-param) or switch to the pull loop. +6. **Service client calls**: `call_async()` + `spin_until_future_complete()` → blocking `client.call(req, timeout=...)`. Add `client.wait_for_service(timeout=...)` before the first call. +7. **Remove `rclpy.spin(node)`**: replace with your own loop. For queue subscribers, loop on `sub.recv(timeout=...)`. For callback subscribers/servers, the work happens on internal threads — just keep the process alive (e.g. `while True: time.sleep(1)` or block on an `Event`). +8. **QoS**: `qos_profile=10` → `qos=10`; `QoSProfile(reliability=ReliabilityPolicy.BEST_EFFORT, depth=5)` → `hiroz_py.QosProfile(reliability=hiroz_py.ReliabilityPolicy.BEST_EFFORT, depth=5)`. +9. **Exceptions**: change `except RuntimeError:` around service/action calls to `except hiroz_py.HirozError:` (or `hiroz_py.TimeoutError` specifically). +10. **Drop sleeps used for discovery**: replace `time.sleep(1.0)` before first publish/call with `pub.wait_for_subscription(...)`, `client.wait_for_service(...)`, or `action_client.wait_for_server(...)`. +11. **Audit unsupported features**: remove or replace timers, parameters, logging, lifecycle (see [What's Not There Yet](#whats-not-there-yet)). + +A useful first sweep (review each hit by hand — these are starting points, not blind rewrites): + +```bash +grep -rn "rclpy.spin\|create_timer\|declare_parameter\|get_logger\|call_async\|spin_until_future_complete" your_pkg/ +``` diff --git a/docs/bindings/python.md b/docs/bindings/python.md index 62d3de748..9b0defb88 100644 --- a/docs/bindings/python.md +++ b/docs/bindings/python.md @@ -122,6 +122,104 @@ Here's a complete publisher and subscriber example from [`crates/hiroz-py/exampl | **Client** | Sends service requests | `node.create_client(service, type)` | | **Server** | Handles service requests | `node.create_server(service, type)` | +!!! tip + `create_subscription` and `create_service` are aliases for `create_subscriber` and `create_server` respectively, for readers coming from `rclpy`. Both forms are equivalent — pick whichever reads more naturally for your team. + +## rclpy Alignment + +hiroz-py's API is close to `rclpy` by design, with a few ergonomic additions that remove common migration friction: + +### Waiting for Discovery + +Instead of a fixed `time.sleep(...)` before the first call, poll the discovery graph directly: + +```python +client = node.create_client("/add_two_ints", AddTwoInts) +if not client.wait_for_service(timeout=5.0): + raise RuntimeError("service never appeared") + +pub = node.create_publisher("/chatter", std_msgs.String) +pub.wait_for_subscription(count=1, timeout=5.0) + +action_client = node.create_action_client("/navigate", NavigateToPose) +action_client.wait_for_server(timeout=5.0) +``` + +All three return `True` once the match is found, `False` if `timeout` elapses first. + +### Swapped-Argument Detection + +`create_publisher`/`create_subscriber` raise a clear `TypeError` if called in the historical `rclpy` argument order (`(msg_type, topic)` instead of hiroz's `(topic, msg_type)`): + +```python +node.create_publisher(std_msgs.String, "/chatter") +# TypeError: arguments appear swapped: expected (topic: str, msg_type), got (msg_type, topic) +``` + +Use keyword arguments to sidestep ordering entirely: `node.create_publisher(topic="/chatter", msg_type=std_msgs.String)`. + +### Grouped Request/Response and Goal/Result/Feedback Types + +Generated service and action types include an rclpy-style grouping class alongside the individual message classes: + +```python +from hiroz_py import example_interfaces + +client = node.create_client("/add_two_ints", example_interfaces.AddTwoInts) +req = example_interfaces.AddTwoInts.Request(a=1, b=2) +resp = client.call(req, timeout=5.0) +``` + +`AddTwoInts.Request` / `AddTwoInts.Response` are the same classes as the standalone `AddTwoIntsRequest` / `AddTwoIntsResponse` — the grouping class is just a namespacing convenience. The same pattern applies to actions: `Fibonacci.Goal`, `Fibonacci.Result`, `Fibonacci.Feedback`. Both `create_client`/`create_server` and `create_action_client`/`create_action_server` accept either the grouping class or the bare per-message classes. + +### Exceptions + +Blocking calls that can time out raise `hiroz_py.TimeoutError` (a subclass of `hiroz_py.HirozError`) rather than a bare `RuntimeError`, so timeout handling can be caught specifically: + +```python +try: + resp = client.call(req, timeout=1.0) +except hiroz_py.TimeoutError: + print("no response within 1s") +except hiroz_py.HirozError as e: + print("call failed:", e) +``` + +!!! note + This applies to `ZClient.call`. `ActionGoalHandle.get_result(timeout=...)` keeps its original contract of returning `None` on timeout rather than raising — check the return value when using actions with a timeout. + +### Push-Mode (Callback) Servers + +`create_server`/`create_service` accept an optional `callback` to run a background dispatch thread, instead of the pull-mode `take_request()` loop: + +```python +def handle_add(req): + return AddTwoInts.Response(sum=req.a + req.b) + +server = node.create_server("/add_two_ints", AddTwoInts, callback=handle_add) +``` + +If the callback raises, the exception is caught, logged to stderr, and recorded on `server.last_error` (a string, or `None` if no error has occurred): + +```python +if server.last_error is not None: + print("callback failed:", server.last_error) +``` + +### QoS Shorthand + +`QosProfile` fields accept the `rclpy`-style policy enums (`ReliabilityPolicy`, `DurabilityPolicy`, `HistoryPolicy`, `LivelinessPolicy`), and `qos=` parameters on `create_publisher`/`create_subscriber` accept a plain `int` as shorthand for `QosProfile(depth=)`: + +```python +pub = node.create_publisher("/chatter", std_msgs.String, qos=10) # depth=10 shorthand + +qos = hiroz_py.QosProfile( + reliability=hiroz_py.ReliabilityPolicy.BEST_EFFORT, + history=hiroz_py.HistoryPolicy.KEEP_LAST, + depth=5, +) +pub = node.create_publisher("/chatter", std_msgs.String, qos=qos) +``` ## Service Patterns @@ -140,7 +238,7 @@ Examples from [`crates/hiroz-py/examples/service_demo.py`](https://github.com/Ze ``` !!! tip - Service servers use a pull model: `take_request()` blocks until a request arrives. This gives you explicit control over when to process requests. + Service servers use a pull model by default: `take_request()` blocks until a request arrives, giving you explicit control over when to process requests. Pass `callback=` to `create_server` for a push-mode server instead — see [Push-Mode (Callback) Servers](#push-mode-callback-servers) above. ## Action Patterns @@ -188,6 +286,7 @@ Each must have a `__msgtype__` class attribute: #### Client Lifecycle +0. `client.wait_for_server(timeout)` — poll discovery until a matching action server appears (see [Waiting for Discovery](#waiting-for-discovery)) 1. `client.send_goal(goal)` → `ActionGoalHandle` — blocks until accepted (raises on rejection) 2. `handle.recv_feedback(timeout)` — receive next feedback; returns `None` when channel closes 3. `handle.get_result(timeout)` — block until terminal state; returns `None` on timeout @@ -367,6 +466,7 @@ cargo test --features python-interop -p hiroz-tests --test python_interop -- --t ## Resources +- **[Migrating from rclpy](./python-migration.md)** - Cheatsheet, API mapping, and checklist for porting rclpy nodes - **[Code Generation Internals](./python-codegen.md)** - How hiroz generates Python bindings - **[Pub/Sub](../core-concepts/pubsub.md)** - Deep dive into pub-sub patterns - **[Services](../core-concepts/services.md)** - Request-response communication diff --git a/mkdocs.yml b/mkdocs.yml index 75a1855a8..52e1d0fda 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -113,6 +113,7 @@ nav: - Python: - Quick Start: bindings/python-quick-start.md - Bindings: bindings/python.md + - Migrating from rclpy: bindings/python-migration.md - Codegen Internals: bindings/python-codegen.md - Go: - Quick Start: bindings/go-quick-start.md From 18c01c96d63f898f042e57d7916b736ae0d1b634 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Sun, 5 Jul 2026 13:47:40 +0800 Subject: [PATCH 05/18] fix(hiroz-py): raise TimeoutError from get_result instead of returning None ActionGoalHandle.get_result(timeout=...) silently returned None on timeout, unlike ZClient.call which raises hiroz_py.TimeoutError for the same condition. Align the two so timeout handling is consistent across services and actions. --- crates/hiroz-py/src/action.rs | 22 ++++++++++--------- crates/hiroz-py/tests/test_action.py | 10 ++++----- crates/hiroz-py/tests/test_rclpy_alignment.py | 12 +++++----- docs/bindings/python-migration.md | 4 ++-- docs/bindings/python.md | 4 ++-- 5 files changed, 26 insertions(+), 26 deletions(-) diff --git a/crates/hiroz-py/src/action.rs b/crates/hiroz-py/src/action.rs index 2c3bf202e..2b89200cd 100644 --- a/crates/hiroz-py/src/action.rs +++ b/crates/hiroz-py/src/action.rs @@ -337,40 +337,42 @@ impl PyZClientGoalHandle { /// Wait for and return the final result, optionally with a timeout (seconds). /// - /// Consumes the goal handle internally. Returns None on timeout. + /// Consumes the goal handle internally. Raises `hiroz_py.TimeoutError` on + /// timeout (mirrors `ZClient.call`'s timeout semantics — see P5). /// Raises RuntimeError if called more than once. #[pyo3(signature = (timeout=None))] - fn get_result(&self, py: Python, timeout: Option) -> PyResult> { + fn get_result(&self, py: Python, timeout: Option) -> PyResult { let handle = self.handle.lock().unwrap().take().ok_or_else(|| { pyo3::exceptions::PyRuntimeError::new_err("Result already retrieved") })?; let rt = get_tokio_rt(); - let result = py.allow_threads(move || { + let bytes = py.allow_threads(move || { rt.block_on(async move { if let Some(t) = timeout.map(Duration::from_secs_f64) { // Use the core `result_with_timeout` primitive rather than // reinventing the timeout wrapper in the binding. match handle.result_with_timeout(t).await { - Ok(msg) => Ok(Some(msg.0)), - Err(e) if hiroz::error::is_timeout(&*e) => Ok(None), // timeout + Ok(msg) => Ok(msg.0), + Err(e) if hiroz::error::is_timeout(&*e) => Err( + crate::error::TimeoutError::new_err(format!( + "Action result not received within {t:?}" + )), + ), Err(e) => Err(pyo3::exceptions::PyRuntimeError::new_err(e.to_string())), } } else { handle .result() .await - .map(|msg| Some(msg.0)) + .map(|msg| msg.0) .map_err(|e| pyo3::exceptions::PyRuntimeError::new_err(e.to_string())) } }) })?; - match result { - Some(bytes) => Ok(Some(msgspec_decode(py, &bytes, &self.result_type)?)), - None => Ok(None), - } + msgspec_decode(py, &bytes, &self.result_type) } /// Request cancellation of this goal. diff --git a/crates/hiroz-py/tests/test_action.py b/crates/hiroz-py/tests/test_action.py index d05e765c6..e47832790 100644 --- a/crates/hiroz-py/tests/test_action.py +++ b/crates/hiroz-py/tests/test_action.py @@ -185,18 +185,18 @@ def test_goal_rejection(action_context): def test_goal_timeout_no_server(action_context): - """get_result returns None when no server is present and timeout expires.""" + """send_goal raises when no server is present; get_result raises TimeoutError + on timeout if a goal handle is ever obtained.""" node_c = action_context.create_node("timeout_client").build() client = node_c.create_action_client( "/nonexistent_action", CountGoal, CountResult, CountFeedback ) with pytest.raises(Exception): - # send_goal should raise (or the goal handle's get_result should time out) + # send_goal should raise (or the goal handle's get_result should raise + # hiroz_py.TimeoutError). handle = client.send_goal(CountGoal(target=1)) - result = handle.get_result(timeout=1.0) - # If send_goal doesn't raise, get_result should return None - assert result is None + handle.get_result(timeout=1.0) def test_server_abort(action_context): diff --git a/crates/hiroz-py/tests/test_rclpy_alignment.py b/crates/hiroz-py/tests/test_rclpy_alignment.py index e72671dc8..9dd5210ec 100644 --- a/crates/hiroz-py/tests/test_rclpy_alignment.py +++ b/crates/hiroz-py/tests/test_rclpy_alignment.py @@ -227,12 +227,9 @@ def _never_respond(): client.call(req, timeout=0.5) -def test_p5_action_result_timeout_returns_none(ctx): - """P5 gap: unlike the service `call()` path, `get_result(timeout=...)` - does not raise `hiroz_py.TimeoutError` on timeout -- it returns `None` - (see action.rs's `get_result` docstring). Documented here so the - behavioral difference is pinned by a test rather than silently assumed. - """ +def test_p5_action_result_timeout_raises_timeout_error(ctx): + """get_result(timeout=...) now raises hiroz_py.TimeoutError, matching + ZClient.call's timeout semantics (previously it returned None).""" node = ctx.create_node("p5_action_client").build() client = node.create_action_client("/p5_never_completes", CountTo) server = node.create_action_server("/p5_never_completes", CountTo) @@ -247,7 +244,8 @@ def _never_finish(): assert client.wait_for_server(timeout=5.0) handle = client.send_goal(CountGoal(target=1)) - assert handle.get_result(timeout=0.5) is None + with pytest.raises(hiroz_py.TimeoutError): + handle.get_result(timeout=0.5) # --- P1 + P4 + P6: end-to-end service with wait_for_service and callback mode --- diff --git a/docs/bindings/python-migration.md b/docs/bindings/python-migration.md index 507b677fa..8c5916abf 100644 --- a/docs/bindings/python-migration.md +++ b/docs/bindings/python-migration.md @@ -131,7 +131,7 @@ if not ac.wait_for_server(timeout=5.0): handle = ac.send_goal(Fibonacci.Goal(order=10)) # blocks until accepted while (fb := handle.recv_feedback(timeout=0.5)) is not None: print(fb) -result = handle.get_result(timeout=10.0) # None on timeout +result = handle.get_result(timeout=10.0) # raises hiroz_py.TimeoutError on timeout ``` If you don't have a generated grouping class, pass the three classes positionally (back-compat): @@ -278,7 +278,7 @@ Notes: - `hiroz_py.TimeoutError` is **not** Python's builtin `TimeoutError`; it subclasses `HirozError`. Update any `except RuntimeError:` blocks migrated from older hiroz-py code to `except hiroz_py.HirozError:`. - A service call with **no server present at all** fails fast with a plain `HirozError` (not a timeout) — guard with `wait_for_service()` first. Timeout classification requires a server that matched but did not respond in time. -- `recv(...)`, `get_result(...)`, and `recv_goal(...)` return **`None`** on timeout rather than raising — that is their documented contract, even for the action client's `get_result`, which does not raise `TimeoutError` the way the service client's `call` does. +- `recv(...)` and `recv_goal(...)` return **`None`** on timeout rather than raising — that is their documented contract. `get_result(...)` is the exception: it raises `hiroz_py.TimeoutError` on timeout, matching `ZClient.call`. ## What's Not There Yet diff --git a/docs/bindings/python.md b/docs/bindings/python.md index 9b0defb88..2680f9cfd 100644 --- a/docs/bindings/python.md +++ b/docs/bindings/python.md @@ -186,7 +186,7 @@ except hiroz_py.HirozError as e: ``` !!! note - This applies to `ZClient.call`. `ActionGoalHandle.get_result(timeout=...)` keeps its original contract of returning `None` on timeout rather than raising — check the return value when using actions with a timeout. + `ActionGoalHandle.get_result(timeout=...)` also raises `hiroz_py.TimeoutError` on timeout, matching `ZClient.call`. ### Push-Mode (Callback) Servers @@ -289,7 +289,7 @@ Each must have a `__msgtype__` class attribute: 0. `client.wait_for_server(timeout)` — poll discovery until a matching action server appears (see [Waiting for Discovery](#waiting-for-discovery)) 1. `client.send_goal(goal)` → `ActionGoalHandle` — blocks until accepted (raises on rejection) 2. `handle.recv_feedback(timeout)` — receive next feedback; returns `None` when channel closes -3. `handle.get_result(timeout)` — block until terminal state; returns `None` on timeout +3. `handle.get_result(timeout)` — block until terminal state; raises `hiroz_py.TimeoutError` on timeout 4. `handle.cancel()` — request cancellation (the server decides when to honour it) ### Goal Status From cfde179f4ebe3fb54319a63d172189c24a13d636 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 28 Jul 2026 14:02:36 +0800 Subject: [PATCH 06/18] fix(hiroz-py): route action errors through map_call_error send_goal and get_result hand-rolled timeout classification and mapped non-timeout failures to PyRuntimeError, while ZClient.call used the shared map_call_error helper and raised HirozError. Same operation shape, two different exception hierarchies. Route both action paths through map_call_error so the documented P5 contract (timeout -> TimeoutError, else -> HirozError) holds uniformly. --- crates/hiroz-py/src/action.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/hiroz-py/src/action.rs b/crates/hiroz-py/src/action.rs index 2b89200cd..e1c41fa3b 100644 --- a/crates/hiroz-py/src/action.rs +++ b/crates/hiroz-py/src/action.rs @@ -180,7 +180,7 @@ impl PyZActionClient { // send_goal is async — release GIL while blocking. // Apply a timeout slightly above the Zenoh querier timeout (10 s) so that - // callers get a clear RuntimeError when no server is present instead of + // callers get a clear TimeoutError when no server is present instead of // blocking forever (the shared flume channel keeps the receiver alive even // after the Zenoh query expires and its error is discarded). let mut handle: RawClientGoalHandle = py.allow_threads(move || { @@ -192,7 +192,7 @@ impl PyZActionClient { "send_goal timed out: no action server responded", ) })? - .map_err(|e| pyo3::exceptions::PyRuntimeError::new_err(e.to_string())) + .map_err(crate::error::map_call_error) }) })?; @@ -360,14 +360,14 @@ impl PyZClientGoalHandle { "Action result not received within {t:?}" )), ), - Err(e) => Err(pyo3::exceptions::PyRuntimeError::new_err(e.to_string())), + Err(e) => Err(crate::error::map_call_error(e)), } } else { handle .result() .await .map(|msg| msg.0) - .map_err(|e| pyo3::exceptions::PyRuntimeError::new_err(e.to_string())) + .map_err(crate::error::map_call_error) } }) })?; From 2fe4621b4daf47e4e7a97822e1297b4e3dfffc86 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 28 Jul 2026 14:12:28 +0800 Subject: [PATCH 07/18] fix(hiroz-py): fail loudly on bad service names and type hashes Two silent-degradation paths added alongside the P1/P4 work: qualify_service_name reimplemented the core helper and swallowed qualification errors with a fallback to the raw name. A malformed name then made wait_for_service/wait_for_server poll the graph for a name that can never appear, which presents as a hang. Delegate to hiroz::topic_name::qualify_service_name and propagate as ValueError. The service grouping-class path fell back to TypeHash::zero() when __hash__ was missing or unparseable, silently building a client that cannot match a typed server. The legacy Request-class path already raises here; match it. --- crates/hiroz-py/src/node.rs | 43 ++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/crates/hiroz-py/src/node.rs b/crates/hiroz-py/src/node.rs index ea802866d..144bad843 100644 --- a/crates/hiroz-py/src/node.rs +++ b/crates/hiroz-py/src/node.rs @@ -130,12 +130,27 @@ fn extract_service_type_info(srv_type: &Bound<'_, PyAny>) -> PyResult<(String, T "Service grouping class with __srvtype__ must define a Request member", ) })?; - let type_hash = request_cls + // Be as strict as the legacy Request-class path: a bad hash means the + // client silently fails to match a typed server, so fail at construction + // rather than building a zero-hash client. + let type_hash_str: String = request_cls .getattr("__hash__") - .ok() - .and_then(|v| v.extract::().ok()) - .and_then(|s| TypeHash::from_rihs_string(&s)) - .unwrap_or_else(TypeHash::zero); + .map_err(|_| { + pyo3::exceptions::PyTypeError::new_err( + "Service grouping class Request member must have a __hash__ class attribute", + ) + })? + .extract() + .map_err(|_| { + pyo3::exceptions::PyTypeError::new_err( + "Service grouping class Request member __hash__ must be a string", + ) + })?; + let type_hash = TypeHash::from_rihs_string(&type_hash_str).ok_or_else(|| { + pyo3::exceptions::PyValueError::new_err(format!( + "Invalid type hash format: {type_hash_str}" + )) + })?; let rust_type_name = python_type_to_rust_type(&srv_type_str); return Ok((srv_type_str, TypeInfo::new(&rust_type_name, type_hash))); } @@ -380,7 +395,7 @@ impl PyZNode { .create_client_impl::(&service, Some(type_info)); let zclient = client_builder.build().map_err(|e| e.into_pyerr())?; let wrapper = GenericClientWrapper::new(zclient); - let qualified = self.qualify_service_name(&service); + let qualified = self.qualify_service_name(&service)?; Ok(PyZClient::new( Box::new(wrapper), srv_type_str, @@ -473,7 +488,7 @@ impl PyZNode { // wait_for_server polls the graph for it. let send_goal_service = format!( "{}/_action/send_goal", - self.qualify_service_name(&action_name) + self.qualify_service_name(&action_name)? ); Ok(PyZActionClient::new( @@ -599,8 +614,16 @@ impl PyZNode { impl PyZNode { /// Qualify a service name against the node's namespace/name so the result /// matches the entries the discovery graph stores. Absolute names pass through. - fn qualify_service_name(&self, service: &str) -> String { - hiroz::topic_name::qualify_topic_name(service, self.inner.namespace(), self.inner.name()) - .unwrap_or_else(|_| service.to_string()) + /// + /// Errors propagate rather than falling back to the raw name: a silently + /// unqualified name makes `wait_for_service` / `wait_for_server` poll the + /// graph for a name that can never appear, which looks like a hang. + fn qualify_service_name(&self, service: &str) -> PyResult { + hiroz::topic_name::qualify_service_name(service, self.inner.namespace(), self.inner.name()) + .map_err(|e| { + pyo3::exceptions::PyValueError::new_err(format!( + "Invalid service name '{service}': {e}" + )) + }) } } From b084999a538d51c3b560e927efb907e167e8b119 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 28 Jul 2026 14:15:21 +0800 Subject: [PATCH 08/18] fix(hiroz-py): make map_call_error accept zenoh errors, fix get_result stub The service path yields anyhow::Error but the action path yields zenoh::Error, so routing actions through map_call_error did not compile. Take anything convertible into a boxed error and render the source chain by hand, since boxing loses anyhow's {:#} chain formatting. Also correct the get_result stub: it raises TimeoutError now rather than returning None, so the return type is no longer Optional. --- crates/hiroz-py/python/hiroz_py/__init__.pyi | 2 +- crates/hiroz-py/src/error.rs | 39 ++++++++++++++------ 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/crates/hiroz-py/python/hiroz_py/__init__.pyi b/crates/hiroz-py/python/hiroz_py/__init__.pyi index e7bfb5d1b..eb7bd7fb2 100644 --- a/crates/hiroz-py/python/hiroz_py/__init__.pyi +++ b/crates/hiroz-py/python/hiroz_py/__init__.pyi @@ -312,7 +312,7 @@ class ActionGoalHandle: def status(self) -> int: ... def recv_feedback(self, timeout: float | None = None) -> Any | None: ... def try_recv_feedback(self) -> Any | None: ... - def get_result(self, timeout: float | None = None) -> Any | None: ... + def get_result(self, timeout: float | None = None) -> Any: ... def cancel(self) -> None: ... # --------------------------------------------------------------------------- diff --git a/crates/hiroz-py/src/error.rs b/crates/hiroz-py/src/error.rs index 9203bb556..68b6b449f 100644 --- a/crates/hiroz-py/src/error.rs +++ b/crates/hiroz-py/src/error.rs @@ -8,26 +8,41 @@ pyo3::create_exception!(hiroz_py, TimeoutError, HirozError); pyo3::create_exception!(hiroz_py, SerializationError, HirozError); pyo3::create_exception!(hiroz_py, TypeMismatchError, HirozError); -/// Returns true if `e` is (or wraps) a core [`hiroz::error::Error::Timeout`]. +/// Render an error and its full source chain as `outer: inner: root`. /// -/// Centralized here so every call site (service `call`, action `send_goal`, …) -/// classifies identically. Delegates to the core's structured detector, which -/// walks the whole source chain — do not string-match on the message. -pub(crate) fn is_timeout_error(e: &anyhow::Error) -> bool { - hiroz::error::is_timeout(&**e) +/// Matches anyhow's `{:#}` output, which we lose once the error is boxed. +fn format_chain(err: &(dyn std::error::Error + 'static)) -> String { + let mut msg = err.to_string(); + let mut source = err.source(); + while let Some(e) = source { + msg.push_str(&format!(": {e}")); + source = e.source(); + } + msg } /// Map a core error to the right Python exception. /// /// Timeout-shaped errors become `hiroz_py.TimeoutError`; everything else /// becomes `hiroz_py.HirozError`. Use this for blocking calls that raise on -/// failure (e.g. `ZClient.call`). Methods whose documented contract is to -/// return `None` on timeout should keep doing so rather than calling this. -pub(crate) fn map_call_error(e: anyhow::Error) -> PyErr { - if is_timeout_error(&e) { - TimeoutError::new_err(format!("{:#}", e)) +/// failure (e.g. `ZClient.call`, `send_goal`, `get_result`). Methods whose +/// documented contract is to return `None` on timeout should keep doing so +/// rather than calling this. +/// +/// Generic over the error type because the service path yields `anyhow::Error` +/// while the action path yields `zenoh::Error`; both classify identically via +/// the core's structured detector, which walks the whole source chain — do not +/// string-match on the message. +pub(crate) fn map_call_error(e: E) -> PyErr +where + E: Into>, +{ + let err = e.into(); + let msg = format_chain(&*err); + if hiroz::error::is_timeout(&*err) { + TimeoutError::new_err(msg) } else { - HirozError::new_err(format!("{:#}", e)) + HirozError::new_err(msg) } } From 6e5b144793ba8a98ab85b8f0ea136308be441329 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 28 Jul 2026 14:19:27 +0800 Subject: [PATCH 09/18] style(hiroz-py): rustfmt the get_result timeout arm --- crates/hiroz-py/src/action.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/hiroz-py/src/action.rs b/crates/hiroz-py/src/action.rs index e1c41fa3b..174d3f313 100644 --- a/crates/hiroz-py/src/action.rs +++ b/crates/hiroz-py/src/action.rs @@ -355,11 +355,11 @@ impl PyZClientGoalHandle { // reinventing the timeout wrapper in the binding. match handle.result_with_timeout(t).await { Ok(msg) => Ok(msg.0), - Err(e) if hiroz::error::is_timeout(&*e) => Err( - crate::error::TimeoutError::new_err(format!( + Err(e) if hiroz::error::is_timeout(&*e) => { + Err(crate::error::TimeoutError::new_err(format!( "Action result not received within {t:?}" - )), - ), + ))) + } Err(e) => Err(crate::error::map_call_error(e)), } } else { From 06a64f0fc67b2d218a18726e9104cf831e3146c7 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 28 Jul 2026 14:33:31 +0800 Subject: [PATCH 10/18] fix(hiroz-py): stop boxing anyhow errors before timeout classification Converting anyhow::Error into Box wraps the value, so is_timeout's downcast no longer sees the real error and every service timeout was misreported as a plain HirozError -- caught by test_p5_call_timeout_raises_timeout_error. Split into map_call_error (anyhow, derefs) and map_zenoh_error (the action paths' Box), sharing the classify/format helpers. --- crates/hiroz-py/src/action.rs | 6 ++--- crates/hiroz-py/src/error.rs | 45 +++++++++++++++++++---------------- 2 files changed, 28 insertions(+), 23 deletions(-) diff --git a/crates/hiroz-py/src/action.rs b/crates/hiroz-py/src/action.rs index 174d3f313..7d79b60bd 100644 --- a/crates/hiroz-py/src/action.rs +++ b/crates/hiroz-py/src/action.rs @@ -192,7 +192,7 @@ impl PyZActionClient { "send_goal timed out: no action server responded", ) })? - .map_err(crate::error::map_call_error) + .map_err(crate::error::map_zenoh_error) }) })?; @@ -360,14 +360,14 @@ impl PyZClientGoalHandle { "Action result not received within {t:?}" ))) } - Err(e) => Err(crate::error::map_call_error(e)), + Err(e) => Err(crate::error::map_zenoh_error(e)), } } else { handle .result() .await .map(|msg| msg.0) - .map_err(crate::error::map_call_error) + .map_err(crate::error::map_zenoh_error) } }) })?; diff --git a/crates/hiroz-py/src/error.rs b/crates/hiroz-py/src/error.rs index 68b6b449f..03322eb8a 100644 --- a/crates/hiroz-py/src/error.rs +++ b/crates/hiroz-py/src/error.rs @@ -10,7 +10,7 @@ pyo3::create_exception!(hiroz_py, TypeMismatchError, HirozError); /// Render an error and its full source chain as `outer: inner: root`. /// -/// Matches anyhow's `{:#}` output, which we lose once the error is boxed. +/// Matches anyhow's `{:#}` output, which a bare `Box` does not give us. fn format_chain(err: &(dyn std::error::Error + 'static)) -> String { let mut msg = err.to_string(); let mut source = err.source(); @@ -21,31 +21,36 @@ fn format_chain(err: &(dyn std::error::Error + 'static)) -> String { msg } -/// Map a core error to the right Python exception. -/// -/// Timeout-shaped errors become `hiroz_py.TimeoutError`; everything else -/// becomes `hiroz_py.HirozError`. Use this for blocking calls that raise on -/// failure (e.g. `ZClient.call`, `send_goal`, `get_result`). Methods whose -/// documented contract is to return `None` on timeout should keep doing so -/// rather than calling this. -/// -/// Generic over the error type because the service path yields `anyhow::Error` -/// while the action path yields `zenoh::Error`; both classify identically via -/// the core's structured detector, which walks the whole source chain — do not -/// string-match on the message. -pub(crate) fn map_call_error(e: E) -> PyErr -where - E: Into>, -{ - let err = e.into(); - let msg = format_chain(&*err); - if hiroz::error::is_timeout(&*err) { +fn classify(is_timeout: bool, msg: String) -> PyErr { + if is_timeout { TimeoutError::new_err(msg) } else { HirozError::new_err(msg) } } +/// Map an `anyhow` error to the right Python exception. +/// +/// Timeout-shaped errors become `hiroz_py.TimeoutError`; everything else +/// becomes `hiroz_py.HirozError`. Use this for blocking calls that raise on +/// failure (e.g. `ZClient.call`). Methods whose documented contract is to +/// return `None` on timeout should keep doing so rather than calling this. +/// +/// Classification goes through the core's structured detector, which walks the +/// whole source chain — do not string-match on the message. +pub(crate) fn map_call_error(e: anyhow::Error) -> PyErr { + // Deref rather than boxing: `Box::from(anyhow::Error)` wraps the + // value so `is_timeout`'s downcast no longer sees the real error and every + // timeout is misreported as a plain HirozError. + classify(hiroz::error::is_timeout(&*e), format!("{e:#}")) +} + +/// Same mapping for the action paths, which yield `zenoh::Error` +/// (`Box`) rather than `anyhow::Error`. +pub(crate) fn map_zenoh_error(e: zenoh::Error) -> PyErr { + classify(hiroz::error::is_timeout(&*e), format_chain(&*e)) +} + /// Trait for converting Rust errors to Python exceptions pub(crate) trait IntoPyErr { fn into_pyerr(self) -> PyErr; From b1df46009d2ac95a6c3d7bda42f2e368b0d1577b Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 28 Jul 2026 16:31:39 +0800 Subject: [PATCH 11/18] feat(hiroz-py): anchor exceptions under RuntimeError and builtins.TimeoutError P5 introduced a typed hierarchy rooted at Exception, which silently broke two catch styles ported rclpy code relies on: except RuntimeError: # what these paths raised before P5 except TimeoutError: # what rclpy's Client.call actually raises Both kept compiling and running while no longer catching. HirozError now derives from RuntimeError, and TimeoutError from both HirozError and the builtin TimeoutError, so either clause keeps working while the typed hierarchy stays available for code that wants it. create_exception! takes a single base, so TimeoutError is built with type(name, bases, dict) at module init and cached for raising. --- crates/hiroz-py/python/hiroz_py/__init__.pyi | 8 ++- crates/hiroz-py/src/action.rs | 12 ++-- crates/hiroz-py/src/error.rs | 67 +++++++++++++++++-- crates/hiroz-py/src/lib.rs | 7 +- crates/hiroz-py/tests/test_rclpy_alignment.py | 44 ++++++++++++ docs/bindings/python-migration.md | 16 +++-- docs/bindings/python.md | 2 +- 7 files changed, 132 insertions(+), 24 deletions(-) diff --git a/crates/hiroz-py/python/hiroz_py/__init__.pyi b/crates/hiroz-py/python/hiroz_py/__init__.pyi index eb7bd7fb2..347d9d0b0 100644 --- a/crates/hiroz-py/python/hiroz_py/__init__.pyi +++ b/crates/hiroz-py/python/hiroz_py/__init__.pyi @@ -2,6 +2,7 @@ from __future__ import annotations +import builtins from typing import Any, Final # Re-export message types from hiroz_msgs_py.types @@ -31,8 +32,11 @@ except ImportError: # Exceptions # --------------------------------------------------------------------------- -class HirozError(Exception): ... -class TimeoutError(HirozError): ... +class HirozError(RuntimeError): ... + +# Also subclasses the builtin TimeoutError, which is what rclpy's Client.call +# raises -- so `except TimeoutError:` in ported code keeps catching. +class TimeoutError(HirozError, builtins.TimeoutError): ... class SerializationError(HirozError): ... class TypeMismatchError(HirozError): ... diff --git a/crates/hiroz-py/src/action.rs b/crates/hiroz-py/src/action.rs index 7d79b60bd..35bd33350 100644 --- a/crates/hiroz-py/src/action.rs +++ b/crates/hiroz-py/src/action.rs @@ -188,8 +188,8 @@ impl PyZActionClient { tokio::time::timeout(Duration::from_secs(11), client.send_goal(goal_msg)) .await .map_err(|_| { - crate::error::TimeoutError::new_err( - "send_goal timed out: no action server responded", + crate::error::timeout_err( + "send_goal timed out: no action server responded".to_string(), ) })? .map_err(crate::error::map_zenoh_error) @@ -355,11 +355,9 @@ impl PyZClientGoalHandle { // reinventing the timeout wrapper in the binding. match handle.result_with_timeout(t).await { Ok(msg) => Ok(msg.0), - Err(e) if hiroz::error::is_timeout(&*e) => { - Err(crate::error::TimeoutError::new_err(format!( - "Action result not received within {t:?}" - ))) - } + Err(e) if hiroz::error::is_timeout(&*e) => Err(crate::error::timeout_err( + format!("Action result not received within {t:?}"), + )), Err(e) => Err(crate::error::map_zenoh_error(e)), } } else { diff --git a/crates/hiroz-py/src/error.rs b/crates/hiroz-py/src/error.rs index 03322eb8a..c57523f6b 100644 --- a/crates/hiroz-py/src/error.rs +++ b/crates/hiroz-py/src/error.rs @@ -1,13 +1,72 @@ #![allow(unexpected_cfgs)] use pyo3::prelude::*; +use pyo3::sync::GILOnceCell; +use pyo3::types::{PyDict, PyTuple, PyType}; -// Custom exception types -pyo3::create_exception!(hiroz_py, HirozError, pyo3::exceptions::PyException); -pyo3::create_exception!(hiroz_py, TimeoutError, HirozError); +// Custom exception types. +// +// `HirozError` derives from `RuntimeError` rather than `Exception` so that +// rclpy code ported to hiroz-py keeps working: the blocking call paths used to +// raise a bare `RuntimeError`, and `except RuntimeError:` is what an rclpy user +// writes today. +pyo3::create_exception!(hiroz_py, HirozError, pyo3::exceptions::PyRuntimeError); pyo3::create_exception!(hiroz_py, SerializationError, HirozError); pyo3::create_exception!(hiroz_py, TypeMismatchError, HirozError); +/// `hiroz_py.TimeoutError`, built at module init. +/// +/// It needs *two* bases — `HirozError` so `except hiroz_py.HirozError:` catches +/// timeouts, and `builtins.TimeoutError` because that is what rclpy's +/// `Client.call` actually raises, so a ported `except TimeoutError:` keeps +/// catching. `create_exception!` only accepts a single base, so the type is +/// constructed with `type(name, bases, dict)` instead. +/// +/// Inheriting `builtins.TimeoutError` also makes these instances `OSError`s, +/// since that is its base. That matches rclpy's behaviour exactly. +static TIMEOUT_ERROR: GILOnceCell> = GILOnceCell::new(); + +/// Build the `TimeoutError` type. Called once from module init. +pub(crate) fn init_timeout_error(py: Python<'_>) -> PyResult> { + let bases = PyTuple::new_bound( + py, + [ + py.get_type_bound::().into_any(), + py.get_type_bound::() + .into_any(), + ], + ); + let dict = PyDict::new_bound(py); + dict.set_item("__module__", "hiroz_py")?; + dict.set_item( + "__doc__", + "Raised when a hiroz-py operation exceeds its timeout.\n\n\ + Subclasses both hiroz_py.HirozError and the builtin TimeoutError.", + )?; + + let cls = py + .get_type_bound::() + .call1(("TimeoutError", bases, dict))? + .downcast_into::()?; + + let cls: Py = cls.unbind(); + TIMEOUT_ERROR.set(py, cls.clone_ref(py)).ok(); + Ok(cls) +} + +/// Construct a `hiroz_py.TimeoutError` carrying `msg`. +pub(crate) fn timeout_err(msg: String) -> PyErr { + Python::with_gil(|py| match TIMEOUT_ERROR.get(py) { + Some(cls) => match cls.bind(py).call1((msg.clone(),)) { + Ok(instance) => PyErr::from_value_bound(instance), + // Falling back keeps the error visible rather than masking the + // original failure behind a construction error. + Err(e) => e, + }, + None => HirozError::new_err(msg), + }) +} + /// Render an error and its full source chain as `outer: inner: root`. /// /// Matches anyhow's `{:#}` output, which a bare `Box` does not give us. @@ -23,7 +82,7 @@ fn format_chain(err: &(dyn std::error::Error + 'static)) -> String { fn classify(is_timeout: bool, msg: String) -> PyErr { if is_timeout { - TimeoutError::new_err(msg) + timeout_err(msg) } else { HirozError::new_err(msg) } diff --git a/crates/hiroz-py/src/lib.rs b/crates/hiroz-py/src/lib.rs index 0ca4774f4..e2dab4a5b 100644 --- a/crates/hiroz-py/src/lib.rs +++ b/crates/hiroz-py/src/lib.rs @@ -37,10 +37,9 @@ fn list_registered_types() -> Vec { fn _native(m: &Bound<'_, PyModule>) -> PyResult<()> { // Register custom exceptions m.add("HirozError", m.py().get_type_bound::())?; - m.add( - "TimeoutError", - m.py().get_type_bound::(), - )?; + // TimeoutError has two bases, so it is built at runtime rather than by + // `create_exception!` — see error.rs. + m.add("TimeoutError", error::init_timeout_error(m.py())?)?; m.add( "SerializationError", m.py().get_type_bound::(), diff --git a/crates/hiroz-py/tests/test_rclpy_alignment.py b/crates/hiroz-py/tests/test_rclpy_alignment.py index 9dd5210ec..31bc5f96d 100644 --- a/crates/hiroz-py/tests/test_rclpy_alignment.py +++ b/crates/hiroz-py/tests/test_rclpy_alignment.py @@ -197,6 +197,50 @@ def test_p5_exception_hierarchy(): assert issubclass(hiroz_py.TypeMismatchError, hiroz_py.HirozError) +def test_p5_hiroz_error_is_runtime_error(): + """Ported rclpy code catching RuntimeError must keep working. + + The blocking call paths raised a bare RuntimeError before P5; anchoring + HirozError under it keeps every existing `except RuntimeError:` live. + """ + assert issubclass(hiroz_py.HirozError, RuntimeError) + + +def test_p5_timeout_error_is_builtin_timeout_error(): + """rclpy's Client.call raises the *builtin* TimeoutError, not a ROS type. + + Without this base a ported `except TimeoutError:` silently stops catching + -- it still compiles and runs, so the failure is invisible. + """ + assert issubclass(hiroz_py.TimeoutError, TimeoutError) + # builtins.TimeoutError derives from OSError, so instances are OSErrors + # too. That is inherited from the builtin and matches rclpy. + assert issubclass(hiroz_py.TimeoutError, OSError) + + +def test_p5_timeout_caught_by_every_documented_except_clause(ctx): + """One raised timeout must satisfy all four documented catch styles.""" + node = ctx.create_node("p5_multi_catch").build() + server = node.create_server("/p5_multi_catch_svc", example_interfaces.AddTwoInts) + assert server is not None + + threading.Thread(target=server.take_request, daemon=True).start() + + client = node.create_client("/p5_multi_catch_svc", example_interfaces.AddTwoInts) + assert client.wait_for_service(timeout=5.0) + + req = example_interfaces.AddTwoInts.Request(a=1, b=2) + try: + client.call(req, timeout=0.5) + raise AssertionError("expected a timeout") + except Exception as exc: + assert isinstance(exc, hiroz_py.TimeoutError) + assert isinstance(exc, hiroz_py.HirozError) + assert isinstance(exc, TimeoutError) # builtin -- the rclpy contract + assert isinstance(exc, RuntimeError) # pre-P5 contract + assert "timed out" in str(exc) + + def test_p5_call_failure_is_hiroz_error(ctx): node = ctx.create_node("p5_client").build() client = node.create_client("/p5_nonexistent", example_interfaces.AddTwoInts) diff --git a/docs/bindings/python-migration.md b/docs/bindings/python-migration.md index 8c5916abf..364d5a442 100644 --- a/docs/bindings/python-migration.md +++ b/docs/bindings/python-migration.md @@ -258,12 +258,16 @@ Plain strings (`reliability="best_effort"`) and dicts still work. hiroz-py raises a small exception hierarchy (all importable from `hiroz_py`): ```text -HirozError (base — catch this to cover everything) -├── TimeoutError (a blocking call timed out) -├── SerializationError (CDR/msgpack encode/decode failure) -└── TypeMismatchError (type hash / type mismatch) +RuntimeError (builtin) +└── HirozError (base — catch this to cover everything) + ├── TimeoutError (a blocking call timed out) + │ └── also inherits builtins.TimeoutError + ├── SerializationError (CDR/msgpack encode/decode failure) + └── TypeMismatchError (type hash / type mismatch) ``` +The two extra bases exist so that ported code keeps working unchanged: `HirozError` inherits `RuntimeError` because that is what these paths raised before the typed hierarchy existed, and `TimeoutError` additionally inherits the **builtin** `TimeoutError` because that is what rclpy's `Client.call` raises. + ```python import hiroz_py try: @@ -276,7 +280,7 @@ except hiroz_py.HirozError as e: Notes: -- `hiroz_py.TimeoutError` is **not** Python's builtin `TimeoutError`; it subclasses `HirozError`. Update any `except RuntimeError:` blocks migrated from older hiroz-py code to `except hiroz_py.HirozError:`. +- `hiroz_py.TimeoutError` **is** catchable as Python's builtin `TimeoutError`, as well as `hiroz_py.HirozError` and `RuntimeError`. An `except TimeoutError:` block ported straight from rclpy keeps working. Because the builtin derives from `OSError`, these instances are `OSError`s too — the same as in rclpy. - A service call with **no server present at all** fails fast with a plain `HirozError` (not a timeout) — guard with `wait_for_service()` first. Timeout classification requires a server that matched but did not respond in time. - `recv(...)` and `recv_goal(...)` return **`None`** on timeout rather than raising — that is their documented contract. `get_result(...)` is the exception: it raises `hiroz_py.TimeoutError` on timeout, matching `ZClient.call`. @@ -310,7 +314,7 @@ Mechanical steps to port an rclpy node: 6. **Service client calls**: `call_async()` + `spin_until_future_complete()` → blocking `client.call(req, timeout=...)`. Add `client.wait_for_service(timeout=...)` before the first call. 7. **Remove `rclpy.spin(node)`**: replace with your own loop. For queue subscribers, loop on `sub.recv(timeout=...)`. For callback subscribers/servers, the work happens on internal threads — just keep the process alive (e.g. `while True: time.sleep(1)` or block on an `Event`). 8. **QoS**: `qos_profile=10` → `qos=10`; `QoSProfile(reliability=ReliabilityPolicy.BEST_EFFORT, depth=5)` → `hiroz_py.QosProfile(reliability=hiroz_py.ReliabilityPolicy.BEST_EFFORT, depth=5)`. -9. **Exceptions**: change `except RuntimeError:` around service/action calls to `except hiroz_py.HirozError:` (or `hiroz_py.TimeoutError` specifically). +9. **Exceptions**: nothing to change — `except RuntimeError:` and `except TimeoutError:` both still catch, by design. Tighten to `except hiroz_py.HirozError:` / `except hiroz_py.TimeoutError:` when you want to catch hiroz failures specifically rather than any runtime error. 10. **Drop sleeps used for discovery**: replace `time.sleep(1.0)` before first publish/call with `pub.wait_for_subscription(...)`, `client.wait_for_service(...)`, or `action_client.wait_for_server(...)`. 11. **Audit unsupported features**: remove or replace timers, parameters, logging, lifecycle (see [What's Not There Yet](#whats-not-there-yet)). diff --git a/docs/bindings/python.md b/docs/bindings/python.md index 2680f9cfd..7c63222a6 100644 --- a/docs/bindings/python.md +++ b/docs/bindings/python.md @@ -174,7 +174,7 @@ resp = client.call(req, timeout=5.0) ### Exceptions -Blocking calls that can time out raise `hiroz_py.TimeoutError` (a subclass of `hiroz_py.HirozError`) rather than a bare `RuntimeError`, so timeout handling can be caught specifically: +Blocking calls that can time out raise `hiroz_py.TimeoutError`, so timeout handling can be caught specifically. It subclasses `hiroz_py.HirozError` (itself a `RuntimeError`) and the builtin `TimeoutError`, so `except RuntimeError:` and `except TimeoutError:` both keep catching in code ported from rclpy: ```python try: From 2a83aa791139c8ef3d6d0209112df1d8aed2a862 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 28 Jul 2026 16:52:50 +0800 Subject: [PATCH 12/18] fix(hiroz-py): address review findings on validation, leaks and docs - Timeout args are unrestricted floats, so -1/NaN/inf reached Duration::from_secs_f64 and panicked. Route every public timeout through checked_timeout, raising ValueError. - The documented ValueError for malformed service/action names never fired: build() validates the same name first and maps failure to HirozError. Resolve before building in create_client/create_server/ create_action_client. - Discovery names skipped remapping, which core applies before qualification -- with a remap rule the entity was created under the new name while wait_for_* polled the old one and timed out. - Callback-mode servers leaked one pending reply per failed request; only send_response removed entries. Added discard_pending and wired it into every error path. - Docs: last_error is read-and-clear, so the example printed None; the migration example discarded the server it told you to keep; the core-gaps table listed parameters, lifecycle and clock as core gaps when they exist in core and merely lack Python exposure. --- crates/hiroz-py/src/action.rs | 13 ++-- crates/hiroz-py/src/graph.rs | 26 +++++++- crates/hiroz-py/src/node.rs | 60 ++++++++++++------ crates/hiroz-py/src/pubsub.rs | 18 +++--- crates/hiroz-py/src/service.rs | 15 +++-- crates/hiroz-py/src/traits.rs | 15 +++++ crates/hiroz-py/tests/test_rclpy_alignment.py | 62 +++++++++++++++++++ docs/bindings/python-migration.md | 22 ++++--- docs/bindings/python.md | 9 ++- 9 files changed, 190 insertions(+), 50 deletions(-) diff --git a/crates/hiroz-py/src/action.rs b/crates/hiroz-py/src/action.rs index 35bd33350..f5826f9d0 100644 --- a/crates/hiroz-py/src/action.rs +++ b/crates/hiroz-py/src/action.rs @@ -260,10 +260,11 @@ impl PyZActionClient { /// Args: /// timeout: Maximum seconds to wait. None waits forever. #[pyo3(signature = (timeout=None))] - fn wait_for_server(&self, py: Python, timeout: Option) -> bool { - py.allow_threads(|| { + fn wait_for_server(&self, py: Python, timeout: Option) -> PyResult { + let timeout = crate::graph::checked_timeout(timeout)?; + Ok(py.allow_threads(|| { crate::graph::wait_for_service_server(&self.graph, &self.send_goal_service, timeout) - }) + })) } /// Get the goal type class (for debugging). @@ -315,7 +316,7 @@ impl PyZClientGoalHandle { fn recv_feedback(&self, py: Python, timeout: Option) -> PyResult> { let rx = self.flume_feedback_rx.clone(); let bytes_opt = py.allow_threads(move || { - if let Some(t) = timeout.map(Duration::from_secs_f64) { + if let Some(t) = crate::graph::checked_timeout(timeout)? { rx.recv_timeout(t).ok().map(|m| m.0) } else { rx.recv().ok().map(|m| m.0) @@ -350,7 +351,7 @@ impl PyZClientGoalHandle { let rt = get_tokio_rt(); let bytes = py.allow_threads(move || { rt.block_on(async move { - if let Some(t) = timeout.map(Duration::from_secs_f64) { + if let Some(t) = crate::graph::checked_timeout(timeout)? { // Use the core `result_with_timeout` primitive rather than // reinventing the timeout wrapper in the binding. match handle.result_with_timeout(t).await { @@ -442,7 +443,7 @@ impl PyZActionServer { let handle_opt = py.allow_threads(move || { rt.block_on(async move { - if let Some(t) = timeout.map(Duration::from_secs_f64) { + if let Some(t) = crate::graph::checked_timeout(timeout)? { match tokio::time::timeout(t, inner.recv_goal()).await { Ok(Ok(h)) => Ok(Some(h)), Ok(Err(e)) => Err(pyo3::exceptions::PyRuntimeError::new_err(e.to_string())), diff --git a/crates/hiroz-py/src/graph.rs b/crates/hiroz-py/src/graph.rs index 52069b473..9858f9aed 100644 --- a/crates/hiroz-py/src/graph.rs +++ b/crates/hiroz-py/src/graph.rs @@ -9,6 +9,28 @@ use std::time::{Duration, Instant}; /// internally for its wait-for-service spin. const POLL_INTERVAL: Duration = Duration::from_millis(50); +/// Convert a Python `float` timeout into a `Duration`, rejecting values +/// `Duration::from_secs_f64` would panic on. +/// +/// The timeout arguments are public API taking an unrestricted `float`, so +/// `timeout=-1`, `float("nan")` and `float("inf")` all reach us. Without this +/// they surface as a Rust panic rather than an ordinary Python error. +pub(crate) fn checked_timeout(timeout: Option) -> pyo3::PyResult> { + match timeout { + None => Ok(None), + Some(t) if t.is_nan() => Err(pyo3::exceptions::PyValueError::new_err( + "timeout must be a number, got NaN", + )), + Some(t) if t.is_infinite() => Err(pyo3::exceptions::PyValueError::new_err( + "timeout must be finite; pass None to wait forever", + )), + Some(t) if t < 0.0 => Err(pyo3::exceptions::PyValueError::new_err(format!( + "timeout must be non-negative, got {t}" + ))), + Some(t) => Ok(Some(Duration::from_secs_f64(t))), + } +} + /// Block until at least one service server matching `service_name` is visible /// in the graph, or `timeout` (seconds) elapses. `None` waits forever. /// @@ -17,9 +39,9 @@ const POLL_INTERVAL: Duration = Duration::from_millis(50); pub(crate) fn wait_for_service_server( graph: &Arc, service_name: &str, - timeout: Option, + timeout: Option, ) -> bool { - let deadline = timeout.map(|t| Instant::now() + Duration::from_secs_f64(t)); + let deadline = timeout.map(|t| Instant::now() + t); loop { if graph.count(EndpointKind::Service, service_name) > 0 { return true; diff --git a/crates/hiroz-py/src/node.rs b/crates/hiroz-py/src/node.rs index 144bad843..bbdbd88d7 100644 --- a/crates/hiroz-py/src/node.rs +++ b/crates/hiroz-py/src/node.rs @@ -390,12 +390,16 @@ impl PyZNode { fn create_client(&self, service: String, srv_type: &Bound<'_, PyAny>) -> PyResult { let (srv_type_str, type_info) = extract_service_type_info(srv_type)?; + // Resolve before building: `build()` performs the same qualification and + // maps failure to HirozError, so a malformed name would never reach a + // check placed after it and the documented ValueError never fires. + let qualified = self.resolve_service_name(&service)?; + let client_builder = self .inner .create_client_impl::(&service, Some(type_info)); let zclient = client_builder.build().map_err(|e| e.into_pyerr())?; let wrapper = GenericClientWrapper::new(zclient); - let qualified = self.qualify_service_name(&service)?; Ok(PyZClient::new( Box::new(wrapper), srv_type_str, @@ -467,6 +471,15 @@ impl PyZNode { let node = Arc::clone(&self.inner); let rt = get_tokio_rt(); + // Resolve before building, for the same reason as `create_client`: the + // builder validates the name itself and maps failure to a RuntimeError. + // The action server advertises `/_action/send_goal`; that is what + // wait_for_server polls for. + let send_goal_service = format!( + "{}/_action/send_goal", + self.resolve_service_name(&action_name)? + ); + let client = py.allow_threads(|| { let _guard = rt.enter(); let mut builder = node.create_action_client::(&action_name); @@ -484,13 +497,6 @@ impl PyZNode { .map_err(|e| pyo3::exceptions::PyRuntimeError::new_err(e.to_string())) })?; - // The action server advertises a `/_action/send_goal` service; - // wait_for_server polls the graph for it. - let send_goal_service = format!( - "{}/_action/send_goal", - self.qualify_service_name(&action_name)? - ); - Ok(PyZActionClient::new( client, goal_obj.clone_ref(py), @@ -594,6 +600,11 @@ impl PyZNode { ) -> PyResult { let (srv_type_str, type_info) = extract_service_type_info(srv_type)?; + // Validate up front so a malformed name raises ValueError, matching the + // documented contract — `build()` would otherwise reject it first and + // surface a HirozError instead. + self.resolve_service_name(&service)?; + let server_builder = self .inner .create_service_impl::(&service, Some(type_info)); @@ -612,18 +623,29 @@ impl PyZNode { } impl PyZNode { - /// Qualify a service name against the node's namespace/name so the result - /// matches the entries the discovery graph stores. Absolute names pass through. + /// Resolve a service/action name to the form the discovery graph stores: + /// remap first, then qualify against the node's namespace/name. + /// + /// The remap step matters — the core builders apply remapping *before* + /// qualification (see `ZActionClientBuilder::build`). Skipping it here would + /// leave `wait_for_service` / `wait_for_server` polling the pre-remap name + /// while the entity is created under the post-remap one, so the wait times + /// out even though a server is present. /// /// Errors propagate rather than falling back to the raw name: a silently - /// unqualified name makes `wait_for_service` / `wait_for_server` poll the - /// graph for a name that can never appear, which looks like a hang. - fn qualify_service_name(&self, service: &str) -> PyResult { - hiroz::topic_name::qualify_service_name(service, self.inner.namespace(), self.inner.name()) - .map_err(|e| { - pyo3::exceptions::PyValueError::new_err(format!( - "Invalid service name '{service}': {e}" - )) - }) + /// unqualified name makes the waits poll for a name that can never appear, + /// which looks like a hang. + fn resolve_service_name(&self, service: &str) -> PyResult { + let remapped = self.inner.apply_remap(service); + hiroz::topic_name::qualify_service_name( + &remapped, + self.inner.namespace(), + self.inner.name(), + ) + .map_err(|e| { + pyo3::exceptions::PyValueError::new_err(format!( + "Invalid service name '{service}': {e}" + )) + }) } } diff --git a/crates/hiroz-py/src/pubsub.rs b/crates/hiroz-py/src/pubsub.rs index 41b54ed74..37a9fb681 100644 --- a/crates/hiroz-py/src/pubsub.rs +++ b/crates/hiroz-py/src/pubsub.rs @@ -50,13 +50,17 @@ impl PyZPublisher { /// count: Number of subscriptions to wait for (default 1). /// timeout: Maximum seconds to wait. None waits effectively forever. #[pyo3(signature = (count=1, timeout=None))] - fn wait_for_subscription(&self, py: Python, count: usize, timeout: Option) -> bool { + fn wait_for_subscription( + &self, + py: Python, + count: usize, + timeout: Option, + ) -> PyResult { // None → wait "forever"; cap at a large but finite duration so the // background thread can still observe interpreter shutdown. - let dur = timeout - .map(Duration::from_secs_f64) + let dur = crate::graph::checked_timeout(timeout)? .unwrap_or(Duration::from_secs(60 * 60 * 24 * 365)); - py.allow_threads(|| self.inner.wait_for_subscription(count, dur)) + Ok(py.allow_threads(|| self.inner.wait_for_subscription(count, dur))) } /// Get the topic name (for debugging) @@ -116,7 +120,7 @@ impl PyZSubscriber { #[pyo3(signature = (timeout=None))] unsafe fn recv(&self, py: Python, timeout: Option) -> PyResult> { let inner = self.require_queue()?; - let timeout_duration = timeout.map(Duration::from_secs_f64); + let timeout_duration = crate::graph::checked_timeout(timeout)?; // Release GIL while waiting to allow other Python threads to run let result = py.allow_threads(|| inner.recv_sample(timeout_duration)); @@ -181,7 +185,7 @@ impl PyZSubscriber { timeout: Option, ) -> PyResult>> { let inner = self.require_queue()?; - let timeout_duration = timeout.map(Duration::from_secs_f64); + let timeout_duration = crate::graph::checked_timeout(timeout)?; // Release GIL while waiting to allow other Python threads to run let result = py.allow_threads(|| inner.recv_serialized(timeout_duration)); @@ -231,7 +235,7 @@ impl PyZSubscriber { #[pyo3(signature = (timeout=None))] unsafe fn recv_raw_view(&self, py: Python, timeout: Option) -> PyResult> { let inner = self.require_queue()?; - let timeout_duration = timeout.map(Duration::from_secs_f64); + let timeout_duration = crate::graph::checked_timeout(timeout)?; // Release GIL while waiting to allow other Python threads to run let result = py.allow_threads(|| inner.recv_sample(timeout_duration)); diff --git a/crates/hiroz-py/src/service.rs b/crates/hiroz-py/src/service.rs index 53b3da924..f1d7084c1 100644 --- a/crates/hiroz-py/src/service.rs +++ b/crates/hiroz-py/src/service.rs @@ -49,7 +49,7 @@ impl PyZClient { timeout: Option, ) -> PyResult { let cdr_bytes = hiroz_msgs::serialize_to_cdr(&self.request_type_name, data.py(), data)?; - let timeout_duration = timeout.map(Duration::from_secs_f64); + let timeout_duration = crate::graph::checked_timeout(timeout)?; let cdr_bytes = py .allow_threads(|| self.inner.call_serialized(&cdr_bytes, timeout_duration)) @@ -66,10 +66,11 @@ impl PyZClient { /// Args: /// timeout: Maximum seconds to wait. None waits forever. #[pyo3(signature = (timeout=None))] - fn wait_for_service(&self, py: Python, timeout: Option) -> bool { - py.allow_threads(|| { + fn wait_for_service(&self, py: Python, timeout: Option) -> PyResult { + let timeout = crate::graph::checked_timeout(timeout)?; + Ok(py.allow_threads(|| { crate::graph::wait_for_service_server(&self.graph, &self.service_name, timeout) - }) + })) } /// Get the service type name (for debugging) @@ -208,6 +209,7 @@ fn spawn_callback_loop( Ok(o) => o, Err(e) => { record_error!(last_error, "request deserialize error", e); + server.discard_pending(&request_id); return; } }; @@ -215,6 +217,7 @@ fn spawn_callback_loop( Ok(o) => o, Err(e) => { record_error!(last_error, "service callback error", e); + server.discard_pending(&request_id); return; } }; @@ -226,11 +229,15 @@ fn spawn_callback_loop( Ok(b) => b, Err(e) => { record_error!(last_error, "response serialize error", e); + server.discard_pending(&request_id); return; } }; if let Err(e) = server.send_response_serialized(&resp_bytes, &request_id) { record_error!(last_error, "send_response error", e); + // send_response only removes the entry once the reply + // succeeds; drop it here so a failing send cannot leak. + server.discard_pending(&request_id); } }); } diff --git a/crates/hiroz-py/src/traits.rs b/crates/hiroz-py/src/traits.rs index 8e7568f3a..db905d0e8 100644 --- a/crates/hiroz-py/src/traits.rs +++ b/crates/hiroz-py/src/traits.rs @@ -119,6 +119,13 @@ pub(crate) trait RawServer: Send + Sync { /// Used by the optional callback-mode server loop. fn try_take_request_serialized(&self) -> Result)>>; fn send_response_serialized(&self, data: &[u8], request_id: &RequestId) -> Result<()>; + /// Drop a pending reply without answering it. + /// + /// `take_request` registers a reply handle that only `send_response` removes, + /// so any path that abandons a request (a failed deserialize, a raising + /// callback) must call this or the handle is retained for the life of the + /// server — an unbounded leak under a repeatedly-failing callback. + fn discard_pending(&self, request_id: &RequestId); } /// Generic client wrapper using RawBytesService @@ -235,4 +242,12 @@ impl RawServer for GenericServerWrapper { .reply_blocking(&response) .map_err(|e| anyhow::anyhow!("Failed to send response: {}", e)) } + + fn discard_pending(&self, request_id: &RequestId) { + // Best-effort: a poisoned lock here means the server is already broken, + // and this runs on error paths that must not mask the original failure. + if let Ok(mut pending) = self.pending.lock() { + pending.remove(request_id); + } + } } diff --git a/crates/hiroz-py/tests/test_rclpy_alignment.py b/crates/hiroz-py/tests/test_rclpy_alignment.py index 31bc5f96d..c9ea2528e 100644 --- a/crates/hiroz-py/tests/test_rclpy_alignment.py +++ b/crates/hiroz-py/tests/test_rclpy_alignment.py @@ -436,3 +436,65 @@ def test_p7_action_grouping_class_end_to_end(ctx): result = handle.get_result(timeout=5.0) assert result is not None assert result.final_count == 3 + + +# --- Review follow-ups: input validation and error contracts --- + + +@pytest.mark.parametrize("bad", [-1.0, float("nan"), float("inf")]) +def test_timeout_rejects_non_finite_and_negative(ctx, bad): + """Timeout args take an unrestricted float, so these reach Rust. + + Duration::from_secs_f64 panics on all three; they must surface as an + ordinary ValueError rather than a panic leaking through PyO3. + """ + node = ctx.create_node("timeout_validation").build() + client = node.create_client("/tv_svc", example_interfaces.AddTwoInts) + sub = node.create_subscriber("/tv_topic", std_msgs.String) + pub = node.create_publisher("/tv_topic", std_msgs.String) + + with pytest.raises(ValueError): + client.wait_for_service(timeout=bad) + with pytest.raises(ValueError): + sub.recv(timeout=bad) + with pytest.raises(ValueError): + pub.wait_for_subscription(timeout=bad) + + +def test_invalid_service_name_raises_value_error(ctx): + """Malformed names must fail at construction with ValueError. + + The builder validates the same name and maps failure to HirozError, so + this only holds if validation runs *before* build. + """ + node = ctx.create_node("name_validation").build() + with pytest.raises(ValueError): + node.create_client("//bad//name", example_interfaces.AddTwoInts) + with pytest.raises(ValueError): + node.create_server("//bad//name", example_interfaces.AddTwoInts) + + +def test_callback_server_survives_repeated_failures(ctx): + """A repeatedly-raising callback must not retain a pending reply each time. + + Every request registers a reply handle that only send_response removes; + the error paths have to discard it explicitly. After the failures, a + working server on a fresh name must still answer normally. + """ + node = ctx.create_node("pending_leak").build() + + def always_raises(_req): + raise ValueError("boom") + + server = node.create_service( + "/leak_svc", example_interfaces.AddTwoInts, callback=always_raises + ) + client = node.create_client("/leak_svc", example_interfaces.AddTwoInts) + assert client.wait_for_service(timeout=5.0) + + for _ in range(5): + with pytest.raises(hiroz_py.HirozError): + client.call(example_interfaces.AddTwoInts.Request(a=1, b=2), timeout=0.4) + + # The callback thread recorded the failures rather than dying. + assert server.last_error is not None diff --git a/docs/bindings/python-migration.md b/docs/bindings/python-migration.md index 364d5a442..908c8b0df 100644 --- a/docs/bindings/python-migration.md +++ b/docs/bindings/python-migration.md @@ -96,7 +96,11 @@ rclpy.spin(node) # hiroz-py (callback returns the response; no resp out-param) def handle(req): return example_interfaces.AddTwoInts.Response(sum=req.a + req.b) -node.create_service("/add_two_ints", example_interfaces.AddTwoInts, callback=handle) +# Keep the returned server alive: dropping it stops the worker and tears down +# the queryable. Binding it is what keeps the internal thread serving. +server = node.create_service( + "/add_two_ints", example_interfaces.AddTwoInts, callback=handle +) # server runs on an internal thread; keep the process alive (no spin needed) ``` @@ -286,17 +290,17 @@ Notes: ## What's Not There Yet -These are **core** feature gaps, not binding omissions — they are unimplemented in hiroz itself: +Unreachable from Python today. The **Status** column distinguishes two very different cases: some of these exist in hiroz core and merely lack a Python surface, while others are unimplemented in core as well. | Feature | Status | Workaround | |---|---|---| -| Timers (`create_timer`) | not implemented | `time.sleep` in your own loop / a `threading.Timer` | -| Parameters (`declare_parameter`, parameter server) | not implemented | plain Python config / env vars | -| Lifecycle nodes | not implemented | manage state yourself | -| Logging (`get_logger()` / rosout) | not implemented | Python `logging` or `print` | -| Sim time / clock | not implemented | `time.time()` | -| Executors / `spin()` | by design | pull (`recv`) or `callback=` | -| Action ROS 2 interop | Python-to-Python only | use typed Rust actions for `rmw_zenoh_cpp` interop | +| Parameters (`declare_parameter`, parameter server) | implemented in core (`ZNode`), **not exposed to Python** | plain Python config / env vars | +| Lifecycle nodes | implemented in core, **not exposed to Python** | manage state yourself | +| Sim time / clock | implemented in core (`ZClock`), **not exposed to Python** | `time.time()` | +| Timers (`create_timer`) | not implemented in core | `time.sleep` in your own loop / a `threading.Timer` | +| Logging (`get_logger()` / rosout) | not implemented in core | Python `logging` or `print` | +| Executors / `spin()` | by design — hiroz is reactive, with no spin loop | pull (`recv`) or `callback=` | +| Action ROS 2 interop | Python-to-Python only (msgpack wire format) | use typed Rust actions for `rmw_zenoh_cpp` interop | Pub/sub and services **do** interoperate with standard ROS 2 nodes through the Zenoh RMW. diff --git a/docs/bindings/python.md b/docs/bindings/python.md index 7c63222a6..118b7d49c 100644 --- a/docs/bindings/python.md +++ b/docs/bindings/python.md @@ -199,11 +199,14 @@ def handle_add(req): server = node.create_server("/add_two_ints", AddTwoInts, callback=handle_add) ``` -If the callback raises, the exception is caught, logged to stderr, and recorded on `server.last_error` (a string, or `None` if no error has occurred): +If the callback raises, the exception is caught, logged to stderr, and recorded on `server.last_error` (a string, or `None` if no error has occurred). + +Reading `last_error` **clears** it, so bind it once rather than reading the property twice: ```python -if server.last_error is not None: - print("callback failed:", server.last_error) +err = server.last_error # reading consumes it +if err is not None: + print("callback failed:", err) ``` ### QoS Shorthand From 771c309a4d37d2f674ab656c8defc31cc35067c5 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 28 Jul 2026 17:02:11 +0800 Subject: [PATCH 13/18] feat(hiroz-py): wire P7 into codegen; make callback-server shutdown safe P7 advertised Fibonacci.Goal/.Result/.Feedback grouping classes, but the Python generator took no actions parameter and hiroz-msgs/build.rs discarded the discovered actions, so no grouping class was ever emitted -- the P7 test only exercised a hand-written class. Thread resolved actions through generate_python_bindings, emit Goal/Result/Feedback structs with the action type hash plus the __actiontype__ grouping class, and cover packages that contribute only actions. Result and Feedback are optional in the .action format, so only existing members are emitted. Callback-mode servers joined their worker from Drop, which runs with the GIL held while the worker takes the GIL to call user code: a slow or blocking callback froze 'del server' and interpreter shutdown, with a deadlock window besides. Drop now only signals; close()/__exit__ do the join with the GIL released. --- .../src/python_msgspec_generator.rs | 147 ++++++++++++++---- crates/hiroz-msgs/build.rs | 12 +- crates/hiroz-py/python/hiroz_py/__init__.pyi | 8 + crates/hiroz-py/src/service.rs | 71 +++++++-- crates/hiroz-py/tests/test_rclpy_alignment.py | 79 ++++++++++ 5 files changed, 276 insertions(+), 41 deletions(-) diff --git a/crates/hiroz-codegen/src/python_msgspec_generator.rs b/crates/hiroz-codegen/src/python_msgspec_generator.rs index 22e79c46d..5acf0290b 100644 --- a/crates/hiroz-codegen/src/python_msgspec_generator.rs +++ b/crates/hiroz-codegen/src/python_msgspec_generator.rs @@ -3,11 +3,12 @@ //! This module generates both Python msgspec structs and complete Rust PyO3 modules //! from ROS message definitions, eliminating the need for manual registry code. -use crate::types::{ArrayType, FieldType, ResolvedMessage, ResolvedService}; +use crate::types::{ArrayType, FieldType, ResolvedAction, ResolvedMessage, ResolvedService}; use anyhow::Result; use proc_macro2::TokenStream; use quote::{format_ident, quote}; use std::collections::BTreeMap; +use std::collections::BTreeSet; use std::collections::HashMap; use std::fs; use std::path::Path; @@ -16,6 +17,7 @@ use std::path::Path; pub fn generate_python_bindings( messages: &[ResolvedMessage], services: &[ResolvedService], + actions: &[ResolvedAction], python_output_dir: &Path, rust_output_path: &Path, ) -> Result<()> { @@ -40,6 +42,40 @@ pub fn generate_python_bindings( .push(srv); } + // Group actions by package so we can emit rclpy-style grouping classes (P7). + let mut action_groups: HashMap> = HashMap::new(); + for action in actions { + action_groups + .entry(action.parsed.package.clone()) + .or_default() + .push(action); + } + + // Group action Goal/Result/Feedback by package, tracking the action type + // hash the same way service Request/Response track the service hash. + let mut action_messages: BTreeMap> = BTreeMap::new(); + let mut action_hashes: BTreeMap> = BTreeMap::new(); + for action in actions { + let action_hash = action.type_hash.to_rihs_string(); + for part in [ + Some(&action.goal), + action.result.as_ref(), + action.feedback.as_ref(), + ] + .into_iter() + .flatten() + { + action_messages + .entry(part.parsed.package.clone()) + .or_default() + .push(part); + action_hashes + .entry(part.parsed.package.clone()) + .or_default() + .insert(part.parsed.name.clone(), action_hash.clone()); + } + } + // Group service Request/Response by package, and track service type hashes let mut service_messages: BTreeMap> = BTreeMap::new(); let mut service_hashes: BTreeMap> = BTreeMap::new(); @@ -74,9 +110,20 @@ pub fn generate_python_bindings( srv_msgs.sort_by(|a, b| a.parsed.name.cmp(&b.parsed.name)); } - // Generate Python msgspec structs (one file per package) - for (package_name, package_msgs) in &packages { - // Combine regular messages with service Request/Response for this package + // One file per package, covering packages that contribute only services or + // only actions as well as those with plain messages. + let all_packages: BTreeSet = packages + .keys() + .chain(service_messages.keys()) + .chain(action_messages.keys()) + .cloned() + .collect(); + + for package_name in &all_packages { + let package_msgs = packages + .get(package_name) + .map(|v| v.as_slice()) + .unwrap_or(&[]); let srv_msgs = service_messages .get(package_name) .map(|v| v.as_slice()) @@ -89,42 +136,32 @@ pub fn generate_python_bindings( .get(package_name) .map(|v| v.as_slice()) .unwrap_or(&[]); + let act_msgs = action_messages + .get(package_name) + .map(|v| v.as_slice()) + .unwrap_or(&[]); + let act_hashes = action_hashes.get(package_name).cloned().unwrap_or_default(); + let act_groups = action_groups + .get(package_name) + .map(|v| v.as_slice()) + .unwrap_or(&[]); + let python_code = generate_python_package_with_services( package_name, package_msgs, srv_msgs, &svc_hashes, srv_groups, + act_msgs, + &act_hashes, + act_groups, )?; let output_path = python_output_dir.join(format!("{}.py", package_name)); fs::write(output_path, python_code)?; } - // Generate Python files for packages that only have service types - for (package_name, srv_msgs) in &service_messages { - if !packages.contains_key(package_name) { - let svc_hashes = service_hashes - .get(package_name) - .cloned() - .unwrap_or_default(); - let srv_groups = service_groups - .get(package_name) - .map(|v| v.as_slice()) - .unwrap_or(&[]); - let python_code = generate_python_package_with_services( - package_name, - &[], - srv_msgs, - &svc_hashes, - srv_groups, - )?; - let output_path = python_output_dir.join(format!("{}.py", package_name)); - fs::write(output_path, python_code)?; - } - } - // Generate __init__.py for Python package - let init_code = generate_python_init(&packages)?; + let init_code = generate_python_init(&all_packages)?; fs::write(python_output_dir.join("__init__.py"), init_code)?; // Generate COMPLETE Rust PyO3 module (replaces python_registry.rs entirely) @@ -142,12 +179,16 @@ fn tokens_to_string(tokens: TokenStream) -> String { } /// Generate Python msgspec structs for a package (messages + service Request/Response) +#[allow(clippy::too_many_arguments)] fn generate_python_package_with_services( package_name: &str, messages: &[&ResolvedMessage], service_messages: &[&ResolvedMessage], service_hashes: &BTreeMap, service_groups: &[&ResolvedService], + action_messages: &[&ResolvedMessage], + action_hashes: &BTreeMap, + action_groups: &[&ResolvedAction], ) -> Result { let mut code = format!( "\"\"\"Auto-generated ROS 2 message types for {}.\"\"\"\n\ @@ -167,12 +208,24 @@ fn generate_python_package_with_services( code.push_str(&generate_msgspec_struct(msg, svc_hash.map(|s| s.as_str()))?); } + // Generate action Goal/Result/Feedback structs with the action type hash + for msg in action_messages { + let act_hash = action_hashes.get(&msg.parsed.name); + code.push_str(&generate_msgspec_struct(msg, act_hash.map(|s| s.as_str()))?); + } + // Emit rclpy-style service grouping classes (P4). These reference the // Request/Response structs above, so they must come after them. for srv in service_groups { code.push_str(&generate_service_grouping_class(srv)); } + // Emit rclpy-style action grouping classes (P7), after the structs they + // reference. + for action in action_groups { + code.push_str(&generate_action_grouping_class(action)); + } + Ok(code) } @@ -194,6 +247,38 @@ fn generate_service_grouping_class(srv: &ResolvedService) -> String { ) } +/// Generate an action grouping class: `Fibonacci.Goal` / `.Result` / `.Feedback` (P7). +/// +/// Lets `create_action_client`/`create_action_server` accept a single +/// rclpy-style type instead of three separate structs. `Result` and `Feedback` +/// are optional in the `.action` format, so only emit the members that exist — +/// referencing an absent struct would produce a NameError on import. +fn generate_action_grouping_class(action: &ResolvedAction) -> String { + let action_name = &action.parsed.name; + let package = &action.parsed.package; + let mut code = format!( + "class {action_name}:\n \ + \"\"\"Action grouping type. Use {action_name}.Goal, .Result and .Feedback.\"\"\"\n \ + __actiontype__: ClassVar[str] = '{package}/action/{action_name}'\n \ + Goal: ClassVar[type] = {}\n", + action.goal.parsed.name + ); + if let Some(result) = &action.result { + code.push_str(&format!( + " Result: ClassVar[type] = {}\n", + result.parsed.name + )); + } + if let Some(feedback) = &action.feedback { + code.push_str(&format!( + " Feedback: ClassVar[type] = {}\n", + feedback.parsed.name + )); + } + code.push('\n'); + code +} + fn rust_to_python_type(field_type: &FieldType, current_package: &str) -> Result { // Get the base field type (without array indicators) let base_type = &field_type.base_type; @@ -752,17 +837,17 @@ fn generate_serialize_to_zbuf( } } -fn generate_python_init(packages: &BTreeMap>) -> Result { +fn generate_python_init(packages: &BTreeSet) -> Result { let mut code = "\"\"\"Auto-generated ROS 2 message types package.\"\"\"\n\n# Import all message types\n" .to_string(); - for package_name in packages.keys() { + for package_name in packages { code.push_str(&format!("from . import {}\n", package_name)); } code.push_str("\n__all__ = [\n"); - for package_name in packages.keys() { + for package_name in packages { code.push_str(&format!(" \"{}\",\n", package_name)); } code.push_str("]\n"); diff --git a/crates/hiroz-msgs/build.rs b/crates/hiroz-msgs/build.rs index 66d1e4158..781a640c9 100644 --- a/crates/hiroz-msgs/build.rs +++ b/crates/hiroz-msgs/build.rs @@ -78,7 +78,7 @@ fn main() -> Result<()> { #[cfg(feature = "python_registry")] { // Use hiroz_codegen's discovery and resolver to get resolved messages - let (messages, services, _actions) = + let (messages, services, actions) = hiroz_codegen::discovery::discover_all(&package_refs)?; // Filter out problematic messages @@ -120,10 +120,19 @@ fn main() -> Result<()> { }) .collect(); + let actions: Vec<_> = actions + .into_iter() + .filter(|act| { + let full_name = format!("{}/{}", act.package, act.name); + !full_name.starts_with("actionlib_msgs/") + }) + .collect(); + // Resolve dependencies using hiroz_codegen resolver let mut resolver = hiroz_codegen::resolver::Resolver::new(is_humble); let resolved_msgs = resolver.resolve_messages(messages)?; let resolved_srvs = resolver.resolve_services(services)?; + let resolved_actions = resolver.resolve_actions(actions)?; // Create Python output directory let python_output_dir = PathBuf::from("python/hiroz_msgs_py/types"); @@ -133,6 +142,7 @@ fn main() -> Result<()> { python_msgspec_generator::generate_python_bindings( &resolved_msgs, &resolved_srvs, + &resolved_actions, &python_output_dir, &out_dir.join("python_bindings.rs"), )?; diff --git a/crates/hiroz-py/python/hiroz_py/__init__.pyi b/crates/hiroz-py/python/hiroz_py/__init__.pyi index 347d9d0b0..25390565b 100644 --- a/crates/hiroz-py/python/hiroz_py/__init__.pyi +++ b/crates/hiroz-py/python/hiroz_py/__init__.pyi @@ -294,6 +294,14 @@ class ZServer: def get_type_name(self) -> str: ... @property def last_error(self) -> str | None: ... + def close(self) -> None: ... + def __enter__(self) -> ZServer: ... + def __exit__( + self, + exc_type: type[BaseException] | None = None, + exc_value: BaseException | None = None, + traceback: Any | None = None, + ) -> bool: ... # --------------------------------------------------------------------------- # ZActionClient diff --git a/crates/hiroz-py/src/service.rs b/crates/hiroz-py/src/service.rs index f1d7084c1..85d5fee6c 100644 --- a/crates/hiroz-py/src/service.rs +++ b/crates/hiroz-py/src/service.rs @@ -85,28 +85,53 @@ impl PyZClient { /// Background-thread state for a callback-mode server (P6). /// /// Holds an `Arc` to the underlying server (keeping its Zenoh queryable alive) -/// and a stop flag the worker thread checks each poll. Dropping this signals the -/// thread to stop and joins it. +/// and a stop flag the worker thread checks each poll. struct CallbackServerState { stop: Arc, handle: Option>, _server: Arc, } -impl Drop for CallbackServerState { - fn drop(&mut self) { +impl CallbackServerState { + /// Stop the worker and wait for it, with the GIL released. + /// + /// This is the blocking shutdown path. It must never run from `Drop` — see + /// the note there — so it is reachable only via `close()` / `__exit__`, + /// where we hold a `Python` token and can hand the GIL back to the worker + /// while it finishes its in-flight callback. + fn close(&mut self, py: Python<'_>) { self.stop.store(true, Ordering::Relaxed); if let Some(h) = self.handle.take() { - let _ = h.join(); + py.allow_threads(|| { + let _ = h.join(); + }); } } } +impl Drop for CallbackServerState { + fn drop(&mut self) { + // Signal, but never join here. + // + // Deallocation runs with the GIL held, and the worker acquires the GIL + // to invoke the user callback. Joining would therefore deadlock if the + // worker is waiting on the GIL, and even without that a slow callback + // would block `del server` and interpreter shutdown for as long as it + // runs. Detaching is safe: the worker owns an `Arc` on the server, so + // the queryable outlives us until the thread observes `stop` on its + // next poll (a few ms) and exits. + // + // Call `close()` — or use the server as a context manager — when you + // need to know the worker has actually stopped. + self.stop.store(true, Ordering::Relaxed); + } +} + /// Python wrapper for service server. /// /// Pull mode (default): `inner` is `Some`; the caller drives `take_request` / /// `send_response`. Callback mode (P6): `inner` is `None` and a background -/// thread (held in `_callback`) services requests via the user callback. +/// thread (held in `callback`) services requests via the user callback. /// Errors from the callback thread are stored in `last_error` and surfaced via /// the `last_error` Python property. #[pyclass(name = "ZServer")] @@ -114,7 +139,7 @@ pub struct PyZServer { inner: Option>>, request_type_name: String, response_type_name: String, - _callback: Option, + callback: Option, last_error: Arc>>, } @@ -126,7 +151,7 @@ impl PyZServer { inner: Some(Mutex::new(inner)), request_type_name, response_type_name, - _callback: None, + callback: None, last_error: Arc::new(Mutex::new(None)), } } @@ -156,7 +181,7 @@ impl PyZServer { inner: None, request_type_name, response_type_name, - _callback: Some(CallbackServerState { + callback: Some(CallbackServerState { stop, handle: Some(handle), _server: server, @@ -323,4 +348,32 @@ impl PyZServer { fn last_error(&self) -> Option { self.last_error.lock().ok().and_then(|mut g| g.take()) } + + /// Stop a callback-mode server and wait for its worker thread to finish. + /// + /// Dropping the server only *signals* the worker (joining during + /// deallocation could deadlock against the GIL), so call this — or use the + /// server as a context manager — when you need a guarantee that the + /// callback is no longer running. Idempotent, and a no-op in pull mode. + fn close(&mut self, py: Python<'_>) { + if let Some(state) = self.callback.as_mut() { + state.close(py); + } + } + + fn __enter__(slf: PyRef<'_, Self>) -> PyRef<'_, Self> { + slf + } + + #[pyo3(signature = (_exc_type=None, _exc_value=None, _traceback=None))] + fn __exit__( + &mut self, + py: Python<'_>, + _exc_type: Option<&Bound<'_, PyAny>>, + _exc_value: Option<&Bound<'_, PyAny>>, + _traceback: Option<&Bound<'_, PyAny>>, + ) -> bool { + self.close(py); + false + } } diff --git a/crates/hiroz-py/tests/test_rclpy_alignment.py b/crates/hiroz-py/tests/test_rclpy_alignment.py index c9ea2528e..17126b484 100644 --- a/crates/hiroz-py/tests/test_rclpy_alignment.py +++ b/crates/hiroz-py/tests/test_rclpy_alignment.py @@ -498,3 +498,82 @@ def always_raises(_req): # The callback thread recorded the failures rather than dying. assert server.last_error is not None + + +# --- P7 grouping classes come from codegen, not just hand-written types --- + + +def test_p7_generated_action_grouping_class_exists(): + """P7 must be wired into codegen, not only satisfiable by hand-written types. + + hiroz_msgs vendors action_msgs; if the generator emitted any action + grouping class it carries __actiontype__ plus a Goal member. + """ + from hiroz_msgs_py import types as msg_types + + found = [] + for pkg_name in getattr(msg_types, "__all__", []): + pkg = getattr(msg_types, pkg_name) + for attr in dir(pkg): + obj = getattr(pkg, attr) + if isinstance(obj, type) and hasattr(obj, "__actiontype__"): + found.append((pkg_name, attr, obj)) + + if not found: + pytest.skip("no .action files in the vendored packages for this distro") + + for pkg_name, attr, obj in found: + assert obj.__actiontype__ == f"{pkg_name}/action/{attr}" + assert hasattr(obj, "Goal"), f"{attr} grouping class must expose Goal" + + +# --- Callback-server shutdown must not block on the GIL --- + + +def test_callback_server_close_is_explicit_and_idempotent(ctx): + """close() joins the worker with the GIL released; Drop only signals. + + Joining from Drop would deadlock against a callback waiting on the GIL, + so the blocking path has to be reachable only from close()/__exit__. + """ + node = ctx.create_node("server_close").build() + + def handle(req): + return example_interfaces.AddTwoInts.Response(sum=req.a + req.b) + + server = node.create_service( + "/close_svc", example_interfaces.AddTwoInts, callback=handle + ) + client = node.create_client("/close_svc", example_interfaces.AddTwoInts) + assert client.wait_for_service(timeout=5.0) + assert ( + client.call(example_interfaces.AddTwoInts.Request(a=1, b=2), timeout=5.0).sum + == 3 + ) + + server.close() + server.close() # idempotent + + +def test_callback_server_context_manager(ctx): + node = ctx.create_node("server_ctx").build() + + def handle(req): + return example_interfaces.AddTwoInts.Response(sum=req.a + req.b) + + with node.create_service( + "/ctx_svc", example_interfaces.AddTwoInts, callback=handle + ) as server: + assert server is not None + client = node.create_client("/ctx_svc", example_interfaces.AddTwoInts) + assert client.wait_for_service(timeout=5.0) + resp = client.call( + example_interfaces.AddTwoInts.Request(a=20, b=22), timeout=5.0 + ) + assert resp.sum == 42 + + +def test_pull_mode_close_is_a_noop(ctx): + node = ctx.create_node("pull_close").build() + server = node.create_server("/pull_close_svc", example_interfaces.AddTwoInts) + server.close() From 866075a6da5f9fe57f737cf336186612e9e4215b Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 28 Jul 2026 17:04:21 +0800 Subject: [PATCH 14/18] fix(hiroz-py): hoist timeout validation out of the allow_threads closures recv_feedback's closure returns Option, so the ? on checked_timeout did not compile there. Validate before entering the closure in all three action paths, which also raises the ValueError before the GIL is released rather than part-way through the blocking call. --- crates/hiroz-py/src/action.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/crates/hiroz-py/src/action.rs b/crates/hiroz-py/src/action.rs index f5826f9d0..8f9ef48c8 100644 --- a/crates/hiroz-py/src/action.rs +++ b/crates/hiroz-py/src/action.rs @@ -315,8 +315,11 @@ impl PyZClientGoalHandle { #[pyo3(signature = (timeout=None))] fn recv_feedback(&self, py: Python, timeout: Option) -> PyResult> { let rx = self.flume_feedback_rx.clone(); + // Validate before entering the closure: it returns Option, so `?` on a + // PyResult is not available inside it. + let timeout = crate::graph::checked_timeout(timeout)?; let bytes_opt = py.allow_threads(move || { - if let Some(t) = crate::graph::checked_timeout(timeout)? { + if let Some(t) = timeout { rx.recv_timeout(t).ok().map(|m| m.0) } else { rx.recv().ok().map(|m| m.0) @@ -349,9 +352,10 @@ impl PyZClientGoalHandle { })?; let rt = get_tokio_rt(); + let timeout = crate::graph::checked_timeout(timeout)?; let bytes = py.allow_threads(move || { rt.block_on(async move { - if let Some(t) = crate::graph::checked_timeout(timeout)? { + if let Some(t) = timeout { // Use the core `result_with_timeout` primitive rather than // reinventing the timeout wrapper in the binding. match handle.result_with_timeout(t).await { @@ -440,10 +444,11 @@ impl PyZActionServer { ) -> PyResult> { let inner = Arc::clone(&self.inner); let rt = get_tokio_rt(); + let timeout = crate::graph::checked_timeout(timeout)?; let handle_opt = py.allow_threads(move || { rt.block_on(async move { - if let Some(t) = crate::graph::checked_timeout(timeout)? { + if let Some(t) = timeout { match tokio::time::timeout(t, inner.recv_goal()).await { Ok(Ok(h)) => Ok(Some(h)), Ok(Err(e)) => Err(pyo3::exceptions::PyRuntimeError::new_err(e.to_string())), From af148a8307463b0ba51dbe633952874e56e17a2b Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 28 Jul 2026 17:11:57 +0800 Subject: [PATCH 15/18] fix(hiroz-py): reject empty path components in service names qualify_topic_name skips empty components instead of rejecting them, so '//bad//name' passed ROS validation and failed later inside Zenoh's key-expression parser -- an error that cites a cargo registry path and never names the offending service. Check for empty chunks and trailing slashes in the binding so the documented ValueError actually covers them, and parametrise the test over all three rejection paths. --- crates/hiroz-py/src/node.rs | 13 +++++++++++ crates/hiroz-py/tests/test_rclpy_alignment.py | 22 ++++++++++++++----- 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/crates/hiroz-py/src/node.rs b/crates/hiroz-py/src/node.rs index bbdbd88d7..84dcfb2e5 100644 --- a/crates/hiroz-py/src/node.rs +++ b/crates/hiroz-py/src/node.rs @@ -637,6 +637,19 @@ impl PyZNode { /// which looks like a hang. fn resolve_service_name(&self, service: &str) -> PyResult { let remapped = self.inner.apply_remap(service); + + // `qualify_topic_name` skips empty path components rather than rejecting + // them, so `//bad//name` survives ROS validation and fails much later in + // Zenoh's key-expression parser — with an error that cites a cargo + // registry path and never mentions the service name. Reject it here so + // the caller gets the documented ValueError naming their own input. + if remapped.contains("//") || (remapped.len() > 1 && remapped.ends_with('/')) { + return Err(pyo3::exceptions::PyValueError::new_err(format!( + "Invalid service name '{service}': empty path components and trailing \ + slashes are not allowed" + ))); + } + hiroz::topic_name::qualify_service_name( &remapped, self.inner.namespace(), diff --git a/crates/hiroz-py/tests/test_rclpy_alignment.py b/crates/hiroz-py/tests/test_rclpy_alignment.py index 17126b484..94b7ee999 100644 --- a/crates/hiroz-py/tests/test_rclpy_alignment.py +++ b/crates/hiroz-py/tests/test_rclpy_alignment.py @@ -461,17 +461,29 @@ def test_timeout_rejects_non_finite_and_negative(ctx, bad): pub.wait_for_subscription(timeout=bad) -def test_invalid_service_name_raises_value_error(ctx): +@pytest.mark.parametrize( + "bad_name", + [ + "//bad//name", # empty chunks: ROS validation skips these, Zenoh rejects them + "/bad name", # space is not a valid topic component + "", # empty + ], +) +def test_invalid_service_name_raises_value_error(ctx, bad_name): """Malformed names must fail at construction with ValueError. - The builder validates the same name and maps failure to HirozError, so - this only holds if validation runs *before* build. + Two things are being pinned. First, validation has to run *before* build, + or the builder rejects the name first and surfaces HirozError instead. + Second, the check has to be stricter than ROS name validation alone -- + that skips empty path components, so `//bad//name` would otherwise reach + Zenoh's key-expression parser and fail with an opaque error citing a cargo + registry path. """ node = ctx.create_node("name_validation").build() with pytest.raises(ValueError): - node.create_client("//bad//name", example_interfaces.AddTwoInts) + node.create_client(bad_name, example_interfaces.AddTwoInts) with pytest.raises(ValueError): - node.create_server("//bad//name", example_interfaces.AddTwoInts) + node.create_server(bad_name, example_interfaces.AddTwoInts) def test_callback_server_survives_repeated_failures(ctx): From 9f72a831cc4ac2dd203a05182a7fccf64c739a44 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 28 Jul 2026 17:28:06 +0800 Subject: [PATCH 16/18] test(hiroz-py): correct the invalid-name cases to what hiroz actually rejects qualify_topic_name validates components only for relative and ~private names; absolute names are passed through unchecked, so '/bad name' is accepted. Swap that case for a relative one that core does validate, and record the absolute-name gap in the docstring rather than asserting behaviour that does not exist. --- crates/hiroz-py/tests/test_rclpy_alignment.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/hiroz-py/tests/test_rclpy_alignment.py b/crates/hiroz-py/tests/test_rclpy_alignment.py index 94b7ee999..f8d8547cc 100644 --- a/crates/hiroz-py/tests/test_rclpy_alignment.py +++ b/crates/hiroz-py/tests/test_rclpy_alignment.py @@ -465,7 +465,7 @@ def test_timeout_rejects_non_finite_and_negative(ctx, bad): "bad_name", [ "//bad//name", # empty chunks: ROS validation skips these, Zenoh rejects them - "/bad name", # space is not a valid topic component + "bad name/rel", # invalid component, relative -> core validates this path "", # empty ], ) @@ -478,6 +478,12 @@ def test_invalid_service_name_raises_value_error(ctx, bad_name): that skips empty path components, so `//bad//name` would otherwise reach Zenoh's key-expression parser and fail with an opaque error citing a cargo registry path. + + Note the deliberate gap: an *absolute* name with an invalid component + (`/bad name`) is NOT rejected. `qualify_topic_name` validates components + only for relative and `~private` names -- absolute names are passed + through unchecked. Tightening that is a core change affecting topics too, + so it is out of scope here. """ node = ctx.create_node("name_validation").build() with pytest.raises(ValueError): From e5a274ade1f82a06f04e8e2d994a90aa5763095e Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Tue, 28 Jul 2026 18:09:40 +0800 Subject: [PATCH 17/18] fix(hiroz-py): wait_for_server polls the full action-server predicate wait_for_server returned True as soon as send_goal was advertised, but a usable action server needs all five endpoints and discovery can surface them one at a time -- so the very next call could fail. Poll the core's has_action_server predicate against the qualified action name instead. Also mark SerializationError/TypeMismatchError as declared-but-unraised in the docs. Wiring them would re-wrap msgspec's TypeError/ValueError and break anyone catching those today, so the honest fix is to stop advertising a contract the code does not implement. --- crates/hiroz-py/src/action.rs | 11 ++++++----- crates/hiroz-py/src/graph.rs | 29 +++++++++++++++++++++++++++++ crates/hiroz-py/src/node.rs | 11 ++++------- docs/bindings/python-migration.md | 6 ++++-- 4 files changed, 43 insertions(+), 14 deletions(-) diff --git a/crates/hiroz-py/src/action.rs b/crates/hiroz-py/src/action.rs index 8f9ef48c8..0cf4f8ee0 100644 --- a/crates/hiroz-py/src/action.rs +++ b/crates/hiroz-py/src/action.rs @@ -139,9 +139,10 @@ pub struct PyZActionClient { goal_type: Py, result_type: Py, feedback_type: Py, - /// Shared graph + the action's `send_goal` service name, used by `wait_for_server`. + /// Shared graph + the qualified action name, used by `wait_for_server` to + /// poll the core's full five-endpoint action-server predicate. graph: Arc, - send_goal_service: String, + action_name: String, } impl PyZActionClient { @@ -152,7 +153,7 @@ impl PyZActionClient { result_type: Py, feedback_type: Py, graph: Arc, - send_goal_service: String, + action_name: String, ) -> Self { Self { inner: Arc::new(inner), @@ -160,7 +161,7 @@ impl PyZActionClient { result_type, feedback_type, graph, - send_goal_service, + action_name, } } } @@ -263,7 +264,7 @@ impl PyZActionClient { fn wait_for_server(&self, py: Python, timeout: Option) -> PyResult { let timeout = crate::graph::checked_timeout(timeout)?; Ok(py.allow_threads(|| { - crate::graph::wait_for_service_server(&self.graph, &self.send_goal_service, timeout) + crate::graph::wait_for_action_server(&self.graph, &self.action_name, timeout) })) } diff --git a/crates/hiroz-py/src/graph.rs b/crates/hiroz-py/src/graph.rs index 9858f9aed..69f965b8b 100644 --- a/crates/hiroz-py/src/graph.rs +++ b/crates/hiroz-py/src/graph.rs @@ -55,6 +55,35 @@ pub(crate) fn wait_for_service_server( } } +/// Block until a *complete* action server for `action_name` is visible, or +/// `timeout` elapses. `None` waits forever. +/// +/// Deliberately uses the core's `has_action_server` predicate rather than +/// polling the `send_goal` service alone: a server advertises five endpoints +/// and discovery can surface them one at a time, so waiting on `send_goal` +/// can return true while result, cancel, feedback and status are still +/// missing — and the very next call then fails. +/// +/// Must be called with the GIL released. +pub(crate) fn wait_for_action_server( + graph: &Arc, + action_name: &str, + timeout: Option, +) -> bool { + let deadline = timeout.map(|t| Instant::now() + t); + loop { + if graph.has_action_server(action_name) { + return true; + } + if let Some(d) = deadline + && Instant::now() >= d + { + return false; + } + std::thread::sleep(POLL_INTERVAL); + } +} + /// Python-accessible graph discovery methods. /// /// These are exposed as methods on PyZNode rather than a separate class, diff --git a/crates/hiroz-py/src/node.rs b/crates/hiroz-py/src/node.rs index 84dcfb2e5..801ab742f 100644 --- a/crates/hiroz-py/src/node.rs +++ b/crates/hiroz-py/src/node.rs @@ -473,12 +473,9 @@ impl PyZNode { // Resolve before building, for the same reason as `create_client`: the // builder validates the name itself and maps failure to a RuntimeError. - // The action server advertises `/_action/send_goal`; that is what - // wait_for_server polls for. - let send_goal_service = format!( - "{}/_action/send_goal", - self.resolve_service_name(&action_name)? - ); + // Keep the qualified action name (not just its send_goal service) so + // wait_for_server can poll the full five-endpoint predicate. + let qualified_action = self.resolve_service_name(&action_name)?; let client = py.allow_threads(|| { let _guard = rt.enter(); @@ -503,7 +500,7 @@ impl PyZNode { result_obj.clone_ref(py), feedback_obj.clone_ref(py), Arc::clone(self.inner.graph()), - send_goal_service, + qualified_action, )) } diff --git a/docs/bindings/python-migration.md b/docs/bindings/python-migration.md index 908c8b0df..e7037f73d 100644 --- a/docs/bindings/python-migration.md +++ b/docs/bindings/python-migration.md @@ -266,12 +266,14 @@ RuntimeError (builtin) └── HirozError (base — catch this to cover everything) ├── TimeoutError (a blocking call timed out) │ └── also inherits builtins.TimeoutError - ├── SerializationError (CDR/msgpack encode/decode failure) - └── TypeMismatchError (type hash / type mismatch) + ├── SerializationError (declared, not currently raised) + └── TypeMismatchError (declared, not currently raised) ``` The two extra bases exist so that ported code keeps working unchanged: `HirozError` inherits `RuntimeError` because that is what these paths raised before the typed hierarchy existed, and `TimeoutError` additionally inherits the **builtin** `TimeoutError` because that is what rclpy's `Client.call` raises. +`SerializationError` and `TypeMismatchError` are exported but nothing raises them yet — encode/decode failures currently surface as whatever `msgspec` raised (typically `TypeError` or `ValueError`). They are listed here so the hierarchy is complete; do not write `except hiroz_py.SerializationError:` expecting it to catch a bad message today. + ```python import hiroz_py try: From 7c08392c856a07f7f16ec5edfdeb60fe46da5692 Mon Sep 17 00:00:00 2001 From: yuanyuyuan Date: Sat, 15 Aug 2026 03:46:13 +0800 Subject: [PATCH 18/18] chore(msgs): regenerate python msgspec stubs after rebase --- .../python/hiroz_msgs_py/types/__init__.py | 2 + .../types/action_tutorials_interfaces.py | 29 +++++++ .../hiroz_msgs_py/types/example_interfaces.py | 35 ++++++-- .../hiroz_msgs_py/types/lifecycle_msgs.py | 12 +-- .../python/hiroz_msgs_py/types/nav_msgs.py | 22 ++--- .../hiroz_msgs_py/types/rcl_interfaces.py | 86 +++++++++++++++---- 6 files changed, 146 insertions(+), 40 deletions(-) create mode 100644 crates/hiroz-msgs/python/hiroz_msgs_py/types/action_tutorials_interfaces.py diff --git a/crates/hiroz-msgs/python/hiroz_msgs_py/types/__init__.py b/crates/hiroz-msgs/python/hiroz_msgs_py/types/__init__.py index 32a45c725..59fff1ef0 100644 --- a/crates/hiroz-msgs/python/hiroz_msgs_py/types/__init__.py +++ b/crates/hiroz-msgs/python/hiroz_msgs_py/types/__init__.py @@ -2,6 +2,7 @@ # Import all message types from . import action_msgs +from . import action_tutorials_interfaces from . import builtin_interfaces from . import example_interfaces from . import geometry_msgs @@ -16,6 +17,7 @@ __all__ = [ "action_msgs", + "action_tutorials_interfaces", "builtin_interfaces", "example_interfaces", "geometry_msgs", diff --git a/crates/hiroz-msgs/python/hiroz_msgs_py/types/action_tutorials_interfaces.py b/crates/hiroz-msgs/python/hiroz_msgs_py/types/action_tutorials_interfaces.py new file mode 100644 index 000000000..d40c7c375 --- /dev/null +++ b/crates/hiroz-msgs/python/hiroz_msgs_py/types/action_tutorials_interfaces.py @@ -0,0 +1,29 @@ +"""Auto-generated ROS 2 message types for action_tutorials_interfaces.""" +import msgspec +from typing import ClassVar + +class FibonacciGoal(msgspec.Struct, frozen=True, kw_only=True): + order: int = 0 + + __msgtype__: ClassVar[str] = 'action_tutorials_interfaces/msg/FibonacciGoal' + __hash__: ClassVar[str] = 'RIHS01_12b2d4be0186b9d26e02c9be2cbbc9438ab3ba78b66806b3f7f4111bb75199cb' + +class FibonacciResult(msgspec.Struct, frozen=True, kw_only=True): + sequence: list[int] = msgspec.field(default_factory=list) + + __msgtype__: ClassVar[str] = 'action_tutorials_interfaces/msg/FibonacciResult' + __hash__: ClassVar[str] = 'RIHS01_12b2d4be0186b9d26e02c9be2cbbc9438ab3ba78b66806b3f7f4111bb75199cb' + +class FibonacciFeedback(msgspec.Struct, frozen=True, kw_only=True): + partial_sequence: list[int] = msgspec.field(default_factory=list) + + __msgtype__: ClassVar[str] = 'action_tutorials_interfaces/msg/FibonacciFeedback' + __hash__: ClassVar[str] = 'RIHS01_12b2d4be0186b9d26e02c9be2cbbc9438ab3ba78b66806b3f7f4111bb75199cb' + +class Fibonacci: + """Action grouping type. Use Fibonacci.Goal, .Result and .Feedback.""" + __actiontype__: ClassVar[str] = 'action_tutorials_interfaces/action/Fibonacci' + Goal: ClassVar[type] = FibonacciGoal + Result: ClassVar[type] = FibonacciResult + Feedback: ClassVar[type] = FibonacciFeedback + diff --git a/crates/hiroz-msgs/python/hiroz_msgs_py/types/example_interfaces.py b/crates/hiroz-msgs/python/hiroz_msgs_py/types/example_interfaces.py index d22b3418e..9eb3db9ff 100644 --- a/crates/hiroz-msgs/python/hiroz_msgs_py/types/example_interfaces.py +++ b/crates/hiroz-msgs/python/hiroz_msgs_py/types/example_interfaces.py @@ -221,11 +221,23 @@ class TriggerResponse(msgspec.Struct, frozen=True, kw_only=True): __msgtype__: ClassVar[str] = 'example_interfaces/msg/TriggerResponse' __hash__: ClassVar[str] = 'RIHS01_cfeeee47f8105dd7685e4c92d46d4074669cb1c477402be1dea37486542a69e0' -class AddTwoInts: - """Service grouping type. Use AddTwoInts.Request and AddTwoInts.Response.""" - __srvtype__: ClassVar[str] = 'example_interfaces/srv/AddTwoInts' - Request: ClassVar[type] = AddTwoIntsRequest - Response: ClassVar[type] = AddTwoIntsResponse +class FibonacciGoal(msgspec.Struct, frozen=True, kw_only=True): + order: int = 0 + + __msgtype__: ClassVar[str] = 'example_interfaces/msg/FibonacciGoal' + __hash__: ClassVar[str] = 'RIHS01_8de2ceb74ed728966498ae1f9498a0a61a3cfb0930c59bbcdf2686ad454dace0' + +class FibonacciResult(msgspec.Struct, frozen=True, kw_only=True): + sequence: list[int] = msgspec.field(default_factory=list) + + __msgtype__: ClassVar[str] = 'example_interfaces/msg/FibonacciResult' + __hash__: ClassVar[str] = 'RIHS01_8de2ceb74ed728966498ae1f9498a0a61a3cfb0930c59bbcdf2686ad454dace0' + +class FibonacciFeedback(msgspec.Struct, frozen=True, kw_only=True): + sequence: list[int] = msgspec.field(default_factory=list) + + __msgtype__: ClassVar[str] = 'example_interfaces/msg/FibonacciFeedback' + __hash__: ClassVar[str] = 'RIHS01_8de2ceb74ed728966498ae1f9498a0a61a3cfb0930c59bbcdf2686ad454dace0' class SetBool: """Service grouping type. Use SetBool.Request and SetBool.Response.""" @@ -233,9 +245,22 @@ class SetBool: Request: ClassVar[type] = SetBoolRequest Response: ClassVar[type] = SetBoolResponse +class AddTwoInts: + """Service grouping type. Use AddTwoInts.Request and AddTwoInts.Response.""" + __srvtype__: ClassVar[str] = 'example_interfaces/srv/AddTwoInts' + Request: ClassVar[type] = AddTwoIntsRequest + Response: ClassVar[type] = AddTwoIntsResponse + class Trigger: """Service grouping type. Use Trigger.Request and Trigger.Response.""" __srvtype__: ClassVar[str] = 'example_interfaces/srv/Trigger' Request: ClassVar[type] = TriggerRequest Response: ClassVar[type] = TriggerResponse +class Fibonacci: + """Action grouping type. Use Fibonacci.Goal, .Result and .Feedback.""" + __actiontype__: ClassVar[str] = 'example_interfaces/action/Fibonacci' + Goal: ClassVar[type] = FibonacciGoal + Result: ClassVar[type] = FibonacciResult + Feedback: ClassVar[type] = FibonacciFeedback + diff --git a/crates/hiroz-msgs/python/hiroz_msgs_py/types/lifecycle_msgs.py b/crates/hiroz-msgs/python/hiroz_msgs_py/types/lifecycle_msgs.py index cdb701100..a841b6ff4 100644 --- a/crates/hiroz-msgs/python/hiroz_msgs_py/types/lifecycle_msgs.py +++ b/crates/hiroz-msgs/python/hiroz_msgs_py/types/lifecycle_msgs.py @@ -78,6 +78,12 @@ class GetStateResponse(msgspec.Struct, frozen=True, kw_only=True): __msgtype__: ClassVar[str] = 'lifecycle_msgs/msg/GetStateResponse' __hash__: ClassVar[str] = 'RIHS01_800a0a5aae599782b02932de0caf563f6dc4e7e94b794eadde075ba2cbef9795' +class GetState: + """Service grouping type. Use GetState.Request and GetState.Response.""" + __srvtype__: ClassVar[str] = 'lifecycle_msgs/srv/GetState' + Request: ClassVar[type] = GetStateRequest + Response: ClassVar[type] = GetStateResponse + class ChangeState: """Service grouping type. Use ChangeState.Request and ChangeState.Response.""" __srvtype__: ClassVar[str] = 'lifecycle_msgs/srv/ChangeState' @@ -96,9 +102,3 @@ class GetAvailableTransitions: Request: ClassVar[type] = GetAvailableTransitionsRequest Response: ClassVar[type] = GetAvailableTransitionsResponse -class GetState: - """Service grouping type. Use GetState.Request and GetState.Response.""" - __srvtype__: ClassVar[str] = 'lifecycle_msgs/srv/GetState' - Request: ClassVar[type] = GetStateRequest - Response: ClassVar[type] = GetStateResponse - diff --git a/crates/hiroz-msgs/python/hiroz_msgs_py/types/nav_msgs.py b/crates/hiroz-msgs/python/hiroz_msgs_py/types/nav_msgs.py index 09f50b8c5..d1bb99fe8 100644 --- a/crates/hiroz-msgs/python/hiroz_msgs_py/types/nav_msgs.py +++ b/crates/hiroz-msgs/python/hiroz_msgs_py/types/nav_msgs.py @@ -103,23 +103,17 @@ class SetMapResponse(msgspec.Struct, frozen=True, kw_only=True): __msgtype__: ClassVar[str] = 'nav_msgs/msg/SetMapResponse' __hash__: ClassVar[str] = 'RIHS01_5e11a5b2ca53d8ae85b666a019f16c9904ebc787828f1f566c4e048a1ddedfb4' -class GetMap: - """Service grouping type. Use GetMap.Request and GetMap.Response.""" - __srvtype__: ClassVar[str] = 'nav_msgs/srv/GetMap' - Request: ClassVar[type] = GetMapRequest - Response: ClassVar[type] = GetMapResponse - class GetPlan: """Service grouping type. Use GetPlan.Request and GetPlan.Response.""" __srvtype__: ClassVar[str] = 'nav_msgs/srv/GetPlan' Request: ClassVar[type] = GetPlanRequest Response: ClassVar[type] = GetPlanResponse -class LoadMap: - """Service grouping type. Use LoadMap.Request and LoadMap.Response.""" - __srvtype__: ClassVar[str] = 'nav_msgs/srv/LoadMap' - Request: ClassVar[type] = LoadMapRequest - Response: ClassVar[type] = LoadMapResponse +class GetMap: + """Service grouping type. Use GetMap.Request and GetMap.Response.""" + __srvtype__: ClassVar[str] = 'nav_msgs/srv/GetMap' + Request: ClassVar[type] = GetMapRequest + Response: ClassVar[type] = GetMapResponse class SetMap: """Service grouping type. Use SetMap.Request and SetMap.Response.""" @@ -127,3 +121,9 @@ class SetMap: Request: ClassVar[type] = SetMapRequest Response: ClassVar[type] = SetMapResponse +class LoadMap: + """Service grouping type. Use LoadMap.Request and LoadMap.Response.""" + __srvtype__: ClassVar[str] = 'nav_msgs/srv/LoadMap' + Request: ClassVar[type] = LoadMapRequest + Response: ClassVar[type] = LoadMapResponse + diff --git a/crates/hiroz-msgs/python/hiroz_msgs_py/types/rcl_interfaces.py b/crates/hiroz-msgs/python/hiroz_msgs_py/types/rcl_interfaces.py index 96272e5d5..d9687a760 100644 --- a/crates/hiroz-msgs/python/hiroz_msgs_py/types/rcl_interfaces.py +++ b/crates/hiroz-msgs/python/hiroz_msgs_py/types/rcl_interfaces.py @@ -25,6 +25,13 @@ class ListParametersResult(msgspec.Struct, frozen=True, kw_only=True): __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/ListParametersResult' __hash__: ClassVar[str] = 'RIHS01_237ae3428413dcbcfb452b510c42355f3a2b021dc091afa3e18526d57022f1cd' +class LoggerLevel(msgspec.Struct, frozen=True, kw_only=True): + name: str = "" + level: int = 0 + + __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/LoggerLevel' + __hash__: ClassVar[str] = 'RIHS01_95785cc42f048ab4f395af65035aeaf2181d8e1c7a44edb8ad4558445fdb43c0' + class Parameter(msgspec.Struct, frozen=True, kw_only=True): name: str = "" value: "rcl_interfaces.ParameterValue | None" = None @@ -83,6 +90,13 @@ class ParameterValue(msgspec.Struct, frozen=True, kw_only=True): __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/ParameterValue' __hash__: ClassVar[str] = 'RIHS01_115fc089a387e23c7ecd3525c9189c379109119d6ab82e8dfbde0fdf6a7f9b68' +class SetLoggerLevelsResult(msgspec.Struct, frozen=True, kw_only=True): + successful: bool = False + reason: str = "" + + __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/SetLoggerLevelsResult' + __hash__: ClassVar[str] = 'RIHS01_9316e5e679a5b72d2dd7fd80c539bae9e106fa0890a06dc5da3a8177a3ff6909' + class SetParametersResult(msgspec.Struct, frozen=True, kw_only=True): successful: bool = False reason: str = "" @@ -102,6 +116,18 @@ class DescribeParametersResponse(msgspec.Struct, frozen=True, kw_only=True): __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/DescribeParametersResponse' __hash__: ClassVar[str] = 'RIHS01_845b484d71eb0673dae682f2e3ba3c4851a65a3dcfb97bddd82c5b57e91e4cff' +class GetLoggerLevelsRequest(msgspec.Struct, frozen=True, kw_only=True): + names: list[str] = msgspec.field(default_factory=list) + + __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/GetLoggerLevelsRequest' + __hash__: ClassVar[str] = 'RIHS01_03bf1bebd0d6514c7ed0ba7c5e08dc9f2f39c759fe99e1e30ea4157d7674f72d' + +class GetLoggerLevelsResponse(msgspec.Struct, frozen=True, kw_only=True): + levels: list["rcl_interfaces.LoggerLevel"] = msgspec.field(default_factory=list) + + __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/GetLoggerLevelsResponse' + __hash__: ClassVar[str] = 'RIHS01_03bf1bebd0d6514c7ed0ba7c5e08dc9f2f39c759fe99e1e30ea4157d7674f72d' + class GetParameterTypesRequest(msgspec.Struct, frozen=True, kw_only=True): names: list[str] = msgspec.field(default_factory=list) @@ -139,17 +165,17 @@ class ListParametersResponse(msgspec.Struct, frozen=True, kw_only=True): __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/ListParametersResponse' __hash__: ClassVar[str] = 'RIHS01_3e6062bfbb27bfb8730d4cef2558221f51a11646d78e7bb30a1e83afac3aad9d' -class SetParametersRequest(msgspec.Struct, frozen=True, kw_only=True): - parameters: list["rcl_interfaces.Parameter"] = msgspec.field(default_factory=list) +class SetLoggerLevelsRequest(msgspec.Struct, frozen=True, kw_only=True): + levels: list["rcl_interfaces.LoggerLevel"] = msgspec.field(default_factory=list) - __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/SetParametersRequest' - __hash__: ClassVar[str] = 'RIHS01_56eed9a67e169f9cb6c1f987bc88f868c14a8fc9f743a263bc734c154015d7e0' + __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/SetLoggerLevelsRequest' + __hash__: ClassVar[str] = 'RIHS01_3ff86cb4e91fbf9abae15c234ecc874448de6ece8e193401c077cf116e4f6d78' -class SetParametersResponse(msgspec.Struct, frozen=True, kw_only=True): - results: list["rcl_interfaces.SetParametersResult"] = msgspec.field(default_factory=list) +class SetLoggerLevelsResponse(msgspec.Struct, frozen=True, kw_only=True): + results: list["rcl_interfaces.SetLoggerLevelsResult"] = msgspec.field(default_factory=list) - __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/SetParametersResponse' - __hash__: ClassVar[str] = 'RIHS01_56eed9a67e169f9cb6c1f987bc88f868c14a8fc9f743a263bc734c154015d7e0' + __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/SetLoggerLevelsResponse' + __hash__: ClassVar[str] = 'RIHS01_3ff86cb4e91fbf9abae15c234ecc874448de6ece8e193401c077cf116e4f6d78' class SetParametersAtomicallyRequest(msgspec.Struct, frozen=True, kw_only=True): parameters: list["rcl_interfaces.Parameter"] = msgspec.field(default_factory=list) @@ -163,17 +189,29 @@ class SetParametersAtomicallyResponse(msgspec.Struct, frozen=True, kw_only=True) __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/SetParametersAtomicallyResponse' __hash__: ClassVar[str] = 'RIHS01_0e192ef259c07fc3c07a13191d27002222e65e00ccec653ca05e856f79285fcd' +class SetParametersRequest(msgspec.Struct, frozen=True, kw_only=True): + parameters: list["rcl_interfaces.Parameter"] = msgspec.field(default_factory=list) + + __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/SetParametersRequest' + __hash__: ClassVar[str] = 'RIHS01_56eed9a67e169f9cb6c1f987bc88f868c14a8fc9f743a263bc734c154015d7e0' + +class SetParametersResponse(msgspec.Struct, frozen=True, kw_only=True): + results: list["rcl_interfaces.SetParametersResult"] = msgspec.field(default_factory=list) + + __msgtype__: ClassVar[str] = 'rcl_interfaces/msg/SetParametersResponse' + __hash__: ClassVar[str] = 'RIHS01_56eed9a67e169f9cb6c1f987bc88f868c14a8fc9f743a263bc734c154015d7e0' + class DescribeParameters: """Service grouping type. Use DescribeParameters.Request and DescribeParameters.Response.""" __srvtype__: ClassVar[str] = 'rcl_interfaces/srv/DescribeParameters' Request: ClassVar[type] = DescribeParametersRequest Response: ClassVar[type] = DescribeParametersResponse -class GetParameterTypes: - """Service grouping type. Use GetParameterTypes.Request and GetParameterTypes.Response.""" - __srvtype__: ClassVar[str] = 'rcl_interfaces/srv/GetParameterTypes' - Request: ClassVar[type] = GetParameterTypesRequest - Response: ClassVar[type] = GetParameterTypesResponse +class GetLoggerLevels: + """Service grouping type. Use GetLoggerLevels.Request and GetLoggerLevels.Response.""" + __srvtype__: ClassVar[str] = 'rcl_interfaces/srv/GetLoggerLevels' + Request: ClassVar[type] = GetLoggerLevelsRequest + Response: ClassVar[type] = GetLoggerLevelsResponse class GetParameters: """Service grouping type. Use GetParameters.Request and GetParameters.Response.""" @@ -181,17 +219,23 @@ class GetParameters: Request: ClassVar[type] = GetParametersRequest Response: ClassVar[type] = GetParametersResponse +class SetParameters: + """Service grouping type. Use SetParameters.Request and SetParameters.Response.""" + __srvtype__: ClassVar[str] = 'rcl_interfaces/srv/SetParameters' + Request: ClassVar[type] = SetParametersRequest + Response: ClassVar[type] = SetParametersResponse + class ListParameters: """Service grouping type. Use ListParameters.Request and ListParameters.Response.""" __srvtype__: ClassVar[str] = 'rcl_interfaces/srv/ListParameters' Request: ClassVar[type] = ListParametersRequest Response: ClassVar[type] = ListParametersResponse -class SetParameters: - """Service grouping type. Use SetParameters.Request and SetParameters.Response.""" - __srvtype__: ClassVar[str] = 'rcl_interfaces/srv/SetParameters' - Request: ClassVar[type] = SetParametersRequest - Response: ClassVar[type] = SetParametersResponse +class GetParameterTypes: + """Service grouping type. Use GetParameterTypes.Request and GetParameterTypes.Response.""" + __srvtype__: ClassVar[str] = 'rcl_interfaces/srv/GetParameterTypes' + Request: ClassVar[type] = GetParameterTypesRequest + Response: ClassVar[type] = GetParameterTypesResponse class SetParametersAtomically: """Service grouping type. Use SetParametersAtomically.Request and SetParametersAtomically.Response.""" @@ -199,3 +243,9 @@ class SetParametersAtomically: Request: ClassVar[type] = SetParametersAtomicallyRequest Response: ClassVar[type] = SetParametersAtomicallyResponse +class SetLoggerLevels: + """Service grouping type. Use SetLoggerLevels.Request and SetLoggerLevels.Response.""" + __srvtype__: ClassVar[str] = 'rcl_interfaces/srv/SetLoggerLevels' + Request: ClassVar[type] = SetLoggerLevelsRequest + Response: ClassVar[type] = SetLoggerLevelsResponse +