Interest in creating a base class for modbus.client classes

[jbm950]

There are similar _methods in the ModbusClientRTU and ModbusClientDeviceTCP classes and so I thought I would do a comparison of the _read method to see if there's interest in creating something along the lines of a ModbusClientBase class. Below is a pseudo code draft of how such a class might look.

class ModbusClientBase:
    def _read(addr, count, op=FUNC_READ_HOLDING, trace_func=None, slave_id=None):
        resp = ''
        len_found = False
        except_code = None

        if slave_id is not None: #an RTU instance instead of TCP
            len_remaining = #RTU version
            req_structure = #RTU structure for the req struct command
            trace_func_structure = #RTU structure for the trace func inputs
        else: #TCP instance and not RTU instance
            len_remaining = #TCP version
            req_structure = #TCP structure for the req struct command
            trace_func_structure = #TCP structure for the trace func inputs

        req = struct.pack(req_structure, #Maybe a list containing the other TCP vs RTU specific arguments)

        if self.trace_func: #RTU has trace_func as an input and TCP has it attached to self. Should a uniform method be chosen?
            s = # This portion would need to be dynamically constructed based on TCP vs RTU
            for c in req:
                s += '%02X' % (ord(c))
            self.trace_func(s)

        # The TCP and RTU differ in their send methods and so binding a brief function in 
        # the above if statement to a variable `send` then call it here. For example, in 
        # the TCP case send = self.socket.sendall

        while len_remaining > 0:
            # Same idea here for a `receive` variable as the `send` variable listed above
            c = receive(len_remaining)
            len_read = len(c)
            if len_read > 0:
                resp += c
                len_remaining -= len_read
                if len_found is False and len(resp) >= #Dynamically set original len_remaining based on TCP vs RTU:
                    if not (ord(resp[#TCP VS RTU specific value]) & 0x80): #This statement
                        # appears in the while loop in the RTU version and outside the loop in 
                        # the TCP version
                else:
                    raise ModbusClientError('Response timeout')

        if trace_func:
            s = # This portion is RTU vs TCP specific and would need to be structured in the first 
                  # if statement
            for c in resp:
                s += '%02X' % (ord(c))
            trace_func(s)

        # There's a CRC check in RTU version but not TCP version. I am not familiar with CRC and so
        # I don't know if this is something that should be in both

        if except_code:
            raise ModbusClientException('Modbus exception %d' % (except_code))

        return resp[# This splice depends on RTU vs TCP and would need to be configured in the first if statement]

The ModbusClientRTU._read method can be found at here and the ModbusClientDeviceTCP._read method can be found here.

Pros:

• Reduces code redundancy. If an error pops up it can be fixed for both TCP and RTU simultaneously

Cons:

• Have to make variables have different meanings in the function based on the client type

[altendky]

I'll try and take a look at this tomorrow but I'll mention that there is some general trouble with too many layers involved in constructing. We end up having to add pass through parameters to several layers to get at the serial object. I recently got a request for a couple more. This also plays into using the faux enumerations to select the type of connection. This is much less needed in Python since we can comfortably pass around classes and callables. I know this is vague but perhaps you have already noticed this. I should probably just implement the extra serial options as an example.

[jbm950]

I was talking to Bob about this yesterday and we felt I was not familiar enough with modbus and there may not have been enough benefit for me to work on this.

[altendky]

I barely know modbus and I made a working twisted version... 1) it's often about programming as much as topic. 2) even more duplication that needs to be tamed. Let alone when someone wants a twisted tcp option. But, I would get py3 integrated first. Notice a theme in my priority? :]

[altendky]

How about extracting all the uncommon in functions and then implementing them in the child classes? This is really quick and dirty but gives an idea. Some functionality changed and may not be acceptable, but the original code needs to be tidied anyways, probably before attempting to extract a base class.

def _read(self, addr, count, op=FUNC_READ_HOLDING):
    resp = ''
    len_remaining = self.initial_read_length()
    len_found = False
    except_code = None

    req = self.pack(op, addr, count)

    self.trace('->', req)

    self.flush_input()
    try:
        self.transport_write(req)
    except Exception, e:
        raise ModbusClientError('%s write error: %s' % (self.type_string, str(e)))

    while len_remaining > 0:
        c = self.transport_read(len_remaining)
        len_read = len(c)
        if len_read > 0:
            resp += c
            len_remaining -= len_read
            if len_found is False and len(resp) >= self.initial_read_length():
                len_remaining, len_found, except_code = self.len_remaining(resp, len_remaining, len_found, except_code)
        else:
            raise ModbusClientTimeout('Response timeout')

    self.trace('<--', resp)

    self.check_result(resp)

    return self.data_from_response(resp)

def trace(direction, data)
    if self.trace_func:
        s = '%s:%s[addr=%s] %s' % (self.trace_identifier(), str(self.slave_id), addr, direction)
        for c in data:
            s += '%02X' % (ord(c))
        self.trace_func(s)