Help me understand PIDController::OnTarget()

[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;
}

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]

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

[wireties]

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]

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]

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]

Originally Posted by 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]

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;
}

Originally Posted by DjScribbles

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).

I also agree with this.

Originally Posted by Ether

On a side note:
It looks like m_tolerance, m_maximumInput, and m_minimumInput are all constants.

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]

Originally Posted by 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]

Originally Posted by Ether

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

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

[wireties]

Originally Posted by jwakeman

I think I will add my own OnTarget implementation to the WPILib and recompile it.

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.

Originally Posted by 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.

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]

Originally Posted by wireties

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.

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/sh...ad.php?t=89131

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

[jwakeman]

Originally Posted by Joe Ross

I've never heard that before. Where did you hear it?

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.

Originally Posted by Joe Ross

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/sh...ad.php?t=89131

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

Originally Posted by Joe Ross

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

The method is virtual so this is an option.

[jwakeman]

Originally Posted by wireties

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.

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]

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]

				
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?