G2 that triggers an ARC_ANGULAR_TRAVEL_EPSILON related calculation error

[bjerrep]

FreeCAD cam generated a gcode with a G2 that makes grblhal fall over. This snippet contains the G2:

G17 G90
G21
G54
M3 S17000
G0 Z1.000
G0 X54.000 Y5.431
G1 X54.000 Y3.600 Z-1.800 F300.000
G1 X54.000 Y4.231 Z-1.800 F300.000
G2 X54.000 Y3.600 Z-1.800 I-1379288.060 J-0.621 K0.000 F300.000
G0 Z1.000
M5
G17 G90
M2

This is an arc with a radius of some 1.38 kilometres. It might be the result of me setting an unreasonable precision in FreeCAD somewhere but as far as I can tell it remains a valid G2 statement nevertheless. Since it is so close to horizontal the following line in motion_control.c produces an angle just below ARC_ANGULAR_TRAVEL_EPSILON:

float angular_travel = (float)atan2(rv.x * rt.y - rv.y * rt.x, rv.x * rt.x + rv.y * rt.y);

which triggers an unfortunate "angular_travel -= 2pi" in the following:

if (turns > 0) { // Correct atan2 output per direction
if (angular_travel <= ARC_ANGULAR_TRAVEL_EPSILON)
angular_travel += 2.0f * M_PI;
} else if (angular_travel >= -ARC_ANGULAR_TRAVEL_EPSILON)
angular_travel -= 2.0f * M_PI;

I guess this issue can now go in a number of directions, the first probably beeing 'dont make rediculous G2 with insane radii' or something like that. But if grblhal shouldn't produce potential tool crashes on otherwise 'valid' gcode then there perhaps is something to look into?

For now I just tried to comment the if-else away and now everything is working as expected with the FreeCAD G2. I suspect the ARC_ANGULAR_TRAVEL_EPSILON checks have been around since the dawn of time with softfloat on 8 bit or what not. I get a little lost trying to figure out why an slightly unreasonable small angle is not just left as beeing excactly that, but need to get translated into a full circle. And there are probably one or more very valid and concrete usecases for the fix somewhere, but i haven't found them, or been able to figure it out.

Just some final notes: changing the precision from float to double might move problematic arc centres to mars or thereabout but it won't really fundamentally fix the 2pi thing. If the atan2 is suspected of some small nasty calculation roundoff errors then it might make more sense to compare the sign to the first (y) argument to atan2 and the sign of the resulting angle. They should be the same or something has gone wrong in atan2. Not sure if that would be useful for what the fixup code above tries to accomplish?

[terjeio]

Some info here.

[bjerrep]

Thanks, but I did read that. Its a nicely written comment but it didn't really help me appriciate the effect of turning a very small arc to a full circle. I still can't help think that this fixed an error found in a very dated math softfloat thingy way before 32 bit. I failed to mention that the few gcode visualizers I tried with the gcode above all worked fine.
I appreciate that it is part of working code and has been for a long time and that I should somehow be able to prove that it isn't valid anymore. And that I am afraid requires someone with floating point skills that I don't have.

I guess this issue should be closed again but I will let that be up to you.

[terjeio]

I still can't help think that this fixed an error found in a very dated math softfloat thingy way before 32 bit.

Softfloat on most 8-bit controllers is likely to be IEEE-754 compatible and should not behave differently from hardware FPU or soft 32-bit implementations. So IMO the only way to get around this issue is to change the codebase to use double precision.

[bjerrep]

Regarding precision:

I guess my primary crime was not to fully comprehend that this natively is a float codebase and actually realise how relatively few decimal places a float is able to handle. So the Freecad 'I-1379288.060' actually becomes -1379288.000 after the input parsing. I don't know if it would be a job for grblhal to reject gcode with floating point that it is unable to parse 100% correctly? (it's not really related to the issue here though)

I don't think I can say anything useful regarding your comment about a general change to double precision. I am working on a STM32H743 which for one disqualifies me for appreciating implications which might exist for more constrained processors.

Regarding the circle correction:

Just to get a feel for how the 'float angular_travel = (float)atan2(y, x);' line works I tried among others the following 3 sets:

G0 X0.00000 Y0.00000
G2 X0.00000 Y0.00002 I1 J0.00001 F100
gives angular_travel=-2.000000e-05

G0 X0.00000 Y0.00000
G2 X0.00000 Y0.00002 I10000 J0.00001 F100
gives angular_travel=-2.000000e-09

G0 X0.00000 Y0.00000
G2 X0.00000 Y0.00002 I1000000000 J0.00001 F100
gives angular_travel=-2.000000e-14

which suggests that the double world of atan2 and its arguments, and its subsequent float conversion, doesn't seem to have any intrinsic problems around ARC_ANGULAR_TRAVEL_EPSILON (again on a STM32H743). Hardly an impressive proof but for now at least I am living happily without the circle correction code.

[bjerrep]

Oh no, not him again. Anyway I am happy to inform you that I now have a new pet theory
regarding the ARC_ANGULAR_TRAVEL_EPSILON. It is so trivial that I think it might be right.

Without the grbl ARC_ANGULAR_TRAVEL_EPSILON code a full circle doesn't work anymore.

G1 X17.342 Y4.086 Z-0.014 F7.000
G3 X17.342 Y4.086 Z-0.014 I-1.250 J0.000 K0.000 F40.000

Here angular_travel gets zero and the circle disappears without the ARC_ANGULAR_TRAVEL_EPSILON
fixup. This makes the grbl comment you linked to earlier make more sense.

Instead I am now trying my luck with this modification:

float angular_travel = 0.0;

if ((rv.x - rt.x == 0.0) && (rv.y - rt.y == 0.0)) {
    // If we end up where we start then this is per definition a full circle covering 2*pi radian.
    angular_travel = 2.0f * M_PI;
}
else {
    // CCW angle between position and target from circle center. Only one atan2() trig computation required.
    angular_travel = (float)atan2(rv.x * rt.y - rv.y * rt.x, rv.x * rt.x + rv.y * rt.y);
}

This brings back the circles. I am now waiting for my overall changes to blow up again.