fix serial read for windows

[aelray]

If serial buffer contained more values than requested by erpc underlying_receive serial_read got stuck.

[dpfrey]

It seems like what the original code was trying to do is read size bytes and size may be larger than RX_BUF_BYTES. When size > RX_BUF_BYTES it is necessary to call ReadFile multiple times. It seems to me like the bytesToRead variable is named badly. I don't really know the details of the Windows serial API, but to me, it seems like the overall flow should be something like this:

char temp[RX_BUF_BYTES];

size_t totalBytesRead = 0;
while (totalBytesRead < size) {
    const DWORD bytesToRead = std::min(RX_BUF_BYTES, size - totalBytesRead);
    DWORD bytesRead;
    if (!ReadFile(hCom, temp, bytesToRead, &bytesRead, &s_readOverlap)) {
        // TODO: error handling
    }

    if (bytesRead == 0) {
        // TODO: can this happen?
    }

    memcpy(&buf[totalBytesRead], temp, bytesRead);
    totalBytesRead += bytesRead;
}

return totalBytesRead;

[aelray]

The problem with the original implementation was that:

if (!ReadFile(hCom, temp, RX_BUF_BYTES - bytesToRead, &bytesRead, &s_readOverlap))

leads to reading all bytes which were in the RX buffer. However, erpc tries to read at first 4 bytes then should return and tries to read the remaining bytes in another call to serial_read. So the original implementation got stuck when it read more than 4 bytes because this loop:

while (bytesToRead != size)

was never exited due to bytesToRead (naming is really confusing, so I changed it) got bigger than size .

Changing the implementation to

if (!ReadFile(hCom, temp, size - totalBytesRead, &bytesRead, &s_readOverlap))

ensures that only the desired amount of data is being retrieved from the buffer.

Cosnidering you proposal:

if (bytesRead == 0) {
     // TODO: can this happen? 
}

yes this can happen, and the loop should continue until it has read the desired amount of data.
For this snippet:

if (!ReadFile(hCom, temp, bytesToRead, &bytesRead, &s_readOverlap)) {
     // TODO: error handling 
}

My aim wasn't to improve error handling and therefore would keep what is implemented, i.e.

{
    if (GetLastError() == ERROR_IO_PENDING)
    {
            if (!GetOverlappedResult(hCom, &s_readOverlap, &bytesRead, FALSE))
            {
                bytesRead = 0;
                totalBytesRead = 0;
            }
        }
        else
        {
            bytesRead = 0;
            totalBytesRead = 0;
        }
    }
    else
    {
        bytesRead = 0;
        totalBytesRead = 0;
    }
}

[MichalPrincNXP]

Thank you @aelray for providing this PR and the detailed description. These changes seems to be reasonable but the internal testing for the v1.8.1 release is almost finished, I would rather integrate this PR after the release, i.e. in about 3w, ok? Thank you.

[Hadatko]

Hi @aelray , as see in Copyright we are not authors of this file. I actually don't know from where this file was took. Any chance to found original code based on Copyright and see their fix if any was done?

[MichalPrincNXP]

Hi @Hadatko , the discussed code has been integrated internally (the person is not accessible now however). That was probably not a good step and we should rather find the latest code from the original author as proposed in #183 . Thanks.

[thewon86]

There is the same issue ReadFile