[WIP] Action support #410
Draft
[WIP] Action support #410
Conversation
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
|
copy of 295 |
|
I'll have some time available beginning this week or next week, and I think the most valuable thing I can do for rclrs right now is help to get action generation finished so we can spin out I'll start by reviewing the current state of the PR and see if I can identify anywhere that I can help, but if @nwn has any guidance on where things could use attention then I'll be happy to take directions. |
Awesome, thanks @mxgrey! I haven't had the time to do much on this in the past month, but I'll try to organize things a bit so it's clearer what our next steps are. |
Update uses of a `Mutex<rcl_node_t>` with a `NodeHandle`, as was done elsewhere in the crate. Also, correct the documented UUID size to 16 rather than 24. The minimal_action_client compiles, but the minimal_action_server does not yet, complaining about expecting the `rmw` versions of messages rather than the idiomatic ones.
This follows generally the same pattern as the service server. It required adding a typesupport function to the Action trait and pulling in some more rcl_action bindings.
There's one error code, ActionNameInvalid, that conflicts with the EventInvalid code. We should consult with upstream about this, as it causes issues for representing error codes as enum values. These two error codes are probably never returned by the same function, but it would be better to keep them unique.
This is incomplete, since the service types aren't yet being generated.
This is just the basic layout. I'm trying to avoid defining the `take_*` functions that rclcpp has to link taking messages to executing on them. I'm not sure if that's actually worthwhile yet. This area should also be revisited once it's functional to see whether portions can be moved into the non-polymorphic subfunctions. Doing so could reduce compile times by avoiding excessive monomorphization.
This results in the exact same file being produced for services, except for some whitespace changes. However, it enables actions to invoke the respective service template for its generation, similar to how the it works for services and their underlying messages.
C++ uses duck typing for this, knowing that for any `Action`, the type `Action::Impl::SendGoalService::Request` will always have a `goal_id` field of type `unique_identifier_msgs::msg::UUID` without having to prove this to the compiler. Rust's generics are more strict, requiring that this be proven using type bounds. The `Request` type is also action-specific as it contains a `goal` field containing the `Goal` message type of the action. We therefore cannot enforce that all `Request`s are a specific type in `rclrs`. This seems most easily represented using associated type bounds on the `SendGoalService` associated type within `ActionImpl`. To avoid introducing to `rosidl_runtime_rs` a circular dependency on message packages like `unique_identifier_msgs`, the `ExtractUuid` trait only operates on a byte array rather than a more nicely typed `UUID` message type. I'll likely revisit this as we introduce more similar bounds on the generated types.
The `rcl_action_accept_new_goal()` function returns a pre-allocated `rcl_action_goal_handle_t` pointer, which is also stored within the action server proper. This means we cannot store a non-pointer version of this in the `ServerGoalHandle`. Instead, we'll keep a mutex-guarded mutable pointer. The `Arc` is unnecessary since this pointer is never shared with anyone. Also, we need to clean up the goal handle by calling `rcl_action_goal_handle_fini()` when the `ServerGoalHandle` is dropped.
The logic in `execute_goal_request()` was starting to get unwieldy, so I split it into three functions. The idea is that the first, `take_goal_request()`, should handle everything up until we call the user's callback. Meanwhile, `send_goal_response()` handles everything after the user callback, sending the response and, if accepted, setting everything up for the action server. `execute_goal_request()` is the overall function coordinating all of this. It passes request data from the `take*` function to the user callback and passes the returned response into the `send*` function. In addition to splitting the logic into digestible pieces, this means we could also easily make the goal-request callback asynchronous by delaying the `send*` function until the user has given their asynchronous response.
This provides a convenient RAII-style way to ensure that a function is always called when a specific value gets dropped.
This skips some of the steps that rclcpp performs, as they appear to be unnecessary. Testing should reveal whether that's true or not.
This trims the send_goal_response() function down to only be a wrapper around the rcl_action equivalent. In addition to improving logical separation, this also simplifies control flow when handling rejection.
Rather than having a bunch of standalone traits implementing various message functions like `ExtractUuid` and `SetAccepted`, with the trait bounds on each associated type in `ActionImpl`, we'll instead add these functions directly to the `ActionImpl` trait. This is simpler on both the rosidl_runtime_rs and the rclrs side.
This requires a mutex to enable simultaneous goal acceptance in a multithreaded executor. We also make ServerGoalHandles implement Send and Sync, since they will be accessed by multiple threads during callbacks. Due to containing a raw pointer field, the type doesn't automatically implement Send/Sync; however, we guarantee that the uses of this pointer is safe and synchronized, allowing us to safely implement the traits.
Adds a trait method to create a feedback message given the goal ID and the user-facing feedback message type. Depending on how many times we do this, it may end up valuable to define a GoalUuid type in rosidl_runtime_rs itself. We wouldn't be able to utilize the `RCL_ACTION_UUID_SIZE` constant imported from `rcl_action`, but this is pretty much guaranteed to be 16 forever. Defining this method signature also required inverting the super-trait relationship between Action and ActionImpl. Now ActionImpl is the sub-trait as this gives it access to all of Action's associated types. Action doesn't need to care about anything from ActionImpl (hopefully).
This still needs to be hooked up to the ActionServerGoalHandle, though.
These still don't build without errors, but it's close.
These aren't actually implemented as callbacks like in rclcpp, but rather just regular method calls. This required making some of the ActionServer and ActionServerBase methods use an Arc receiver, but the ActionServer is only ever created within an Arc, so this is fine.
rclrs needs to be able to generically construct result responses, including the user-defined result field.
These now call into the action server to send a response to prior (queued) and future goal result requests. There are still some synchronization tweaks needed for this to be correct.
This adds methods to ActionImpl for creating and accessing action-specific message types. These are needed by the rclrs ActionClient to generically read and write RMW messages. Due to issues with qualified paths in certain places (rust-lang/rust#86935), the generator now refers directly to its service and message types rather than going through associated types of the various traits. This also makes the generated code a little easier to read, with the trait method signatures from rosidl_runtime_rs still enforcing type-safety.
|
In the interest of breaking this work into smaller pieces and also getting the message generation side merged sooner, I've split off all the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Picking up @esteve's work in #295 to add support for actions.