Notes about ReturnValue inconsistencies during NWS/NWC communication failures

[randaz81]

Return value is supposed to be: return_value_error_nws_nwc_communication_error when NWS/NWC communication fails.

However, let's consider the following code (1):

yarp::dev::ReturnValue RobotDescription_nwc_yarp::getAllDevices(std::vector<DeviceDescription>& dev_list)
{
    std::lock_guard<std::mutex> lg(m_mutex);
    return_getAllDevices ret = m_RPC.getAllDevicesRPC();
    if (!ret.ret) {
        yCError(ROBOTDESCRIPTION_NWC_YARP, "Unable to getAllDevices");
        return ret.ret;
    }
    dev_list = ret.devices;
    return ReturnValue_ok;
}

the ReturnValue is propagated from getAllDevicesRPC(), which is generated by Thrift (2):

return_getAllDevices IRobotDescriptionMsgs::getAllDevicesRPC()
{
    if (!yarp().canWrite()) {
        yError("Missing server method '%s'?", IRobotDescriptionMsgs_getAllDevicesRPC_helper::s_prototype);
    }
    IRobotDescriptionMsgs_getAllDevicesRPC_helper helper{};
    bool ok = yarp().write(helper, helper);
    return ok ? helper.reply.return_helper : return_getAllDevices{};
}

If the write() function fails, it returns the dafault value of: return_getAllDevices which is also generated (3):

class return_getAllDevices :
        public yarp::os::idl::WirePortable
{
public:
    // Fields
    yarp::dev::ReturnValue ret{};
    std::vector<yarp::dev::DeviceDescription> devices{};

    // Default constructor
    return_getAllDevices() = default;

    // Constructor with field values
    return_getAllDevices(const yarp::dev::ReturnValue& ret,
                         const std::vector<yarp::dev::DeviceDescription>& devices);
   ///...

In the end, yarp::dev::ReturnValue ret{} is return_value_uninitialized and not return_value_error_nws_nwc_communication_error.

This is not a big issue, because both values can be casted to false, but in the future we might consider fixing this.

---

The following considerations have been made:

Fixing this in 1) can be done in the following hacky way:

    if (!ret.ret) {
        yCError(ROBOTDESCRIPTION_NWC_YARP, "Unable to getAllDevices");
     if (ret.ret == return_value_uninitialized) ret.ret = return_value_error_nws_nwc_communication_error;
        return ret.ret;
    }

From a theoretical standpoint, this is acceptable, as we are indeed assigning return_value_error_nws_nwc_communication_error in the relevant section of the code within the NWC.
It would also be beneficial to create a common method or base class for all NWCs to handle this uniformly.

Another possibility is to fix this (option 2). However, this is not ideal from a theoretical point of view, as the generated Thrift code is agnostic to the existence of the NWS/NWC—it could represent a generic communication protocol.

Additionally, it's difficult to implement because it would require assigning a value within a nested data structure, which makes it challenging to support in a generic code generator.

This issue is being kept as a reminder, but there is currently no ongoing activity to resolve it.