Help me understand PIDController::OnTarget()

jwakeman · February 23, 2012, 2:04pm

bool PIDController::OnTarget()
{
	bool temp;
	CRITICAL_REGION(m_semaphore)
	{
		temp = fabs(m_error) < (m_tolerance / 100 * 
			(m_maximumInput - m_minimumInput));
	}
	END_REGION;
	return temp;
}

I’m having trouble understanding the implementation of this function. In the context that I was using it my max input was 100 and my minimum input was 0. I was using the default tolerance of 0.05 (assuming this means 5%).

0.05 / 100 *100 = 0.05.

This means my error would have to less than 0.05 to be OnTarget regardless of setpoint.

For a setpoint of 10 I would have to be within 0.05 which is 0.5%, not 5%.

For a setpoint of 100 I still have to be within 0.05 which is 0.05% of 100, not 5%.

Shouldn’t the implementation be the percent error formula?

% error = (|Your Result - Accepted Value| / Accepted Value) x 100

Something like:

bool PIDController::OnTarget()
{
	bool temp;
	CRITICAL_REGION(m_semaphore)
	{
		temp = fabs(m_error)/m_setpoint < m_tolerance;
	}
	END_REGION;
	return temp;
}

rbmj · February 23, 2012, 2:17pm

If you want a tolerance of 5%, you should use a tolerance of 5.0, not 0.05. I made the same mistake.

wireties · February 23, 2012, 2:21pm

The implementation looks correct to me. It looks for the error to be less than the tolerance % of the full range. Since m_tolerance is divided by 100, I assume that 5% should be 5.0, not 0.05.

HTH

DjScribbles · February 23, 2012, 2:40pm

I’m having a little trouble figuring out where your confused, so I’m going to make two seperate explanations:

a)
To get a tolerence of 5% (as in you example setpoint ± 5.0) you would pass in a tolerence of 5.0; the math converts the percentage as a value from 0 to 100 to a value of 1.00 to 0.00, and uses that for the computation.

The percentage is literally a percentage value, 100 = 100%, 1.0 = 1%. It’s exactly the opposite of what I expected it would be (especially in an engineering setting like this); also, to make matters worse, theres an extra unnecessary floating point division to taking place in the OnTarget function (this division should take place in the set tolerence function instead, but oh well)…

b)
The reason OnTarget uses the Min/Max input is fairly good actually (I actually thought at first it worked the other way, and it added a lot of complexity to this scenario)
If you consider a turret, which autotargets the hoop and uses a gyro to determine it’s rotation, you will frequently changing the setpoint to the angle you need to turn; however you need to be within 1deg of the hoop…

If the tolerence were based on the setpoint, a 10% tolerence and a setpoint of 10deg, your target would be 10deg ±1deg, for a setpoint of 20deg, your tolerence would be 20deg±2deg.

So, in that scenario, if you wanted a 1 deg error, you would have to compute the % error based on the setpoint you chose each time it changed.

