diff --git a/crates/hiroz-codegen/src/python_msgspec_generator.rs b/crates/hiroz-codegen/src/python_msgspec_generator.rs index 853de83ca..5acf0290b 100644 --- a/crates/hiroz-codegen/src/python_msgspec_generator.rs +++ b/crates/hiroz-codegen/src/python_msgspec_generator.rs @@ -3,11 +3,13 @@ //! 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; @@ -15,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<()> { @@ -30,6 +33,49 @@ 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 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(); @@ -64,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()) @@ -75,32 +132,36 @@ 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 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 python_code = - generate_python_package_with_services(package_name, &[], srv_msgs, &svc_hashes)?; - 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) @@ -118,11 +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\ @@ -142,9 +208,77 @@ 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) } +/// 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" + ) +} + +/// 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; @@ -703,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-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_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/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 5312a438d..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,3 +221,46 @@ class TriggerResponse(msgspec.Struct, frozen=True, kw_only=True): __msgtype__: ClassVar[str] = 'example_interfaces/msg/TriggerResponse' __hash__: ClassVar[str] = 'RIHS01_cfeeee47f8105dd7685e4c92d46d4074669cb1c477402be1dea37486542a69e0' +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.""" + __srvtype__: ClassVar[str] = 'example_interfaces/srv/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 f32b96bf9..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,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 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' + 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 + 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..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,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 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 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.""" + __srvtype__: ClassVar[str] = 'nav_msgs/srv/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 9e06d274e..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 @@ -201,3 +201,51 @@ class SetParametersResponse(msgspec.Struct, frozen=True, kw_only=True): __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 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.""" + __srvtype__: ClassVar[str] = 'rcl_interfaces/srv/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 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.""" + __srvtype__: ClassVar[str] = 'rcl_interfaces/srv/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 + 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..25390565b 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): ... @@ -79,6 +83,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 +203,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 +255,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 +281,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: ... # --------------------------------------------------------------------------- @@ -246,6 +292,16 @@ 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: ... + 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 @@ -253,6 +309,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: ... @@ -267,7 +324,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/action.rs b/crates/hiroz-py/src/action.rs index b23130817..0cf4f8ee0 100644 --- a/crates/hiroz-py/src/action.rs +++ b/crates/hiroz-py/src/action.rs @@ -139,20 +139,29 @@ pub struct PyZActionClient { goal_type: Py, result_type: Py, feedback_type: Py, + /// Shared graph + the qualified action name, used by `wait_for_server` to + /// poll the core's full five-endpoint action-server predicate. + graph: Arc, + action_name: String, } impl PyZActionClient { + #[allow(clippy::too_many_arguments)] pub fn new( inner: RawActionClient, goal_type: Py, result_type: Py, feedback_type: Py, + graph: Arc, + action_name: String, ) -> Self { Self { inner: Arc::new(inner), goal_type, result_type, feedback_type, + graph, + action_name, } } } @@ -172,7 +181,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 || { @@ -180,11 +189,11 @@ impl PyZActionClient { tokio::time::timeout(Duration::from_secs(11), client.send_goal(goal_msg)) .await .map_err(|_| { - pyo3::exceptions::PyRuntimeError::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(|e| pyo3::exceptions::PyRuntimeError::new_err(e.to_string())) + .map_err(crate::error::map_zenoh_error) }) })?; @@ -243,6 +252,22 @@ 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) -> PyResult { + let timeout = crate::graph::checked_timeout(timeout)?; + Ok(py.allow_threads(|| { + crate::graph::wait_for_action_server(&self.graph, &self.action_name, timeout) + })) + } + /// Get the goal type class (for debugging). #[getter] fn goal_type(&self, py: Python) -> PyObject { @@ -291,8 +316,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) = timeout.map(Duration::from_secs_f64) { + if let Some(t) = timeout { rx.recv_timeout(t).ok().map(|m| m.0) } else { rx.recv().ok().map(|m| m.0) @@ -314,40 +342,41 @@ 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 timeout = crate::graph::checked_timeout(timeout)?; + 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) = timeout { // 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 - Err(e) => Err(pyo3::exceptions::PyRuntimeError::new_err(e.to_string())), + Ok(msg) => Ok(msg.0), + 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 { handle .result() .await - .map(|msg| Some(msg.0)) - .map_err(|e| pyo3::exceptions::PyRuntimeError::new_err(e.to_string())) + .map(|msg| msg.0) + .map_err(crate::error::map_zenoh_error) } }) })?; - 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. @@ -416,10 +445,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) = timeout.map(Duration::from_secs_f64) { + 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())), diff --git a/crates/hiroz-py/src/error.rs b/crates/hiroz-py/src/error.rs index 5d8d1cd35..c57523f6b 100644 --- a/crates/hiroz-py/src/error.rs +++ b/crates/hiroz-py/src/error.rs @@ -1,13 +1,115 @@ #![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. +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 +} + +fn classify(is_timeout: bool, msg: String) -> PyErr { + if is_timeout { + timeout_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; diff --git a/crates/hiroz-py/src/graph.rs b/crates/hiroz-py/src/graph.rs index 62fcd011d..69f965b8b 100644 --- a/crates/hiroz-py/src/graph.rs +++ b/crates/hiroz-py/src/graph.rs @@ -3,6 +3,86 @@ 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); + +/// 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. +/// +/// 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() + 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); + } +} + +/// 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. /// 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/src/node.rs b/crates/hiroz-py/src/node.rs index 6e43eb4dc..801ab742f 100644 --- a/crates/hiroz-py/src/node.rs +++ b/crates/hiroz-py/src/node.rs @@ -113,6 +113,123 @@ 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", + ) + })?; + // 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__") + .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))); + } + // 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 +306,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 +332,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 +383,29 @@ 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)?; + + // 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); - Ok(PyZClient::new(Box::new(wrapper), srv_type_str)) + Ok(PyZClient::new( + Box::new(wrapper), + srv_type_str, + Arc::clone(self.inner.graph()), + qualified, + )) } // -- Graph discovery methods -- @@ -310,26 +444,39 @@ 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(); + // Resolve before building, for the same reason as `create_client`: the + // builder validates the name itself and maps failure to a RuntimeError. + // 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(); let mut builder = node.create_action_client::(&action_name); @@ -349,9 +496,11 @@ impl PyZNode { 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()), + qualified_action, )) } @@ -361,22 +510,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 +556,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 +578,84 @@ 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)?; + + // 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)); 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 { + /// 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 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); + + // `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(), + 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 16fada65e..37a9fb681 100644 --- a/crates/hiroz-py/src/pubsub.rs +++ b/crates/hiroz-py/src/pubsub.rs @@ -40,6 +40,29 @@ 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, + ) -> PyResult { + // None → wait "forever"; cap at a large but finite duration so the + // background thread can still observe interpreter shutdown. + let dur = crate::graph::checked_timeout(timeout)? + .unwrap_or(Duration::from_secs(60 * 60 * 24 * 365)); + Ok(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() @@ -97,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)); @@ -162,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)); @@ -212,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/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..85d5fee6c 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::atomic::{AtomicBool, Ordering}; +use std::sync::{Arc, Mutex}; 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, } } } @@ -36,20 +49,30 @@ 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)) - .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) -> 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) unsafe fn get_type_name(&self) -> String { format!( @@ -59,12 +82,65 @@ 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. +struct CallbackServerState { + stop: Arc, + handle: Option>, + _server: Arc, +} + +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() { + 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. +/// 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: std::sync::Mutex>, + inner: Option>>, request_type_name: String, response_type_name: String, + callback: Option, + last_error: Arc>>, } impl PyZServer { @@ -72,11 +148,132 @@ 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(Mutex::new(inner)), + request_type_name, + response_type_name, + callback: None, + last_error: Arc::new(Mutex::new(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 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 { + inner: None, request_type_name, response_type_name, + callback: Some(CallbackServerState { + stop, + handle: Some(handle), + _server: server, + }), + last_error, } } + + 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. \ + 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, + 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. + 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) => { + record_error!(last_error, "request deserialize error", e); + server.discard_pending(&request_id); + return; + } + }; + let resp_obj = match callback.call1(py, (req_obj,)) { + Ok(o) => o, + Err(e) => { + record_error!(last_error, "service callback error", e); + server.discard_pending(&request_id); + return; + } + }; + let resp_bytes = match hiroz_msgs::serialize_to_cdr( + &response_type_name, + py, + resp_obj.bind(py), + ) { + 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); + } + }); + } + Ok(None) => std::thread::sleep(Duration::from_millis(2)), + Err(e) => { + record_error!(last_error, "service poll error", e); + std::thread::sleep(Duration::from_millis(50)); + } + } + } + }) } #[allow(unsafe_op_in_unsafe_fn)] @@ -84,9 +281,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 +323,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) @@ -143,4 +340,40 @@ 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()) + } + + /// 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/src/traits.rs b/crates/hiroz-py/src/traits.rs index 021ce00f2..db905d0e8 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,7 +115,17 @@ 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<()>; + /// 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 @@ -186,6 +205,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()); @@ -200,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_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 new file mode 100644 index 000000000..f8d8547cc --- /dev/null +++ b/crates/hiroz-py/tests/test_rclpy_alignment.py @@ -0,0 +1,597 @@ +#!/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() + 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 + + +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 --- + + +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_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) + req = example_interfaces.AddTwoInts.Request(a=1, b=2) + with pytest.raises(hiroz_py.HirozError): + 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_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) + + 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)) + 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 --- + + +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_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) + 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"] + + +# --- 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_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 + + +# --- P7: action grouping class, exercised through an actual goal send --- + + +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 + + +# --- 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) + + +@pytest.mark.parametrize( + "bad_name", + [ + "//bad//name", # empty chunks: ROS validation skips these, Zenoh rejects them + "bad name/rel", # invalid component, relative -> core validates this path + "", # empty + ], +) +def test_invalid_service_name_raises_value_error(ctx, bad_name): + """Malformed names must fail at construction with ValueError. + + 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. + + 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): + 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 + + +# --- 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() 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") 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..e7037f73d --- /dev/null +++ b/docs/bindings/python-migration.md @@ -0,0 +1,331 @@ +# 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) +# 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) +``` + +### 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) # raises hiroz_py.TimeoutError 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 +RuntimeError (builtin) +└── HirozError (base — catch this to cover everything) + ├── TimeoutError (a blocking call timed out) + │ └── also inherits builtins.TimeoutError + ├── 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: + 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** 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`. + +## What's Not There Yet + +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 | +|---|---|---| +| 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. + +## 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**: 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)). + +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..118b7d49c 100644 --- a/docs/bindings/python.md +++ b/docs/bindings/python.md @@ -122,6 +122,107 @@ 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`, 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: + 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 + `ActionGoalHandle.get_result(timeout=...)` also raises `hiroz_py.TimeoutError` on timeout, matching `ZClient.call`. + +### 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). + +Reading `last_error` **clears** it, so bind it once rather than reading the property twice: + +```python +err = server.last_error # reading consumes it +if err is not None: + print("callback failed:", err) +``` + +### 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 +241,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,9 +289,10 @@ 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 +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 @@ -367,6 +469,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