How can I safely turn this into threaded code?

[Jamie Risk]

I'm attempting to improve some serially executing code (that
uses the SerialPort class) bogging Windows down when it runs.
To do the 'antibogging' I'm following the example from MSDN
Windows.IO.Ports.SerialPort page and use threading.

I'm not sure if I'm creating problems with this implementation
and would appreciate your input.

The original serial code:
{
class myPacket
{
...
public bool isValid { get { ... } };
...
// append()
// scan input array (stream) for a valid packet
// set 'isValid' when complete
public append(byte[] data) { ... };
public byte[] ToByteArray() { ... };
...
}

class myComm : System.IO.Ports.SerialPort
{
public myPacket Transaction(myPacket packet)
{
myPacket response;
if (this.IsOpen)
{
// Write out the packet.
Write(packet.ToByteArray(), 0, packet.Length);

// Read until a valid packet is collected
myPacket response = new myPacket();

byte[] data;
while (!response.IsValid)
{
base.Read(data,0,this.BytesToRead);
response.append(data);
}
}
return response;
}
}
}

The class is used thusly:
{
...
myPacket Send;
myPacket Rcv;
...
myComm port = new myComm();
port.Open();
Rcv = port.Transaction(Send);
port.Close();
...
}

My attempt to 'antibog' is to add a simple thread to the
Transaction() function:

class myComm : System.IO.Ports.SerialPort
{
myPacket response;
public myPacket Transaction(myPacket packet)
{
response = null;
if (this.IsOpen)
{
// As a precaution clear out the buffer
while (this.BytesToRead > 0 )
base.Read(data,0,this.BytesToRead);

// Write out the packet.
Write(packet.ToByteArray(), 0, packet.Length);

Thread rdThrd = new Thread(TransactionRead);

myPacket response = new myPacket();

rdThrd.Start();
// kill the thread when a valid packet is created
while (!response.IsValid)
{

}
rdThrd.Join();
}
return response;
}
private void TransactionRead()
{
byte[] data;

while(base.BytesToRead > 0)
{
base.Read(data,0,this.BytesToRead);
response.append(data);
}
}
}

 

[Peter Duniho]

I'm attempting to improve some serially executing code (that uses the
SerialPort class) bogging Windows down when it runs. To do the
'antibogging' I'm following the example from MSDN
Windows.IO.Ports.SerialPort page and use threading.

I'm not sure if I'm creating problems with this implementation and would
appreciate your input.

[code snipped]

Some thoughts:

1) Why not use SerialPort.BaseStream so that you can use the async i/o
methods (Begin/EndWrite, Begin/EndRead)?

2) Why do you create a thread, only to just sit and wait for the thread to
do the exact same thing that the in-line version of the code does? Even if
you were simply blocking the thread rather than looping (see below), this
would offer zero benefit over doing all the work in the original thread,
since the original thread is not allowed to proceed until all of the work is
done.

As far as the code you've posted goes, it has several serious problems...

One problem is that the thread will exit as soon as you first reach the
condition that there are no bytes available to read. This may or may not
occur after you've read enough data to make a valid response. If you want
the thread to remain running long enough to form a valid response, that
needs to be the condition on which the thread itself loops before exiting.

Another is that since you can't guarantee that the thread will remain
running long enough to form a valid response, the original thread may simply
block forever on the loop.

Yet another is that you are looping in the original thread. This, polling
on the status of the response, is the very worst way imaginable to wait for
another thread to do its thing. It ensures that the thread will consume
100% of its timeslice, causing it to be the primary consumer of the CPU on
the computer. This is a colossal waste of CPU time, hardly the right thing
to do if responsiveness is your goal (never mind the fact that the code
doing the looping is the original thread, negating any benefit to running
the work on another thread in the first place).

Finally, a problem that IMHO exists with your original non-threaded code is
that your code has no way to terminate gracefully should it never turn out
to receive a valid response. Basic serial hardware isn't known for being
100% reliable...unless you've got something in there to manage error
correction, transmission errors could cause your code to simply lock up.
Even if you do error correction, something like a disconnected cable or
power failure on the connected serial device could also do that. Maybe your
code isn't intended for anything serious, but if it is, it needs to
implement some sort of timeout or user-initiated cancelling mechanism so
that the code doesn't just lock up when an error occurs. This is true
whether you use the original code or some asynchronous version of it.

