Clarification on memory layout in _modbus_mapping_t.tab_registers

[ghorwin]

Hi,

the patch (49af73d) that attempted to fix the correct modbus_set_float_xxxx() and modbus_get_float_xxxx() functionality, introduced a bug (see #665). Now the setter and getter function are inconsistent.

The problem is directly visible in the test suite, see:

.unit-test.h:77...

const float UT_REAL = 123456.00;

/*
 * The following arrays assume that 'A' is the MSB,
 * and 'D' is the LSB.
 * Thus, the following is the case:
 * A = 0x47
 * B = 0xF1
 * C = 0x20
 * D = 0x00
 * 
 * There are two sets of arrays: one to test that the setting is correct,
 * the other to test that the getting is correct.
 * Note that the 'get' values must be constants in processor-endianness,
 * as libmodbus will convert all words to processor-endianness as they come in.
*/
const uint8_t UT_IREAL_ABCD_SET[] = {0x47, 0xF1, 0x20, 0x00};
const uint16_t UT_IREAL_ABCD_GET[] = {0x47F1, 0x2000};

and

.unit-test-client.c:324:...

printf("1/4 Set/get float ABCD: ");
modbus_set_float_abcd(UT_REAL, tab_rp_registers);
ASSERT_TRUE(is_memory_equal(tab_rp_registers, UT_IREAL_ABCD_SET, 4),
            "FAILED Set float ABCD");
real = modbus_get_float_abcd(UT_IREAL_ABCD_GET);
ASSERT_TRUE(real == UT_REAL, "FAILED (%f != %f)\n", real, UT_REAL);

The function modbus_set_float_abcd() in its current implementation sets the floating point value into the memory in "big endian" format (0x47, 0xF1, 0x20, 0x00). Which seems correct, when working on the assumption, that the registers are expected to hold the data in big endian format (which is wrong, however!).

However, the modbus_get_float_abcd() returns the floating point value from memory array: 0xF1, 0x47, 0x00, 0x20 (notice the swapped bytes). This is not easily visible, as the function takes a different memory content defined in UT_IREAL_ABCD_GET as opposed to UT_IREAL_ABCD_SET.

const uint16_t UT_IREAL_ABCD_GET[] = {0x47F1, 0x2000};  

means on big endian: 0x47, 0xF1, 0x20, 0x00
but on little endian: 0xF1, 0x47, 0x00, 0x20

So, getter and setter function do not use the same memory representation (at least on x86 little endian systems) and this is the cause of all problems (the comment above about the two different arrays says it clearly). This is a regression that was introduced in commit 49af73d and should be fixed.

To explain the correct behavior let's back up a bit: the Modbus code itself stores originally and intentionally the data in platform specific endianess, as can be seen from the receive code:

.modbus.c:1318:1321

        for (i = 0; i < rc; i++) {
            /* shift reg hi_byte to temp OR with lo_byte */
            dest[i] = (rsp[offset + 2 + (i << 1)] << 8) | rsp[offset + 3 + (i << 1)];
        }

The message buffer rsp is a byte stream (according to Modbus specs in big endian), and due to the shift operation becomes a uint16_t in system endianess. This is also stated in the comment in the code block above.

The original (now marked as deprecated) modbus_get_float() and modbus_set_float() functions actually do the right thing, as they use the platform endianess:

/* DEPRECATED - Set a float to 4 bytes in a sort of Modbus format! */
void modbus_set_float(float f, uint16_t *dest)
{
    uint32_t i;

    memcpy(&i, &f, sizeof(uint32_t));
    dest[0] = (uint16_t) i;
    dest[1] = (uint16_t) (i >> 16);
}

/* DEPRECATED - Get a float from 4 bytes in sort of Modbus format */
float modbus_get_float(const uint16_t *src)
{
    float f;
    uint32_t i;

    i = (((uint32_t) src[1]) << 16) + src[0];
    memcpy(&f, &i, sizeof(float));

    return f;
}

To summarize: the memory layout in the modbus mapping tables(modbus_mapping_t.tab_registers etc.) is local system endianess, and NOT ALWAYS Big Endian!!!

And the test suite needs to be fixed as well, so that setter and getter functions work on the same memory representation.

I suggest reverting the faulty commit 49af73d and then accept a new patch for handling the proper encoding on big endian systems.

[stephane]

Done. Thank you.
Do you want to add information about the fact internal layout is processor-endianess and byte stream is big endian?

[ghorwin]

Hi Stephane, thanks for merging.
The code contains the documentation about processor endianess in tab storage members. The big endian of the byte stream is AFAIK defined in the Modbus standard. So, I guess no other documentation is necessary for now.