Use, abuse or erroneous implementation of overlapping hal_float_t/hal_u32_t

[BsAtHome]

While doing the HAL getter/setter changes...

@pcw-mesa In hal/drivers/mesa-hostmot2/abs_encoder.c lines 617-624 we find this little gem:

617:    case HM2_GTAG_FABS:
618:        if (chan->params->timer_num > 4) chan->params->timer_num = 4;
619:        if (chan->params->u32_param > 15) chan->params->u32_param = 15;
620:        buff3 = chan->num_read_bits << 24
621:                | (rtapi_u32)(8.0e-6 * hm2->absenc.clock_frequency) << 14;
622:        buff2 = chan->params->u32_param << 28
623:                | ((rtapi_u32)(0x100000 * (chan->params->float_param * 1000
624:                / hm2->absenc.clock_frequency)));
...

The addressed variables are part of a union in hm2_sserial_params_t defined in sserial.h:

• chan->params->u32_param - a parameter typed hal_u32_t
• chan->params->float_param - a parameter typed hal_float_t

Please note, these are addressing a HAL parameter, so they are visible from userland and can be changed and inspected by, f.ex, halcmd.

Both variables share the same underlying memory and the 32-bit unsigned is addressing some of the mantissa bits of the 64-bit floating point.

The code first uses the unsigned value in line 619 and rewrites it to be 15 when larger than 15, isolating 4 lower bits of the 32-bit value. Then, in line 622 it reads the value and takes the lower 4 bits to the top (the ones limited in line 619). However, the next line, 623, reads the same memory as a floating point value. This assumes that there is valid data in the higher 32 bits of the 64-bit memory read for the floating point value, which is doubtful, and uses the read value as is.

This is obviously either a bad hack or an error. What is the purpose of this code? How is this in any way predictable what will happen? If the parameter is created as a HAL_U32, then the upper 32-bit are undefined. If the parameter is created as HAL_FLOAT, then the lower 4 bits are for all practical purposes and intends not controllable.

Was this code written before the HAL_FLOAT became a double? If so, then there would have been a full variable size overlap. But then this code was never run or tested again after the change. Even so, I still don't get it because using a float is a potentially unstable (especially after division).

The question is now how to fix it. Insights what is actually required and suggestions are welcome.

[BsAtHome]

The abuse seems to be intentional. The (presumed) initialization is in hm2_absenc_setup_fabs() and contains:
https://github.com/LinuxCNC/linuxcnc/blame/888cb941fbf23ee32fc8e15ca180997d99a39117/src/hal/drivers/mesa-hostmot2/abs_encoder.c#L222-L229
So it creates a 32-bit unsigned parameter and then assumes it has access to a 64-bit float, setting it to 1024.0, effectively setting the double's exponent and then setting the actual parameter to 0xf, forcing the lowest mantissa bits to 15.

Then the calculation is, according to the values: 0x100000 * (1024.0 * 1000 / frequency). This is then truncated to integer and the upper 4 bits are set to the value of the actual parameter's lowest 4 bits.
(actually, the 1024 is more something like 1024.000000000001 or so, due to the setting of the lowest mantissa bits)

Why is this abusing an indirectly double cast'ed storage in the calculation? The exact same thing would be:

buff2 = (chan->params->u32_param << 28)
        | ((rtapi_u32)(0x100000 * (1024000.0 / hm2->absenc.clock_frequency)));

The current situation is actually even worse because it forces an expensive unconditional memory read where a simple constant would have sufficed.

I really would like to hear the background for doing all of this.

[andypugh]

This is almost certainly my code, and git blame agrees:
https://github.com/LinuxCNC/linuxcnc/blame/master/src/hal/drivers/mesa-hostmot2/abs_encoder.c#L618

I will need to take a long hard look at why I did that. (ie not when I am on a train with patchy hotspot internet access through my phone)

[BsAtHome]

The code has been replaced with a simple equivalent and the overlapping initialization has been removed.

I am still very much interested in hearing the story behind the code.

The specific lines of code (622-623) now read:

    buff2 = (hal_get_ui32(chan->params->param.u) << 28)
            | ((rtapi_u32)(0x100000ull * 1024000ull / hm2->absenc.clock_frequency));

The floating point was initialized to 1024.0, then multiplied by 1000 and the whole lot again multiplied by 0x100000. The constants combined (the compiler merges them) are about 40 bit while the calculation is 64 bit, so that fits. And, no more floating point.