Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nabijaczleweli
Copy link

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

@cla-bot
Copy link

cla-bot bot commented Feb 11, 2023

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...

@nabijaczleweli
Copy link
Author

  1. Grant of Copyright Lilcense

lmfao

@nabijaczleweli
Copy link
Author

nabijaczleweli commented Feb 11, 2023

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

@morgoth6
Copy link

morgoth6 commented Apr 2, 2023

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)           \

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
@cla-bot
Copy link

cla-bot bot commented Apr 2, 2023

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...

@nabijaczleweli
Copy link
Author

Yep; updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants