Actual movements during failed simulation

jay_s_uk

@ÖrjanE its probably still something to be looked at.
@Phaedrux or @dc42 to comment as I've not seen any other reports of movement during simulation

dc42

@ÖrjanE, if you make the radius very much too small in the G2 or G3 command then I think you will still get the "Invalid combination" error.

ÖrjanE

@dc42 Surprisingly, I don't. I actually started looking into the source code of your commit (c8ebe81fdaf91fe447ada36a67f5b30b169d80cb), and I see that it is supposed to check if hDivD is negative but very close to zero. But hDivD is the result of a call to sqrtf; will that really give a negative result if the argument is negative? Is it not necessary to check the sign of hSquared instead?

dc42

@ÖrjanE, quite so! The test should be on hSquared instead. Something like this I think:

		const float hSquared = fsquare(rParam) - dSquared/4;				// square of the length of the perpendicular from the mid point to the arc centre

		// When the arc is exactly 180deg, rounding error may make hSquared slightly negative instead of zero
		float hDivD;
		if (hSquared >= 0.0)
		{
			hDivD = sqrtf(hSquared/dSquared);
		}
		else
		{
			if (hSquared < -0.01 * fsquare(rParam))
			{
				err = badArcParametersMessage;
				return true;
			}
			hDivD = 0.0;													// this has the effect of increasing the radius slightly so that the maths works
		}

Do you agree?

ÖrjanE

@dc42 Looks correct. The interpretation of the constant -0.01 is not transparent. I guess it implies that the radius can be 10% (sqrt(0.01)) too short without throwing an error.

The current (3.2.2) code does not throw an error at all, but does not print the arc correctly if the radius is slightly too short. The resulting arc is basically a straight line, probably because the hDivD value actually used is rediculously high.

dc42

I've changed it to:

			if (hSquared < -0.001 * fsquare(rParam))						// allow the radius to be up to sqrt(0.001) ~= 3.2% too short

In the 3.2.2 code, hDivD will be a NaN if the radius is too short, so it's not surprising that it misbehaves.

dc42

Actually that's still wrong. (r + xr)^2 ~= r^2(1 + 2x) if x is small, so I will use this instead:

			if (hSquared < -0.02 * fsquare(rParam))						// allow the radius to be up to 1% too short

ÖrjanE

@dc42 Looks good!

Now, back to the other problem; that an error during simulation may cause physical movement. In 3.2.2 I can't cause the "too small radius"-error any more, but looking at the source code, I realized that I can provoke an error by setting identical start end end points for G2 or G3 with an R parameter.

This code:

G28 ; Home all axes
G21 ; set units to millimeters
G90 ; use absolute coordinates
M83 ; use relative distances for extrusion

G1 Z20.000 F7800.000
G1 F1800.000
G1 X115 Y115
G1 F200.000
G3 X115 Y115 R5
M0 ; Stop print

will cause the message "Error: SetPositions called when DDA ring not empty" as well as "Error: G2/G3: radius is too small to reach endpoint" when run in simulation mode. It also causes a physical movement (some 10mm in the negative X direction).

Hope you can replicate this behaviour.

dc42

Thanks, it sounds to me that when the error occurs, the simulation is exiting before the DDA ring has been emptied, which ties in the message.

ÖrjanE

@dc42 I have also noted another symptom, probably due to the same problem.

When printing (not simulating) the code above, the same unexpected movement as in simulation takes place when the error occurs and printing is aborted. The axes remain in homed-status but the coordinates have not been updated with that last movement.

dc42

@ÖrjanE said in Actual movements during failed simulation:

@dc42 I have also noted another symptom, probably due to the same problem.

When printing (not simulating) the code above, the same unexpected movement as in simulation takes place when the error occurs and printing is aborted. The axes remain in homed-status but the coordinates have not been updated with that last movement.

Do you get the "SetPositions called when DDA ring not empty" message again?

ÖrjanE

@dc42 said in Actual movements during failed simulation:

Do you get the "SetPositions called when DDA ring not empty" message again?

No.
And now I can't replicate that second behaviour (extra movement when not simulation). Perhaps it is state-dependent and du to some leftover state from the failed simulation.

ÖrjanE

@dc42 Forget about the "second symptom" (problem during actual print). I have restarted the duet and fail to repeat the problem. It may be that I misinterpreted what happened when I tested this.

Regarding the move when simulating; it makes sense that it is the last move/moves that remain queued when simulation mode is turned off prematurely. The move does not seem to be random, but consistent with the last move that should have been done (in simulation) before the error occurred.

dc42

Thanks for your example. I have reproduced the issue, and fixed it in the 3.3 branch source code. Moves already prepared and put into the movement queue are now completed when a job is aborted.

ÖrjanE

@dc42 Great!
Thanks for your work in maintaining this software.

dc42

@ÖrjanE said in Actual movements during failed simulation:

@dc42 Great!
Thanks for your work in maintaining this software.

Thanks for your clear description of the issue and GCode file to reproduce it.