Check received data length

[ccatterina]

Hi @dhoomakethu ,
I made this PR mostly to solve the problem discussed in #188, this would fix that issue.
In the _recv function, the TransactionManager could receive an incomplete message like 0xFF, 0x10 or 0x040101 causing the raise of several exceptions because the actual length of the received message wasn't checked.

In addition i change the way that a message is received through a socket connection, so that recv(size) waits until full data is received, or timeout is expired. (That is consistent with read(size) function of serial client)

Let me know what you think.
Thanks

[dhoomakethu]

@ccatterina thanks for the PR, I will take a look .

[dhoomakethu]

This would throw TypeError (None - min_size) for messages with out the get_response_pdu_size method defined (for e.g. all mei_messages and diagnostic messages). You can run synchronous_client_ext.py under examples\common to check this behaviour.

[dhoomakethu]

We can not pass None as response_length, in cases where a transaction returns no response, the argument full would be set to true and subsequent transactions would try to read the full length (None in some cases) throwing an error. Can we have 1024 back here ?

[ccatterina]

I have made a wrong assumption, that is all messages have an expected_response_length.

If i get the 1024 back, the client._recv function will wait until it receives 1024 bytes or the timeout expires, so probably it always waits until the timeout expires, making the communication very slow.

Another solution should be to make the client._recv(size) to return the data as soon as at least 1 byte is received, without waits that timeout expires when size=None is passed.

What do you think about it?

[dhoomakethu]

In most of the normal cases (read/write registers/coils) expected_length is always calculated so it should not be an issue, for some diagnostic or mei requests the response length is dynamic and hence we could choose to read 1024 bytes till time out ,which is still OK with me.

[dhoomakethu]

@ccatterina I see that your changes are applicable only for Tcp client , do you plan to have these changes for serial client as well ?

[ccatterina]

I see that your changes are applicable only for Tcp client , do you plan to have these changes for serial client as well ?

Are you referring to the changes of ModbusTcpClient and ModbusUdpClient _recv method?
If yes, the serial doesn't need any changes because serial.read has the same behavior as the has the new _recv methods of tcp and udp clients.

Anyway, as you already noticed i've not considered the messages for which there isn't an expected size.
I'll try to find a solution as soon as i have time.

[dhoomakethu]

@ccatterina Thanks for the clarification. Looking forward to merge this.

[dhoomakethu]

@ccatterina I did some work based on your branch here, please give a look when you get time. For UDP, there are high possibility that the data is lost if we try to read data in a loop, so I am sticking to 1024 size (default), for serial interaface ,I am now checking for the bytes available in the read buffer and reading. I am sure that needs further tuning in cases where the slave responds a bit slow.

[ccatterina]

@ccatterina I did some work based on your branch here, please give a look when you get time.

Thank you for your effort, now i give a look to the branch.

For UDP, there are high possibility that the data is lost if we try to read data in a loop, so I am sticking to 1024 size (default).

I'm missing something, what is the difference between TCP and UDP that cause this problem?

for serial interaface ,I am now checking for the bytes available in the read buffer and reading. I am sure that needs further tuning in cases where the slave responds a bit slow.

I think not, the serial.read(size) waits until size charachers are received or timeout is expired, so if the timeout is big enough the serial client works well also with slow slaves, isn't it?

[ccatterina]

• In transaction.py, in the _recv method:

              read_min = self.client.framer.recvPacket(min_size)
              if read_min:
                 if isinstance(self.client.framer, ModbusSocketFramer):
                      func_code = byte2int(read_min[-1])
                  elif isinstance(self.client.framer, ModbusRtuFramer):
                      func_code = byte2int(read_min[-1])
                  elif isinstance(self.client.framer, ModbusAsciiFramer):
                      func_code = int(read_min[3:5], 16)
                  elif isinstance(self.client.framer, ModbusBinaryFramer):
                      func_code = byte2int(read_min[-1])
                  else:
                      func_code = -1

  we probably need to check if the read_min has the right size, for example if read_min = '' an
  IndexError is raised when byte2int(read_min[-1]) is executed.

• If _recv method of tcp client accept None as value of size I think that also the serial and udp clients _recv should accept it.

