[WIP] Action support #410
Draft
[WIP] Action support #410
Conversation
This file contains hidden or 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. |
nwn
force-pushed
the
nwn-actions
branch
2 times, most recently
from
9e6f572 to
75fd275
Compare
Merged
|
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 |
|
@nwn Would you be able to describe what work remains, or what would enable this feature to move forward? Thank you so much for your contributions to this btw! |
|
@jonatanolofsson We're planning to resume working on actions by building off of #446 so that we can take advantage of async capabilities. It'll probably take a while for #446 to get through the review process because of how huge the changes are, but I'll probably start prototyping it soon, while the review is still ongoing. |
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.
I'm not sure that this is actually the signature that we'll want, but we'll start from there.
They still aren't handled in the .wait() method, though.
Currently, action servers and clients that are ready in multiple ways are returned to the executor as a list of pairs, with one readiness mode per entry. This could alternatively be encoded as a bitfield or similar struct, with any given client/server only occurring once in the list. However, to ensure that the executor has control over execution order, we would need to expose individual `execute_readiness_mode()` methods from the client and server, rather than a unified `execute(Mode)` method. That's fine too, but something to keep in mind.
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 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.
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 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.
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.
Just the bare minimum to get things working after the rebase. Lots of clean-up still to do.
When compiling the examples that use the action client/server, you got "undefined reference" errors for any `rcl_action_*` functions during the linking phase. Telling rustc to link against the rcl_action shared library fixes this.
This matches the pattern used by the other endpoint structs.
This works similarly to the option struct used for the other endpoint types, except that it cannot use the PrimitiveOptions stuff since actions have multiple different QoS settings.
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.