Revert "Fix float endianness issue on big endian architecture." and fix test suite and setter functions.

This reverts commit 49af73d.

Author: ghorwin

src/modbus-data.c

@@ -26,50 +26,6 @@
 
 #include "modbus.h"
 
-#if defined(HAVE_BYTESWAP_H)
-#  include <byteswap.h>
-#endif
-
-#if defined(__APPLE__)
-#  include <libkern/OSByteOrder.h>
-#  define bswap_16 OSSwapInt16
-#  define bswap_32 OSSwapInt32
-#  define bswap_64 OSSwapInt64
-#endif
-
-#if defined(__GNUC__)
-#  define GCC_VERSION (__GNUC__ * 100 + __GNUC_MINOR__ * 10)
-#  if GCC_VERSION >= 430
-// Since GCC >= 4.30, GCC provides __builtin_bswapXX() alternatives so we switch to them
-#    undef bswap_32
-#    define bswap_32 __builtin_bswap32
-#  endif
-#  if GCC_VERSION >= 480
-#    undef bswap_16
-#    define bswap_16 __builtin_bswap16
-#  endif
-#endif
-
-#if defined(_MSC_VER) && (_MSC_VER >= 1400)
-#  define bswap_32 _byteswap_ulong
-#  define bswap_16 _byteswap_ushort
-#endif
-
-#if !defined(bswap_16)
-#  warning "Fallback on C functions for bswap_16"
-static inline uint16_t bswap_16(uint16_t x)
-{
-    return (x >> 8) | (x << 8);
-}
-#endif
-
-#if !defined(bswap_32)
-#  warning "Fallback on C functions for bswap_32"
-static inline uint32_t bswap_32(uint32_t x)
-{
-    return (bswap_16(x & 0xffff) << 16) | (bswap_16(x >> 16));
-}
-#endif
 // clang-format on
 
 /* Sets many bits from a single byte value (all 8 bits of the byte value are
@@ -128,11 +84,14 @@ float modbus_get_float_abcd(const uint16_t *src)
     uint32_t i;
     uint8_t a, b, c, d;
 
-    a = (src[0] >> 8) & 0xFF;
-    b = (src[0] >> 0) & 0xFF;
-    c = (src[1] >> 8) & 0xFF;
-    d = (src[1] >> 0) & 0xFF;
+    // Mind: src contains 16-bit numbers in processor-endianness, hence
+    //       we use shift operations and do not access memory directly
+    a = (src[0] >> 8) & 0xFF; // high byte of first word
+    b = (src[0] >> 0) & 0xFF; // low byte of first word
+    c = (src[1] >> 8) & 0xFF; // high byte of second word
+    d = (src[1] >> 0) & 0xFF; // low byte of second word
 
+    // we assemble 32bit integer always in abcd order via shift operations
     i = (a << 24) | (b << 16) | (c << 8) | (d << 0);
     memcpy(&f, &i, 4);
 
@@ -146,12 +105,14 @@ float modbus_get_float_dcba(const uint16_t *src)
     uint32_t i;
     uint8_t a, b, c, d;
 
-    a = (src[0] >> 8) & 0xFF;
-    b = (src[0] >> 0) & 0xFF;
-    c = (src[1] >> 8) & 0xFF;
-    d = (src[1] >> 0) & 0xFF;
+    // byte order is defined when reading from src: dcba
+    d = (src[0] >> 8) & 0xFF;
+    c = (src[0] >> 0) & 0xFF;
+    b = (src[1] >> 8) & 0xFF;
+    a = (src[1] >> 0) & 0xFF;
 
-    i = (d << 24) | (c << 16) | (b << 8) | (a << 0);
+    // we assemble 32bit integer always in abcd order via shift operations
+    i = (a << 24) | (b << 16) | (c << 8) | (d << 0);
     memcpy(&f, &i, 4);
 
     return f;
@@ -164,12 +125,14 @@ float modbus_get_float_badc(const uint16_t *src)
     uint32_t i;
     uint8_t a, b, c, d;
 
-    a = (src[0] >> 8) & 0xFF;
-    b = (src[0] >> 0) & 0xFF;
-    c = (src[1] >> 8) & 0xFF;
-    d = (src[1] >> 0) & 0xFF;
+    // byte order is defined when reading from src: badc
+    b = (src[0] >> 8) & 0xFF;
+    a = (src[0] >> 0) & 0xFF;
+    d = (src[1] >> 8) & 0xFF;
+    c = (src[1] >> 0) & 0xFF;
 
-    i = (b << 24) | (a << 16) | (d << 8) | (c << 0);
+    // we assemble 32bit integer always in abcd order via shift operations
+    i = (a << 24) | (b << 16) | (c << 8) | (d << 0);
     memcpy(&f, &i, 4);
 
     return f;
@@ -182,12 +145,14 @@ float modbus_get_float_cdab(const uint16_t *src)
     uint32_t i;
     uint8_t a, b, c, d;
 
-    a = (src[0] >> 8) & 0xFF;
-    b = (src[0] >> 0) & 0xFF;
-    c = (src[1] >> 8) & 0xFF;
-    d = (src[1] >> 0) & 0xFF;
+    // byte order is defined when reading from src: cdab
+    c = (src[0] >> 8) & 0xFF;
+    d = (src[0] >> 0) & 0xFF;
+    a = (src[1] >> 8) & 0xFF;
+    b = (src[1] >> 0) & 0xFF;
 
-    i = (c << 24) | (d << 16) | (a << 8) | (b << 0);
+    // we assemble 32bit integer always in abcd order via shift operations
+    i = (a << 24) | (b << 16) | (c << 8) | (d << 0);
     memcpy(&f, &i, 4);
 
     return f;
@@ -196,97 +161,81 @@ float modbus_get_float_cdab(const uint16_t *src)
 /* 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;
+    return modbus_get_float_cdab(src);
 }
 
 /* Set a float to 4 bytes for Modbus w/o any conversion (ABCD) */
 void modbus_set_float_abcd(float f, uint16_t *dest)
 {
-    uint32_t i;
-    uint8_t *out = (uint8_t *) dest;
+    // The straight-forward type conversion won't work because of type-punned pointer aliasing warning
+    // uint32_t i = *(uint32_t*)(&f);
+    float * fptr = &f;
+    uint32_t * iptr = (uint32_t *)fptr;
+    uint32_t i = *iptr;
     uint8_t a, b, c, d;
 
-    memcpy(&i, &f, sizeof(uint32_t));
     a = (i >> 24) & 0xFF;
     b = (i >> 16) & 0xFF;
     c = (i >> 8) & 0xFF;
     d = (i >> 0) & 0xFF;
 
-    out[0] = a;
-    out[1] = b;
-    out[2] = c;
-    out[3] = d;
+    dest[0] = (a << 8) | b;
+    dest[1] = (c << 8) | d;
 }
 
 /* Set a float to 4 bytes for Modbus with byte and word swap conversion (DCBA) */
 void modbus_set_float_dcba(float f, uint16_t *dest)
 {
-    uint32_t i;
-    uint8_t *out = (uint8_t *) dest;
+    float * fptr = &f;
+    uint32_t * iptr = (uint32_t *)fptr;
+    uint32_t i = *iptr;
     uint8_t a, b, c, d;
 
-    memcpy(&i, &f, sizeof(uint32_t));
     a = (i >> 24) & 0xFF;
     b = (i >> 16) & 0xFF;
     c = (i >> 8) & 0xFF;
     d = (i >> 0) & 0xFF;
 
-    out[0] = d;
-    out[1] = c;
-    out[2] = b;
-    out[3] = a;
+    dest[0] = (d << 8) | c;
+    dest[1] = (b << 8) | a;
 }
 
 /* Set a float to 4 bytes for Modbus with byte swap conversion (BADC) */
 void modbus_set_float_badc(float f, uint16_t *dest)
 {
-    uint32_t i;
-    uint8_t *out = (uint8_t *) dest;
+    float * fptr = &f;
+    uint32_t * iptr = (uint32_t *)fptr;
+    uint32_t i = *iptr;
     uint8_t a, b, c, d;
 
-    memcpy(&i, &f, sizeof(uint32_t));
     a = (i >> 24) & 0xFF;
     b = (i >> 16) & 0xFF;
     c = (i >> 8) & 0xFF;
     d = (i >> 0) & 0xFF;
 
-    out[0] = b;
-    out[1] = a;
-    out[2] = d;
-    out[3] = c;
+    dest[0] = (b << 8) | a;
+    dest[1] = (d << 8) | c;
 }
 
 /* Set a float to 4 bytes for Modbus with word swap conversion (CDAB) */
 void modbus_set_float_cdab(float f, uint16_t *dest)
 {
-    uint32_t i;
-    uint8_t *out = (uint8_t *) dest;
+    float * fptr = &f;
+    uint32_t * iptr = (uint32_t *)fptr;
+    uint32_t i = *iptr;
     uint8_t a, b, c, d;
 
-    memcpy(&i, &f, sizeof(uint32_t));
     a = (i >> 24) & 0xFF;
     b = (i >> 16) & 0xFF;
     c = (i >> 8) & 0xFF;
     d = (i >> 0) & 0xFF;
 
-    out[0] = c;
-    out[1] = d;
-    out[2] = a;
-    out[3] = b;
+    dest[0] = (c << 8) | d;
+    dest[1] = (a << 8) | b;
 }
 
 /* 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);
+    modbus_set_float_cdab(f, dest);
 }

src/modbus.h

@@ -156,6 +156,10 @@ extern const unsigned int libmodbus_version_micro;
 
 typedef struct _modbus modbus_t;
 
+/*! Memory layout in tab_xxx arrays is processor-endianness.
+    When receiving modbus data, it is converted to processor-endianness,
+    see read_registers().
+*/
 typedef struct _modbus_mapping_t {
     int nb_bits;
     int start_bits;

tests/unit-test-client.c

@@ -323,30 +323,30 @@ int main(int argc, char *argv[])
     /** FLOAT **/
     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),
