Yarp 3.11 Feature preview: Introducing yarp::dev::ReturnValue type

[randaz81]

Starting from yarp 3.11, we are going to gradually migrate all return values from yarp dev interfaces to the new type yarp::dev::ReturnValue which allows a better understanding of the system behavior (e. g. what happened during an RPC call)

For example, a method like this:

yarp::dev::IChatBot
{
   virtual bool setLanguage(const std::string& language="auto") = 0;
}

become:

yarp::dev::IChatBot
{
   virtual yarp::dev::ReturnValue setLanguage(const std::string& language="auto") = 0;
}

The new type yarp::dev::ReturnValue can assume one of these values:

    return_value_ok = 0,                                /// Method was successfully executed
    return_value_error_generic = 1,
    return_value_error_not_implemented = 2,      /// Method is not (yet) implemented
    return_value_error_nws_nwc_communication_error = 3, /// Command answer lost during network transmission. Status unknown.
    return_value_error_deprecated = 4,                  /// Method is deprecated
    return_value_error_method_failed = 5,               /// Method failed due to invalid internal status/invalid request
    return_value_error_not_ready = 6,                   /// Method failed because some initialization is missing
    return_value_uninitialized = 100                    /// Default value, should never be explicitly assigned

yarp::dev::ReturnValue is automatically convertible to bool. All values different from return_value_ok (which is obviously true) are considered as false this means that:

yarp::dev::ReturnValue r (return_value_ok);
bool b = r;  // b is true;

yarp::dev::ReturnValue r1 (return_value_error_method_failed );
bool b1 = r1;  // b1 is false

Logical AND operation between different error type will result in an return_value_error_generic error type, i.e.

yarp::dev::ReturnValue r2 (return_value_error_not_ready );
yarp::dev::ReturnValue r3 = r1 & r2;     //r3 is now equal to `return_value_error_generic`
bool b3 = r3;  // b3 is false

MIGRATION GUIDE

IMPLICATIONS ON THE APPLICATIONS CODE

Since yarp::dev::ReturnValue is automatically casted to bool, all user applications will continue to work, without any change to the code. i.e.

if (interface->method (a, b))
{
// ... 
}

However, in the future, users might want to migrate to a more fine-grained handling of error codes:

auto ret = interface->method (a, b);
if (ret)
{
// ...
}
else if (ret == yarp::dev::ReturnValue::return_code::return_value_error_nws_nwc_communication_error)
{
//retry communication
}
else
{
//do nothing
}

IMPLICATIONS ON THE DEVICES CODE

If a device inherits from a interface whose methods use the yarp::dev::ReturnValue, some modification is required to compile successfully.
The required modifications are very easily since just the return values of the methods need to be modified.

• use return_value_ok if the method was successful
• use return_value_error_method_failed if the method failed, because the given parameters are invalid and the output cannot be computed
• use return_value_error_deprecated or return_value_error_not_implemented if the method is not available
• use return_value_error_not_ready if the device is not correctly initialized because one parameter is missing, a pointer is invalid or it requires to be activated before processing meaningful data

IMPLICATIONS ON THE NWS/NWC CODE

The modification on these devices, is slightly more difficult, since some logic might apply. Wrappers are REQUIRED to propagate the error code from the attached device.
For n NWS device:

ReturnValue Device_nws_yarp::doSomethingRPC()
{
    // check if the attach operation has been already performed
    if (!m_iAttachedInterface)
    {
       return ReturnValue::return_code::return_value_error_not_ready;
    }
    
    //call the attached device method
    auto ret = m_iAttachedInterface->doSomething();
    if (!ret)
    {
        //propagates the return value
        yError("Unable to doSomething");
        return ret;
    }
    
    //propagates the return value
    return ret;
}

For a NWC device:

ReturnValue Battery_nwc_yarp::getBatteryInfo(std::string &battery_info)
{
    Bottle cmd, response;
    cmd.addVocab32(VOCAB_IBATTERY);
    cmd.addVocab32(VOCAB_BATTERY_INFO);
    bool ok = rpcPort.write(cmd, response);
    if (CHECK_FAIL(ok, response)!=false)
    {
        battery_info = response.get(2).asString();
        ReturnValue ret(response.get(0).asInt32()); //propagates the error transmitted by the NWS
        return ret;
    }
    return ReturnValue::return_code::return_value_error_nws_nwc_communication_error;
}

Please note that the required modifications might be slightly different if the NWC/NWS employs a THRIFT-based communication and that also the protocol (i.e. the .thrift file) needs to employ the yarp::dev::ReturnValue type to propagate the return value from the device to the client.

[PeterBowman]

I wonder if the names could be shortened a bit. Instead of:

yarp::dev::ReturnValue::return_code::return_value_error_nws_nwc_communication_error

...have something like:

yarp::dev::ReturnValue::code::error_nws_nwc_communication_error (even though error appears twice here)

Or keep return_code. I am mainly concerned about the recurring return_value_ prefix in the enumeration. Then, in toString():

case code::error_nws_nwc_communication_error:
    return std::string("error_nws_nwc_communication_error");

Or:

case code::error_nws_nwc_communication_error:
    return std::string("command answer lost during network transmission; status unknown");

[randaz81]

The prefix used in the enumeration are used to facilitate regex expressions for future changes/replacements in all Yarp-related repositories. However, users might want to define their custom shortened typedefs in the application code (preferably not in Yarp).
For example, in ReturnValue.h, it's already defined:

#define ReturnValue_ok ReturnValue(yarp::dev::ReturnValue::return_code::return_value_ok)

The description text string can be improved, yes.

[PeterBowman]

It would be nice to be able to do something like this:

auto ret = iface->doSomething();

switch (ret)
{
    case yarp::dev::ReturnValue::return_code::return_value_error_nws_nwc_communication_error: // lengthy, see my above comment
        // do something
}

I have tried to add another conversion operator:

operator return_code() const
{
    return value_b;
};

And then I can do this:

switch (static_cast<yarp::dev::ReturnValue::return_code>(ret))
{
    case yarp::dev::ReturnValue::return_code::return_value_error_nws_nwc_communication_error:
        // do something
}

I am not sure about the pitfalls of this, and yet a static_cast seems to be necessary. Is there any way to achieve the result of the first snippet?

[randaz81]

Yes, a conversion operator will do the trick.
But I would add the explicit keyword to force the use of the cast.
Without it, an automatic conversion could lead to unsafe behaviors and I'd prefer to avoid it.
This feature could be added in an separate PR.