You aren't very clear about your goals, but it sounds as though you are
concerned that the GUI itself becomes non-responsive while waiting for the
transaction to complete. If this is the case, I think the simplest way to
address the issue is to use the async i/o methods of the Stream instance you
can get from the SerialPort.BaseStream property. This will allow you to
specify a method to be called when the i/o has completed, avoiding you from
having to deal with threading explicitly.

If you decide that instead you really need to create your thread explicitly,
or if accessing the port using the Stream model is inappropriate, you need
to fix the problems I've noted above. One particular design change you need
to do is to have the original thread simply start the i/o thread and then
get back to doing whatever it was doing. The i/o thread would then have the
logic necessary to deal with doing the entire transaction, including reading
the response, and performing whatever work has to be done once a complete,
valid response has been received. What that thread does upon completing the
transaction depends on your goals, but the simplest thing would be for it to
use Invoke to run a delegate on the main thread, allowing the main thread to
process the completed transaction as you desire.

Hope that helps...

Pete

 

[Jamie Risk]

Thanks for taking the time for posting. I appreciate your
comments and will research the BaseStream option you suggest.

- Jamie

I'm attempting to improve some serially executing code (that uses the
SerialPort class) bogging Windows down when it runs. To do the
'antibogging' I'm following the example from MSDN
Windows.IO.Ports.SerialPort page and use threading.

I'm not sure if I'm creating problems with this implementation and would
appreciate your input.

[code snipped]

Some thoughts:

1) Why not use SerialPort.BaseStream so that you can use the async i/o
methods (Begin/EndWrite, Begin/EndRead)?

2) Why do you create a thread, only to just sit and wait for the thread to
do the exact same thing that the in-line version of the code does? Even if
you were simply blocking the thread rather than looping (see below), this
would offer zero benefit over doing all the work in the original thread,
since the original thread is not allowed to proceed until all of the work is
done.

As far as the code you've posted goes, it has several serious problems...

One problem is that the thread will exit as soon as you first reach the
condition that there are no bytes available to read. This may or may not
occur after you've read enough data to make a valid response. If you want
the thread to remain running long enough to form a valid response, that
needs to be the condition on which the thread itself loops before exiting.

Another is that since you can't guarantee that the thread will remain
running long enough to form a valid response, the original thread may simply
block forever on the loop.

Yet another is that you are looping in the original thread. This, polling
on the status of the response, is the very worst way imaginable to wait for
another thread to do its thing. It ensures that the thread will consume
100% of its timeslice, causing it to be the primary consumer of the CPU on
the computer. This is a colossal waste of CPU time, hardly the right thing
to do if responsiveness is your goal (never mind the fact that the code
doing the looping is the original thread, negating any benefit to running
the work on another thread in the first place).

Finally, a problem that IMHO exists with your original non-threaded code is
that your code has no way to terminate gracefully should it never turn out
to receive a valid response. Basic serial hardware isn't known for being
100% reliable...unless you've got something in there to manage error
correction, transmission errors could cause your code to simply lock up.
Even if you do error correction, something like a disconnected cable or
power failure on the connected serial device could also do that. Maybe your
code isn't intended for anything serious, but if it is, it needs to
implement some sort of timeout or user-initiated cancelling mechanism so
that the code doesn't just lock up when an error occurs. This is true
whether you use the original code or some asynchronous version of it.

You aren't very clear about your goals, but it sounds as though you are
concerned that the GUI itself becomes non-responsive while waiting for the
transaction to complete. If this is the case, I think the simplest way to
address the issue is to use the async i/o methods of the Stream instance you
can get from the SerialPort.BaseStream property. This will allow you to
specify a method to be called when the i/o has completed, avoiding you from
having to deal with threading explicitly.

If you decide that instead you really need to create your thread explicitly,
or if accessing the port using the Stream model is inappropriate, you need
to fix the problems I've noted above. One particular design change you need
to do is to have the original thread simply start the i/o thread and then
get back to doing whatever it was doing. The i/o thread would then have the
logic necessary to deal with doing the entire transaction, including reading
the response, and performing whatever work has to be done once a complete,
valid response has been received. What that thread does upon completing the
transaction depends on your goals, but the simplest thing would be for it to
use Invoke to run a delegate on the main thread, allowing the main thread to
process the completed transaction as you desire.

Hope that helps...

Pete

 

[google]

Look at http://csharp.simpleserial.com for ideas.