+    ASSERT_TRUE(is_memory_equal(tab_rp_registers, UT_IREAL_ABCD, 4),
                 "FAILED Set float ABCD");
-    real = modbus_get_float_abcd(UT_IREAL_ABCD_GET);
+    real = modbus_get_float_abcd(UT_IREAL_ABCD);
     ASSERT_TRUE(real == UT_REAL, "FAILED (%f != %f)\n", real, UT_REAL);
 
     printf("2/4 Set/get float DCBA: ");
     modbus_set_float_dcba(UT_REAL, tab_rp_registers);
-    ASSERT_TRUE(is_memory_equal(tab_rp_registers, UT_IREAL_DCBA_SET, 4),
+    ASSERT_TRUE(is_memory_equal(tab_rp_registers, UT_IREAL_DCBA, 4),
                 "FAILED Set float DCBA");
-    real = modbus_get_float_dcba(UT_IREAL_DCBA_GET);
+    real = modbus_get_float_dcba(UT_IREAL_DCBA);
     ASSERT_TRUE(real == UT_REAL, "FAILED (%f != %f)\n", real, UT_REAL);
 
     printf("3/4 Set/get float BADC: ");
     modbus_set_float_badc(UT_REAL, tab_rp_registers);
-    ASSERT_TRUE(is_memory_equal(tab_rp_registers, UT_IREAL_BADC_SET, 4),
+    ASSERT_TRUE(is_memory_equal(tab_rp_registers, UT_IREAL_BADC, 4),
                 "FAILED Set float BADC");
-    real = modbus_get_float_badc(UT_IREAL_BADC_GET);
+    real = modbus_get_float_badc(UT_IREAL_BADC);
     ASSERT_TRUE(real == UT_REAL, "FAILED (%f != %f)\n", real, UT_REAL);
 
     printf("4/4 Set/get float CDAB: ");
     modbus_set_float_cdab(UT_REAL, tab_rp_registers);
-    ASSERT_TRUE(is_memory_equal(tab_rp_registers, UT_IREAL_CDAB_SET, 4),
+    ASSERT_TRUE(is_memory_equal(tab_rp_registers, UT_IREAL_CDAB, 4),
                 "FAILED Set float CDAB");
-    real = modbus_get_float_cdab(UT_IREAL_CDAB_GET);
+    real = modbus_get_float_cdab(UT_IREAL_CDAB);
     ASSERT_TRUE(real == UT_REAL, "FAILED (%f != %f)\n", real, UT_REAL);
 
     printf("\nAt this point, error messages doesn't mean the test has failed\n");

tests/unit-test.h.in

@@ -77,31 +77,15 @@ const uint16_t UT_INPUT_REGISTERS_TAB[] = { 0x000A };
 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.
+ * libmodbus will convert all words to processor-endianness as they come in and also converts them
+ * back to big endian as they are sent out.
+ * The memory layout in modbus_mapping_t.tab_registers is thus processor-endianess.
+ * Hence we define the comparison constants to check against the content of modbus_mapping_t.tab_registers
+ * in terms of 16 bit integer values, that are likewise stored in memory in processor-endianess.
  */
-const uint8_t UT_IREAL_ABCD_SET[] = {0x47, 0xF1, 0x20, 0x00};
-const uint16_t UT_IREAL_ABCD_GET[] = {0x47F1, 0x2000};
-const uint8_t UT_IREAL_DCBA_SET[] = {0x00, 0x20, 0xF1, 0x47};
-const uint16_t UT_IREAL_DCBA_GET[] = {0x0020, 0xF147};
-const uint8_t UT_IREAL_BADC_SET[] = {0xF1, 0x47, 0x00, 0x20};
-const uint16_t UT_IREAL_BADC_GET[] = {0xF147, 0x0020};
-const uint8_t UT_IREAL_CDAB_SET[] = {0x20, 0x00, 0x47, 0xF1};
-const uint16_t UT_IREAL_CDAB_GET[] = {0x2000, 0x47F1};
-
-/* const uint32_t UT_IREAL_ABCD = 0x47F12000);
-const uint32_t UT_IREAL_DCBA = 0x0020F147;
-const uint32_t UT_IREAL_BADC = 0xF1470020;
-const uint32_t UT_IREAL_CDAB = 0x200047F1;*/
+const uint16_t UT_IREAL_ABCD[] = {0x47F1, 0x2000};
+const uint16_t UT_IREAL_DCBA[] = {0x0020, 0xF147};
+const uint16_t UT_IREAL_BADC[] = {0xF147, 0x0020};
+const uint16_t UT_IREAL_CDAB[] = {0x2000, 0x47F1};
 
 #endif /* _UNIT_TEST_H_ */