Re: EV_RXCHAR event received, but ReadFile sometimes failed to read a
byte from serial port
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...@flounder.com> wrote:
See below...
On Sun, 19 Oct 2008 04:28:00 -0700 (PDT), "bangzh...@gmail.com" <bangzh..=
..@gmail.com>
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 h=
eard 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 pref=
ix ('f', which
means a value full of flag bits)? Use the notation correctly, or not a=
t 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 i=
s 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 t=
he 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-
se[response->len]=chRead;
response->len++;
printf("total le=
n=%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 re=
ad more characters, total=%d\n",response->len);
esponse->len;i++)
****
Why did you declare a variable i somewhere at the top? The correct cod=
e is
for(int i=
= 1; i <= response->len; i++)
and notice that the code is more readable if you put spaces around the op=
erators; there is
nothing gained by jamming everything together, unless obscure code is a d=
esign goal.
Since i does not need to exist until this loop, and is not needed after i=
t, why give it a
scope that exceeds the loop?
Also, what did you do with the first character you read? You seem to n=
ot print it out at
all. You only print out 15 characters on the first line, for some reas=
on. Wouldn't it be
more correct to write
for(int i = 0; i < resp=
onse->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 initia=
l newline and
then accessing [i-1] seems downright silly.
****
{
=
printf("%02X ",response->response[i-1]);
if((i%16)==0)
{
printf("\n");
}
=0\n");
E;
}
}
return FALSE; //return FALSE to stop WaitCom=
mEvent
}
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 r=
ead\n",dwRead);
}
****
This code is unnecessarily complex for solving a simple problem. The i=
ntroduction 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 writ=
ing good code)
for no particularly useful purpose seems pointless
****
while(fWaitingOnRead && (dwRetry<3))
****
Retry 3 times? There's aready some serious design failures here. Wh=
y do you think this
will loop only three times?
****
//wait only once
{
dwRes = WaitForSingleObject(osReader.hEvent, R=
EAD_TIMEOUT);
switch(dwRes)
{
// Read completed.
case WAIT_OBJECT_0:
if (!GetOverlappedResult(hComm, =
&osReader, &dwRead, FALSE))
, 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 c=
hange 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 sho=
uld 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 po=
rt 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. Als=
o, compile at /W4
so errors like this are caught.
Furthermore, since you are only reading one character, this would be bett=
er 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...@flounder.com
Web:http://www.flounder.com
MVP Tips:http://www.flounder.com/mvp_tips.htm