-
Notifications
You must be signed in to change notification settings - Fork 140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Action message support #417
Merged
Merged
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
110d965
Added action template
esteve 047c8f1
Added action generation
esteve 556a606
Added basic create_action_client function
esteve dfdcbd3
dded action generation
esteve 67c3b8b
checkin
esteve b197155
Fix missing exported pre_field_serde field
esteve 13474d1
Removed extra code
esteve 3e70087
Sketch out action server construction and destruction
nwn 17cd980
Fix action typesupport function
nwn fb9b0e4
Add ActionImpl trait with internal messages and services
nwn 5f3373f
Split srv.rs.em into idiomatic and rmw template files
nwn 562132b
Generate underlying service definitions for actions
nwn dc90b21
Add runtime trait to get the UUID from a goal request
nwn 1ed2981
Integrate RMW message methods into ActionImpl
nwn 568bb7c
Add rosidl_runtime_rs::ActionImpl::create_feedback_message()
nwn 23e9c94
Add GetResultService methods to ActionImpl
nwn 6d7021a
Implement ActionImpl trait methods in generator
nwn 7a39794
Replace set_result_response_status with create_result_response
nwn c5bd258
Implement client-side trait methods for action messages
nwn 2748b5f
Format the rosidl_runtime_rs::ActionImpl trait
nwn 507a7af
Wrap longs lines in rosidl_generator_rs action.rs
nwn 7d30fb1
Use idiomatic message types in Action trait
nwn 3c1663f
Cleanup ActionImpl using type aliases
mxgrey 0493705
Formatting
nwn 8bedfe0
Switch from std::os::raw::c_void to std::ffi::c_void
nwn 5dece63
Clean up rosidl_generator_rs's cmake files
nwn db575ef
Add a short doc page on the message generation pipeline
nwn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Fix action typesupport function
- Loading branch information
commit 17cd98053ef199194b45aa674611ff53d7dfc3f5
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little confusing to me. Looking at (what I believe to be) the impl for this typesupport function (link)
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
?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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c_void
. However, doing so hasn't been necessary yet, and this is following the same pattern as the existingget_type_support
functions for messages and services. To make this happen, we would have to create a Rust binding for therosidl_{message,service,action}_type_support_t
structs, probably inrosidl_runtime_rs
. I would be inclined to leave this for a separate PR to avoid growing the scope of this one.@(type_name)
struct is empty since it's not really meant to be instantiated. It would only be used as a genericimpl Action
argument and to access the specificGoal
,Feedback
, andResult
associated message types.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 returnvoid*
here.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.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as
casts. I think the only thing preventing that would be defining the appropriate binding type in therosidl_runtime_rs
package. I can make an attempt at that in a follow-up PR.