Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix serial read for windows #190

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

fix serial read for windows #190

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2a8c13e
fix serial read for windows
aelray Jun 18, 2021
4aaa0c0
remove unused variable
aelray Jun 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 9 additions & 10 deletions erpc_c/port/erpc_serial.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,21 +207,20 @@ int serial_read(int fd, char *buf, int size)
{
#ifdef _WIN32
HANDLE hCom = (HANDLE)fd;
unsigned long bread = 0;
char temp[RX_BUF_BYTES] = { 0 };
DWORD errors;
DWORD bytesToRead = 0;
DWORD totalBytesRead = 0;
DWORD bytesRead = 0;
DWORD ret = 0;

while (bytesToRead != size)
while (totalBytesRead != size)
{
do
{

ClearCommError(hCom, &errors, NULL);

if (!ReadFile(hCom, temp, RX_BUF_BYTES - bytesToRead, &bytesRead, &s_readOverlap))
if (!ReadFile(hCom, temp, size - totalBytesRead, &bytesRead, &s_readOverlap))
Copy link
Contributor

@dpfrey dpfrey Jun 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
    }
}

{
if (GetLastError() == ERROR_IO_PENDING)
{
Expand All @@ -232,34 +231,34 @@ int serial_read(int fd, char *buf, int size)
if (!GetOverlappedResult(hCom, &s_readOverlap, &bytesRead, FALSE))
{
bytesRead = 0;
bytesToRead = 0;
totalBytesRead = 0;
}
}
else
{
bytesRead = 0;
bytesToRead = 0;
totalBytesRead = 0;
}
}
else
{
bytesRead = 0;
bytesToRead = 0;
totalBytesRead = 0;
}
}

bytesToRead += bytesRead;
totalBytesRead += bytesRead;

if (bytesRead)
{
memcpy(buf, temp, bytesRead);
buf += bytesRead;
}

} while ((bytesRead > 0) && (RX_BUF_BYTES >= bytesToRead));
} while ((bytesRead > 0) && (size <= RX_BUF_BYTES) && (RX_BUF_BYTES >= totalBytesRead));
}

return bytesToRead;
return totalBytesRead;
#else
int len = 0;
int ret = 0;
Expand Down