Fix/tp circle curvature by Rimas2200 · Pull Request #4643 · LinuxCNC/linuxcnc

Rimas2200 · 2026-10-06T04:43:49Z

pmCircleEffectiveMinRadius() uses the total axial rise of a command in its curvature estimate. For a constant-radius helix with R=10 mm, rise=20 mm and a full turn, it gives a radius of 50 mm. Splitting the same path into four arcs changes the estimate to 12.5 mm per arc. The geometric curvature radius is about 11.0132 mm in both cases.

Change

Calculate curvature using axial rise per radian and find the maximum curvature along a spiral.
Apply the corrected normal-acceleration limit to constant-radius helices. Keep the existing jerk limits and the existing motion limits for general spirals.
Recalculate limits when blending changes arc geometry, and add geometry, limit and headless motion tests.

Testing and limits

The TP regression tests and headless simulation tests passed. The committed motion test checks completion and endpoints; it does not assert acceleration.

In a separate POSIX simulation capture (1 ms servo period, F6000, acceleration limit 100 mm/s^2), peak commanded acceleration decreased from about 134 to 104 mm/s^2. Some overshoot remains, so this change should not be taken as full acceleration-limit compliance.

grandixximo · 2026-10-06T04:53:37Z

@Rimas2200 shellcheck fails on the test script

@thomam04 could you check this, I know you do spirals a lot :-)

BsAtHome

Only looked at form. Content/calculation review must be done by someone more familiar with this code.

BsAtHome · 2026-10-06T07:26:57Z

+"${CC:-cc}" ${CFLAGS:--O2} -std=gnu11 -Wall -Wextra -fno-fast-math \
+    -ffunction-sections -fdata-sections -DULAPI \
+    -I"$HEADERS" -I"$TOPDIR/src" -I"$TOPDIR/src/emc" -I"$TOPDIR/src/emc/tp" \
+    test.c "$TOPDIR/src/emc/tp/tc.c" "$TOPDIR/src/emc/tp/blendmath.c" \
+    "$TOPDIR/src/emc/tp/circle_curvature.c" "$TOPDIR/src/emc/tp/cruckig/roots.c" \
+    -L"$LIBDIR" -Wl,-rpath,"$LIBDIR" ${LDFLAGS:-} -Wl,--gc-sections -lposemath -lm \


This is problematic. You are recreating compiler options which you do not control and will be hard to maintain.

Can't you create a component and do the test in rtapi_app_main(), just like other tests?

BsAtHome · 2026-10-06T07:27:08Z

