polar kinematics speed limiting code

achrn

In PolarKinematics.cpp there's something I don't understand, which might be a bug (but I'm nervous of saying that because normally when I think I've found something, it's me that's wrong).

The LimitSpeedAndAcceleration function is:

// Limit the speed and acceleration of a move to values that the mechanics can handle.
// The speeds in Cartesian space have already been limited.
void PolarKinematics::LimitSpeedAndAcceleration(DDA& dda, const float *normalisedDirectionVector, size_t numVisibleAxes, bool continuousRotationShortcut) const noexcept
{
	int32_t turntableMovement = labs(dda.DriveCoordinates()[1] - dda.GetPrevious()->DriveCoordinates()[1]);
	if (turntableMovement != 0)
	{
		const float stepsPerDegree = reprap.GetPlatform().DriveStepsPerUnit(1);
		if (continuousRotationShortcut)
		{
			const int32_t stepsPerRotation = lrintf(360.0 * stepsPerDegree);
			if (turntableMovement > stepsPerRotation/2)
			{
				turntableMovement -= stepsPerRotation;
			}
			else if (turntableMovement < -stepsPerRotation/2)
			{
				turntableMovement += stepsPerRotation;
			}
		}
		if (turntableMovement != 0)
		{
			const float stepRatio = dda.GetTotalDistance() * stepsPerDegree/abs(turntableMovement);
			dda.LimitSpeedAndAcceleration(stepRatio * maxTurntableSpeed, stepRatio * maxTurntableAcceleration);
		}
	}
}

At line 5 of the snippet, turntableMovement is set to an absolute (ie positive) number:

int32_t turntableMovement = labs(dda.DriveCoordinates()[1] - dda.GetPrevious()->DriveCoordinates()[1]);

So then what is this doing: else if (turntableMovement < -stepsPerRotation/2)
This will never evaluate true:

If reprap.GetPlatform().DriveStepsPerUnit(1) is positive (which is reasonable, but I haven't checked) it plainly won't evaluate true because stepsPerRotation will be positive and turntableMovement is positive, so we have 'if [+ve number] < [-ve number]'.
If reprap.GetPlatform().DriveStepsPerUnit(1) is negative, this won't evaluate at all because the previous if (turntableMovement > stepsPerRotation/2) will become 'if [+ve number] > [-ve number]', so will always be true, so the else if will never be tested.

I think turntableMovement should be created signed, which also makes sense of the subsequent abs(turntableMovement) at line 23 of the snippet.

dc42

@achrn I agree, it looks like the labs() call should be removed. I will remove it in 3.4beta7.