Conclusion:
The whole thing would be much simpler (and function the same) if tolerence simply took in a float for the acceptable error value rather than percentage. (ex SetTolerence(1.0); //Tolerence of 1 degree).

Also, a word of caution…
On target will return true if (m_error == 0), so you have to be sure the calculate function has run at least once after each enable before you make any action based on OnTarget().

Joe_Ross · February 23, 2012, 2:52pm

I think depending on your application, you could make an argument for either method.

For example, if you’re controlling the angle of an arm, you probably want the same tolerance range throughout the motion of the arm (as implemented). If you’re controlling the speed of a shooter, you’d want to use the percent error formula, so that you have a tighter tolerance as the speed of the wheel gets smaller.

In Java, here’s the java-doc for setTolerance.


    /**
     * Set the percentage error which is considered tolerable for use with
     * OnTarget. (Input of 15.0 = 15 percent)
     * @param percent error which is tolerable
     */

Ether · February 23, 2012, 3:06pm

jwakeman:

bool PIDController::OnTarget()
{
	bool temp;
	CRITICAL_REGION(m_semaphore)
	{
		temp = fabs(m_error) < (m_tolerance / 100 * 
			(m_maximumInput - m_minimumInput));
	}
	END_REGION;
	return temp;
}

On a side note:

It looks like m_tolerance, m_maximumInput, and m_minimumInput are all constants.

Take them, and the calculation, out of the critical region

bool PIDController::OnTarget()
{
	double temp;
	CRITICAL_REGION(m_semaphore)
	{
         temp=m_error;
	}
	END_REGION;
	return (fabs(temp) < (m_tolerance / 100 * (m_maximumInput - m_minimumInput)));
}

jwakeman · February 23, 2012, 3:40pm

At least I undestand the intent now. I think Joe Ross gave the best answer. It makes sense in the turret application. I was using it for my shooter speed where I think percent error makes more sense.

I think percent error is more natural to me because whenever we discussed the steady state error requirements of a control loop in school we always talked about it as a percentage error of the setpoint. i.e. steady state error of +/- 2% might be a requirement. Of course I think we were always talking about controlling speed and not controlling position.

I think I will add my own OnTarget implementation to the WPILib and recompile it. Something like this:

bool PIDController::OnTarget(bool percent_error)
{
	bool temp;
CRITICAL_REGION(m_semaphore)
{
    if(percent_error)
    {
         temp = fabs(m_error)/m_setpoint < m_tolerance;
    }
    else
    {
        temp = fabs(m_error) < (m_tolerance / 100 * 
            (m_maximumInput - m_minimumInput));
     }
}
END_REGION;
return temp;
}

I also agree with this.

I think the reason they are part of the critical region is that they are accessed in the calculation of the output of the controller and can also be modified at any time by the application threads via methods such as this one.

void PIDController::SetInputRange(float minimumInput, float maximumInput)
{
	CRITICAL_REGION(m_semaphore)
	{
		m_minimumInput = minimumInput;
		m_maximumInput = maximumInput;	
	}
	END_REGION;

	SetSetpoint(m_setpoint);
}

Ether · February 23, 2012, 3:44pm

jwakeman:

I think the reason they are part of the critical region is that they are accessed in the calculation of the output of the controller and can also be modified at any time by the application threads via methods such as this one.
void PIDController::SetInputRange(float minimumInput, float maximumInput)
{
	CRITICAL_REGION(m_semaphore)
	{
		m_minimumInput = minimumInput;
		m_maximumInput = maximumInput;	
	}
	END_REGION;

	SetSetpoint(m_setpoint);
}

Fair enough. They’re not constants.

The purist in me would grab copies of the variables in the critical region and move the calculation outside the critical region, but it would hardly make any difference in this case.

wireties · February 23, 2012, 4:30pm

They are class members but still - good advice. Critical regions should contain as little code as possible.

wireties · February 23, 2012, 4:35pm

Just override the class (or copy it) - we can’t recompile the lib and even if we could the FIRST infrastructure checks the MD5 checksum.

True but these class members are native machine word-sized thus changing them is an atomic operation in this context. It may not be desirable but it won’t screw up.

HTH

Joe_Ross · February 23, 2012, 5:24pm

I’ve never heard that before. Where did you hear it? I would have thought that Joe Hershberger would have mentioned the MD5 check instead of helping with instructions on how to rebuild the library in the following thread. http://www.chiefdelphi.com/forums/showthread.php?t=89131

I do agree it’s better to override a method if possible.

jwakeman · February 23, 2012, 6:40pm

Doesn’t sound right to me either. I don’t think there exists any requirement that we have to use WPILib at all when programming in C++. It is there for convenience.

Makes sense, I was on that thread and that is where I learned to do the recompile.

The method is virtual so this is an option.

jwakeman · February 23, 2012, 6:53pm

I think you’re correct that there is no danger of segmentation faults. However, if access to the variables is not synchronized then the potential for logic errors exist.

Example, in the following code the first if-statement could execute with the values of min/max input set to one set of values. The thread executing this code could then be preempted (depending on the thread priority and scheduling algorithm being used) by the application thread and the values of min/max input changed. The original thread would then continue executing the code with new values of min/max input causing undesireable results.

				
if (fabs(m_error) > (m_maximumInput - m_minimumInput) / 2)
    {
        if (m_error > 0)
       {
            m_error = m_error - m_maximumInput + m_minimumInput;
       }
       else
       {
           m_error = m_error + m_maximumInput - m_minimumInput;
       }
}

jwakeman · February 23, 2012, 6:58pm

Can someone explain the difference between these two uses of the semaphore? I see both being used in the PIDController class.

CRITICAL_REGION(m_semaphore)
{
}
END_REGION;

{
    Synchronized sync(m_semaphore);
}

vamfun · February 23, 2012, 7:46pm

				
if (fabs(m_error) > (m_maximumInput - m_minimumInput) / 2)
    {
        if (m_error > 0)
       {
            m_error = m_error - m_maximumInput + m_minimumInput;
       }
       else
       {
           m_error = m_error + m_maximumInput - m_minimumInput;
       }
}

Now that you brought this code up, would someone mind explaining why it is there? What is maximumInput and minimumInput? They both have a default value of zero. If maximumInput = 1 and minimumInput = -1 then the error is being biased by -2 for m_error>0 and 2 for m_error<0 if abs(m_error) > 1. So lets say that m_error = 1.2 then m_error is biased to become -.8 . What is this accomplishing?

wireties · February 23, 2012, 10:38pm

My apologies - I could swear I tried this the first year we had the cRIOs and you got a library version error. When I asked about it, the WPI folks said you had to have special permissions (and be on the “build team”) to access the svn repository and do a proper build. Obviously that is no longer the case (if it ever was). Sorry to steer anybody wrong!

wireties · February 23, 2012, 10:41pm

… thus the “may not be desirable” caveat ;o)

