[BsAtHome]

A recent change in the kinematics setup has with #4702 (re-)introduced the old HAL naming scheme. This PR updates all kinematics HAL interfacing to adhere to the new naming scheme. Simultaneously, it updates the code to run 64-bit clean.

@grandixximo A few points to consider:

1. include/kins_module.h: The old names KINS_PARAM_{FLOAT,BIT,S32,U32} are still declared but will emit a deprecation warning. Since this is a recent addition, it would probably be alright just to delete the enums with the old naming scheme. Please let me know.
2. The default init value to the pins is now a union that needs to be initialized explicitly into the correct field that matches the type. Otherwise you will potentially have a problem converting from floating point. However, the pin writing procedure is unchanged (except see point 4).
3. emc/kinematics/kins_util.c: The initialization in kinsParamsInit() now hooks into the correct source type for the default.
4. emc/kinematics/kins_util.c: Writing pins in kinsParamsPinsWrite()with sint/uint will now clamp because the source of the data is of type double and may cause rather "experimental" results when blindly converted (see next point).
5. Are there special handling requirements with NaN and Inf when writing to sint/uint? It seems that kinematics is a candidate to be possibly prone to this problem with complex calculations.

[grandixximo]

Thanks. On your points:

On your point 1: deleting the old names is fine with me, #4702 is recent. I will rename the few uses in my multiaxis branch when I rebase.

On your point 2: I think a union default makes a positional initializer silently wrong: { "x", KINS_PARAM_UINT, KINS_IN, 0, 120 } fills .r, and .u then reads 4638144666238189568 (a BOOL 1 reads false), with only a missing-braces warning. All tables here are designated, so the tree is right, but #4702's tables and any module copied from them are positional. Would keeping double dflt and converting per type where it is used be simpler? A default is small and exact in a double. Or am I missing a case where the double loses something?

On your point 5: NaN passes all three clamp comparisons and reaches the cast, which is undefined; Inf is clamped. Maybe skip the write on NaN and keep the pin's last value? In tree only the iteration counts are written to integer pins, so it would only matter for out-of-tree modules.

[BsAtHome]

Using a single type for multiple targets is basically wrong. A double does not cover the range of sint/uint properly. Sure, it may be a "small" compromise, but I think it is wrong. I suggest to change the ordering of the union fields to:

union {
    rtapi_bool b;
    rtapi_real r;
    rtapi_sint s;
    rtapi_uint u;
}

That way {} unguarded assignment will end up as boolean. Besides, compiling the tree with "missing braces" warnings cannot survive CI. It would have to be a local compile where you ignore the obvious. FWIW, I think -Werror should be enabled by default on local compiles too. Warnings are simply errors that have not yet cut off your fingers, toes, arms and legs. They will, eventually.

BTW, I also think it is wrong not to keep the internals at the correct type and always use a floating point (and do a read/write conversion). We've seen this discussion before, I think. However, I was not prepared to refactor the whole code ;-)

The NaN issue, well, not writing may be the better option. Adding a test to skip writing if isnan().

Pushed a new version, have a look what you think now.

[grandixximo]

Thanks, the bool first ordering and the isnan skip look good to me, and fair enough on the exact types.

One leftover: two doc examples still use the removed names with positional defaults, docs/src/motion/switchkins.adoc lines 708-709 ({ "pivot-length", KINS_PARAM_FLOAT, KINS_IN, 0, 100.0 }) and docs/src/motion/kinematics-conventions.adoc line 510. With the names gone they no longer compile, and a rename alone gives a pivot length of about 0 (in tree CI's -Werror catches the missing braces, but out-of-tree halcompile builds only warn, and those authors copy from these docs). Could you switch them to KINS_PARAM_REAL with {.r = 100.0}?

[BsAtHome]

Sure will fix the docs. Was silently waiting for consensus on this PR first ;-)

For out-of-tree compiles, well, as I said, I think we should default to enabling -Werror and only allow disabling with an extra option. Also halcompile stuff should be compiling cleanly or you may experience unexpected problems. But that is for another PR.

[grandixximo]

Thanks, docs look right now. Built clean and the kins tests pass here (kins-params, kins-switch*, kins-frames, kins-jacobian, kins-tool-offset, genhexkins-forward, pumakins-unwrap, tool-frame, matrixkins, maxkins). I will rename the uses in my multiaxis branch when I rebase.