Hi - Using various resources available on the web, I came up with a
multi-threaded program to read from and write to the serial port in
Windows XP. It works quite well - except for one problem - the read
thread consumes every last available clock cycle! Can anybody reccomend
to me a way to fix it? I will include the two important bits of code -
the setup function that opens the com port, and the read thread, which
simply checks for new data over and over then prints it. Here is the
setup function:

int setupcomport()
{
hSerial = CreateFile("COM1",
GENERIC_READ | GENERIC_WRITE,
0,
0,
OPEN_EXISTING,
FILE_FLAG_OVERLAPPED,
0);
if(hSerial==INVALID_HANDLE_VALUE)
{
if(GetLastError()==ERROR_FILE_NOT_FOUND) g_print("COM port not
found\n");
//serial port does not exist. Inform user.
else g_print("error opening COM1\n");
//some other error occurred. Inform user.
printerror();
return 0;
}

DCB dcbSerialParams = {0};
dcbSerialParams.DCBlength=sizeof(dcbSerialParams);
if (!GetCommState(hSerial, &dcbSerialParams))
{
//error getting state
g_print("Error acquiring state of com port\n");
printerror();
return 0;
}
dcbSerialParams.BaudRate=CBR_9600;
dcbSerialParams.ByteSize=8;
dcbSerialParams.StopBits=ONESTOPBIT;
dcbSerialParams.Parity=NOPARITY;
if(!SetCommState(hSerial, &dcbSerialParams))
{
//error setting serial port state
g_print("Error setting state of com port\n");
printerror();
return 0;
}
COMMTIMEOUTS timeouts={0};
timeouts.ReadIntervalTimeout=MAXDWORD;
timeouts.ReadTotalTimeoutConstant=0;
timeouts.ReadTotalTimeoutMultiplier=0;
timeouts.WriteTotalTimeoutConstant=0;
timeouts.WriteTotalTimeoutMultiplier=0;
if(!SetCommTimeouts(hSerial, &timeouts))
{
//error occureed. Inform user
g_print("Error setting timeouts\n");
printerror();
return 0;
}
gtk_button_set_label((GtkButton *)combutton, "Disconnect");
return 1;
}


And here is the read thread:

void *receivethread(void *args)
{
DWORD dwRead;
OVERLAPPED osReader = {0};
char charin[2] = "";
for (;
{
osReader.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
ReadFile(hSerial, charin, 1, &dwRead, &osReader);
WaitForSingleObject(osReader.hEvent, INFINITE);
CloseHandle(osReader.hEvent);
if (dwRead)
{
//data was read in, process it.
g_print("%s", charin);
}
}
}

Any suggestions for how to lower the CPU usage of this program? I'm
assuming it's the for (; loop that is causing the problem, but I
thought the WaitForSingleObject call would keep that from executing
over and over. My entire goal with the read thread is to read in a
single char at a time, and then after a certain char is received it
will signal to me that it is time to process the incoming data. For now
I'm just printing out the data to the terminal though. (g_print is the
GTK+ equivalent of printf).

Thanks!

-Mike

nleahcim@gmail.com wrote:
I think this is your problem, according to MSDN:
"ReadIntervalTimeout
Specifies the maximum time, in milliseconds, allowed to elapse
between the arrival of two characters on the communications line.
During a ReadFile operation, the time period begins when the first
character is received. If the interval between the arrival of any
two characters exceeds this amount, the ReadFile operation is
completed and any buffered data is returned. A value of zero
indicates that interval time-outs are not used.
A value of MAXDWORD, combined with zero values for both the
ReadTotalTimeoutConstant and ReadTotalTimeoutMultiplier members,
specifies that the read operation is to return immediately with the
haracters that have already been received, even if no characters have
been received."

So you tell the system to return immediately when you try to receive
even if nothing has been received.
Your receive loop try to handle this by using an event, but I don't
think that it works.
A better approach would be to set an timeout of e.g. 1 second, or
tell the system to have no timeout ("A value of zero indicates
that interval time-outs are not used") and read 1 char at a time.

--
Just another homepage:
http://damb.dk
But it's mine - Bertel

Bertel Brander wrote:
Hi Bertel - thanks for the fast response. I previously had the
ReadIntervalTimeout set to 0, but I was missing approximately 1/4
characters. CPU usage was normal (almost 0), however. But missing that
much data simply won't work. You should note I am only trying to read
one char at a time - that's all I want to read at any given time. I
tried giving it a 1 second timeout, and the results were the same as
with a 0 ReadIntervalTimeout. If it matters, the chars being received
when it's not missing data (ReadIntervalTimeout = MAXDWORD) are:
uM96V10 and when it is (ReadIntervalTimeout = 0 or 1000): M9610

Thanks!

-Mike

nleahcim@gmail.com wrote:
Asking for only one byte at a time will add tremendous overhead.
Setting the read timeout to a few character times (at the baud rate you
are using) should return data without dropping anything. I suggest you
enlarge the amount you ask for, then search for your messages/key
characters within the buffer.

--
Scott McPhillips [VC++ MVP]

nleahcim@gmail.com wrote:
You should not miss any chars at all!

With ReadIntervalTimeout set to 0 or 1 second, did you then
have the event thing in your receive thread?

I would think that a receive thread of simply:
void *receivethread(void *args)
{
DWORD dwRead;
char charin;
for (;
{
ReadFile(hSerial, &charin, 1, &dwRead, 0);
if (dwRead)
{
//data was read in, process it.
g_print("%c", charin);
}
}
}

Should do.

In your current loop you create and destroy the event
for each read, I don't think it is a good idea.
Try to create it only once, before the loop, and
do not call CloseHandle

--
Just another homepage:
http://damb.dk
But it's mine - Bertel

Scott McPhillips [MVP] wrote:
Hi Scott - the problem is that the messages coming in are of variable
length, and will be coming in at fairly random intervals. The terminal
program that I use, "Bray's Terminal", consumes 0% system resources
according to windows task manager, but also has no idea about the
length of data coming in, and it prints out characters immediately, not
after a certain number of bytes have come in or anything like that, so
surely what I am trying to do is possible? Doing it your way would add
a significant delay between data coming in and the program processing
that data, which would be very problematic.

Thanks!

-Mike

nleahcim@gmail.com wrote:
I use code very similar to yours and it works fine: the CPU usage is
negligible. The only significant differences I can see are:

1. I create and destroy the event handle just once, outside the loop.
2. I call ResetEvent before ReadFile.
3. I check the return value from ReadFile and only if GetLastError is
ERROR_IO_PENDING do I call WaitForSingleObject.

I don't know whether any of these is important but it may be worth a
try.

Richard.
http://www.rtrussell.co.uk/
To reply be email change 'news' to my forename.

Bertel Brander wrote:
Yes, I left it in. Was that a mistake?

I just attempted using this thread, with ReadIntervalTimeout =
MAXDWORD, then = 0, then =1000, and I still was using 100% of my CPU.

You are absolutely correct about this - it's just one of those tidying
up things I haven't gotten to as my code is only half functional.

Thanks,

-Mike

nleahcim@gmail.com wrote:
I think you should try without the event


I have used this function to set the timeout:

void SetTimeOut(HANDLE Port, int sec)
{
COMMTIMEOUTS commtimeouts;
GetCommTimeouts(Port, &commtimeouts);
commtimeouts.ReadIntervalTimeout = 1000*sec;
commtimeouts.ReadTotalTimeoutMultiplier = 10;
commtimeouts.ReadTotalTimeoutConstant = 1000*sec;
SetCommTimeouts(Port, &commtimeouts);
}

Try to call it with a timeout of 1, then use the loop
without the event, and try to se what happens.

--
Just another homepage:
http://damb.dk
But it's mine - Bertel

nleahcim@gmail.com wrote:
No, it does not add a significant delay. We're talking a few to 20
milliseconds here. What ReadFile does is return if it fills your buffer
-OR- if the timeout goes by without receiving more characters. So it
will keep you very close to read time if you set it for a few character
times.

rtrussel's point number 3 also sounds like a good hint.

--
Scott McPhillips [VC++ MVP]

Irrespective of what has been written so far, there might be another
thing to consider. You didn't mention what kind of serial interface
you are using. As real serial ports are becoming scarce nowadays,
you might be dealing with some of that USB-attached devices. In my
experience the quality of there drivers is varying, but one trait is
common: don't rely on timeouts working as expected, especially inter-
character timeouts.

Markus Schaaf wrote:
motherboard of my Dell Inspirion 600M laptop. The device being
interfaced to over serial is a microcontroller. I'm not using
intercharacter time outs, I believe, as I'm only trying to read one
byte at a time. Thanks,

-Mike

In article <45008bce$0$14021$edfadb0f@dread15.news.tele.dk> ,
bertel@post4.tele.dk (Bertel Brander) writes:

You don't have to read just one character at a time. Set aside a
buffer and let ReadFile() grab as much as it can in one go. It'll
tell you how many bytes it actually read and you can scan them all.
It might reduce system overhead.

Another trick I use is to set a flag if ReadFile() got anything.
If so, I loop back and try to read again immediately; this will
enable me to drain a backlog quickly. If no data is ready, I
reset the flag and wait for an event. This gives me low overhead
and quick response, even if I'm doing round-robin reads from
multiple ports in a single thread.

I set up my COMMTIMEOUTS structure exactly the way you do.

--
/~\ cgibbs@kltpzyxm.invalid (Charlie Gibbs)
\ / I'm really at ac.dekanfrus if you read it the right way.
X Top-posted messages will probably be ignored. See RFC1855.
/ \ HTML will DEFINITELY be ignored. Join the ASCII ribbon campaign!

news@rtrussell.co.uk wrote:
Hi Richard - I have attempted to implement all 3 of your suggestions.
This is how my receive thread looks now:

void *receivethread(void *args)
{
DWORD dwRead;
OVERLAPPED osReader = {0};
char charin[2] = "";
osReader.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
for (;
{
ResetEvent(osReader.hEvent);
if ((!ReadFile(hSerial, charin, 1, &dwRead, &osReader)) &&
(GetLastError() == ERROR_IO_PENDING))
{
WaitForSingleObject(osReader.hEvent, INFINITE);

if (dwRead)
{
//data was read in, process it.
g_print("%s", charin);
}
}
}
CloseHandle(osReader.hEvent);
}

Still at 100% CPU usage, unfortunately. Did I implement your
suggestions properly? Thanks!

-Mike

Charlie Gibbs wrote:
Hi Charles - I really would prefer to read 1 char at a time - it would
speed up system response time and just make processing more
straightforward. Do you really think that that is causing the whole
overhead issue? I mean my understanding was, with the way it is written
in my first post, that it would call for a readfile, then wait until
readfile returned successfully. During that time I assumed it was using
0 clock cycles during waiting. Where am I wrong?

Thanks,

-Mike

Bertel Brander wrote:
OK well I just tried out this thread:

void *receivethread(void *args)
{
DWORD dwRead;
OVERLAPPED osReader = {0};
char charin[2] = "";
for (;
{
ReadFile(hSerial, charin, 1, &dwRead, &osReader);
if (dwRead)
{
//data was read in, process it.
g_print("%s", charin);
}
}
}

With the receive timeouts setup like so:

timeouts.ReadIntervalTimeout = 1000*1000;
timeouts.ReadTotalTimeoutConstant = 1000*1000;
timeouts.ReadTotalTimeoutMultiplier = 10;

And now the program crashes!

I should call ReadFile with a timeout of 1? How do I set a timeout when
making a ReadFile call? I thought all timeouts were set by the
COMMTIMEOUTS structure?

Thanks,

-Mike

nleahcim@gmail.com wrote:
charin is not a nul ('\0') terminated string
Eigther use:
g_print("%c", charin[0]);
or set charin[1] to 0
This is probably way it chrash.

The idea was to call my SetTimeOut function with 1 as parameter.

--
Just another homepage:
http://damb.dk
But it's mine - Bertel

nleahcim@gmail.com wrote:
I see one bug. If the read returns ERROR_IO_PENDING then dwRead does
not have the desired result. You have to call GetOverlappedResult to
get it.

What's your baud rate? At high baud rates this code might not keep up
with the port because of the overhead in one byte reads. That would
cause 100% CPU usage.

--
Scott McPhillips [VC++ MVP]

Scott McPhillips [MVP] wrote:
In the original code the port was set to 9600 baud, this is one char
ms, at that rate there should be no problem with the overhead.

Even at the max standard baud rate of 115200 or 11.520 char/sec the
overhead should not cause any problems.

--
Just another homepage:
http://damb.dk
But it's mine - Bertel

In article <1157839568.196615.104440@e3g2000cwe.googlegroups. com>,
nleahcim@gmail.com (nleahcim@gmail.com) writes:

Probably not - especially if you're not running at a high baud rate,
and especially not during times that no data is coming in. Perhaps
once you get things working you can try it both ways and compare.
For now, though, we have bigger fish to fry.

Your SetCommTimeouts() call causes ReadFile() to be non-blocking.
The way I wait for an event (see above) is to do a GetMessage().
Then your program uses no CPU until the next event (which might
be a timer tick).

--
/~\ cgibbs@kltpzyxm.invalid (Charlie Gibbs)
\ / I'm really at ac.dekanfrus if you read it the right way.
X Top-posted messages will probably be ignored. See RFC1855.
/ \ HTML will DEFINITELY be ignored. Join the ASCII ribbon campaign!