[dhoomakethu]

@ccatterina thanks for the review comment.
if read_min: should take care of cases with empty string, the condition will pass only if atleast one character is available in the received buffer.

[dhoomakethu]

@ccatterina re. UDP, UDP protocol does not handle error checks , the packets are just sent to the recipients and will not wait for the recipient actually received the packet, if we miss the packet its gone where as with tcp, the packets sent are tracked and ensures that recipient actually receives those packets. So, with UDP if try to read in loop, there are high possibility that the sender sends all but those packets are not lost in the next iterations.
Re. your question with inWaiting/in_waiting what I meant was if the sender is too slow and in_waiting returns zero, things might fail. in_waiting\InWaiting returns a positive value when there is data in the receive buffer ready to be processed.

[ccatterina]

if read_min: should take care of cases with empty string, the condition will pass only if atleast one character is available in the received buffer.

Yes, sorry my bad, but if we receive, for example, 1 byte, it will raise a TypeError if we are using a ModbusAsciiFramer .
Or a struct.error if we're using a ModbusSocketFramer with a function code < 0x80.

[ccatterina]

Re. your question with inWaiting/in_waiting what I meant was if the sender is too slow and in_waiting returns zero, things might fail. in_waiting\InWaiting returns a positive value when there is data in the receive buffer ready to be processed.

Ok, now i get it, maybe the best solution is still to wait for 1024 characters if there isn't an expected response length.

[dhoomakethu]

@ccatterina I have modified the logic a little to handle the slow reception of message. Let me know if that looks ok.

[ccatterina]

@dhoomakethu yeah, it looks fine to me, waiting until there is something in the buffer should solve the problem.

[ccatterina]

Maybe you can also replace pass with a sleep in the tcp client recv:

        if size is not None:
             while len(data) < size:
                 try:
                     data += self.socket.recv(size - len(data))
                 except socket.error:
                     time.sleep(0.01)
                 if not self.timeout or (time.time() - begin > self.timeout):
                     break

[ccatterina]

@dhoomakethu Anyway i'm testing your branch in one of our PV plants and it seems to work, now is running for an hour without crash. (With the v1.5.0 and previous it crashed after few minutes)

[dhoomakethu]

@ccatterina thanks for the confirmation. I did pushed some more changes to better handle some corner cases. I am testing locally as well and will release an official version by end of this week

[ccatterina]

sorry @dhoomakethu, have you seen my comment?

if read_min: should take care of cases with empty string, the condition will pass only if atleast one character is available in the received buffer.

Yes, sorry my bad, but if we receive, for example, 1 byte, it will raise a TypeError if we are using a ModbusAsciiFramer .
Or a struct.error if we're using a ModbusSocketFramer with a function code < 0x80.

What you think about it? Am i wrong?

[dhoomakethu]

@ccatterina I think it is already taken care here, if we are not getting the minimum data , we raising exception.

read_min = self.client.framer.recvPacket(min_size)
if not read_min:
         return read_min
if len(read_min) < min_size:
         raise InvalidMessageReceivedException(
                    "Incomplete message received, expected at least %d bytes (%d received)"
                    % (min_size, len(read_min)))

[ccatterina]

@dhoomakethu yes, but there isn't that code in your branch.

I thought that your branch replace my pull request, isn'it?

[dhoomakethu]

That's correct, I will have a look. Thanks ಗುರು, ಏಪ್ರಿ 26, 2018 9:28 ಅಪರಾಹ್ನ ದಿನಾಂಕದಂದು Claudio Catterina < notifications@github.com> ಅವರು ಬರೆದಿದ್ದಾರೆ:

…

@dhoomakethu <https://github.com/dhoomakethu> yes, but there isn't that code in your branch <https://github.com/riptideio/pymodbus/tree/patch-1>. I thought that your branch replace my pull request, isn'it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#288 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI2OLhP9lQT4tqr60BRo9XuDkH3dynFEks5tse65gaJpZM4TZv2_> .

[dhoomakethu]

its taken care now https://github.com/riptideio/pymodbus/pull/294/files#diff-40a48d0b5cf576d37ccece3543183f65R237

[ccatterina]

👍 I close the pull request.