openhab / openhab1-addons

Add-ons for openHAB 1.x
Eclipse Public License 2.0
3.43k stars 1.69k forks source link

Modbus: Little Endian 32-bit data types #3558

Closed masoud-omidvar closed 8 years ago

masoud-omidvar commented 8 years ago

I think the order of putting read registers in ByteBuffer is incorrect (in ModbusBinding.java). As you can see in the following screenshot (a simple program in a debugger), ByteBuffer class expect to put higher value word (register) first !

image

So, I change the code as:

image

This issue has been created from a comment in https://github.com/openhab/openhab/issues/3537

steve-bate commented 8 years ago

@masoud-omidvar What's the relationship between this and the previous issue? Can the previous issue be closed?

steve-bate commented 8 years ago

@diverseg, should this issue be tagged as a bug or an enhancement?

masoud-omidvar commented 8 years ago

yes, I think this issue should be tagged as a bug.

The other issue (as you have already tagged as 'enhancement') is independent of this item. It would be so fine and helpful, if you can add this enhancement in the next supporting release.

ssalonen commented 8 years ago

I'm not sure I follow.. I think it is arbitrary how to encode 32bit values into 16bit registers in modbus since the standard does not define any interpretation over registers. I can verify that the the current logic of big-endian registers (high 16bits in big endian order followed by low 16bits in big endian format) is something seen in the wild: my Siemens S7 PLC uses this convention for 32bit floats (or REALs as they are called) as well.

In preparation of some future enchanments, I have been writing unit tests for the whole modbus binding. The float32 reading has been tested as well, see my branch. Can you comment on this as well?

Regarding this issue, I would see this more like an enchantment to support different conventions (big endian, little endian, and mixed cases). This approach has been taken in libmodbus, for example.

What do you think guys?

ssalonen commented 8 years ago

I just noted the original discussion in #3537. Did I understand correctly that different endianness combinations (comment from @diverseg) shoud apply here?

masoud-omidvar commented 8 years ago

Dear Sami, If you write a simple unit test for extractStateFromRegisters method based on big endian registers, you will get the point. I do think that most Modbus slaves use this endianness.

ssalonen commented 8 years ago

Hi @masoud-omidvar,

Good to have conversations on the binding! I've been thinking similar issues as well.

Actually I have already made an integration test (jamod modbus server (modbus slave), connected using openhab modbus binding) similar to this topic.

In my test I encode 64 bit integer 123456 (high bytes first) (see java docs) to the registers (see here).

If this endianness is followed, reading using valuetype of int32 seems to work fine in the test case.

But I do understand your point that most/some modbus slaves might follow a differrent logic for encoding 32bit values into registers. But actually the current behaviour is correct for some slaves, see my comment above regarding Siemens S7 PLC.

Per my understanding, the current implementation assumes most-significant-bit (MSB) first ordering. For example, integer value 123456 (decimal) = 0x0001E240 (hex) = 00000000 00000001 11100010 01000000 (binary) = A B C D (bytes) = R1 R2 (registers, i.e. 16bit blocks) is stored into two registers R1 and R2. First register (R1) would have bytes A and B, in MSB order (00000000 00000001), while the second register (R2) would have bytes C and D (11100010 01000000). The bits inside each byte is in big endian format. I used this online converter.

Since the modbus implementation guide (p.11, remark) says that the register data should be encoded in big endian format, I think we are just talking about the order of 16bit dwords (AB and CD), right?

If I understand corrently, you want to switch the order of registers, i.e. interpret two consecutive registers as float32/int32 using the order CD AB (or R2 R1)

Do you agree my interpretation of the current implementation?

masoud-omidvar commented 8 years ago

When I store 123456 at address 400101 in my Modbus slave, it will store it as: |400101 -> E240 |400102 -> 0001

That means, the higher significant word is stored at higher address! Now I realize that most Modbus slaves (including "Siemens S7 PLC") store the higher significant word in the lower address: |400101 -> 0001 |400102 -> E240

So, the issue is an enhancement. To adjust the endianness of the master and slave, we need a property for each slave. as you have already said: ABCD BADC CDAB DCBA

ssalonen commented 8 years ago

