interp_g7x: make the G7x roughing cycle independent of rounding - #4487
interp_g7x: make the G7x roughing cycle independent of rounding#4487grandixximo wants to merge 3 commits into
Conversation
The g7x offsetting code takes discrete threshold branches on iterated floating-point values (segment deletion at radius<1e-3, monotonicity, arc insertion at gap>1e-2), so a 1-ulp rounding difference can flip a branch and diverge the whole toolpath. GCC defaults to -ffp-contract=fast; when targeting x86-64-v3 (ubuntu-26.04 CI runner gcc default, --with-arch-64=x86-64-v3) this emits FMA instructions and changes rounding, failing g71-with-g70 and g71-endless-loop_2 in CI. Verified: rebuilding only interp_g7x.o with -march=x86-64-v3 fails, -ffp-contract=off passes; -O0/-O2/-O3/arch without FMA are all byte-identical, so contraction is the only variable. Disable FP contraction for interp_g7x.o (same semantics as ISO C's default). Investigating the divergence also exposed latent bugs that the flipped branches merely reach: - erase(p--) on a list iterator at begin() is undefined behavior; in practice it ends the loop early, skipping offsets for all remaining segments. Use the p=erase(p) idiom (two sites). - round_segment::intersect_end(round_segment*) silently clamped |cosB|>1 (non-intersecting circles) and set p's end to a point on the wrong circle, producing arcs the interpreter itself rejects. Trim to the closest points on the line of centers instead. - the add_distance() accumulation loop requires exact FP equality; under some roundings the step becomes too small to change the sum and the loop never terminates. Break when the sum stops changing. - the add_distance() junction snap moves arc endpoints off their circle; refuse to snap across a gap larger than 1e-3 instead of manufacturing an invalid arc. All fixes keep byte-identical output on the g71 tests with the baseline arch; they only change behavior in cases that previously produced undefined or geometrically invalid results.
Discrete cases were decided by comparing floats that are meant to be equal, and lathe profiles produce those ties by construction: an arc starting at its own apex, an exactly flat facing cut, a scan line landing on top of an arc. Moving the coordinates in tests/interp/g71-with-g70 by a relative 1e-12, far below the 1e-6 tolerance the cycle works to, changed the tool path in 16 of 20 runs and hung the interpreter in another 4. Contracting a*b+c into an FMA is a much smaller disturbance than that, which is why the test failed only on a compiler defaulting to x86-64-v3. Those comparisons now carry the tolerance. The apex tie was the expensive one: broken the wrong way, pocket() re-enters the same pocket forever, which is the four hangs. add_distance() ends on the requested distance rather than on an exact comparison against an accumulated sum, replacing the stall guard from the previous commit. The thresholds are plain numbers, so they only fit a part of a few tens of millimetres; scaled up, the same perturbation broke the path at 190 mm and every run at 305 mm. The profile is normalised into a canonical range on entry and the tool path scaled back on output, by a power of two so the scaling adds no rounding of its own and is 1 at the size the thresholds were chosen for. The tool path now survives that perturbation on parts from 0.4 mm to 381 m, and interp_g7x.o gives identical output built with -march=x86-64-v3, -ffp-contract=off, -mfpmath=387, -O0 and -O3. The two lines added to the g71-with-g70 expected output are the contour segment at the flatness test, now climbed like the rest. Both are zero length repeats of moves already present, so the path is unchanged.
That would probably cover most use cases. Only machines for Dyson sphere construction and macro-molecular-manipulators would no longer fit without scaling on the hardware side. Can't have it all, I guess.
Are these final position moves to end at the correct place and not a tolerance influenced end-point?
I really don't think we should make exceptions for any file. This patch fixes the problem and that is the correct way to handle it. Any other places in the code that will be exposed as containing instabilities must get the same treatment. No problems should be hidden. And this patch fixes the problem, so why cover up for potential future problems? |
The exception was there to keep the G71 tool path away from FMA while the cycle still decided cases on exact float comparisons. It does not any more, so the file can be built like every other one and CI is free to catch it if some other part of the code cannot.
|
On the two added lines: they are not new positions. The path visits the same points in the same order; only the record count changes. Both are exact repeats of a position the path already occupies. The endpoints are the offset contour itself: X11.0000 is the programmed X10 plus the D1 allowance, Z-25.1780 and Z-27.1780 come straight from the profile. No tolerance enters the endpoint. The tolerance only decides whether On the contraction flag: agreed, dropped in the commit on top, Submakefile is back to identical with master. Kept separate from the earlier commit because that one is also in #4477 and has to stay byte identical there to rebase away cleanly. Note that this PR builds on ubuntu-24.04, so its own CI never emits an FMA and cannot exercise the case. To prove the maths instead of waiting on CI I wrote a small harness that injects perturbations far larger than contraction does: 100 lines, Python and shell, one script rewriting every coordinate in a .ngc by a given relative amount and one comparing the resulting tool paths numerically. At a relative 1e-12, four orders of magnitude above what contraction changes, master diverges in 16 of 20 runs and hangs in 4; the branch is unchanged in 30 of 30. A second pair of scripts, 51 lines, sweeps the same profile across part sizes. Happy to put either in tree if you want it, though it needs a tolerance-comparing |
2 participants
Follow-up to #4477, which hit G71 test failures on the ubuntu-26.04 runners because their gcc defaults to x86-64-v3 and contracts
a*b+cinto an FMA. That PR disables contraction for this file, which unblocks CI but leaves the fragility in place, as @BsAtHome pointed out.Root cause
The offsetting code decided discrete cases by comparing floats that are meant to be equal. Lathe profiles produce those ties by construction: an arc starting at its own apex, an exactly flat facing cut, a scan line landing on top of an arc. FMA is not the problem, it is just a disturbance small enough to expose one.
Measured by perturbing every coordinate in
tests/interp/g71-with-g70by a relative 1e-12, six orders of magnitude below the 1e-6 tolerance the cycle works to:The hangs are the failure mode the
g71-endless-looptests exist for: with the apex tie broken the wrong way,pocket()re-enters the same pocket forever.Part size
The thresholds in the file are plain numbers, so they only fit a part of a few tens of millimetres. The tangential intersections the offsetting relies on carry an error of about
r*sqrt(2*eps), which passes the tolerance on a larger part. Same perturbation, profile scaled up:The profile is normalised into a canonical range on entry and the tool path scaled back on output. The factor is a power of two, so it adds no rounding of its own, and it is 1 at the size the thresholds were chosen for. Working range goes from 20 to 114 mm, to roughly 0.4 mm to 381 m. Below that the interpreter rejects the arcs itself as zero radius, which is an error rather than a wrong path. Inch mode is covered by the same mechanism.
Verification
tests/interpandtests/m70-m73interp_g7x.ogives identical output built with-march=x86-64-v3,-march=native,-ffp-contract=off,-mfpmath=387,-O0and-O3nc_files/lathe_g7x_*demos byte-identical to masterNotes for review
g71-with-g70expected output, from makingstraight_segment::climb_only()use the same flatness test asclimb(). That contour segment is now climbed like the rest; both lines are zero length repeats of moves already present, so the path is unchanged.motion_machine, where it ran on scaled output against the interpreter's fixedTOLERANCE_EQUAL, intoscaled_motion, where it sees the normalised profile. At unit scale it is the same decision on the same numbers.-ffp-contract=offis redundant. I left it in place because only CI can confirm that on gcc 15 with a v3 default. Happy to drop it in a separate commit if you would rather have CI prove the point.