Is passing an extra unused parameter/message to a callback good practice?

asked 2020-01-28 20:25:55 -0500

joaocandre gravatar image

I'm writing a node that advertises several (15+) services, and to simplify the interface and unclutter the code, I am using a generic service callback using a ros::ServiceEvent to specialize the execution within the callback, although differentiating between services that require passing a float parameter or just simple triggers :

bool triggerServiceCallback(onst ros::ServiceEvent<std_srvs::Trigger::Request, std_srvs::Trigger::Response>& event) {
    const ros::M_string& header = event.getConnectionHeader();
    std::string service_name = header.at("service");

    auto response_ptr = header.getResponse();

    switch(service_name) {
        case "service_a":
            response_ptr->message = "A";
            response_ptr->success=true;
        case "service_b":
            response_ptr->message = "B";
            response_ptr->success=true;
        /* ... */
        default:
            return false;
    }
    return true;
}

bool paramServiceCallback(onst ros::ServiceEvent<std_msgs::Float32, std_srvs::Trigger::Response>& event) {
    const ros::M_string& header = event.getConnectionHeader();
    std::string service_name = header.at("service");

    auto response_ptr = header.getResponse();

    switch(service_name) {
        case "service_c":
            response_ptr->message = "C: " + std::to_string(header.getRequest()->data);
            response_ptr->success=true;
        case "service_c":
            response_ptr->message = "D: " + std::to_string(header.getRequest()->data);;
            response_ptr->success=true;
        /* ... */
        default:
            return false;
    }
    return true;
}

My question is related to both code correctness and runtime efficiency. I could further simplify this approach and use one single callback for all services, ignoring the request entirely in the first set of services where no data needs to be passed to the server node. That would greatly simplify the code (e.g. allowing me instantiate ros::ServiceServer on a loop without having to match each ros::ServiceServer to each type of callback), but that would entail transporting/passing an object without needing it - would that entail a significant overhead (it's just a float after all) - what is the customary approach/rule of thumb regarding this kind of implementation?

edit retag flag offensive close merge delete

Comments

1

I'm not too worried about any overhead, but the switch in your callbacks is rather ugly.

The whole point of having typed callbacks is exactly to avoid this imo. You've essentially migrated the complexity (you see in creating all the callbacks) to a (potentially) giant switch-case. With all the problems that could cause.

What sort of nrs are we talking about here? 1000s of ServiceServers? How many services? What do they do?

gvdhoorn gravatar image gvdhoorn  ( 2020-01-29 03:30:46 -0500 )edit

15-20 services in total. The callbacks and their instantiation are part of a "ROS wrapper" class - each service routine (each switch case) would just be comprised of a external call to a specialized class member function. They would be pretty trivial in general, it's about interfacing with an USB device (getting metadata e.g. vendor, name, serial id, or enabling/disabling functionality or setting parameters). I guess I overlooked the complexity of a switch/case structure, but imo the code is more readable as the multiplicity/specialization of the service routines would all be in one place (vs multiple declarations on the class declaration + multiple instantiations on the class construtor + multiple member callback definitions)

joaocandre gravatar image joaocandre  ( 2020-01-29 07:56:59 -0500 )edit

Lambdas are supported as service callbacks. I typically use these for very short callbacks, which it sounds like yours are.

I'm not entirely sure I understand the "class declarations etc". Callbacks are not classes.

gvdhoorn gravatar image gvdhoorn  ( 2020-01-29 07:59:14 -0500 )edit

In my particular implementation using typed callbacks, I am declaring multiple ros::ServiceServer objects as members of my ROSWrapper class (not sure if that's strictly necessary - but contextually it makes sense) and thus also have to call advertiseService on the class constructor for each service. On top of that, I also have the callbacks as member functions and therefore both declare them on the class declaration on the header file and define them individually on my source file. It's all quite trivial but I was mostly looking for a way to decrease the amount of code.

joaocandre gravatar image joaocandre  ( 2020-01-29 08:12:27 -0500 )edit

You'll still have to instantiate the service servers, that's true. And advertise(..) them.

It's probably personal and depends on experience, but if I were to find your code (at some point in the future), seeing multiple instantiations and advertise(..) calls is a recognisable pattern and I would immediately know what the code is doing. It would also be immediately clear where I would have to look for the callback.

If I see a single callback with a switch and trickery with a ServiceEvent, that would be "strange" and would take time to understand.

If you're coding only "for yourself" (dangerous assumption to make), then you could do whatever you wanted.

if this is code to be shared with others, principle of least surprise and coding for readability would factor in. Personally I would then use accepted/best practices and patterns.

But again: it's programming/software ...(more)

gvdhoorn gravatar image gvdhoorn  ( 2020-01-29 08:16:46 -0500 )edit