problem in rlmodbusclient with negative values

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

craigtao commented 6 years ago

Hi What are you trying to achieve?

craig

pvbrowser commented 6 years ago

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

gentooza commented 6 years ago

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

pvbrowser commented 6 years ago

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 https://github.com/pvbrowser/pvb/issues/12#issuecomment-395747215, 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);

gentooza commented 6 years ago

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

pvbrowser commented 6 years ago

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

gentooza commented 6 years ago

perfect!

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

pvbrowser commented 6 years ago

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.

pvbrowser commented 6 years ago

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

pvbrowser / pvb

problem in rlmodbusclient with negative values #12

perhaps at least we should, at documentation, inform that function really expects a 16 bit unsigned int?