Re: EV_RXCHAR event received, but ReadFile sometimes failed to read a
byte from serial port
Hi, Joe
I am going to try worker threads. but I have to wait for the response
when the request was sent. The procedure is:
Assume I have CMyClass to handle the request/response.
CMyClass: Request()
{
send request
wait for write done
wait for the response.
}
It is something like a HTTP request and response.
If I use two worker threads, CWnd will be notified after data written,
then I need CWnd to notify CMyClass again. Is it feasible to use a
event to notify CMyClass. Is there any better solution?
The revised structure will be:
CMyClass::Request()
{
send request via PostThreadMessage;
Wait for written event //the event will be signalled after CWnd got
notified.
Wait for read event
}
CWnd::OnDataWritten()
{
SetEvent();
}
What's your proposed solution? thanks
On Oct 20, 12:13 pm, Joseph M. Newcomer <newco...@flounder.com> wrote:
I would strongly suggest using two worker threads for this; I would never=
attempt this in
the main thread, because it blocks the thread, and can hang the GUI while=
it is waiting.
With non-overlapped ReadFile, if a ReadFile is pending, a WriteFile issue=
d after the
ReadFile will be forced to wait for the ReadFile to complete. Generall=
y, you cannot use
synchronous I/O for serial ports unless you can only use them in half-dup=
lex mode, which
usually compromises reliability of communication.
Since I would never attempt to use the events to determine if there is so=
mething to read,
I have no experience as to whether or not they work. Since I consider =
the whole approach
incorrect, trying to fix something I would never consider a good solution=
isn't something
I'd spend any time worrying about. You shouldn't either.
=
joe
On Sun, 19 Oct 2008 18:59:30 -0700 (PDT), "bangzh...@gmail.com" <bangzh..=
..@gmail.com>
wrote:
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" <bangz=
h...@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 (mos=
t
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_RXC=
HAR, I know the
design is wrong. Don't waste your time with this. Forget you eve=
r 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 meani=
ng, 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 p=
refix ('f', which
means a value full of flag bits)? Use the notation correctly, or no=
t 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 prefi=
x 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 meaning=
ful
//i=
ssue read
****
Don't do this. Just issue a ReadFile. Done. Nobody cares abou=
t 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,&chRe=
ad);
****
Why read only one character? Just read in as many as you have.
****> if(dwRead)
{
printf("one b=
yte: read %02X, response->len=%d\n",chRead, response-
ponse[response->len]=chRead;
++;
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);
=response->len;i++)
****
Why did you declare a variable i somewhere at the top? The correct =
code is
t 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 afte=
r it, why give it a
scope that exceeds the loop?
Also, what did you do with the first character you read? You seem t=
o not print it out at
all. You only print out 15 characters on the first line, for some r=
eason. Wouldn't it be
more correct to write
for(int i = 0; i < r=
esponse->len; i++)
so you print all the characters out? The correct test for the newli=
ne is
if(i > 0 && (i % 16== 0)
so creating something totally weird that starts at 1 to prevent an ini=
tial newline and
then accessing [i-1] seems downright silly.
****
{
=
printf("%02X ",response->response[i-1]);
if((i%16)==0)
{
printf("\n");
}
ead=0\n");
ALSE;
}
}
return FALSE; //return FALSE to stop Wait=
CommEvent
}
return TRUE; //return TRUE to issue another WaitCommEv=
ent
}
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 Err=
or\n");
}
else
{
printf("ReadChar, IO pending\=
n");
fWaitingOnRead=TRUE;
}
}
else
{
// read completed immediately
printf("ReadChar returns immediately, %d byte=
s read\n",dwRead);
}
****
This code is unnecessarily complex for solving a simple problem. Th=
e introduction of a
boolean variable (WHY are you using the incorrect prefix 'f' instead o=
f 'b'? Don't use
these prefixes unless you use them correctly! Better still, don't u=
se them at all,
because they convey no useful information and just get in the way of w=
riting 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(hCom=
m, &osReader, &dwRead, FALSE))