Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Action message support #417

Merged
merged 27 commits into from
Oct 26, 2024
Merged

Action message support #417

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
110d965
Added action template
esteve Nov 28, 2022
047c8f1
Added action generation
esteve Nov 28, 2022
556a606
Added basic create_action_client function
esteve Nov 28, 2022
dfdcbd3
dded action generation
esteve Nov 28, 2022
67c3b8b
checkin
esteve Nov 17, 2023
b197155
Fix missing exported pre_field_serde field
esteve Jan 17, 2024
13474d1
Removed extra code
esteve Jan 17, 2024
3e70087
Sketch out action server construction and destruction
nwn Jun 6, 2024
17cd980
Fix action typesupport function
nwn Jun 11, 2024
fb9b0e4
Add ActionImpl trait with internal messages and services
nwn Jul 13, 2024
5f3373f
Split srv.rs.em into idiomatic and rmw template files
nwn Jul 20, 2024
562132b
Generate underlying service definitions for actions
nwn Jul 20, 2024
dc90b21
Add runtime trait to get the UUID from a goal request
nwn Jul 20, 2024
1ed2981
Integrate RMW message methods into ActionImpl
nwn Aug 7, 2024
568bb7c
Add rosidl_runtime_rs::ActionImpl::create_feedback_message()
nwn Aug 9, 2024
23e9c94
Add GetResultService methods to ActionImpl
nwn Aug 16, 2024
6d7021a
Implement ActionImpl trait methods in generator
nwn Aug 16, 2024
7a39794
Replace set_result_response_status with create_result_response
nwn Aug 23, 2024
c5bd258
Implement client-side trait methods for action messages
nwn Sep 28, 2024
2748b5f
Format the rosidl_runtime_rs::ActionImpl trait
nwn Sep 30, 2024
507a7af
Wrap longs lines in rosidl_generator_rs action.rs
nwn Oct 1, 2024
7d30fb1
Use idiomatic message types in Action trait
nwn Oct 11, 2024
3c1663f
Cleanup ActionImpl using type aliases
mxgrey Oct 3, 2024
0493705
Formatting
nwn Oct 11, 2024
8bedfe0
Switch from std::os::raw::c_void to std::ffi::c_void
nwn Oct 12, 2024
5dece63
Clean up rosidl_generator_rs's cmake files
nwn Oct 13, 2024
db575ef
Add a short doc page on the message generation pipeline
nwn Oct 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions rosidl_generator_rs/cmake/rosidl_generator_rs_generate_interfaces.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,13 @@ foreach(_idl_file ${rosidl_generate_interfaces_ABS_IDL_FILES})

