From d0cb19a36ee2a302500ba8b03e209c9b94c5d579 Mon Sep 17 00:00:00 2001 From: Andreas Nicolai Date: Thu, 19 Sep 2024 14:59:19 +0200 Subject: [PATCH] Revert "Fix float endianness issue on big endian architecture." and fix test suite and setter functions. This reverts commit 49af73debd756be68497a61bd53a07c02673da96. --- src/modbus-data.c | 155 +++++++++++++-------------------------- src/modbus.h | 4 + tests/unit-test-client.c | 16 ++-- tests/unit-test.h.in | 34 +++------ 4 files changed, 73 insertions(+), 136 deletions(-) diff --git a/src/modbus-data.c b/src/modbus-data.c index 8aeef0cc7..1e3900418 100644 --- a/src/modbus-data.c +++ b/src/modbus-data.c @@ -26,50 +26,6 @@ #include "modbus.h" -#if defined(HAVE_BYTESWAP_H) -# include -#endif - -#if defined(__APPLE__) -# include -# 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); } diff --git a/src/modbus.h b/src/modbus.h index 9b1ef6b7f..fa7ec4a2f 100644 --- a/src/modbus.h +++ b/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; diff --git a/tests/unit-test-client.c b/tests/unit-test-client.c index 84d5840b1..a1813b1eb 100644 --- a/tests/unit-test-client.c +++ b/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"); diff --git a/tests/unit-test.h.in b/tests/unit-test.h.in index 21d09e30e..918cc438a 100644 --- a/tests/unit-test.h.in +++ b/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_ */