Seeking advice on design/threading strategy

[styles00]

Help. I've been bashing my head on this for a while.

I'm creating an application that is meant to control a hardware device
via a socket. It's main purpose is to display the current state of the
hardware (operating temperature, current operating state, etc...), and
also allow the user to change various settings on the hardware. The
application must poll the device periodically to check for any status
changes.

Below is a simplistic representation of the class I am using to
represent the device (Not my actual code, but it illustrates what I am
currently doing)

class RemoteDevice
{
private Socket sock;

private Byte[] recvBytes = new Byte[256];

public void Connect(string addr)
{
// Connect via socket
}

public void Disconnect()
{
// Disconnect socket
}

private string uptime;
public string Uptime
{
get { return uptime; }
}

private int currentOperatingLevel;
public int CurrentOperatingLevel
{
get { return CurrentOperatingLevel }
}

private bool enableDevice;
public bool EnableDevice
{
get { return enableDevice }
}

public void EnableDevice(bool enable)
{
string enableMsg = "Enable device " + enable.ToString();
Byte[] msg = ASCII.GetBytes(enableMsg);
sock.Send(msg);
}

public void SetOperatingLevelDevice(int level)
{
string levelMsg = "Set Operating Level " + level.ToString();
Byte[] msg = ASCII.GetBytes(levelMsg);
sock.Send(msg);
}

public int Poll()
{
string pollMsg = "Get Current Stats"
Byte[] msg = ASCII.GetBytes(pollMsg);
sock.Send(msg);

Int32 bytes = sock.Receive(recvBytes, recvBytes.Length, 0);

string recvMsg = ASCII.GetString(recvBytes, 0, bytes);

string items[] = recvMsg.Split(" ".ToCharArray());

uptime = items[0];
enableDevice = bool.Parse(items[1]);
currentOperatingLevel = int.Parse(items[2]);
}
}

I've created a thread queue to queue the messages being sent to the
device as the device itself can only handle one operation at time.

Polling is implemented with the use of a timer. When the timer expires
I queue the 'Poll()' operation. The 'SetOperatingLevelDevice(int
level)' and 'EnableDevice(bool enable)' operations ' also run in a
thread to prevent the UI from freezing.

The UI displays all the properties of the connected device, and
provides some controls to configure the connected device.

Because the UI is using the "RemoteDevice" class to also display the
representation of the device to the user, I would have to provide
thread synchronization on all properties within the class. In my real
class I have at least 2 dozen properties which means that would be a
lot of lock() 's to provide.

If I wanted to use this class in another application which didn't have
a UI (and didn't need to be threaded) then that's a lot of locking/
unlocking for no reason. Also, I have an issue if I want to implement
databinding, as any updates would not take effect on the UI thread.

Is there a better way to implement this? I'd like to think that I'm
not the only one who had a problem similar to this. All the examples
of threading worker tasks had nice clean ways of separating the work
from the model. Mind you the examples were pretty simplistic too =).


Thanks,

- Mike

 

[Peter Duniho]

[...]
I've created a thread queue to queue the messages being sent to the
device as the device itself can only handle one operation at time.

Polling is implemented with the use of a timer. When the timer expires
I queue the 'Poll()' operation. The 'SetOperatingLevelDevice(int
level)' and 'EnableDevice(bool enable)' operations ' also run in a
thread to prevent the UI from freezing.

The UI displays all the properties of the connected device, and
provides some controls to configure the connected device.

Because the UI is using the "RemoteDevice" class to also display the
representation of the device to the user, I would have to provide
thread synchronization on all properties within the class. In my real
class I have at least 2 dozen properties which means that would be a
lot of lock() 's to provide.

A couple of thoughts:

1) With respect to the various threads, it seems to me that what you
really want is just one thread that actually communicates with the
device. It should have a queue for commands, protected by a lock of
course, and should manage all of the internal state for the RemoteDevice
class according to responses it gets from the device. This doesn't
address your thread synchronization issue (the thread would still be
separate from the GUI thread), but it at least could simplify part of the
design, leaving you more time to worry about other things. You can queue
commands from the GUI, a timer, etc. without having any other dedicated
threads to worry about.