if(_parent_folder STREQUAL "msg")
set(_has_msg TRUE)
set(_idl_file_without_actions ${_idl_file_without_actions} ${_idl_file})
set(_idl_files ${_idl_files} ${_idl_file})
elseif(_parent_folder STREQUAL "srv")
set(_has_srv TRUE)
set(_idl_file_without_actions ${_idl_file_without_actions} ${_idl_file})
set(_idl_files ${_idl_files} ${_idl_file})
elseif(_parent_folder STREQUAL "action")
set(_has_action TRUE)
message(WARNING "Rust actions generation is not implemented")
set(_idl_files ${_idl_files} ${_idl_file})
else()
message(FATAL_ERROR "Interface file with unknown parent folder: ${_idl_file}")
endif()
Expand Down Expand Up @@ -81,12 +81,13 @@ endforeach()
set(target_dependencies
"${rosidl_generator_rs_BIN}"
${rosidl_generator_rs_GENERATOR_FILES}
"${rosidl_generator_rs_TEMPLATE_DIR}/action.rs.em"
"${rosidl_generator_rs_TEMPLATE_DIR}/msg_idiomatic.rs.em"
"${rosidl_generator_rs_TEMPLATE_DIR}/msg_rmw.rs.em"
"${rosidl_generator_rs_TEMPLATE_DIR}/msg.rs.em"
"${rosidl_generator_rs_TEMPLATE_DIR}/srv.rs.em"
${rosidl_generate_interfaces_ABS_IDL_FILES}
${_idl_file_without_actions}
${_idl_files}
${_dependency_files})
foreach(dep ${target_dependencies})
if(NOT EXISTS "${dep}")
Expand All @@ -99,7 +100,7 @@ rosidl_write_generator_arguments(
"${generator_arguments_file}"
PACKAGE_NAME "${PROJECT_NAME}"
IDL_TUPLES "${rosidl_generate_interfaces_IDL_TUPLES}"
ROS_INTERFACE_FILES "${_idl_file_without_actions}"
ROS_INTERFACE_FILES "${_idl_files}"
ROS_INTERFACE_DEPENDENCIES "${_dependencies}"
OUTPUT_DIR "${_output_path}"
TEMPLATE_DIR "${rosidl_generator_rs_TEMPLATE_DIR}"
Expand Down
175 changes: 175 additions & 0 deletions rosidl_generator_rs/resource/action.rs.em
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,175 @@
@{
from rosidl_parser.definition import (
ACTION_FEEDBACK_MESSAGE_SUFFIX,
ACTION_FEEDBACK_SUFFIX,
ACTION_GOAL_SERVICE_SUFFIX,
ACTION_GOAL_SUFFIX,
ACTION_RESULT_SERVICE_SUFFIX,
ACTION_RESULT_SUFFIX,
SERVICE_REQUEST_MESSAGE_SUFFIX,
SERVICE_RESPONSE_MESSAGE_SUFFIX,
)

action_msg_specs = []

for subfolder, action in action_specs:
action_msg_specs.append((subfolder, action.goal))
action_msg_specs.append((subfolder, action.result))
action_msg_specs.append((subfolder, action.feedback))
action_msg_specs.append((subfolder, action.feedback_message))

action_srv_specs = []

for subfolder, action in action_specs:
action_srv_specs.append((subfolder, action.send_goal_service))
action_srv_specs.append((subfolder, action.get_result_service))
}@

pub mod rmw {
@{
TEMPLATE(
'msg_rmw.rs.em',
package_name=package_name, interface_path=interface_path,
msg_specs=action_msg_specs,
get_rs_name=get_rs_name, get_rmw_rs_type=get_rmw_rs_type,
pre_field_serde=pre_field_serde,
get_idiomatic_rs_type=get_idiomatic_rs_type,
constant_value_to_rs=constant_value_to_rs)

TEMPLATE(
'srv_rmw.rs.em',
package_name=package_name, interface_path=interface_path,
srv_specs=action_srv_specs,
get_rs_name=get_rs_name, get_rmw_rs_type=get_rmw_rs_type,
pre_field_serde=pre_field_serde,
get_idiomatic_rs_type=get_idiomatic_rs_type,
constant_value_to_rs=constant_value_to_rs)
}@
} // mod rmw

@{
TEMPLATE(
'msg_idiomatic.rs.em',
package_name=package_name, interface_path=interface_path,
msg_specs=action_msg_specs,
get_rs_name=get_rs_name, get_rmw_rs_type=get_rmw_rs_type,
pre_field_serde=pre_field_serde,
get_idiomatic_rs_type=get_idiomatic_rs_type,
constant_value_to_rs=constant_value_to_rs)
}@

@{
TEMPLATE(
'srv_idiomatic.rs.em',
package_name=package_name, interface_path=interface_path,
srv_specs=action_srv_specs,
get_rs_name=get_rs_name, get_rmw_rs_type=get_rmw_rs_type,
pre_field_serde=pre_field_serde,
get_idiomatic_rs_type=get_idiomatic_rs_type,
constant_value_to_rs=constant_value_to_rs)
}@

@[for subfolder, action_spec in action_specs]

@{
type_name = action_spec.namespaced_type.name
}@

#[link(name = "@(package_name)__rosidl_typesupport_c")]
extern "C" {
fn rosidl_typesupport_c__get_action_type_support_handle__@(package_name)__@(subfolder)__@(type_name)() -> *const std::os::raw::c_void;
}

// Corresponds to @(package_name)__@(subfolder)__@(type_name)
pub struct @(type_name);

impl rosidl_runtime_rs::Action for @(type_name) {
type Goal = crate::@(subfolder)::rmw::@(type_name)@(ACTION_GOAL_SUFFIX);
type Result = crate::@(subfolder)::rmw::@(type_name)@(ACTION_RESULT_SUFFIX);
type Feedback = crate::@(subfolder)::rmw::@(type_name)@(ACTION_FEEDBACK_SUFFIX);
nwn marked this conversation as resolved.
Show resolved Hide resolved

fn get_type_support() -> *const std::os::raw::c_void {
// SAFETY: No preconditions for this function.
unsafe { rosidl_typesupport_c__get_action_type_support_handle__@(package_name)__@(subfolder)__@(type_name)() }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little confusing to me. Looking at (what I believe to be) the impl for this typesupport function (link)

  1. Why are we actually returning a void pointer here? This function is not marked as unsafe, but any handling of the return value may well need to use unsafe code. Could we not return a well formed type, such as rosidl_service_type_support_t?

  2. This function seems to initialize the request and response members, but I don't actually see where those members are. The @(type_name) struct appears empty. How is this working?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. You're right, we could have this return a more specific type than just c_void. However, doing so hasn't been necessary yet, and this is following the same pattern as the existing get_type_support functions for messages and services. To make this happen, we would have to create a Rust binding for the rosidl_{message,service,action}_type_support_t structs, probably in rosidl_runtime_rs. I would be inclined to leave this for a separate PR to avoid growing the scope of this one.
  2. I'm not sure what you mean here. The @(type_name) struct is empty since it's not really meant to be instantiated. It would only be used as a generic impl Action argument and to access the specific Goal, Feedback, and Result associated message types.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Sure, I'm fine leaving this to another PR. Just took a look at our usages (1) of this function historically and we do have as casts everywhere. as casts can be problematic. So it would be nice to eventually not need this. However, maybe there is some other reason I'm missing as to why we return void* here.

  2. I was referring to the data that function actually mutates in its implementation. I was misunderstanding and assumed that we actually held a handle to that data, but apparently we do not. It is a global variable managed by I guess the rosidl_runtime(?) that will be generated for each service.

static rosidl_service_type_support_t @(function_prefix)__@(spec.srv_name)_service_type_support_handle = {
  0,
  &@(function_prefix)__@(spec.srv_name)_service_members,
  get_service_typesupport_handle_function,
};

This is outside the scope of this PR though. Just calling attention to it because we've been bitten by global variables we directly interact with via FFI before, see #386

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Yes, you're right, it would be good to avoid those as casts. I think the only thing preventing that would be defining the appropriate binding type in the rosidl_runtime_rs package. I can make an attempt at that in a follow-up PR.
  2. The typesupport structs are global statics that are managed by the various typesupport implementations. Some of them seem to handle it a bit more dubiously than others, so that's a good thing to watch out for.

}
}

impl rosidl_runtime_rs::ActionImpl for @(type_name) {
type GoalStatusMessage = action_msgs::msg::rmw::GoalStatusArray;
type FeedbackMessage = crate::@(subfolder)::rmw::@(type_name)@(ACTION_FEEDBACK_MESSAGE_SUFFIX);

type SendGoalService = crate::@(subfolder)::rmw::@(type_name)@(ACTION_GOAL_SERVICE_SUFFIX);
type CancelGoalService = action_msgs::srv::rmw::CancelGoal;
type GetResultService = crate::@(subfolder)::rmw::@(type_name)@(ACTION_RESULT_SERVICE_SUFFIX);

fn create_goal_request(goal_id: &[u8; 16], goal: crate::@(subfolder)::rmw::@(type_name)@(ACTION_GOAL_SUFFIX)) -> crate::@(subfolder)::rmw::@(type_name)@(ACTION_GOAL_SERVICE_SUFFIX)@(SERVICE_REQUEST_MESSAGE_SUFFIX) {
crate::@(subfolder)::rmw::@(type_name)@(ACTION_GOAL_SERVICE_SUFFIX)@(SERVICE_REQUEST_MESSAGE_SUFFIX) {
goal_id: unique_identifier_msgs::msg::rmw::UUID { uuid: *goal_id },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we didn't want to have a dependency on unique_identifier_msgs here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to avoid creating a dependency on unique_identifier_msgs from the rosidl_runtime_rs package, since the latter is a dependency of all message packages. This would cause a cyclic dependency.

However, the crates generated by the rosidl_generator_rs do have dependencies on any message packages they use as fields. In this case, unique_identifier_msgs/msg/UUID is a field of the underlying goal service request type.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I see. So the function signature of create_goal_request can't reference unique_identifier_msgs but the impl of that function actually can.

Yeah, we should explore some of the ideas discussed earlier to simplify this. Not needed for this PR though.

goal,
}
}

fn get_goal_request_uuid(request: &crate::@(subfolder)::rmw::@(type_name)@(ACTION_GOAL_SERVICE_SUFFIX)@(SERVICE_REQUEST_MESSAGE_SUFFIX)) -> &[u8; 16] {
&request.goal_id.uuid
}

fn create_goal_response(accepted: bool, stamp: (i32, u32)) -> crate::@(subfolder)::rmw::@(type_name)@(ACTION_GOAL_SERVICE_SUFFIX)@(SERVICE_RESPONSE_MESSAGE_SUFFIX) {
crate::@(subfolder)::rmw::@(type_name)@(ACTION_GOAL_SERVICE_SUFFIX)@(SERVICE_RESPONSE_MESSAGE_SUFFIX) {
accepted,
stamp: builtin_interfaces::msg::rmw::Time {
sec: stamp.0,
nanosec: stamp.1,
},
}
}

fn get_goal_response_accepted(response: &crate::@(subfolder)::rmw::@(type_name)@(ACTION_GOAL_SERVICE_SUFFIX)@(SERVICE_RESPONSE_MESSAGE_SUFFIX)) -> bool {
response.accepted
}

fn get_goal_response_stamp(response: &crate::@(subfolder)::rmw::@(type_name)@(ACTION_GOAL_SERVICE_SUFFIX)@(SERVICE_RESPONSE_MESSAGE_SUFFIX)) -> (i32, u32) {
(response.stamp.sec, response.stamp.nanosec)
}

fn create_feedback_message(goal_id: &[u8; 16], feedback: crate::@(subfolder)::rmw::@(type_name)@(ACTION_FEEDBACK_SUFFIX)) -> crate::@(subfolder)::rmw::@(type_name)@(ACTION_FEEDBACK_MESSAGE_SUFFIX) {
let mut message = crate::@(subfolder)::rmw::@(type_name)@(ACTION_FEEDBACK_MESSAGE_SUFFIX)::default();
message.goal_id.uuid = *goal_id;
message.feedback = feedback;
message
}

fn get_feedback_message_uuid(feedback: &crate::@(subfolder)::rmw::@(type_name)@(ACTION_FEEDBACK_MESSAGE_SUFFIX)) -> &[u8; 16] {
&feedback.goal_id.uuid
}

fn get_feedback_message_feedback(feedback: &crate::@(subfolder)::rmw::@(type_name)@(ACTION_FEEDBACK_MESSAGE_SUFFIX)) -> &crate::@(subfolder)::rmw::@(type_name)@(ACTION_FEEDBACK_SUFFIX) {
&feedback.feedback
}

fn create_result_request(goal_id: &[u8; 16]) -> crate::@(subfolder)::rmw::@(type_name)@(ACTION_RESULT_SERVICE_SUFFIX)@(SERVICE_REQUEST_MESSAGE_SUFFIX) {
crate::@(subfolder)::rmw::@(type_name)@(ACTION_RESULT_SERVICE_SUFFIX)@(SERVICE_REQUEST_MESSAGE_SUFFIX) {
goal_id: unique_identifier_msgs::msg::rmw::UUID { uuid: *goal_id },
}
}

fn get_result_request_uuid(request: &crate::@(subfolder)::rmw::@(type_name)@(ACTION_RESULT_SERVICE_SUFFIX)@(SERVICE_REQUEST_MESSAGE_SUFFIX)) -> &[u8; 16] {
&request.goal_id.uuid
}

fn create_result_response(status: i8, result: crate::@(subfolder)::rmw::@(type_name)@(ACTION_RESULT_SUFFIX)) -> crate::@(subfolder)::rmw::@(type_name)@(ACTION_RESULT_SERVICE_SUFFIX)@(SERVICE_RESPONSE_MESSAGE_SUFFIX) {
crate::@(subfolder)::rmw::@(type_name)@(ACTION_RESULT_SERVICE_SUFFIX)@(SERVICE_RESPONSE_MESSAGE_SUFFIX) {
status,
result,
}
}

fn get_result_response_result(response: &crate::@(subfolder)::rmw::@(type_name)@(ACTION_RESULT_SERVICE_SUFFIX)@(SERVICE_RESPONSE_MESSAGE_SUFFIX)) -> &crate::@(subfolder)::rmw::@(type_name)@(ACTION_RESULT_SUFFIX) {
&response.result
}

fn get_result_response_status(response: &crate::@(subfolder)::rmw::@(type_name)@(ACTION_RESULT_SERVICE_SUFFIX)@(SERVICE_RESPONSE_MESSAGE_SUFFIX)) -> i8 {
response.status
}
}

@[end for]
4 changes: 4 additions & 0 deletions rosidl_generator_rs/resource/lib.rs.em
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,7 @@ pub mod msg;
@[if len(srv_specs) > 0]@
pub mod srv;
@[end if]@

@[if len(action_specs) > 0]@
pub mod action;
@[end if]@
69 changes: 4 additions & 65 deletions rosidl_generator_rs/resource/srv.rs.em
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,84 +1,23 @@
@{
req_res_specs = []

for subfolder, service in srv_specs:
req_res_specs.append((subfolder, service.request_message))
req_res_specs.append((subfolder, service.response_message))
}@

@{
TEMPLATE(
'msg_idiomatic.rs.em',
'srv_idiomatic.rs.em',
package_name=package_name, interface_path=interface_path,
msg_specs=req_res_specs,
srv_specs=srv_specs,
get_rs_name=get_rs_name, get_rmw_rs_type=get_rmw_rs_type,
pre_field_serde=pre_field_serde,
get_idiomatic_rs_type=get_idiomatic_rs_type,
constant_value_to_rs=constant_value_to_rs)
}@

@[for subfolder, srv_spec in srv_specs]

@{
type_name = srv_spec.namespaced_type.name
}@

#[link(name = "@(package_name)__rosidl_typesupport_c")]
extern "C" {
fn rosidl_typesupport_c__get_service_type_support_handle__@(package_name)__@(subfolder)__@(type_name)() -> *const std::os::raw::c_void;
}

// Corresponds to @(package_name)__@(subfolder)__@(type_name)
pub struct @(type_name);

impl rosidl_runtime_rs::Service for @(type_name) {
type Request = crate::@(subfolder)::@(type_name)_Request;
type Response = crate::@(subfolder)::@(type_name)_Response;

fn get_type_support() -> *const std::os::raw::c_void {
// SAFETY: No preconditions for this function.
unsafe { rosidl_typesupport_c__get_service_type_support_handle__@(package_name)__@(subfolder)__@(type_name)() }
}
}

@[end for]

pub mod rmw {
@{
TEMPLATE(
'msg_rmw.rs.em',
'srv_rmw.rs.em',
package_name=package_name, interface_path=interface_path,
msg_specs=req_res_specs,
srv_specs=srv_specs,
get_rs_name=get_rs_name, get_rmw_rs_type=get_rmw_rs_type,
pre_field_serde=pre_field_serde,
get_idiomatic_rs_type=get_idiomatic_rs_type,
constant_value_to_rs=constant_value_to_rs)
}@

@[for subfolder, srv_spec in srv_specs]

@{
type_name = srv_spec.namespaced_type.name
}@

#[link(name = "@(package_name)__rosidl_typesupport_c")]
extern "C" {
fn rosidl_typesupport_c__get_service_type_support_handle__@(package_name)__@(subfolder)__@(type_name)() -> *const std::os::raw::c_void;
}

// Corresponds to @(package_name)__@(subfolder)__@(type_name)
pub struct @(type_name);

impl rosidl_runtime_rs::Service for @(type_name) {
type Request = crate::@(subfolder)::rmw::@(type_name)_Request;
type Response = crate::@(subfolder)::rmw::@(type_name)_Response;

fn get_type_support() -> *const std::os::raw::c_void {
// SAFETY: No preconditions for this function.
unsafe { rosidl_typesupport_c__get_service_type_support_handle__@(package_name)__@(subfolder)__@(type_name)() }
}
}

@[end for]

} // mod rmw
44 changes: 44 additions & 0 deletions rosidl_generator_rs/resource/srv_idiomatic.rs.em
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
@{
req_res_specs = []

for subfolder, service in srv_specs:
req_res_specs.append((subfolder, service.request_message))
req_res_specs.append((subfolder, service.response_message))
}@

@{
TEMPLATE(
'msg_idiomatic.rs.em',
package_name=package_name, interface_path=interface_path,
msg_specs=req_res_specs,
get_rs_name=get_rs_name, get_rmw_rs_type=get_rmw_rs_type,
pre_field_serde=pre_field_serde,
get_idiomatic_rs_type=get_idiomatic_rs_type,
constant_value_to_rs=constant_value_to_rs)
}@

@[for subfolder, srv_spec in srv_specs]

@{
type_name = srv_spec.namespaced_type.name
}@

#[link(name = "@(package_name)__rosidl_typesupport_c")]
extern "C" {
fn rosidl_typesupport_c__get_service_type_support_handle__@(package_name)__@(subfolder)__@(type_name)() -> *const std::os::raw::c_void;
}

// Corresponds to @(package_name)__@(subfolder)__@(type_name)
pub struct @(type_name);

impl rosidl_runtime_rs::Service for @(type_name) {
type Request = crate::@(subfolder)::@(type_name)_Request;
type Response = crate::@(subfolder)::@(type_name)_Response;

fn get_type_support() -> *const std::os::raw::c_void {
// SAFETY: No preconditions for this function.
unsafe { rosidl_typesupport_c__get_service_type_support_handle__@(package_name)__@(subfolder)__@(type_name)() }
}
}

@[end for]
Loading
Loading