wireties · February 23, 2012, 10:48pm

jwakeman:

Can someone explain the difference between these two uses of the semaphore? I see both being used in the PIDController class.
CRITICAL_REGION(m_semaphore)
{
}
END_REGION;
{
    Synchronized sync(m_semaphore);
}

the first is a semaphore used for mutual exclusion (to protect the critical region), in this use-case the semaphore is taken before and given after a critical region of code and always in the same context - the semaphore includes the concept of ownership (by the thread/task) and recursion

the second is a semaphore used for synchronization, in this use-case the semaphore is taken in one context/thread/task and given or flushed in another context - for example given/flushed in a periodic timer callback function (or maybe an interrupt service routine) and taken in a task/thread

hth

jwakeman · February 24, 2012, 11:05am

I figured. So in QNX parlance (which I am most familar with) these would be a Mutex and Condition Variable. However, I don’t see where the semaphore is being flushed in the PIDController implementation. The periodic loop is accomplished with a Notifier calling the Calculate() method periodically. Is the sync actually being used somehow here?

jwakeman · February 24, 2012, 11:32am

It is simply the minimum and maximum setpoint that the controller will accept. In the case of my shooter speed controller they are set to 0 and 100 respectively (units are feet per second in this case). For my turret position controller I am using -45 to 45 (degrees) and my turret speed controller accepts -250 to 250 (degrees/sec)…i think you get the idea.

The code I posted actually falls within one more if-statement, if(m_continuous). I’ve also always wondered what this code is meant to represent. What is meant by continuous in this case? Also running some values though the code doesn’t help me understand at all.

Using my example of a shooter speed controller with minimum input of 0 and maximum input of 100 fps. Say I step my setpoint from 0 to 100. My error would be equal to 100 on the first iteration. So 100 is greater than (100 - 0) / 2 and 100 is greater than zero so m_error ends up being set equal to 100 - 100 + 0. So if I step my speed controller from zero to maximum the output will be computed as zero?!

Another scenario, I step the setpoint from 0 to 60. The error of 60 is greater than 50 and greater than zero so the m_error becomes 60 - 100 + 0 = -40. Now my positive error became negative error…I’m confused.

if (m_continuous)
{
    if (fabs(m_error) > (m_maximumInput - m_minimumInput) / 2)
    {
        if (m_error > 0)
        {
            m_error = m_error - m_maximumInput + m_minimumInput;
        }
        else
        {
            m_error = m_error + m_maximumInput - m_minimumInput;
        }
    }
}