ros2_rust icon indicating copy to clipboard operation
ros2_rust copied to clipboard

Improve service server ergonomics

Open marcbone opened this issue 1 year ago • 3 comments

This PR improves (at least IMO) the usage for the service server.

First of all a create_service now only takes one generic argument, which is the service type. So this

node.create_service::<AddTwoInts,_>(..);

becomes this

node.create_service::<AddTwoInts>(..);

which I think is much cleaner.

Then I removed the request_header from the service callback, as almost nobody is using it. People who still want it can use the create_service_with_header method.

And lastly I made the callbacks FnMut, which makes services like this work:

use std::env;

use anyhow::{Error, Result};
use example_interfaces::srv::{AddTwoInts, AddTwoInts_Request, AddTwoInts_Response};

fn main() -> Result<(), Error> {
    let context = rclrs::Context::new(env::args())?;

    let node = rclrs::create_node(&context, "minimal_service")?;
    let mut counter = 0;
    let add_two_ints_and_counter = move |request: AddTwoInts_Request| {
        counter += 1;
        AddTwoInts_Response {
            sum: request.a + request.b + counter,
        }
    };

    let _server = node.create_service::<AddTwoInts>("add_two_ints", add_two_ints_and_counter)?;

    println!("Starting server");
    rclrs::spin(node).map_err(|err| err.into())
}

marcbone avatar Mar 19 '24 08:03 marcbone

I've been planning on doing something similar to this, but I think we can take it even further and make it so that rclrs can infer the service type without the user explicitly stating it.

First we would introduce a new trait to rosidl_runtime_rs::traits that looks like this:

pub trait ServiceRequest {
    type Service: Service<Request=Self, Response=Self::Response>;
    type Response: Message;
}

That trait would get automatically generated by rosidl_generator_rs for every service request message.

Then we can introduce a convenience struct to help with destructuring the callback argument:

pub struct Req<R> {
    request: R,
    header: rmw_request_id_t,
}

Then we can change the signature of create_service to this:

pub fn create_service<S, F>(
    &self,
    topic: &str,
    callback: F,
) -> Service<S::Service>
where
    S: ServiceRequest,
    F: Fn(Req<S>) -> S::Response;

Then users can create a service like this:

let service = node.create_service(
    "my_service"
    |Req::<MyService_Request> { request, .. }| {
        /* ... do stuff ... */
        MyService_Response { /* ... fill in data ... */ }
    }
);

I tried to come up with a way to allow this with the same function signature, like so:

let service = node.create_service(
    "my_service"
    |request: MyService_Request| {
        /* ... do stuff ... */
        MyService_Response { /* ... fill in data ... */ }
    }
);

but I don't think it's possible until the Rust language can support specialization for generics.

Anyway, I think the key benefit to my recommendation is that

  1. We can funnel all service creation through the same method signature instead of introducing multiple signatures
  2. That one signature can support future changes to the service callback arguments without impacting API (as long as the changes are additions)

Here is a link to a Rust playground that prototypes my recommendations.

mxgrey avatar Mar 19 '24 10:03 mxgrey

On a separate note, I strongly urge us to keep the signature of services as Fn and not FnMut because we will not be able to support FnMut for unconstrained services when we introduce async execution. Instead FnMut will need to be restricted to services that are associated with a Worker.

mxgrey avatar Mar 19 '24 11:03 mxgrey

After more thought, I've decided that my previous suggestion that involved introducing more traits wouldn't accomplish much.

But I do think we should bundle everything into one argument instead of creating two separate signatures. I've opened a PR that targets your PR branch. That catches your PR up to main and also reduces everything to one create_service method.

mxgrey avatar Apr 09 '24 04:04 mxgrey

@marcbone thanks for the great work. Now that the API includes your changes, I'm closing this PR. Thanks!

esteve avatar Aug 21 '25 15:08 esteve