https://github.com/LinuxCNC/linuxcnc/pull/3334 affects RTAI build

[andypugh]

#3334 uses "labs()" which is not defined in kernel mode in rtapi_math.h

There are many ways to fix this, from a local fix in sserial.c, up to a complete rethink of rtapi_math.h (possibly using <linux/math,h> which was apparently introduced in 2020.

[BsAtHome]

Including kernel header linux/math.h might be an option, but the userland labs() prototype is in stdlib.h.

It may be best just to add the inline version from #3427 (comment) to rtapi/rtapi_math.h when __KERNEL__ is defined.

[BsAtHome]

A modification defining rtapi_labs() in rtapi/rtapi_math.h should alleviate the pains between uspace and kernel builds. The change led to a funny other problem where the inclusion of stdlib.h caused the max31855 component to fail compilation because of the variable 'state'. Not sure why, but a simple rename fixed that.

The below diff should fix the problem for both uspace and RTAI compiles. Please test on RTAI to see if it should become a PR.

diff --git a/src/rtapi/rtapi_math.h b/src/rtapi/rtapi_math.h
index 262bb99532..3ea3992fac 100644
--- a/src/rtapi/rtapi_math.h
+++ b/src/rtapi/rtapi_math.h
@@ -61,9 +61,18 @@ extern double floor(double);
 #include "rtapi_math_i386.h"
 #endif
 
+// The kernel has no labs() function. It has a type-adapting abs() define in
+// linux/math.h which we can use.
+#include <linux/math.h>
+#define rtapi_labs(x)	abs(x)
+
 #else
 #include <math.h>
-#endif
+
+#include <stdlib.h>
+#define rtapi_labs(x)	labs(x)
+
+#endif // if defined(__KERNEL__)
 
 #include "rtapi_byteorder.h"
 
diff --git a/src/hal/drivers/mesa-hostmot2/sserial.c b/src/hal/drivers/mesa-hostmot2/sserial.c
index 7cb724316b..e6bebfcea7 100644
--- a/src/hal/drivers/mesa-hostmot2/sserial.c
+++ b/src/hal/drivers/mesa-hostmot2/sserial.c
@@ -1550,7 +1550,7 @@ fail1:
                                 case 64:
                                     shift = 0;
                                 }
-                            if (labs((p->s64_param - p->s64_written) >> shift) > 2) break;
+                            if (rtapi_labs((p->s64_param - p->s64_written) >> shift) > 2) break;
                             *inst->state2 = 2; // increment indices
                             return *inst->state2;
                         default:
diff --git a/src/hal/components/max31855.comp b/src/hal/components/max31855.comp
index 3706df8c83..3ee9b785fa 100644
--- a/src/hal/components/max31855.comp
+++ b/src/hal/components/max31855.comp
@@ -38,7 +38,7 @@ pin out bit fault.# [15 : (personality & 0xf)]  "Fault condition detected.";
 pin out unsigned fault_flags.# [15 : (personality & 0xf)]  "Fault flags: 0x1  = open sensor, 0x2 short to gnd, 0x3 short to vcc.";
 
 variable unsigned data_frame [15];
-variable unsigned state = 1;
+variable unsigned spistate = 1;
 
 option period no;
 function bitbang_spi fp;
@@ -153,9 +153,9 @@ static float read_celsius_adjusted(int32_t sensor_data) {
 }
 
 FUNCTION(bitbang_spi) {
-  int nbit = (state >> 1) & 0x3f;
-  int delay = (state >> 7) & 0x3ff;
-  int cs = state & 0x1;
+  int nbit = (spistate >> 1) & 0x3f;
+  int delay = (spistate >> 7) & 0x3ff;
+  int cs = spistate & 0x1;
 
   clk_out ^= 0x1;
   if (cs) {
@@ -193,6 +193,6 @@ FUNCTION(bitbang_spi) {
     }
     delay++;
   }
-  state = (delay << 7) | (nbit << 1) | cs;
+  spistate = (delay << 7) | (nbit << 1) | cs;
   cs_out = cs;
 }

[andypugh]

Having tried this, somewhat annoyingly the usual RTAI kernel for Bookworm is 5.4.258 and does not have the linux/math library.

Also, according to "man labs" "By default, GCC handles abs(), labs(), and (since GCC 3.0) llabs() and imaxabs() as built-in functions." (I guess we don't compile "by default")

I am thinking that the answer might be

#define labs(x) (x < 0) ? -x : x

It's not like abs, labs etc are complicated functions....

[BsAtHome]

We should not use a #define because x is evaluated several times.

I suggest that we just add a local inline function in sserial.c to prevent that from happening:

static inline rtapi_s64 xlabs(rtapi_s64 x) { return x < 0 ? -x : x; }

and change the call to xlabs(). I'll make a PR.

[andypugh]

Why xlabs and not rtapi_labs?

[BsAtHome]

Because the prefix 'rtapi_' is for functions declared/defined in the rtapi abstraction files. This is a local fix that only exists in the sserial.c file.

[andypugh]

Because the prefix 'rtapi_' is for functions declared/defined in the rtapi abstraction files. This is a local fix that only exists in the sserial.c file.

I realised this as I was walking to the cafetaria for a coffee, and planned to delete the question on my return :-)