libmodbus icon indicating copy to clipboard operation
libmodbus copied to clipboard

Only load each wide integer once in MODBUS_SET_INT*_TO_INT*() macros

Open nabijaczleweli opened this issue 2 years ago • 6 comments

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 (for example, "PmpaF1" ends up as "PPppFF"); 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

nabijaczleweli avatar Feb 11 '23 12:02 nabijaczleweli

We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please fill https://forms.gle/5635zjphDo5JEJQSA to get added. Your document will be manually checked by the maintainer. Be patient...

cla-bot[bot] avatar Feb 11 '23 12:02 cla-bot[bot]

  1. Grant of Copyright Lilcense

lmfao

nabijaczleweli avatar Feb 11 '23 12:02 nabijaczleweli

yeah im not posting you all of my PII im not fucking insane. apply if you want, dont if you dont

nabijaczleweli avatar Feb 11 '23 12:02 nabijaczleweli

Insane or not the idea seems to be fine (macros as ugly in some cases)

There is a little mistake in SET_INT16 and SET_INT32:

@@ -300,13 +300,13 @@ MODBUS_API int modbus_disable_quirks(modbus_t *ctx, unsigned int quirks_mask);
 #define MODBUS_SET_INT16_TO_INT8(tab_int8, index, value)  \
   do {                                                    \
     uint16_t _val = (value);                              \
-    ((int8_t *) (tab_int8))[(index)] = (int8_t) _val;     \
+    ((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 {                                                       \
     uint32_t _val = (value);                                 \
-    ((int16_t *) (tab_int16))[(index)] = (int16_t) _val;     \
+    ((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)           \

morgoth6 avatar Apr 02 '23 22:04 morgoth6

We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please fill https://forms.gle/5635zjphDo5JEJQSA to get added. Your document will be manually checked by the maintainer. Be patient...

cla-bot[bot] avatar Apr 02 '23 22:04 cla-bot[bot]

Yep; updated.

nabijaczleweli avatar Apr 02 '23 22:04 nabijaczleweli