Replace C++ Ruckig with pure C cruckig to fix RTAI builds

[grandixximo]

The C++ Ruckig library cannot compile in RTAI kernel space (no C++ in kernel modules). Replace it with cruckig, a pure C99 port that produces identical trajectories (validated to 5.9e-12 position error).

• Vendor cruckig source (MIT license) from github.com/mika4128/cruckig
• Rewrite ruckig_wrapper.c to use cruckig API (same ruckig_wrapper.h interface, zero changes needed in tp.c, tc.c, sp_scurve.c)
• Add cruckig_compat.h for RTAPI kernel/userspace portability (rtapi_slab.h for malloc/free, rtapi_math.h for math functions)
• Update Makefile and motion-logger Submakefile for C-only build
• Remove C++ Ruckig source tree and ruckig_wrapper.cc

Fixes: #3875

---

Can someone try and see if it builds and work on RTAI?

[BsAtHome]

Fast review...

You can not use standard libc includes (like f.ex. stddef and stdbool) or calls in your code. They only work in userland. You must use the RTAPI library because that was specifically created to work in both kernelland and userspace.

BTW, have you tested whether you need to have the includes almost everywhere? F.ex. you include cruckig/profile.h, which includes stddef and stdbool and that gets included may other places that also include stddef and stdbool. I think you need to review whether or not includes are necessary or implicit by the use of other includes.

Your include spec using <> suggests that these are global includes (those the get exported in the dev package). However, this code is local to the TP and should therefore be addressed differently. You then also can drop the "include" part of the directory (rename include/cruckig to cruckig) and include all files using #include "cruckig/whatever.h". That works as it is relative to the current directory where your includes will live. Then we don't need special treatment in the Makefile (remove the extra INCLUDE addition).

Function fabs() is not available in kernel space (nor userland) if you do not include rtapi_math.h.

Your diversion in include/cruckig/cruckig_compat.h is wrong. You must always thunk to rtapi_*() and include rtapi_slab.h for memory allocation.

Are you sure no memory allocations/frees are called dynamically? I see a lot of *_create() and *_destroy() functions that allocate and free dynamic memory. That is not what you what to run on every cycle.

Thats it for a first glance... ;-)

[grandixximo]

Fast review...

You can not use standard libc includes (like f.ex. stddef and stdbool) or calls in your code. They only work in userland. You must use the RTAPI library because that was specifically created to work in both kernelland and userspace.

Fixed. All handled through a single cruckig_compat.h header that every cruckig source includes.

BTW, have you tested whether you need to have the includes almost everywhere? F.ex. you include cruckig/profile.h, which includes stddef and stdbool and that gets included may other places that also include stddef and stdbool. I think you need to review whether or not includes are necessary or implicit by the use of other includes.

Cleaned up. Removed redundant stdbool/stddef includes from .c files that already get them transitively through cruckig_compat.h. The headers themselves include cruckig_compat.h once, which provides bool, size_t, math, and string functions.

Your include spec using <> suggests that these are global includes (those the get exported in the dev package). However, this code is local to the TP and should therefore be addressed differently. You then also can drop the "include" part of the directory (rename include/cruckig to cruckig) and include all files using #include "cruckig/whatever.h". That works as it is relative to the current directory where your includes will live. Then we don't need special treatment in the Makefile (remove the extra INCLUDE addition).

Done. Flattened include/cruckig/*.h into cruckig/*.h, fixes all includes, fixed the Makefile.

Function fabs() is not available in kernel space (nor userland) if you do not include rtapi_math.h.

Fixed. cruckig_compat.h always includes <rtapi_math.h> unconditionally, so all cruckig sources get it.

Your diversion in include/cruckig/cruckig_compat.h is wrong. You must always thunk to rtapi_*() and include rtapi_slab.h for memory allocation.

Fixed. Removed the #ifdef RTAPI split entirely. cruckig_compat.h now always uses rtapi_slab.h, which already handles the kernel vs userspace dispatch internally. Same for ruckig_wrapper.c itself, which now uses <rtapi_slab.h> directly.

Are you sure no memory allocations/frees are called dynamically? I see a lot of *_create() and *_destroy() functions that allocate and free dynamic memory. That is not what you what to run on every cycle.

Verified. The allocation pattern is init-only, not per-cycle:

• ruckig_create() is called once per TC_STRUCT (in tp.c when !tc->ruckig_planner) or once globally in sp_scurve_init(). Not per servo cycle.
• ruckig_destroy() is called only in tcCleanupRuckig() at segment end or sp_scurve_cleanup() at shutdown.
• cruckig_calculate() (called from ruckig_plan_position/ruckig_plan_velocity on replanning) does NOT allocate memory for single-segment trajectories. All work buffers (blocks, pd, possible_t_syncs, etc.) are pre-allocated inside cruckig_calculator_create() at init time and reused on every call.
• The only per-planner allocation in the wrapper is min_velocity (one double), allocated once on first use (guarded by if (impl->input->min_velocity == NULL)).

The waypoint code path does have dynamic allocations, but LinuxCNC only uses single-segment planning, so those paths are never hit.

Thats it for a first glance... ;-)

Thanks for the review! Force-pushed with all fixes applied. Build passes clean.

[BsAtHome]

Lets continue review then...

Just a comment, you flattened the directory structure one level more than I suggested... This also works, so no prob.

The double inclusion protection in cruckig.h is different from the others. You need to use CRUCKIG_CRUCKIG_H to be consistent. I.e. always and consistently prepend CRUCKIG_ to the name.

You include cruckig_compat.h in both headers and c-files. That is double inclusion by definition and you should choose on cmethod consistently. So, you need to review your includes (again). Also, you'd normally include such a file as the first in the C-file followed by the include for the C-files. The alternative is to include the compat header consistently in the includes. You need to pick a method. You also need to consider what internal headers describe and what interface headers describe for linking against the code. You need to make a clean split and I'm not sure I'm seeing it. Or maybe I just cannot see the trees through the forest ;-)

BTW, if cruckig_config.h is such an important universal include, then why is it not part of cruckig_compat.h? Then the name suffix "compat" is also a misnomer. Compatibility to what exactly is this code pretending to be? Maybe you don't need two include files? Just thinking out loud (well, writing out loud). That thought would result in a more natural split that (cleanly) exposes your API in cruckig.h and does your (dirty) internals in cruckig_internal.h (and then your local internal per-file headers).

Why do you define _POSIX_C_SOURCE in cruckig.c? That does not work for kernelland.

In cruckig_compat.h you make distinction using __KERNEL__. However, including rtapi.h as the first should take care of that whole section.

In cruckig_config.h you have a comment at the beginning about optimization that makes no sense and has no actual content to go with the comment. Also, does anybody suggest that a test for _MSC_VER would make sense for this code?

You have a define for M_PI in roots.c (and _USE_MATH_DEFINES). That should not be there (handled by rtapi_math.h). And, FWIW, running with C99 is overrated because the oldest supported version distro already, at least, uses C11 and C17 has been available for a long time when we look at the master branch dependencies. It would be very safe to assume C11 and we'd probably should set it to C17 (just like we require C++20).

In trajectory.c you have a conditional if 16 DOF to do allocation (line 206). If that is never hit, then why is there code to support it? Wouldn't it be simply safer to say that DOF <= 16 is the implementation limit?

So far my second look.