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

Improve service server ergonomics #373

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

marcbone
Copy link
Contributor

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 marcbone force-pushed the improve-service-ergonomics branch from 4522c98 to 68b0ef5 Compare March 19, 2024 09:18
@mxgrey
Copy link
Collaborator

mxgrey commented Mar 19, 2024

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
Copy link
Collaborator

mxgrey commented Mar 19, 2024

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
Copy link
Collaborator

mxgrey commented Apr 9, 2024

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.

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.

2 participants