From 6ee7b3f19f23d70cec7ce236e931dc396ca06d6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=BD=D0=B0=D0=B1?= Date: Sat, 11 Feb 2023 13:27:46 +0100 Subject: [PATCH] Only load each wide integer once in MODBUS_SET_INT*_TO_INT*() macros If you have a union like this as part of the read data: union { uint8_t serial[6]; uint16_t serial_raw[3]; }; and do the obvious for(size_t i = 0; i < sizeof(rdng.serial_raw) / sizeof(*rdng.serial_raw); ++i) { MODBUS_SET_INT16_TO_INT8(rdng.serial, i*2, rdng.serial_raw[i]); } then because serial_raw[i] is updated through serial[i] at first, then loaded again, you end up with rdng.serial[i*2]=rdng.serial[i*2+1] for all i, which is both confusing and wrong; instead, you must do for(size_t i = 0; i < sizeof(rdng.serial_raw) / sizeof(*rdng.serial_raw); ++i) { uint16_t r = rdng.serial_raw[i]; MODBUS_SET_INT16_TO_INT8(rdng.serial, i*2, rdng.serial_raw[i]); } but there's really no reason to require this, and this patch lets you use them directly --- src/modbus.h | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/src/modbus.h b/src/modbus.h index 55ef08a0d..c212df943 100644 --- a/src/modbus.h +++ b/src/modbus.h @@ -283,22 +283,25 @@ MODBUS_API int modbus_disable_quirks(modbus_t *ctx, unsigned int quirks_mask); (((int32_t) tab_int16[(index)] << 16) | (int32_t) tab_int16[(index) + 1]) #define MODBUS_GET_INT16_FROM_INT8(tab_int8, index) \ (((int16_t) tab_int8[(index)] << 8) | (int16_t) tab_int8[(index) + 1]) -#define MODBUS_SET_INT16_TO_INT8(tab_int8, index, value) \ - do { \ - ((int8_t *) (tab_int8))[(index)] = (int8_t) ((value) >> 8); \ - ((int8_t *) (tab_int8))[(index) + 1] = (int8_t) (value); \ +#define MODBUS_SET_INT16_TO_INT8(tab_int8, index, value) \ + do { \ + uint16_t _val = (value); \ + ((int8_t *) (tab_int8))[(index)] = (int8_t) (_val >> 8); \ + ((int8_t *) (tab_int8))[(index) + 1] = (int8_t) _val; \ } while (0) -#define MODBUS_SET_INT32_TO_INT16(tab_int16, index, value) \ - do { \ - ((int16_t *) (tab_int16))[(index)] = (int16_t) ((value) >> 16); \ - ((int16_t *) (tab_int16))[(index) + 1] = (int16_t) (value); \ +#define MODBUS_SET_INT32_TO_INT16(tab_int16, index, value) \ + do { \ + uint32_t _val = (value); \ + ((int16_t *) (tab_int16))[(index)] = (int16_t) (_val >> 16); \ + ((int16_t *) (tab_int16))[(index) + 1] = (int16_t) _val; \ } while (0) -#define MODBUS_SET_INT64_TO_INT16(tab_int16, index, value) \ - do { \ - ((int16_t *) (tab_int16))[(index)] = (int16_t) ((value) >> 48); \ - ((int16_t *) (tab_int16))[(index) + 1] = (int16_t) ((value) >> 32); \ - ((int16_t *) (tab_int16))[(index) + 2] = (int16_t) ((value) >> 16); \ - ((int16_t *) (tab_int16))[(index) + 3] = (int16_t) (value); \ +#define MODBUS_SET_INT64_TO_INT16(tab_int16, index, value) \ + do { \ + uint64_t _val = (value); \ + ((int16_t *) (tab_int16))[(index)] = (int16_t) (_val >> 48); \ + ((int16_t *) (tab_int16))[(index) + 1] = (int16_t) (_val >> 32); \ + ((int16_t *) (tab_int16))[(index) + 2] = (int16_t) (_val >> 16); \ + ((int16_t *) (tab_int16))[(index) + 3] = (int16_t) _val; \ } while (0) MODBUS_API void modbus_set_bits_from_byte(uint8_t *dest, int idx, const uint8_t value);