PIDController: race condition for enable/disable and PIDWrite #30

Open
virtuald opened this Issue May 15, 2016 · 2 comments

Projects

None yet

3 participants

@virtuald
Member

Note: I have only examined the java implementation, but I assume this holds true for C++ as well.

As a user, if I call disable() on a PIDController, I reasonable expect that after that point PIDWrite will cease to be called. However, a race condition exists that makes this not the case.

In calculate(), the lock is not held while checking enabled -- which may be ok. However, after that line, potentially enabled could be changed, and the calculation is performed. PIDWrite is called after the lock is released, but once again enabled could have changed by that point.

One thing I'm torn on is whether the lock should be held while calling PIDWrite. I feel like it should -- but I also feel that could potentially cause deadlocks if the PIDWrite function called something that locked on something that another thread was holding while calling a PID function. Unsure on the best solution there (maybe use different locks, or perhaps atomic primitives instead?).

@PeterJohnson
Member

To ensure this, there needs to be a separate recursive mutex that's used only for PIDWrite and Disable. You do not want to hold the main pidcontroller mutex over the PIDWrite call. Disable grabs the main pidcontroller mutex and the PIDWrite mutex, then clears the enabled flag. Calculate grabs in the same order, checks the enable flag, but releases only the pidcontroller mutex before calling PIDWrite, releasing the PIDWrite mutex only after that function has returned.

@calcmogul
Member

In addition to this, we could stop the Notifier when Disable() is called. Calculate() doesn't do anything unless the controller is enabled anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment