swap_uint_length improperly does byte-level swap in bit mode when little endian is selected

[eacmen]

When a CAN mapping is configured in bit mode and the data on the bus is formatted Least Significant Bit the swap_uint_length function does an endian (byte level) swap when in fact it needs to be doing a Most Significant Bit to Least Significant Bit conversion.

Lets use the decimal number 4099 as an example. 4099 encoded in binary is:
1000000000011 (13 bits Most Significant Bit)

"On the wire" it will be:
1100000000001 (13 bits Least Significant Bit)

Entering the canmapping_extract_value function that Least Significant Bit value will be embedded in that 64bit uint somewhere (as indicated by length/offset), carved out by a bitmask to a uint_32 native to the host processor.

What I believe is happening is the swap_uint_length is treating that 13 bit value as a 16 bit short and doing a byte swap so swap_unit_length is basically doing the following

00011000 00000001 (0-padded to 16 bit value from above)
00000001 00011000 (byte swapped)

which equals 280 not 4099

What I believe needs to happen is that the whole uint_16 needs to be shifted left to a byte boundary (in this case 3 bits) and the order of the BITS in each byte reversed. Then the bytes themselves need to be swapped.

Demonstrated:
00011000 00000001 (0-padded to 16 bit value from above)
11000000 00001000 (whole uint_16 value shifted left by 3 bits to)
00000011 00010000 (both upper and lower bytes BITS rotated)
00010000 00000011 (byte swapped)

RaceCapture-Pro_firmware/src/CAN/can_mapping.c

Line 46 in b34d101

raw_value = swap_uint_length(raw_value, length);

[eacmen]

my opinion now may differ from what i wrote up in the github issue ticket above

at the top of canmapping_extract_value raw_data is coming in as a uint64_t represented as a native CPU integer. The CAN receive message task takes the bytes directly off the wire and into the CAN_msg struct in memory:

RaceCapture-Pro_firmware/src/CAN/can_mapping.c

Lines 26 to 30 in b34d101

	float canmapping_extract_value(uint64_t raw_data, const CANMapping *mapping)
	{
	#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
	raw_data = swap_uint64(raw_data);
	#endif

RaceCapture-Pro_firmware/src/CAN/CAN.c

Lines 65 to 67 in fa3f884

	int CAN_rx_msg(CAN_msg *msg, const unsigned int timeoutMs)
	{
	const int rc = CAN_device_rx_msg(msg, timeoutMs);

The ARM cortex is little endian so at the top of the canmapping_extract_value function the raw value as it came off the wire is endian-swapped.

Based on the code it seems belief is that this swap_uint64 call converts the data from little to big endian. However what it is actually doing is converting from network-byte-order to host-byte-order. Which allows you to mask off certain bytes and get values that make sense when de-referenced in the host processor. However lets consider what this does to the value in memory:

On the wire the CAN message is represented as 8 bytes:

Byte1 Byte2 Byte3 Byte4 Byte5 Byte6 Byte7 Byte8
in the can rx queue those bytes from the wire are copied right into struct memory and then de-referenced as a uint64_t. In RCP memory it is still:

Byte1 Byte2 Byte3 Byte4 Byte5 Byte6 Byte7 Byte8
However since RCP is little endian you cannot simply mask off bytes in this uint64_t and get values that make sense on the CPU. So at the top of canmapping_extract_value you endian-swap it which converts the value from network-byte-order in memory (as it was on the wire) to host byte order. So in memory the CAN message is now:

Byte8 Byte7 Byte6 Byte5 Byte4 Byte3 Byte2 Byte1
So when you then try to index into this in a bit-wise way bits that were adjacent on byte boundaries on the wire are no longer next to one another in RCP memory.

So lets take the case of the E46 wheel speed sensors which use the first 8 bits of Byte1 and the first 4 bits of Byte2. If we expand what is happening in memory you are actually end up selecting the trailing 4 bits of Byte2 and all 8 bits of Byte1.

so tl;dr - What I would propose is:

• do not endian swap at the top of canmapping_extract_value.
• mask off the value that the user selected FIRST then endian swap AFTER

[RainsDoigens]

Hi eacman,
I agree with you. Excatly this is what I'm doing in my custom build. I had the issue that values in bit mode with lengths being not an even multiple of 1 byte did not came through properly.

This here is working well fro me:

float canmapping_extract_value(uint64_t raw_data, const CANMapping *mapping)
{
#if BYTE_ORDER == ORDER_LITTLE_ENDIAN
// TODO_RB: Important !!! No swap here !!!
// raw_data = swap_uint64(raw_data);
#endif

    uint8_t offset = mapping->offset;
    uint8_t length = mapping->length;
    if (! mapping->bit_mode) {
            length *= 8;
            offset *= 8;
    }
    /* create the bitmask of the appropriate length */
    uint32_t bitmask = (1UL << length) - 1;

    /* extract the raw value from the 64 bit representation of the CAN message */

// uint32_t raw_value = (raw_data >> (64 - offset - length)) & bitmask;

    uint32_t raw_value = (raw_data >> (offset)) & bitmask;

    /* normalize endian */
    if (mapping->big_endian) {
            raw_value = swap_uint_length(raw_value, length);
    }

    /* convert type */
    switch (mapping->type) {
    case CANMappingType_unsigned:
            return (float)raw_value;

// TODO_RB: Important for non byte even values !!!

    case CANMappingType_signed:
            if (length == 8) {
                    return (float)*((int8_t*)&raw_value);
            }
            else if (length == 16) {
                    return (float)*((int16_t*)&raw_value);
            }
            else if (length == 32) {
                    return (float)*((int32_t*)&raw_value);
            } 
            else{
                    int32_t half = 1 << (length - 1);
                    return raw_value < half ? (float)raw_value : ((float)(raw_value & (half-1))-(float)(half));
           } 


    case CANMappingType_IEEE754:
            return *((float*)&raw_value);
    case CANMappingType_sign_magnitude:
            /**
             *  sign-magnitude is used in cases where there's a sign bit
             *  and an absolute value indicating magnitude.
             *  e.g. BMW E46 steering angle sensor
             **/
    {
            uint32_t sign = 1 << (length - 1);
            return raw_value < sign ? (float)raw_value : -(float)(raw_value & (sign - 1));
    }
    default:
            /* We reached an invalid enum */
            panic(PANIC_CAUSE_UNREACHABLE);
            return 0;
    }

}