problem in rlmodbusclient with negative values

Question

problem in rlmodbusclient with negative values

Closed this issue 6 years ago · 9 comments

gentooza commented 6 years ago

Hi!

I've encounter a problem writting signed integer with function:

writePresetSingleRegister(int slave, int adr, int value)

the problematic line is:

data[2] = value/256; data[3] = value & 0x0ff;

where I think we find a calculation error in value/256 , changing this, perhaps a little bit out of control operation (unsigned char = int/int), with a bit moving operation solve the issue for me.

data[2] = value>>8; data[3] = value & 0x0ff;

So that, I can now correctly write/read signed integers to a PLC.

Also:

perhaps we should also check int (32 bits) limits to fit in a 16 bits INT/WORD
perhaps we should treat address also in the way: addr>>8 to avoid possible problems? (I'm not sure as address should be always unsigned)
perhaps we should build another abstraction layer where writeInt, writeFloat, etc.?

cheers, and thanks for the effort!

pd: tests I did:

----- FOR VALUE: 32643 -----
RLMODBUSCLIENT: old telegram, slave:1 , highaddress:3 lowaddress:232, highvalue:127 lowvalue:131
RLMODBUSCLIENT: new telegram, slave:1 , highaddress:3 lowaddress:232, highvalue:127 lowvalue:131

----- FOR VALUE: -32643 -----
RLMODBUSCLIENT: old telegram, slave:1 , highaddress:3 lowaddress:232, highvalue:129 lowvalue:125
RLMODBUSCLIENT: new telegram, slave:1 , highaddress:3 lowaddress:232, highvalue:128 lowvalue:125

----- FOR VALUE: 255 -----
RLMODBUSCLIENT: old telegram, slave:1 , highaddress:3 lowaddress:232, highvalue:0 lowvalue:255
RLMODBUSCLIENT: new telegram, slave:1 , highaddress:3 lowaddress:232, highvalue:0 lowvalue:255

----- FOR VALUE: -255 -----
RLMODBUSCLIENT: old telegram, slave:1 , highaddress:3 lowaddress:232, highvalue:0 lowvalue:1
RLMODBUSCLIENT: new telegram, slave:1 , highaddress:3 lowaddress:232, highvalue:255 lowvalue:1

Answer 1 · 2018-06-07T11:19:38.000Z

Hi
What are you trying to achieve?

craig

Answer 2 · 2018-06-07T13:02:44.000Z

Modbus does not really define int / unsigned int / float ... registers. But in reality people use it for that. See: http://www.simplymodbus.ca/FAQ.htm#Stored If we use the methods "write" and "response" from https://pvbrowser.de/pvbrowser/sf/manual/rllib/html/classrlModbus.html we can set data as needed for any of the above data types. The methods: int readCoilStatus <https://pvbrowser.de/pvbrowser/sf/manual/rllib/html/classrlModbus.html#a3650d910bd0435f31f46b30142c3cffa> (int slave, int start_adr, int number_of_coils, unsigned char *status, int timeout=1000) int readInputStatus <https://pvbrowser.de/pvbrowser/sf/manual/rllib/html/classrlModbus.html#afbecaf38fc73f8d3082d95be141eeb37> (int slave, int start_adr, int number_of_inputs, unsigned char *status, int timeout=1000) int readHoldingRegisters <https://pvbrowser.de/pvbrowser/sf/manual/rllib/html/classrlModbus.html#ab490c9a04a756f524ae7cc5f138e41f7> (int slave, int start_adr, int number_of_registers, int *registers, int timeout=1000) int readInputRegisters <https://pvbrowser.de/pvbrowser/sf/manual/rllib/html/classrlModbus.html#a60f684a14a1f9c25301ecdecc565538b> (int slave, int start_adr, int number_of_registers, int *registers, int timeout=1000) int forceSingleCoil <https://pvbrowser.de/pvbrowser/sf/manual/rllib/html/classrlModbus.html#a60656255e0053bcaa84c39086b09cf6c> (int slave, int coil_adr, int value, int timeout=1000) int presetSingleRegister <https://pvbrowser.de/pvbrowser/sf/manual/rllib/html/classrlModbus.html#a0c251adb0b9a2029d41f2074566ba5af> (int slave, int register_adr, int value, int timeout=1000) int forceMultipleCoils <https://pvbrowser.de/pvbrowser/sf/manual/rllib/html/classrlModbus.html#abed878560af9610d84bfe2c3d1e54f43> (int slave, int coil_adr, int number_of_coils, unsigned char *coils, int timeout=1000) int presetMultipleRegisters <https://pvbrowser.de/pvbrowser/sf/manual/rllib/html/classrlModbus.html#a7f822ef6a539266b8a2afdc80150b747> (int slave, int start_adr, int number_of_registers, int *registers, int timeout=1000) Could be seen as conveniance methods for unsigned int registers. Best regards: Rainer

Answer 3 · 2018-06-08T12:35:01.000Z

Hi Rainer!

I already know modbus is simply communicating unsigned bytes, but I feel a bit confused with the function, as it allows signed int (32bit) as parameter!

Taking care of the line I say, we can use the function for both 16-bit WORD and 16-bit INT. (I've tested it in a Schneider M340 PLC)

just now it's only for 16-bit WORD. So another solution could be accept only unsigned 16 bit int as parameter. (libmodbus does that)

at least it's what a I think :-D I don't feel 100% sure about.

cheers!
Joa

Answer 4 · 2018-06-08T13:06:59.000Z

Am 08.06.2018 um 14:35 schrieb Joa:

Hi Rainer! I already know modbus is simply communicating unsigned bytes, but I feel a bit confused with the function, as it allows signed int (32bit) as parameter! Taking care of the line I say, we can use the function for both 16-bit WORD and 16-bit INT. (I've tested it in a Schneider M340 PLC) just now it's only for 16-bit WORD. So another solution could be accept only unsigned 16 bit int as parameter. (libmodbus does that) <http://libmodbus.org/documentation/> at least it's what a I think :-D I don't feel 100% sure about. cheers! Joa — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#12 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA4GglyKCeh2b9LJAtAX4eAyU82Cy0BPks5t6m9_gaJpZM4UeMIt>.

Hello Joa, we must stay compatible with ourself when extending rllib. Otherwise we can talk about everything. In this case i would suggest to define a union like typedef union { unsigned char b[4]; unsigned short us[2]; short s[2]; unsigned int ui; int i; float f; }SWAP; Then we can handle all data types. We still have to respect the byte order on the CPU and on the Network. What do you think, would it be sufficient to add the above union definition to class rlModbus ? Best regards: Rainer PS: Eventually add 2 helper methods. void hton(SWAP &data); void ntoh(SWAP &data);

Answer 5 · 2018-06-12T08:53:15.000Z

Hi Rainer!

ok, I'm not used to use union's :-S so the best sollution you think I guess it will the best.
For now I've been using an upper layer.
So I like the point of view of communications staying the simplest and other tools above for managing more complex data.

my questions are:

why not use bit moving operation here?
perhaps at least we should, at documentation, inform that function really expects a 16 bit unsigned int?

Joa

Answer 6 · 2018-06-27T07:12:02.000Z

Hello Joa,

up to now i did not really use the github issue tracker.
May be github should be used more in future.

I added the solution with the union now to the repository. See:
https://github.com/pvbrowser/pvb/blob/master/rllib/lib/rlmodbus.h

Within the Doxygen comment an example for using the union is included.

If you feel that we should have more "conveniance methods" for the different data types,
please send a patch that does it the way you would do it.

Rainer

Answer 7 · 2018-06-27T07:28:23.000Z

perfect!

ps: have you got any other platform for issues and/or patches? I really don't like github either :-D

Answer 8 · 2018-06-27T10:17:33.000Z

There is a little playground at https://github.com/pvbrowser/pvbaddon

There we could create demos / templates that solve a problem we are currently working on.
Within that code we could extend our libraries / document our solution.
Later the extensions can be pulled into the main repository or our documentation.

Answer 9 · 2018-06-27T10:22:59.000Z

any other platform ?
There is https://groups.yahoo.com/neo/groups/pvbrowser/conversations/topics
But people do no longer seem to like it.