pganssle/Magnetometer-Controller — Issue #30: Fix up thread locks
Author: pganssle
Created: 2012-02-11 01:24:34+00:00
State: open
Labels: High

OK, so there's actually a pretty serious stability problem with this version of the program, and I'm pretty sure I understand why, now that I've been doing a bit more development. The issue all relates to thread locking, and what things are being done asynchronously.

When I wrote the program, I didn't fully appreciate how asynchronous these calls were going to be, and as a result I didn't take into account certain race conditions, I wrote everything, then thread locked it later. That is **not** a good idea. Design safely as you go.

Multithreaded programming is very much like linear programming, but you have to make sure that the tasks that you have the program doing are either completely unrelated or if they are basically doing similar tasks, that they don't get in each others' way. When two threads are trying to modify the same variable, the result is often undefined. This is called a [race condition](http://en.wikipedia.org/wiki/Race_condition#Computing).

The way to solve this is to use local copies of any variable whenever possible, and when modifying a global variable, utilize thread locking (tell all other threads that they can't do anything with the variable until they're done with it). The main problem with this is that because this is C and not Java, I've stored a *huge* amount of stuff in various struct arrays, to emulate something like classes. These variables are huge and everything uses them, in all the threads. I've set up several thread locks (lock_uipc, lock_uidc, lock_ce, etc), for the commonly modified ones.

The problem is that I somehow got the impression that race conditions only matter when *writing* to a variable, not reading from it. This is true when you are talking about issues where two threads access the same piece of memory and try to write it at the same time (which crashes the computer in a horrible way), but it still leads to undefined behavior when thread 1 reads a variable, then thread 2 writes to it, while thread 1 is doing calculations based on an out-dated value.

The way to fix this is to go through and thread lock *entire functions*, rather than thread-locking within functions. However, we can't just go through and do this just like that, because there are many instances where you have a case like this:

```
void change_uidc_stuff() {
int m;
GetCtrlVal(pan, ctrl, &m);
for(int i = 0; i < uidc.blah; i++) {
do_other_thing();
}
}

void do_other_thing() {
CmtGetLock(lock_uidc);
uidc.r = m;
CmtReleaseLock(lock_uidc);
}
```

Ideally we'd re-write it so that CmtGetLock(lock_uidc)/CmtReleaseLock(lock_uidc) would be able to surround that for loop (which checks a value on uidc each iteration), but then you'd have a problem where the first function has the lock and the second function would hang waiting for it. I have a sense of how to do this, so if anyone wants to take this on (it's *very* important), please let me know and I can give you an outline.