Re: EV_RXCHAR event received, but ReadFile sometimes failed to read a byte from serial port
- From: "bangzhong@xxxxxxxxx" <bangzhong@xxxxxxxxx>
- Date: Sun, 19 Oct 2008 18:59:30 -0700 (PDT)
Thanks for your proposal. Must I use two worker threads to do this?
Now read and write are in the main thread. I issue the write request
first, then I wait for the response.
I also have tried to not use EV_RXCHAR, use non-overlapped ReadFile
only, but I can't read anything.
The second problem is even if I got a EV_RXCHAR, ClearCommEvent
indicates there is no byte in the queue. Need I get rid of the printf?
On Oct 20, 12:51 am, Joseph M. Newcomer <newco...@xxxxxxxxxxxx> wrote:
See below...
On Sun, 19 Oct 2008 04:28:00 -0700 (PDT), "bangzh...@xxxxxxxxx" <bangzh....@xxxxxxxxx>
wrote:
Hi, All
I have a machine connected to PC using serial port. PC sends request
to the machine and the machine responds. My code is copied below (most
code are from MSDN "serial communication in win32")
When debug the code, I found that EV_RXCHAR is received. but when I
issue the ReadFile, it sometimes (NOT ALWAYS) failed to read a byte,
the read operation time out. Don't know why, please help me.
****
Essentially, as soon as I see someone talking about events like EV_RXCHAR, I know the
design is wrong. Don't waste your time with this. Forget you ever heard of it. It
doesn't solve anything of value.
****
BOOL ProcessEvent(HANDLE hComm,DWORD dwCommEvent, RESPONSE *response)
{
BOOL fRING, fRLSD, fRXCHAR, fRXFLAG, fTXEMPTY;
BOOL fBREAK, fCTS, fDSR, fERR;
****
One variable per line. Since none of these variables have any meaning, eliminate all of
them, and every piece of code that uses them.
****> OVERLAPPED osReader = {0};
****
Why do you have an OVERLAPPED structure you don't use?
****> unsigned char chRead;
BOOL fRes=TRUE;
****
Why do you declare BOOL variables (prefix 'b') with some meaningless prefix ('f', which
means a value full of flag bits)? Use the notation correctly, or not at all. Preferrably,
not at all.
****> DWORD dwRead;
int i=0;
BOOL fContinue=TRUE;
****
BOOL variables use the 'b' prefix, not the 'f' prefix. But no prefix is required because
there is no value in using them
****
fCTS = EV_CTS & dwCommEvent;
fDSR = EV_DSR & dwCommEvent;
fERR = EV_ERR & dwCommEvent;
fRING = EV_RING & dwCommEvent;
fRLSD = EV_RLSD & dwCommEvent;
fBREAK = EV_BREAK & dwCommEvent;
fRXCHAR = EV_RXCHAR & dwCommEvent;
fRXFLAG = EV_RXFLAG & dwCommEvent;
fTXEMPTY = EV_TXEMPTY & dwCommEvent;
****
None of these matter, lose all of them. They convey nothing meaningful
****
if(fRXCHAR) //issue read
****
Don't do this. Just issue a ReadFile. Done. Nobody cares about the state of the
EV_RXCHAR flag.
****> {
printf("Char Received\n");
****
You should not be doing a printf in the middle of trying to read from the serial port.
This puts too much delay in what is going on.
****
while(fContinue)
{
dwRead=ReadChar(hComm,&chRead);
****
Why read only one character? Just read in as many as you have.
****> if(dwRead)
{
printf("one byte: read %02X, response->len=%d\n",chRead, response-
len);response->response[response->len]=chRead;
response->len++;
printf("total len=%d\n", response->len);
****
This is a horrendously complicated way to read characters. Why are you only reading one
character at a time? This seems an unnecessary burden. It seems to be an amazing waste
of effort. In addition, your criterion for when you decide you need to return is flawed,
because you are doing timeouts, which are never a reliable technique.
****> }
else
{
printf("Can't read more characters, total=%d\n",response->len);
for(i=1;i<=response->len;i++)
****
Why did you declare a variable i somewhere at the top? The correct code is
for(int i = 1; i <= response->len; i++)
and notice that the code is more readable if you put spaces around the operators; there is
nothing gained by jamming everything together, unless obscure code is a design goal.
Since i does not need to exist until this loop, and is not needed after it, why give it a
scope that exceeds the loop?
Also, what did you do with the first character you read? You seem to not print it out at
all. You only print out 15 characters on the first line, for some reason. Wouldn't it be
more correct to write
for(int i = 0; i < response->len; i++)
so you print all the characters out? The correct test for the newline is
if(i > 0 && (i % 16== 0)
so creating something totally weird that starts at 1 to prevent an initial newline and
then accessing [i-1] seems downright silly.
****
{
printf("%02X ",response->response[i-1]);
if((i%16)==0)
{
printf("\n");
}
}
printf("\ndwRead=0\n");
fContinue=FALSE;
}
}
return FALSE; //return FALSE to stop WaitCommEvent
}
return TRUE; //return TRUE to issue another WaitCommEvent
}
int ReadChar(HANDLE hComm, unsigned char *chRead)
{
DWORD dwRead=0;
BOOL fWaitingOnRead = FALSE;
OVERLAPPED osReader = {0};
DWORD dwRes;
DWORD dwRetry=0;
DWORD dwError;
COMSTAT commstat;
printf("calling ReadChar\n");
osReader.hEvent=CreateEvent(NULL, TRUE, FALSE, NULL);
if(osReader.hEvent==NULL)
{
return ERROR_SYSTEM;
}
ClearCommError(hComm,&dwError,&commstat);
***
Lose this line.
****> printf("there are %d bytes in queue\n",commstat.cbInQue);
****
The above information is irrelevant
****
dwRes=ReadFile(hComm, chRead,1, &dwRead, &osReader); //read one byte
//printf("ReadFile return %d\n",dwRes);
if (!dwRes) //ReadFile Error
{
if (GetLastError() != ERROR_IO_PENDING)
{
printf("ReadBuffer System Error\n");
}
else
{
printf("ReadChar, IO pending\n");
fWaitingOnRead=TRUE;
}
}
else
{
// read completed immediately
printf("ReadChar returns immediately, %d bytes read\n",dwRead);
}
****
This code is unnecessarily complex for solving a simple problem. The introduction of a
boolean variable (WHY are you using the incorrect prefix 'f' instead of 'b'? Don't use
these prefixes unless you use them correctly! Better still, don't use them at all,
because they convey no useful information and just get in the way of writing good code)
for no particularly useful purpose seems pointless
****
while(fWaitingOnRead && (dwRetry<3))
****
Retry 3 times? There's aready some serious design failures here. Why do you think this
will loop only three times?
****
//wait only once
{
dwRes = WaitForSingleObject(osReader.hEvent, READ_TIMEOUT);
switch(dwRes)
{
// Read completed.
case WAIT_OBJECT_0:
if (!GetOverlappedResult(hComm, &osReader, &dwRead, FALSE))
{
printf("ReadChar, Error in GetOverlappedResult\n");
}
else
{
printf("ReadChar successfully, %d bytes read\n",dwRead);
}
fWaitingOnRead=FALSE;
****
There's something seriously wrong here; why do you need to time out, or change this
unnecessary variable?
****> break;
case WAIT_TIMEOUT: //continue wait
printf("ReadChar Timeout\n");
dwRetry++;
****
Lose the timeout. You don't need it, it just complicates the code
****> break; //get out of the loop
default:
printf("ReadChar Unknown Error\n");
****
It isn't "unknown", because GetLastError will give it to you, and you should print out
that value!
****> fWaitingOnRead=FALSE;
break;
}
}
****
So why are you creating an event for the purpose of reading one character? There's
something seriously wrong here.
Run this in a separate thread. See my essay on multithreaded serial port handling on my
MVP Tips site
***> CloseHandle(osReader.hEvent);
return dwRead;
****
Why are you returning a DWORD value to a function that clearly states its return type is
'int'? If you are going to use the silly prefixes, I suggest that you use them for the
purpose for which they are intended, which is to provide type information, so errors like
declaring a return type as int and returning a DWORD don't happen. Also, compile at /W4
so errors like this are caught.
Furthermore, since you are only reading one character, this would be better as a BOOL
function. But overall, this whole structure, which is reading only one character at a
time, is deeply flawed.
This entire mechanism is peculiar in that you go through a massive amount of effort to
read just a single character. You should create the event at the time the COM port is
opened and not close the event until the COM port is closed. There is no reason I know of
to use the events in serial ports at all.
Overall, this is one of the most complex means I have seen to try to read from a serial
port. I wouldn't even try to debug it, I'd rewrite it from scratch. In fact, I did, see
my essay.
www.flounder.com/serial.htm
joe
****>}
Joseph M. Newcomer [MVP]
email: newco...@xxxxxxxxxxxx
Web:http://www.flounder.com
MVP Tips:http://www.flounder.com/mvp_tips.htm
.
- Follow-Ups:
- Re: EV_RXCHAR event received, but ReadFile sometimes failed to read a byte from serial port
- From: Joseph M . Newcomer
- Re: EV_RXCHAR event received, but ReadFile sometimes failed to read a byte from serial port
- References:
- EV_RXCHAR event received, but ReadFile sometimes failed to read a byte from serial port
- From: bangzhong@xxxxxxxxx
- Re: EV_RXCHAR event received, but ReadFile sometimes failed to read a byte from serial port
- From: Joseph M . Newcomer
- EV_RXCHAR event received, but ReadFile sometimes failed to read a byte from serial port
- Prev by Date: Re: Is ADMIN$ available on Windows XP Home?
- Next by Date: Re: EV_RXCHAR event received, but ReadFile sometimes failed to read a byte from serial port
- Previous by thread: Re: EV_RXCHAR event received, but ReadFile sometimes failed to read a byte from serial port
- Next by thread: Re: EV_RXCHAR event received, but ReadFile sometimes failed to read a byte from serial port
- Index(es):
Relevant Pages
|