-    if (!line || tc->motion_type != TC_LINEAR) {
+STATIC __attribute__((__noinline__))
+int tpPrepareGeometryChange(TC_STRUCT const *tc, PmCircle const *circle,
+        PmCartLine const *line, int previous, tp_prepared_geometry_t *prepared)


Why is this marked noinline? (and tpValidateFinalization below too)
The compiler is a better judge that most humans.

BsAtHome · 2026-10-06T07:28:35Z

+"${CC:-cc}" ${CFLAGS:--O2 -g} -std=gnu11 -Wall -Wextra -fno-fast-math \
+    -ffunction-sections -fdata-sections -DULAPI \
+    -I"$root/src/emc/tp" -I"$root/src/libposemath" -I"$root/src/rtapi" \
+    -I"$root/src/emc/nml_intf" -I"$root/src/emc/motion" \
+    -I"$root/src/emc/kinematics" -I"$root/src/hal" -I"$root/include" \
+    "$here/test.c" "$root/src/emc/tp/circle_curvature.c" \
+    "$root/src/emc/tp/blendmath.c" "$root/src/libposemath/_posemath.c" \
+    ${LDFLAGS:-} -Wl,--gc-sections -lm -o "$build/test"


This command line is even more convoluted and uses include statements that are incompatible and unmaintainable. Additionally, we've been removing all these extra -I options in the Makefile and now you are adding them here in the test.

BsAtHome · 2026-10-06T07:32:47Z

+    if (!tc || !isfinite(tc->target) || tc->target < 0.0 ||
+        !isfinite(tc->cycle_time) || tc->cycle_time <= 0.0 ||
+        !isfinite(tc->maxvel) || tc->maxvel < 0.0) {


Can a value become -1e-15 or so due to rounding?

BsAtHome · 2026-10-06T07:34:59Z

+/* SPDX-License-Identifier: GPL-2.0-only */
+#include <rtapi_math.h>
+#include "circle_curvature.h"
+


Is this new file used anywhere other than once? The header only seems to be included once.

BsAtHome · 2026-10-06T07:37:50Z

+    if (!isfinite(radius) || radius <= 0.0 ||
+        !isfinite(jerk_radius) || jerk_radius <= 0.0 ||
+        !isfinite(angle) || angle <= 0.0 ||
+        !isfinite(a_max) || a_max <= 0.0 ||
+        !isfinite(input_maxvel) || input_maxvel < 0.0 ||
+        !isfinite(tc->cycle_time) || tc->cycle_time <= 0.0) {


When do infinities/singularities occur?
Can they be prevented from occurring? Having to deal with them here seems too late if they could have been prevented.

BsAtHome · 2026-10-06T07:40:08Z

-/**
- * Given a PmCircle and a circular segment, copy the circle in as the XYZ portion of the segment, then update the motion parameters.
- * NOTE: does not yet support ABC or UVW motion!
- */


Why all the extra pointer arguments. Can' t they be passed as one structure reference or are these not related to belong together?

grandixximo

The helix curvature formula checks out, and keeping the old estimate as a floor for sweeps under one radian makes sense. Bertho covered the form; my comments are on the content.

The fix itself is about twenty lines (rise per radian, minimum against the old radius, same radius for jerk). Could the isfinite hardening and the prepare/commit rework go in a separate PR? Bertho's questions about infinities and rounding are really about that part.

For the tests, tests/kins-jacobian (#4586) is an example of the component pattern Bertho describes.

Please squash the two fixup commits and give the PR a descriptive title.

grandixximo · 2026-10-07T09:00:48Z

+            }
+            legacy_radius = pmCircleLegacyMinRadius(circle);
+            /* For a spiral, progress is not spatial arc length. */
+            radius = circle->spiral == 0.0 ? 1.0 / curvature : legacy_radius;


Spirals still take legacy_radius here. So what uses the spiral branch of tpCircleMaxCurvature, apart from the validity checks? If nothing does, could this PR stay helix-only, as you suggested in #4642?

grandixximo · 2026-10-07T09:00:48Z

+                                rise, kappa_max);
+}
+
+double pmCircleEffectiveMinRadius(const PmCircle *circle)


Who calls this now? Same question for tcSetCircleXYZ.

grandixximo · 2026-10-07T09:00:48Z

        tpHandleBlendArc(tp, &tc);
    }
-    tcFinalizeLength(prev_tc);
+    if (prev_tc && tcFinalizeLength(prev_tc) < 0) {


By this point tpHandleBlendArc() may already have changed prev_tc and queued a blend arc. What state is the queue in if this fails? tpValidateFinalization() checks prev_tc before the blend changed it. The same applies at line 2358.

grandixximo · 2026-10-07T09:00:48Z

+    if (!tc) {
+        return TP_ERR_OK;
+    }
+    TC_STRUCT candidate = *tc;


This is a 1744-byte copy on the motion thread stack, next to the TC_STRUCT that tpAddLine already holds. Is this what the noinline attributes work around?

grandixximo · 2026-10-07T09:00:48Z

+        if result == linuxcnc.RCS_ERROR or time.monotonic() > deadline:
+            raise RuntimeError(f"move failed or timed out: {gcode}")
+    if max(abs(a - b) for a, b in zip(xyz, endpoint)) > 1e-4:
+        raise RuntimeError(f"wrong endpoint for {gcode}: {xyz}, expected {endpoint}")


Would this test fail on master? It checks endpoints only, so the change is not covered. Could it assert peak acceleration on the one-turn helix instead?

grandixximo · 2026-10-07T09:00:48Z

+        }
+        /* Keep the tighter of the old and geometric limits. */
+        maxvel = fmin(maxvel, geometric_maxvel);
+        tangent_ratio = fmin(tangent_ratio, geometric_tangent_ratio);


The ratio and the velocity can come from different radii here. That is conservative, but worth a one-line comment.

Rimas2200 added 2 commits October 6, 2026 09:23

tp: correct helical arc curvature estimate

c68212c

tests/tp: fix helix test permissions and skip system builds

de3db33

tests: fix ShellCheck SC2086 in TP test scripts

726e492

BsAtHome reviewed Oct 6, 2026

View reviewed changes

grandixximo reviewed Oct 7, 2026

View reviewed changes

Conversation

Rimas2200 commented Oct 6, 2026

Change

Testing and limits

Uh oh!

grandixximo commented Oct 6, 2026

Uh oh!

BsAtHome left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grandixximo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants