Merge pull request #4739 from BsAtHome/fix_mux-demux-power-of-two

components: Fix bit-sel inputs of generic mux/demux when not a power of two

Author: grandixximo

docs/src/man/man9/demux_generic.9.adoc

@@ -41,8 +41,6 @@ demux-gen.NN.in-[bool/real/sint/uint] ───┤   ─────o   o──
   changes. This helps to ignore 'noisy' switches.
 **demux-gen**.__N__.**sel-bit-**__BB__ bool in (BB=0..bit width of _size_)::
 **demux-gen**.__N__.**sel-int** uint in::
-  The sel-bit pins are only created when the size of the demux_gen
-  component is an integer power of two.
   Together, these determine to which **out**-__MM__ the **in** value is forwarded.
   The bit pins are interpreted as binary bits, and the result is simply
   added on to the integer pin input. It is expected that either one or

docs/src/man/man9/mux_generic.9.adoc

@@ -45,10 +45,8 @@ mux-gen.NN.in-[bool/real/sint/uint]-MM ───┤   ─────o
   added on to the integer pin input. It is expected that either one or
   the other would normally be used. However, the possibility exists to
   use a higher-order bit to "shift" the values set by the integer pin.
-  The sel-bit pins are only created when the size of the mux_gen
-  component is an integer power of two. This component (unlike mux16)
-  does not offer the option of decoding Gray-code, however the same
-  effect can be achieved by arranging the order of the input values to suit.
+  This component (unlike mux16) does not offer the option of decoding Gray-code,
+  however the same effect can be achieved by arranging the order of the input values to suit.
 **mux-gen**.__N__.**in-**[**bool**/**real**/**sint**/**uint**]**-**__MM__ variable-type in::
   The possible output values that are selected by the selection pins.
 **mux-gen**.__N__.**out-**[**bool**/**real**/**sint**/**uint**] variable-type out::

src/hal/components/demux_generic.c

@@ -91,7 +91,6 @@ int rtapi_app_main(void){
     // Parse the config string
     for (i = 0; i < demux->num_insts; i++) {
         char c;
-        int s, p = 0;
         demux_inst_t *inst = &demux->insts[i];
         inst->in_type = -1;
         inst->out_type = -1;
@@ -176,21 +175,32 @@ int rtapi_app_main(void){
                     " failed\n");
             goto fail0;
         }
+
         // Input pins
-        
-        // if the demux size is a power of 2 then create the bit inputs
-        s = inst->size;
-        for(inst->num_bits = 1; (!((s >>= 1) & 1)); inst->num_bits++);
-        if (s !=1){
-            inst->num_bits = 0;
-        } else { //make the bit pins
-            inst->sel_bit = hal_malloc(inst->num_bits * sizeof(*inst->sel_bit));
-            for (p = 0; p < inst->num_bits; p++) {
-                retval = hal_pin_new_bool(comp_id, HAL_IN, &inst->sel_bit[p], 0,
-                    "demux-gen.%02i.sel-bit-%02i", i, p);
-                if (retval != 0) {
-                    goto fail0;
-                }
+        inst->num_bits = 0;
+        // Count the position of the highest bit set in inst->size-1.
+        // We know that inst->size >= 2 from tests above. This will work
+        // for any value, regardless whether it is a power of two or not
+        // because of the -1 on the size and counting until all bits
+        // have become zero.
+        for (int s = inst->size - 1; 0 != s; inst->num_bits++) {
+            s >>= 1;
+        }
+        if (inst->num_bits < 1) {
+            // This is a safety guard. It should never happen.
+            rtapi_print_msg(RTAPI_MSG_ERR, "demux_generic: INTERNAL ERROR: num_bits == 0\n");
+            goto fail0;
+        }
+        inst->sel_bit = hal_malloc(inst->num_bits * sizeof(*inst->sel_bit));
+        if (!inst->sel_bit) {
+            rtapi_print_msg(RTAPI_MSG_ERR, "demux_generic: ERROR: out of HAL memory\n");
+            goto fail0;
+        }
+        for (int p = 0; p < inst->num_bits; p++) {
+            retval = hal_pin_new_bool(comp_id, HAL_IN, &inst->sel_bit[p], 0,
+                "demux-gen.%02i.sel-bit-%02i", i, p);
+            if (retval != 0) {
+                goto fail0;
             }
         }
 
@@ -240,7 +250,7 @@ int rtapi_app_main(void){
 
         //output pins
         inst->outputs = hal_malloc(inst->size * sizeof(*inst->outputs));
-        for (p = 0; p < inst->size; p++) {
+        for (int p = 0; p < inst->size; p++) {
             retval = rtapi_snprintf(hal_name, sizeof(hal_name),
                          "demux-gen.%02i.out-%s-%02i", i, types[inst->out_type], p);
             if (retval > HAL_NAME_LEN) {
@@ -262,40 +272,37 @@ int rtapi_app_main(void){
     hal_ready(comp_id);
     return 0;
 
-    fail0:
+fail0:
     hal_exit(comp_id);
     return -1;
-
 }
 
 // From 'f' to 't' conversion
 #define FT(f,t) ((((f) & 0x0f) << 4) + ((t) & 0x0f))
 
 void write_fp(void *arg, long period) {
     demux_inst_t *inst = arg;
-    int i = 0;
-    rtapi_uint s = 0;
-    if (inst->num_bits > 0) {
-        while (i < inst->num_bits) {
-            s += (hal_get_bool(inst->sel_bit[i]) != 0) << i;
-            i++;
-        }
+    rtapi_sint s = 0;
+    for (int i = 0; i < inst->num_bits; i++) {
+        s += (rtapi_sint)hal_get_bool(inst->sel_bit[i]) << i;
     }
     // if you document it, it's not a bug, it's a feature. Might even be useful
-    s += hal_get_uint(inst->sel_int);
+    rtapi_uint u = hal_get_uint(inst->sel_int);
+    // The calculation here is to prevent signed/unsigned overflow.
+    s += u > MAX_SIZE ? MAX_SIZE : u;
 
     if (hal_get_bool(inst->suppress) && s == 0)
         return;
     rtapi_uint timer = hal_get_uint(inst->timer);
-    if (s != hal_get_uint(inst->selection) && timer < hal_get_uint(inst->debounce)) {
+    if ((rtapi_uint)s != hal_get_uint(inst->selection) && timer < hal_get_uint(inst->debounce)) {
         hal_set_uint(inst->timer, timer + period / 1000);
         return;
     }
 
     hal_set_uint(inst->selection, s);
     hal_set_uint(inst->timer, 0);
 
-    if ((rtapi_sint)s >= inst->size)
+    if (s >= inst->size)
         s = inst->size - 1;
 
     switch (FT(inst->in_type, inst->out_type)) {
@@ -370,6 +377,6 @@ void write_fp(void *arg, long period) {
 }
 
 void rtapi_app_exit(void){
-//everything is hal_malloc-ed which saves a lot of cleanup here
-hal_exit(comp_id);
+    // everything is hal_malloc-ed which saves a lot of cleanup here
+    hal_exit(comp_id);
 }

src/hal/components/mux_generic.c

@@ -91,7 +91,6 @@ int rtapi_app_main(void){
     // Parse the config string
     for (i = 0; i < mux->num_insts; i++) {
         char c;
-        int s, p = 0;
         mux_inst_t *inst = &mux->insts[i];
         inst->in_type = -1;
         inst->out_type = -1;
@@ -178,20 +177,30 @@ int rtapi_app_main(void){
         }
 
         // Input pins
-
-        // if the mux size is a power of 2 then create the bit inputs
-        s = inst->size;
-        for(inst->num_bits = 1; (!((s >>= 1) & 1)); inst->num_bits++);
-        if (s !=1){
-            inst->num_bits = 0;
-        } else { //make the bit pins
-            inst->sel_bit = hal_malloc(inst->num_bits * sizeof(*inst->sel_bit));
-            for (p = 0; p < inst->num_bits; p++) {
-                retval = hal_pin_new_bool(comp_id, HAL_IN, &inst->sel_bit[p], 0,
-                        "mux-gen.%02i.sel-bit-%02i", i, p);
-                if (retval != 0) {
-                    goto fail0;
-                }
+        inst->num_bits = 0;
+        // Count the position of the highest bit set in inst->size-1.
+        // We know that inst->size >= 2 from tests above. This will work
+        // for any value, regardless whether it is a power of two or not
+        // because of the -1 on the size and counting until all bits
+        // have become zero.
+        for (int s = inst->size - 1; 0 != s; inst->num_bits++) {
+            s >>= 1;
+        }
+        if (inst->num_bits < 1) {
+            // This is a safety guard. It should never happen.
+            rtapi_print_msg(RTAPI_MSG_ERR, "mux_generic: INTERNAL ERROR: num_bits == 0\n");
+            goto fail0;
+        }
+        inst->sel_bit = hal_malloc(inst->num_bits * sizeof(*inst->sel_bit));
+        if (!inst->sel_bit) {
+            rtapi_print_msg(RTAPI_MSG_ERR, "mux_generic: ERROR: out of HAL memory\n");
+            goto fail0;
+        }
+        for (int p = 0; p < inst->num_bits; p++) {
+            retval = hal_pin_new_bool(comp_id, HAL_IN, &inst->sel_bit[p], 0,
+                    "mux-gen.%02i.sel-bit-%02i", i, p);
+            if (retval != 0) {
+                goto fail0;
             }
         }
 
@@ -202,7 +211,7 @@ int rtapi_app_main(void){
         }
 
         inst->inputs = hal_malloc(inst->size * sizeof(*inst->inputs));
-        for (p = 0; p < inst->size; p++) {
+        for (int p = 0; p < inst->size; p++) {
             retval = rtapi_snprintf(hal_name, HAL_NAME_LEN,
                     "mux-gen.%02i.in-%s-%02i", i, types[inst->in_type], p);
             if (retval >= HAL_NAME_LEN) {
@@ -263,39 +272,36 @@ int rtapi_app_main(void){
     hal_ready(comp_id);
     return 0;
 
-    fail0:
+fail0:
     hal_exit(comp_id);
     return -1;
-
 }
 
 // From 'f' to 't' conversion
 #define FT(f,t) ((((f) & 0x0f) << 4) + ((t) & 0x0f))
 
 void write_fp(void *arg, long period) {
     mux_inst_t *inst = arg;
-    int i = 0;
-    rtapi_uint s = 0;
-    if (inst->num_bits > 0) {
-        while (i < inst->num_bits) {
-            s += (hal_get_bool(inst->sel_bit[i]) != 0) << i;
-            i++;
-        }
+    rtapi_sint s = 0;
+    for (int i = 0; i < inst->num_bits; i++) {
+        s += (rtapi_sint)hal_get_bool(inst->sel_bit[i]) << i;
     }
     // if you document it, it's not a bug, it's a feature. Might even be useful
-    s += hal_get_uint(inst->sel_int);
+    rtapi_uint u = hal_get_uint(inst->sel_int);
+    // The calculation here is to prevent signed/unsigned overflow.
+    s += u > MAX_SIZE ? MAX_SIZE : u;
 
     if (hal_get_bool(inst->suppress) && s == 0)
         return;
-    if (s != hal_get_uint(inst->selection) && hal_get_uint(inst->timer) < hal_get_uint(inst->debounce)) {
+    if ((rtapi_uint)s != hal_get_uint(inst->selection) && hal_get_uint(inst->timer) < hal_get_uint(inst->debounce)) {
         hal_set_uint(inst->timer, hal_get_uint(inst->timer) + period / 1000);
         return;
     }
 
     hal_set_uint(inst->selection, s);
     hal_set_uint(inst->timer, 0);
 
-    if ((rtapi_sint)s >= inst->size)
+    if (s >= inst->size)
         s = inst->size - 1;
 
     switch (FT(inst->in_type, inst->out_type)) {
@@ -370,6 +376,6 @@ void write_fp(void *arg, long period) {
 }
 
 void rtapi_app_exit(void){
-//everything is hal_malloc-ed which saves a lot of cleanup here
-hal_exit(comp_id);
+    // everything is hal_malloc-ed which saves a lot of cleanup here
+    hal_exit(comp_id);
 }