read_tag does not parse properly if plc response with a proper error packet about pycomm

[ruscito]

That sounds good Thanks Ago

…

On Sun, Apr 9, 2017 at 10:07 PM, Ahnaf Siddiqi ***@***.***> wrote: Hi ruscito, read_tag() generates an error when a plc responds with a properly constructed error_packet. Traceback (most recent call last): .. ... File "/home/ahnafsidd/Github/pycomm/ab_comm/clx.py", line 414, in read_tag raise DataError(e) After a bit of debugging, the problem seems to lie in this section of the code (clx.py line 397 onwards) and *_check_reply()*: if self.send_unit_data( build_common_packet_format( DATA_ITEM['Connected'], ''.join(message_request), ADDRESS_ITEM['Connection Based'], addr_data=self._target_cid, )) is None: raise DataError("send_unit_data returned not valid data") if multi_requests: return self._parse_multiple_request_read(tag) else: # Get the data type data_type = unpack_uint(self._reply[50:52]) try: return UNPACK_DATA_FUNCTION[I_DATA_TYPE[data_type]](self._reply[52:]), I_DATA_TYPE[data_type] except Exception as e: raise DataError(e) as you know, *_check_reply()* is called by *send_unit_data()* which parses the error response properly, but it is not handled when *send_unit_data* is called as it only checks for *None* and hence the parsing later on raises an error. As of now the latter half of the code is not differentiating between a malformated packet which causes a problem in parsing and a properly formatted error packet which should return with a message without raising an error. I have a fix below, but I am not sure if its the best one. # Get the data type if self._check_reply(): ###--- calling self._check_reply() data_type = unpack_uint(self._reply[50:52]) try: return UNPACK_DATA_FUNCTION[I_DATA_TYPE[data_type]](self._reply[52:]), I_DATA_TYPE[data_type] except Exception as e: raise DataError(e) else: return False, "Check Encapsulation and Message Router Error" # or any messages for that matter. If you think this is good, I can create a pull request as I have already made a modification of it in my fork. Thanks! - Ahnaf — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#23>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABL5oZVt62OP7Qnl91bpvnmKeKN74Dt3ks5ruZzogaJpZM4M4RcP> .

[gman8a]

For this reason I always send a single tag request as a 'multi' tag request. i.e., read_tag(['tag_1']). Then an error does NOT occur, if tag_1 does not exist. The function response returns None.

If indeed, the tag_1 does exist, then a read_tag('tag_1') works fine.

[ruscito]

Ahnaf first of all thank, that helped to improve the read_tag method. I slightly change from what you propose because I decided to check self._status and return None instead of a string. There is a method called get_status that normally return a tuple with a number (sort of error number) and a string (containing the error message). See the code below for details

        # Get the data type
        if self._status[0] == SUCCESS:
            data_type = unpack_uint(self._reply[50:52])
            try:
                return UNPACK_DATA_FUNCTION[I_DATA_TYPE[data_type]](self._reply[52:]), I_DATA_TYPE[data_type]
            except Exception as e:
                raise DataError(e)
        else:
            return None

[aporan]

aah, that looks neater. Glad to have helped. I'll close the issue then. Perhaps, you could delete the pr.