Repository navigation
[FEATURE] Add a timeout to TransportArbitrator for clientReceive #367
Description
Activity
Hi eRPC user. Thank you for your interest and welcome. We hope you will enjoy this framework well.
Related to this request, Serial Transport doesn't actually ever return a timeout error code when the read times out. Is the following a good change to erpc_serial_transport.cpp?
erpc_status_t SerialTransport::underlyingReceive(uint8_t *data, uint32_t size) { uint32_t bytesRead = serial_read(m_serialHandle, reinterpret_cast<char *>(data), size); - - return (size != bytesRead) ? kErpcStatus_ReceiveFailed : kErpcStatus_Success; + return (size != bytesRead) ? + ((bytesRead == 0 && m_vtime > 0) ? kErpcStatus_Timeout : kErpcStatus_ReceiveFailed) : + kErpcStatus_Success; }I added m_vtime to capture the vtime parameter from init. The idea being if vtime is > 0, the read can return with less bytes than were requested if it times out.
Part of the issue is that code in the Transport Arbitrator checks the return code for timeout to to unblock the other client requests. So maybe a better fix is to just patch that to check anything other than success to run the clean up code.
Will this problem cause the whole system to lock up and not communicate? Only in Transport Arbitrator does it appear up there? Nothing else Arbitrator it's normal, right?
Will this problem cause the whole system to lock up and not communicate? Only in Transport Arbitrator does it appear up there? Nothing else Arbitrator it's normal, right?
I use
TransportArbitratorto create a bidirectional connection. Device and PC both has a client and a server. The way erpc implements arbitrator on one side is like: 1. Client call a function, send a request to hardware transport and wait for semaphore from server. 2. Server is polling, so it is always trying to receive a message , and if it is a call request from client, then process the related function and send the reply, and if it receives a reply, then this reply is must for a client, it will find the client and active it by emit the semaphore. 3. Client receives semaphore and reply from server and it can handles the return value of the function.The problem is that sometimes transport is unreliable, so if call request or the reply which the client is waiting for is lost, the client will hold up, because it is waiting for a semaphore which is not going to be emitted. And the server will still be OK, because it is not blocked.
The ways to tackle the problem:
- The best way is to make sure that the hardware transport is reliable enough.
- Use only
onewayfunction on device client, because it will not wait for reply, but you can not make sure if this call is successful. - Add a timeout and set request's status to timeout. but it maybe a bad way to fix it, and may cause more errors. And I didn't try it.
Related to this request, Serial Transport doesn't actually ever return a timeout error code when the read times out. Is the following a good change to erpc_serial_transport.cpp?
erpc_status_t SerialTransport::underlyingReceive(uint8_t *data, uint32_t size) { uint32_t bytesRead = serial_read(m_serialHandle, reinterpret_cast<char *>(data), size); - - return (size != bytesRead) ? kErpcStatus_ReceiveFailed : kErpcStatus_Success; + return (size != bytesRead) ? + ((bytesRead == 0 && m_vtime > 0) ? kErpcStatus_Timeout : kErpcStatus_ReceiveFailed) : + kErpcStatus_Success; }I added m_vtime to capture the vtime parameter from init. The idea being if vtime is > 0, the read can return with less bytes than were requested if it times out.
Part of the issue is that code in the Transport Arbitrator checks the return code for timeout to to unblock the other client requests. So maybe a better fix is to just patch that to check anything other than success to run the clean up code.
I am sorry to reply to your comment so late. I read your comment at April but I forgot to reply.
I think your way is better to add timeout for this case, it is more flexible. And when timeout is occurred the transport must receives all bytes the caller asks for or return zero bytes to make sure there is no data is unused.
And it's necessary to unblock all pending client(s). Because if there is a timeout happened, which mean the request or reply is lost. And caller of client need to know what is happened.
erpc_transport_arbitrator.cpp@@ -70,6 +70,7 @@ erpc_status_t TransportArbitrator::receive(MessageBuffer *message) { if (client->m_isValid) { + client->m_request->getCodec()->updateStatus(kErpcStatus_Timeout); client->m_sem.put(); } } @@ -195,7 +196,7 @@ erpc_status_t TransportArbitrator::clientReceive(client_token_t token) removePendingClient(info); - return kErpcStatus_Success; + return info->m_request->getCodec()->getStatus(); }
Then client manager will call the error handler to notify client that there is a error happened.
Is your feature request related to a problem? Please describe.
If for some reason, transmission is failed. Therefore, client will not able to receive any response from server. But
TransportArbitratorwill stuck forever because it is waiting for semaphore.erpc/erpc_c/infra/erpc_transport_arbitrator.cpp
Lines 165 to 178 in 781fb03
ClientManagerdo not have this porblem because it doesn't wait for semaphore.Describe the solution you'd like
I know unreliable transport causes this problem. but it is inefficient to implement a reliable transport for serial and spi. It will need a ack from the other side.
TransportArbitratorandArbitratedClientManagerDescribe alternatives you've considered
Steps you didn't forgot to do
Arbitrated client stucks -> dead lock #171
deadlock fix proposals #121
Additional context