Yes, thanks for clarifying this more. I sometimes get confused with the exact terms but this explanation made it very clear!

From the user perspective, I would like to see alternatives to to valuetypes "int32" and "float32". We could introduce the following: int32abcd, int32badc, int32cdab, int32dcba. Naturally it should be well explained in the wiki to be usable...

What do you think?

masoud-omidvar commented 8 years ago

Approach (A) Based on Modbus specification word must be stored as big-endian. So, we can omit byte-swap combinations (badc & dcba) and stick to word-swap cases (Using less types is more convenient). So we could have the following types: int32abcd, int32cdab, float32abcd, float32cdab

Approach (B) we can use the current types and introduce a new boolean property as MSWF (Most Significant Word First) with false as default value.

Thanks for your kindly follow up.

mbs38 commented 8 years ago

masoud-omidvar:

Based on Modbus specification word must be stored as big-endian.

Reference?

In Modbus the Hi-byte is transmitted first.

For each register, the first byte contains the high order bits and the second contains the low order bits.

Found on page 31 in this document: http://modbus.org/docs/PI_MBUS_300.pdf On the page there is also an example of a response to the "Read Input Registers" command:

Field Name (Hex) Slave Address 11 Function 04 Byte Count 02 Data Hi (Register 30009) 00 Data Lo (Register 30009) 0A Error Check (LRC or CRC) ––

Note that "Data Hi" (value 0x00) is transmitted first.

The example is followed by:

The contents of register 30009 are shown as the two byte values of 00 0A hex, or 10 decimal.

Therefore: 0x000A, if interpreted as 10 decimal is big endian if the bytes are counted from the right side on.

IMHO we should omit abcd & cdab and NOT badc & dcba

What am I getting wrong here? :)

Best regards

ssalonen commented 8 years ago

@mbs38 I think you found the reference yourself? In any case, here's the other reference quoted before

Since the modbus implementation guide (p.11, remark) says that the register data should be encoded in big endian format, ...

Please see the examples section in wikipedia. In our case we have atomic element size of 16bits (=size of one register). Bits inside the "atomic element" must be in big endian format just like as you described. I find it unlike that we would find modbus slaves not respecting this (although everything is possible :smile:) -- the "badc" or "dcba" ordering would be called middle/mixed endian according to the wikipedia article.

In the wikipedia example they are encoding 32bit number 0x0A0B0C0D Big endian ("word/register ordering"):

The most significant atomic element stores now the value 0x0A0B, followed by 0x0C0D.

Little endian ("word/register ordering"):

The least significant 16-bit unit stores the value 0x0C0D, immediately followed by 0x0A0B. Note that 0C0Dh and 0A0Bh represent integers, not bit layouts.

It's really about how we interpret the data in the registers. Let's assume the first register have 0x0A0B and value 0x0C0D. We know that these two registers together represent 32bit signed integer stored in two's complement format. Do we interpret it as

In this example we have the following bytes corresponding to our abcd notation

I agree with @masoud-omidvar's last comment, and I propose we go with option A since the boolean MSWF is not applicable to all value types (only the 32bit types).

Hope this clarifies things and I got everything correct...

mbs38 commented 8 years ago

Hope this clarifies things and I got everything correct...

It does. Thanks a lot!

w2vy commented 8 years ago

From my work with MODBUS the Modbus spec does specify the bit order for the 16 bit words, but there is nothing in the spec that covers larger data units.

In this thread I suggested

For example if my UINT32 register contains 0x12345678 and I read it I get 0x56781234

I'd like to have a slave configuration option something like

modbus.tcp.slave1.wordorder= [normal | swapped] and of course modbus.rtu.slave1.wordorder= [normal | swapped]

Obviously the default would be normal!


From the example UINT32 you see that the 16 bit value is encoded big endian but it is up to the slave to define the WORD order.

From my usage of the binding I believe this is not a bug but a very desirable Improvement!

I would be willing to help get this done! But I am need help with the logistics of getting the code, generating the patch... yes I know git... so clone, pull? push? I forget

tom

ssalonen commented 8 years ago

By the way, I believe the title should be renamed Little Endian 32-bit data types as that is actually what is wanted by the author.