Missing CAN_ISOTP_WAIT_TX_DONE flag to trigger a timeout

[muehlke]

Hi @driftregion ,

I'm using your library in an embedded Linux project so I use the ISOTP kernel module. I had a situation where I would send a download request, which does not fit into one CAN frame - so ISOTP comes into play - while the an UDS server was still not available. This meant that the Flow Control Frame did not arrive within one second after sending the request. This leads to a timeout within the ISOTP kernel module, but with the problem that it does not recognize that the FC frame did not arrive since the socket was not created by setting the flag CAN_ISOTP_WAIT_TX_DONE.

This mistake had the effect, that the UDS client starts to wait for the response, even though the request has not yet been fully sent, so it threw the UDS_ERR_TIMEOUT error, even though it was not really a timeout in a general sense but the situation described above. By setting this flag and recreating the mistake, the UDS client prints write: Communication error on send which corresponds to the Linux error ECOMM and this should be the error that someone using this library on an embedded Linux should get to really know what's happening.

I patched the code of src/tp/isotp_sock.c this way:

    struct can_isotp_options opts;
    memset(&opts, 0, sizeof(opts));
    // configure socket to wait for tx completion to recognize FC frame timeouts
    opts.flags |= CAN_ISOTP_WAIT_TX_DONE;
 
    if (functional) {
        printf("configuring fd: %d as functional\n", fd);
        // configure the socket as listen-only to avoid sending FC frames
        opts.flags |= CAN_ISOTP_LISTEN_MODE;
    }
 
    if (setsockopt(fd, SOL_CAN_ISOTP, CAN_ISOTP_OPTS, &opts, sizeof(opts)) < 0) {
        perror("setsockopt (isotp_options):");
        return -1;
    }

I'm creating a PR #30 , please let me know what you think 👍

[driftregion]

Hi @muehlke , thanks for this issue and PR. I have a few questions and requests:

Would it be possible to implement a test in https://github.com/driftregion/iso14229/blob/main/test/test_tp_compliance.c that tests for the correct behavior that you have described above and implemented in #30 ? These tests use vcan which is very fast and may not show the behavior you've outlined above.

Please also check that bazel test //test:all passes on your machine. The github runners have no vcan support and can't run those tests at the moment.

Is your server running in its own thread? I have been assuming all IO to nonblocking. This is important for real-time systems where the server is not running in a separate thread. However, this assumption seems less relevant on embedded linux where the system certainly has thread support.

[muehlke]

Hi @driftregion ,

I implemented a test that - taking my changes into consideration - makes a client send a multi-frame message without having started a server socket so no response comes and trigger the timeout that should be perceived by an UDS client when the server sends no Flow Control frame within 1s after First Frame.

I was able to run the tests succesfully with bazel test //test:all --test_tag_filters=isotp_sock on my Ubuntu development machine with vcan setup.

For clarification, I did not describe the system setup so well in the issue. In the real use case the UDS client is an embedded Linux system while the UDS server is running another UDS stack - not iso14229 from you - on a microcontroller. I realized this does not matter much but was has to be clear is the situation in which this error is reproduced:

When an UDS client running on an ISOTP socket for Linux tries to send a multi-frame request to an UDS server that will not send a FC frame within 1s - which is an error specified in ISOTP - the UDS client will still "think" it did send it, because the call to write(fd, buf, len) will still return the expected send bytes. For this error to be raised I've proposed to set the flag CAN_ISOTP_WAIT_TX_DONE on the socket.

Let me know if the PR is fine 👍

[muehlke]

I have created a new PR #32 to close this issue after having updated the tests according to @driftregion 's suggestions.