From your description it sounds like you're almost already there...I'm
just proposing that you don't need additional threads for things like
"SetOperatingLevelDevice", "EnableDevice", etc.

2) With respect to synchronization, if you need the class to be
thread-safe, then you're stuck with some form of synchronization. That's
all there is to it. Now, that's not to say you have to make the entire
class thread-safe in order to work with a GUI. One alternative would be
to provide events for any properties that could change, where the event
arguments include a copy of the value for the property that's changed.
Your GUI can subscribe to the events (using Invoke or BeginInvoke to get
back to the GUI thread, of course) and receive the data as it changes
without ever having to access the RemoteDevice properties directly. If
you don't want a lot of events, I suppose you could even have a single
event that either provides a copy of the entire state, or is itself
parameterized somehow so that the same event can report a variety of state
changes.

How all that relates to data binding I can't say...I don't have enough
experience in that area.

Pete

 

[styles00]

[...]
I've created a thread queue to queue the messages being sent to the
device as the device itself can only handle one operation at time.

Polling is implemented with the use of a timer. When the timer expires
I queue the 'Poll()' operation. The 'SetOperatingLevelDevice(int
level)' and 'EnableDevice(bool enable)' operations ' also run in a
thread to prevent the UI from freezing.

The UI displays all the properties of the connected device, and
provides some controls to configure the connected device.

Because the UI is using the "RemoteDevice" class to also display the
representation of the device to the user, I would have to provide
thread synchronization on all properties within the class. In my real
class I have at least 2 dozen properties which means that would be a
lot of lock() 's to provide.

A couple of thoughts:

1) With respect to the various threads, it seems to me that what you
really want is just one thread that actually communicates with the
device. It should have a queue for commands, protected by a lock of
course, and should manage all of the internal state for the RemoteDevice
class according to responses it gets from the device. This doesn't
address your thread synchronization issue (the thread would still be
separate from the GUI thread), but it at least could simplify part of the
design, leaving you more time to worry about other things. You can queue
commands from the GUI, a timer, etc. without having any other dedicated
threads to worry about.

Sorry, I didn't state that polling and any device operations are using
the same thread.

From your description it sounds like you're almost already there...I'm
just proposing that you don't need additional threads for things like
"SetOperatingLevelDevice", "EnableDevice", etc.

2) With respect to synchronization, if you need the class to be
thread-safe, then you're stuck with some form of synchronization. That's
all there is to it.

I guess your right. I haven't done a lot of thread-safe coding so it
just seems kind of kludgy to me to sprinkle lock()'s all throughout
the class...

- Mike

 

[Peter Duniho]

[...]
I guess your right. I haven't done a lot of thread-safe coding so it
just seems kind of kludgy to me to sprinkle lock()'s all throughout
the class...

Well, it seems to me that it'd be nice if the compiler had some shorthand
for making simple properties thread-safe (ignoring for the moment the
variety of issues such a shorthand syntax would introduce ), so you
didn't have to write all the lock() statements. But I don't see anything
kludgy about it. You're not "sprinkling locks", you're adding them
carefully to exactly the places they may be required.

That said, I did point out a way to avoid having to do that. The
"lock()" statement isn't the only way to synchronize threads or to provide
a safe way for data to move from one thread to another. And when you are
dealing with a GUI thread and will be required to do some kind of
cross-thread calls with Invoke or BeginInvoke anyway, often it makes more
sense to leverage that rather than adding full thread-safety to the class.

Pete

 

[Sergey Zyuzin]

Well, it seems to me that it'd be nice if the compiler had some shorthand  

for making simple properties thread-safe (ignoring for the moment the  
variety of issues such a shorthand syntax would introduce ), so you  
didn't have to write all the lock() statements.

There is [MethodImpl(MethodImplOptions.Synchronized)] attribute which
you can place on a method or getter/setter. It is equivalent to
placing 'lock (this)' around the whole method body. Although it is
violation of best practice (to lock(this)) I sometimes use this
attribute to not add a lot of 'lock' lines.

HTH,
Sergey Zyuzin