COVESA / capicxx-someip-tools

Common API C++ SOMEIP tooling
Mozilla Public License 2.0
76 stars 55 forks source link

Adding error section to existing method inserts error byte into first byte of reply body rather than last byte of SOME/IP header #49

Open srl100 opened 5 months ago

srl100 commented 5 months ago

I have a method defined in .fidl that works OK, taking a UInt32 input parameter and returning a 4 byte data structure, e.g.

SOME/IP Protocol (Service ID: 0x433f, Method ID: 0x0103, Length: 12)
    Service ID: 0x433f
    Method ID: 0x0103
    Length: 12
    Client ID: 0x0000
    Session ID: 0x0001
    SOME/IP Version: 0x01
    Interface Version: 0x01
    Message Type: 0x80 (Response)
    Return Code: 0x00 (Ok)
    Payload: 00000000

I now want to do extend my code to do some validity checking on the input parameter and insert a non-zero return code into the SOME/IP reply header if the checking fails.

I have naiively added an error clause to my .fidl and updated my implementation to fill in the error code, but all that seems to do is to insert my error code byte into the start of the data, i.e. returning 5 bytes of user data instead of 4 bytes.

SOME/IP Protocol (Service ID: 0x433f, Method ID: 0x0103, Length: 13)
    Service ID: 0x433f
    Method ID: 0x0103
    Length: 13        <--- Was 12
    Client ID: 0x0000
    Session ID: 0x0001
    SOME/IP Version: 0x01
    Interface Version: 0x01
    Message Type: 0x80 (Response)
    Return Code: 0x00 (Ok)   <--- Not what I want
    Payload: 3000000000

Note the change to the Length field and the extra "30" byte at the start of the data.

Here is my method specification with the new error section:

package commonapi.examples

interface Test {

  version {major 1 minor 0}

  enumeration eErrorCode {
    E_NO_ERROR=0x00                     /* No error */
    E_INVALID_INDEX=0x30                /* The requested index is out of range. */
  }

  struct sData {
    UInt32          Data1
  }

  method getData {
    in {
        UInt32 dataIndex
    }
    out {
        sData data
    }
    error eErrorCode
  }
}

My stub implementation is as follows:

#include "TestStubImpl.hpp"
#include <v1/commonapi/examples/Test.hpp>

using namespace v1::commonapi::examples;

TestStubImpl::TestStubImpl()
{
}

TestStubImpl::~TestStubImpl()
{
}

void TestStubImpl::getData(const std::shared_ptr<CommonAPI::ClientId> _client,
                                                uint32_t _dataIndex,
                                                getDataReply_t _reply)
{
    (void)_client;
    Test::eErrorCode error = Test::eErrorCode::E_INVALID_INDEX;
    Test::sData data = {};

    if (_dataIndex == 0UL)
    {
        error = Test::eErrorCode::E_NO_ERROR;
    }
    _reply(error, data);
}

In CommonAPI-SOMEIP_deployment_spec.fdepl I can see that SomeIpErrorCoding defaults to 'Header', but there does not appear to be another option - I don't know if that is relevant to this issue.

It looks to me like either I am missing a call into the SOME/IP stack to tell it that there has been an error, or alternatively that I am missing some configuration that tells the SOME/IP stack that the stub implementation of this method is providing the return code.

[Edited to display complete .fidl file, to show trace outputs straight from WireShark and to include stub implementation.]

Any ideas?

rashen commented 1 week ago

I did some digging in this, and it looks like org.genivi.commonapi.someip/src/org/genivi/commonapi/someip/generator/FrancaSomeIPGeneratorExtensions.xtend has several places where it treats errors and out-arguments as a tuple, but I would expect it to be a variant instead. I just want to put a disclaimer here that I am not very familiar with the code base, or fluent with xtend, but I have the same problem.

srl100 commented 1 week ago

I get the impression from discussions such as here https://github.com/COVESA/vsomeip/discussions/712 that the developers consider there to be a strict boundary between the transport protocol (in this case SOME/IP) and the stub implementation, which stops the stub from seeing or changing anything in the transport protocol or below. I hope I am wrong.

rashen commented 1 week ago

Yeah, I get the same impression. In the SomeIP speciifcation (R22-11, section 4.2.6.2) it say that having different layout depending on the error flag is recommended, but this generator is not following that recommendation. And since we can't see the error code from the stub you need to put everything in the payload instead.

srl100 commented 1 week ago

I'd say the requirement was stonger than a recommendation... R22-11 section 4.2.6.1 says that error codes 0x20..0x5E are "Reserved for specific errors of services and methods." which, to me, implies that a server's method implementation must be able to set the error code.

Putting "everything in the payload" is OK for home-brewed services where one has control of the specification, but my use case is to implement a VCIC (ISO 17215) server, which requires that methods check their input parameters and change the SOME/IP return code if the parameters are not acceptable.

rashen commented 1 week ago

You are completely right, I missed that part.