Why does BinaryPayloadBuilder.add_bits flip bit order
MrWaloo opened this issue · 8 comments
Versions
- Python: 3.11
- OS: debian 11
- Pymodbus: 3.7.2
- Modbus Hardware (if used): Wago PLC, Modbus TCP
Description
I am encountering the same issue than teddyrendahl: the bit order in the bytes is reversed. Like him, I absolutely am aware of the byte and word endianess, that's not the point.
I took the same issue title.
I have a Wago PLC in which I can read the memory with Modbus requests. I am only encountering an issue when I read bits/coils.
In this PLC, I test pymodbus coil reading function on a word addressed at %MW203. Every 16 bit of this word can be seen here for the test case:
So the 3 first bits are set, every others aren't. The value of the word is 7:
2 ** 0 + 2 ** 1 + 2 ** 2 = 1 + 2 + 4 = 7
I want to read the first byte of this word. This byte should have the value b'\x07'
Code and Logs
>>> from pymodbus import FramerType
>>> from pymodbus.payload import BinaryPayloadDecoder
>>> from pymodbus.client import ModbusTcpClient
>>> from pymodbus.pdu.bit_read_message import ReadCoilsRequest
>>> from pymodbus.pdu.register_read_message import ReadHoldingRegistersRequest
>>>
>>> rq1 = ReadCoilsRequest(15536, 8, 1)
>>>
>>> client = ModbusTcpClient(host='192.168.1.20', port=502, framer=FramerType.SOCKET)
>>> client.connect()
True
>>> resp = client.execute(rq1)
>>> resp.bits
[True, True, True, False, False, False, False, False]
Here we can see that the bits are read correctly: only the 3 first bits are set. But the good practice for endianess converts is to work with BinaryPayloadDecoder, let's do this :
>>> dec = BinaryPayloadDecoder.fromCoils(resp.bits)
>>> dec.decode_bits()
[False, False, False, False, False, True, True, True]
>>> dec._payload
b'\xe0'
The result of decode_bits()
is reversed from what I expected.
This isn't due to the functions pack_bitstrings
and unpack_bitstrings
, because, as transistorgrab wrote, they perfectly do their job. It is due to the fromCoils()
class method:
@classmethod
def fromCoils(
cls,
coils,
byteorder=Endian.LITTLE,
_wordorder=Endian.BIG,
):
"""Initialize a payload decoder with the result of reading of coils."""
if isinstance(coils, list):
payload = b""
if padding := len(coils) % 8: # Pad zeros
extra = [False] * padding
coils = extra + coils
chunks = cls.bit_chunks(coils)
for chunk in chunks:
payload += pack_bitstring(chunk[::-1]) # Here, chunk is reversed and should not be
return cls(payload, byteorder)
raise ParameterException("Invalid collection of coils supplied")
In the class BinaryPayloadBuilder
, the function to_coils()
has the same issue:
def to_coils(self) -> list[bool]:
"""Convert the payload buffer into a coil layout that can be used as a context block.
:returns: The coil layout to use as a block
"""
payload = self.to_registers()
coils = [bool(int(bit)) for reg in payload for bit in format(reg, "016b")] # Here ...
return coils
Since format(reg, "016b")
returns a representation for human of reg the result cannot be used to loop over it.
>>> format(7, "016b")
'0000000000000111'
>>> format(7, "008b") # just for the example
'00000111'
The representation returned by format
is correct and shows the order from the highest significance to the lowest significance.
But a code returning the bit order from the lowest significance to the highest should be used.
Now: I have searched fromCoils
and to_coils
in the entire repository and have not found any detailed documentation of the expected results, but the way bits payload are managed does not fit my expectations and need. Am I the only one? Should it be corrected? Can I make a pull request?
A couple of points:
- no need to call execute, we provide high level methods for all modbus functions.
- endianess is handled by the execute function, so its wrong to handle it additionally.
- According to the modbus standard first coil is in the first bit of the data, which is what you get.
- binarypayloader can revert the bits, to what you want (dec.decode_bits(), as you have already seen).
I am not sure what the problem is, as far as I can see
>>> resp = client.execute(rq1)
>>> resp.bits
[True, True, True, False, False, False, False, False]
Is correct according to the standard, and it is corrected for endianness, which you would see more clearly if you read more then 8 bits.
>>> dec = BinaryPayloadDecoder.fromCoils(resp.bits)
>>> dec.decode_bits()
[False, False, False, False, False, True, True, True]
Reverses the bits correctly, mainly because you do not specify which endianness you want.
- no need to call execute, we provide high level methods for all modbus functions.
See the documentation:
No need, but it can be done this way.
I try to use pymodbus as a plugin for Jeedom... I have my reasons.
- endianess is handled by the execute function, so its wrong to handle it additionally.
As I said, no endianess here, only one single byte
- According to the modbus standard first coil is in the first bit of the data, which is what you get.
Exactly, but only in resp.bits, not in what decode_bits returns.
The first coil must be at index 0, not at index 7
- binarypayloader can revert the bits, to what you want (dec.decode_bits(), as you have already seen).
That's wrong. The bits must not been reversed. The endianess only applies to byte and word order.
I am not sure what the problem is, as far as I can see
>>> resp = client.execute(rq1) >>> resp.bits [True, True, True, False, False, False, False, False]
Is correct according to the standard, and it is corrected for endianness, which you would see more clearly if you read more then 8 bits.
>>> dec = BinaryPayloadDecoder.fromCoils(resp.bits) >>> dec.decode_bits() [False, False, False, False, False, True, True, True]
Reverses the bits correctly, mainly because you do not specify which endianness you want.
That's wrong. The bits within a byte must not been touched. The endianess only applies to byte order. The byte content have to stay identical. By reversing the bits, you change the value of the byte! b'\x07' becomes b'\xe0'.
As shown in the code, the bit reversing is always done and is not done according to endianess (which would be wrong either), as you may think:
@classmethod
def fromCoils(
cls,
coils,
byteorder=Endian.LITTLE,
_wordorder=Endian.BIG,
):
"""Initialize a payload decoder with the result of reading of coils."""
if isinstance(coils, list):
payload = b""
if padding := len(coils) % 8: # Pad zeros
extra = [False] * padding
coils = extra + coils
chunks = cls.bit_chunks(coils)
for chunk in chunks:
payload += pack_bitstring(chunk[::-1]) # <<<<<----- Here, chunk is reversed and should not be <<<<<< ----- HERE
return cls(payload, byteorder)
raise ParameterException("Invalid collection of coils supplied")
IRL, I am an automation Engineer since 2003 and I have never seen bit swapping into a byte for endianess adaptation reason. Because it is wrong.
Prompt for chatGPT or Gemini:
Is it correct to reverse the bit order inside a byte to adapt the endianess?
It basically have nothing to the with endianness, only indirectly.....but long time ago, someone got tired of answering issues, why coil 0, was in bit 7 and not bit 0, hence the current function.
You might call it wrong, but there are still users out there, who prefer coil 1 == bit 0, and for these uses it works....in your case simply forget binary payload, since you already have what you want.
There are no practical need nowadays for binarypayloader (revert a list is a simple call in python), and it will soon be removed, since it is all handled internally in the API.
I have been working in automation industry since 1985, a lot of years defining protocols, so I think I also know a bit or two.
You might call it wrong, but there are still users out there, who prefer coil 1 == bit 0, and for these uses it works....
Coil 1 is bit 0 in a list. But sadly not in the internal payload of the BinaryPayloadDecoder.
It is not because hundred of people think that the Earth is flat that it actually is. The binary representation of a value is with the less significant bit on the right side (as for decimal representation btw). This does not mean, that the representation in a list must fit this order. It is only a matter of representation but it has its importance. If the list would been represented vertically, the problem would be identical: top to bottom or bottom to top.
Different python representation of a byte containing the value 7:
7
b'\x07'
'00000111'
[1, 1, 1, 0, 0, 0, 0, 0]
[True, True, True, False, False, False, False, False]
All are correct. With the 3 True at the end of the list, it is a mistake.
There are no practical need nowadays for binarypayloader (revert a list is a simple call in python), and it will soon be removed, since it is all handled internally in the API.
Yes of course there are needs. This class is very useful to extract real. for instance, from a payload with the correct endianess. It is a good tool and it works but for boolean/coil. I would miss this class if it disappears.
Btw, I did subclasses BinaryPayloadStrictDecoder and Builder with the best practice handling of coils and strings. If you think it would help other people, I can make a PR.
I have been working in automation industry since 1985, a lot of years defining protocols, so I think I also know a bit or two.
I love my job and you might love yours too. There are rules, not just for fun, but because of the logic and the mathematics.
Why would you ever extract a real from coils ? The binary payload will be gone in 3.8.
The is basically the only remainder of payload, for registers we have provided other methods to convert registers <-> int32/64, float32/64 for a long time.
The app should not know about endianess, and our current API secures that these conversions are done internally.
I use fromRegisters when I read registers, but when coils and read, I have to use fromCoils. All decode_* work fine.
Are we misunderstanding each other?
My initial need was to read coils out of a Wago PLC. Only to test the functionality. I could need to read digital inputs out of a Wago Modbus coupler, the need is the same. If I read 32 bits (only one request to save time) and want to extract the bits I want, the bit order and the endianess inside pymodbus has to be correct. If I read %IW0 and %IW1 and need to extract %IX0.5, %IX0.7, %IX0.12, %IX0.15, %IX1.0, %IX1.3 and %IX1.11, I can do it this way:
rq1 = ReadCoilsRequest(15536, 32, 1) # sample address
client = ModbusTcpClient(host='192.168.1.20', port=502, framer=FramerType.SOCKET)
client.connect()
result = client.execute(rq1)
byteorder = whatever
wordorder = whatever_too
decoder = BinaryPayloadDecoder.fromCoils(result, byteorder, wordorder)
byte0 = decoder.decode_bits()
ix0_5 = byte0[5]
ix0_7 = byte0[7]
byte1 = decoder.decode_bits()
ix0_12 = byte1[4]
ix0_15 = byte1[7]
byte2 = decoder.decode_bits()
ix1_0 = byte2[0]
ix1_3 = byte2[3]
byte3 = decoder.decode_bits()
ix1_11 = byte3[3]
This would theoretically work. But only if the bit order (nothing to do with the endianess) is correct into decoder.
Again forget about the binarypayload, it does not make sense to use that !
do a readCoils and you get a list returned in the endianess of the CPU executing the code...you can then manipulate the list as you want.
Ohh just to be sure, you can continue to use the execute call. As you can see min mixin,py readCoils does nothing but calls execute.
In fact, I have just discovered the client class methods convert_to_registers and convert_from_registers, since you have just written, that BinaryPayload does not make any sense. A fresh perspective for me. I will definitively give it a try!
Sadly, there is nothing in the examples that could have shown me the existence of these functions.
Now I understand you. It indeed was a misunderstanding.
Ohh just to be sure, you can continue to use the execute call. As you can see min mixin,py readCoils does nothing but calls execute.
OK, I will.
Thank you Jan