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.

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

Have a single signature for create_service by bundling arguments #2

Open
wants to merge 31 commits into
base: improve-service-ergonomics
Choose a base branch
from

Conversation

mxgrey
Copy link

@mxgrey mxgrey commented Apr 9, 2024

This PR catches your PR branch up to the main of ros2_rust and also tweaks the approach to reducing service callback arguments.

We should avoid having multiple Service creation methods because that won't scale well if we ever want to provide more arguments to the service callbacks. For example rclcpp has a signature that can provide a reference to the service handle. When we implement async execution we may want to provide access to Executor Commands.

By bundling everything into a Req<T>, we can expand this struct in the future without any changes to the API, and users can pick and choose which fields they care about from inside their callback.

esteve and others added 30 commits March 20, 2024 12:50
…st#380)

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Remove references to rclrs not being on crates.io
Move the tests in the `rclrs_tests` crate into `rclrs`.
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
…ID set to 99

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
…mpty

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Correct rcl entity lifecycles and fix spurious test failures
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
@marcbone
Copy link
Owner

Honestly, I am not a big fan of that solution. To me that seems even more complicated than the way it currently is. Maybe you guys just rename rmw_request_id_t to something like RequestId and call it a day. My main goal was to get rid of the second generics parameter, the rest are just some extra goodies. However, I see a problem with not allowing FnMut. Dont you think that as soon as you get rid of the static requirement for services and you actually want to implement a non-toy-service, like for a camera, you would need it to be able to mutate the state of the camera?

@mxgrey
Copy link
Author

mxgrey commented Apr 16, 2024

Dont you think that as soon as you get rid of the static requirement for services and you actually want to implement a non-toy-service, like for a camera, you would need it to be able to mutate the state of the camera?

Fn does not imply that you can't have mutable state inside the service; it just means you need to protect any mutable state with something like Mutex. I realize that can feel like unnecessary noise or cognitive burden, but it's due to the more important difference between Fn and FnMut: You can run Fn multiple times simultaneously from different threads, but can only run FnMut once at a time. This is going to matter a lot when we introduce async execution, which is something I'm beginning soon.

With async execution we have a plan for how to improve ergonomics for callbacks with mutable state, including mutable state that the callbacks can share with each other. The idea is to introduce a Worker<T> struct and create services and subscriptions that are children of the Worker (where the Worker is a child of a Node). Those services and subscriptions would always be run one-at-a-time (similar to callback groups in rclcpp) and therefore the callbacks can be FnMut and also have mutable access to the worker's payload &mut T.

But existing the Node::create_service method would remain as-is, except that it will be able to run concurrently and across multiple threads. For that to be possible, it needs to have the Fn trait, not FnMut. I think it would be less of a rug pull for users if we just keep it as Fn rather than changing to FnMut very briefly only to turn it right back to Fn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants