Call to PIDController::GetError() and ::OnTarget() locks program.

[KGU]

We have been working with the PID controller and have a simple set up with a motor and potentiometer. The motor tries to control to set point as it should, but only if the we are not using the GetError or OnTarget methods. If either of these methods are called, the program locks up and does not continue on to the rest of the code. Here is a snippet of our code (we initialize everything in a separate header not shown):

CRobotMain::CRobotMain() : SimpleRobot()
{
	// Create our object pointers.
	m_pPot		= new AnalogChannel(1, 2);
	m_pMotor	= new Talon(1, 8);
	m_pPID		= new PIDController(0.1, 0.001, 0.0, m_pPot, m_pMotor);
}

void CRobotMain::OperatorControl()
{
	float	fError;
	bool	bOnTarget;
	
	m_pPID->SetOutputRange(-0.25, 0.25);
	m_pPID->SetInputRange(0, 1023);
	m_pPID->SetAbsoluteTolerance(50);
	m_pPID->SetContinuous(true);
	m_pPID->Enable();
	
	// Continuously run loop while in teleop mode.
	while (IsOperatorControl())
	{
		m_pPID->SetSetpoint(250);

///		fError = m_pPID->GetError();
///		bOnTarget = m_pPID->OnTarget();
		
		Wait(0.050);
	}
}

If either of the "m_pPID->GetError();" or "bOnTarget = m_pPID->OnTarget();" lines are uncommented, the code no longer functions.

[Joe Ross]

See if the following bug report seems like it could cause your issue: http://firstforge.wpi.edu/sf/go/artf1595

[jwakeman]

I ran into this tonight as well. Last year the implementation of PIDController:GetError() just set error = m_error inside the critical section. Anyone know the reason for now trying to set error = GetSetpoint() - m_pidInput->PIDGet()? Seemed like just returning m_error was good enough..

[jwakeman]

Originally Posted by jwakeman

I ran into this tonight as well. Last year the implementation of PIDController:GetError() just set error = m_error inside the critical section. Anyone know the reason for now trying to set error = GetSetpoint() - m_pidInput->PIDGet()? Seemed like just returning m_error was good enough..

After this weekend I think I understand now why they attempted to update the GetError() method this year. With last year's implementation the following code results in a race condition. It will not work if the OnTarget() method ends up running before the PID gets a chance to run its calculation loop at least once.

myPID->SetSetpoint();
while(!myPID->OnTarget()){}

[Jefferson]

Replace your GetError function with this. It should work.

/**
* Retruns the current difference of the input from the setpoint
* @return the current error
*/
float PIDController::GetError() {
float error;
PIDSource *pidInput;
CRITICAL_REGION(m_semaphore)
{
pidInput = m_pidInput;
error = GetSetpoint() - pidInput->PIDGet();
}END_REGION;
return error;
}

[JDNovak]

Scratch that last post. I thought this fixed the issues I was having during the Beta season, but it's blowing up for me now. If anybody has a work-around, I'd love to try it out.

[Jefferson]

And ^ that's what I get for not checking who is logged in to CD on the programming computer!

Sorry John. I'm going to bed now.

[jwakeman]

I don't have the code in front of me right now but i think i ended up using this:

float PIDController::GetError() 
{
   float error;
   CRITICAL_REGION(m_semaphore)
   {
      error = m_setpoint - pidInput->PIDGet();
   }END_REGION;
}
return error;

Also in the OnTarget() method I do the same calculation to get the error instead of calling GetError().

Even with this updated implementation of GetError() I also learned last night not to call GetError() within PIDWrite().

[BradAMiller]

Sorry about the bug. There will be a fix posted shortly as soon as we finish fixing another issue with SmartDashboard.

Basically the problem is that the critical region uses a binary semaphore which blocks on multiple takes from the same thread. The code has a bunch of CRITICAL_REGIONS nested so it's hanging.

The fix was to replace the semaphore with a Mutual Exclusion semaphore which will block on multiple tasks trying to access the code, but not when the same task tries.

Brad

[jwakeman]

Originally Posted by BradAMiller

Sorry about the bug.

No need to apologize! Thanks for your hard work and support! I just felt the need to say that because I have seen a few posts on CD where folks are incensed because they have found an issue with WPILib. I try to remind myself that they are probably just young kids or don't understand that WPILib is put out by a few hard working people (volunteers?) and not some large corporation. When Microsoft Word crashes and takes all my work with it I get mad but when I find an issue with WPILib I try to report it and offer a solution if possible.

[Jefferson]

Originally Posted by jwakeman

No need to apologize!

Seconded! We appreciate your hard work, Brad.