Slave ID for request not verified when accepting response

[platinummonkey]

It appears that the validation process is not checking that the slave id in the request matches the slave id in the response. For a synchronous client a response from a rogue device should not be considered a valid response when reading coils/holding/input/discrete registers.

Edit: oops, referring to server code.

[picarro-cvala]

pyModbus version: 1.3.2

I am also seeing same problem. Problem is not checking slave id, but real problem is it does not able to reach code where it checks slave id and throw exception before that. In this scenario server fails in CRC check of package and throw exception (as it try to parse message as request and CRC is for response).

Location: transaction.py file ModbusRtuFramer->processIncomingPacket(self, data, callback). It will fail in self.checkFrame() method as it will take request as from client, and try to calculate CRC as request.

#-----------------------------------------------------------------------#
# Public Member Functions
#-----------------------------------------------------------------------#
def processIncomingPacket(self, data, callback):
    ''' The new packet processing pattern

    This takes in a new request packet, adds it to the current
    packet stream, and performs framing on it. That is, checks
    for complete messages, and once found, will process all that
    exist.  This handles the case when we read N + 1 or 1 / N
    messages at a time instead of 1.

    The processed and decoded messages are pushed to the callback
    function to process and send.

    :param data: The new packet data
    :param callback: The function to send results to
    '''
    self.addToFrame(data)
    while True:
        if self.isFrameReady():
            if self.checkFrame():
                self._process(callback)
            else:
                # Could be an error response
                if len(self.__buffer):
                    # Possible error ???
                   self._process(callback, error=True)
        else:
            if len(self.__buffer):
                # Possible error ???
                if self.__header.get('len', 0) < 2:
                    self._process(callback, error=True)
            break

[dhoomakethu]

@picarro-cvala please share more information on your setup , the client used ,the modbus device you are trying to control etc

[picarro-cvala]

@dhoomakethu I have one client (master) and two server (salves) connected using serial. My client and server is configure for RTU.

[dhoomakethu]

@picarro-cvala is that a sync client or async client? And are both the slaves connected on the same serial port ? Please share your client code on how you are communicating with the slave devices.

[picarro-cvala]

@dhoomakethu yes server and client are sync. and yes both slaves are connected on the same serial port.

I notice there is one thread for same problem on stack over flow too:
https://stackoverflow.com/questions/45235640/pymodbus-multiple-slaves-error

Client code:

import sys
import time
from struct import pack, unpack
from pymodbus.client.sync import ModbusSerialClient as ModbusClient

com_setting = {"port": "COM2", "baudrate": 9600, "timeout": 3}
client = ModbusClient(method='rtu', **com_setting)
client.connect()
print("Connected to server")

while True:
    try:
      time.sleep(1)
      result = client.read_input_registers(1,12, unit=1)
      values = pack(">12H", *result.registers)
      print("(Timestamp , H2O2 , H2O , Cavity Pressure)")
      print unpack(">12sfff", values)
    
        print("-------------------------------------------------------------------------------")
    except Exception, e:
        print "Error in reading Modbus: %s " % e
        print("-------------------------------------------------------------------------------")

client.close()

[dhoomakethu]

@picarro-cvala if you are using pymodbus sync server to represent your slave, You do not have to have two instances running , you can create multiple slaves on the same server with a slave store dict, something like this.

slaves = {
    0x01: slave1
    0x02: slave2
}

context = ModbusServerContext(slaves=slaves, single=False)

[picarro-cvala]

@dhoomakethu our use case is different, slave are different HW. And server are running on individual HW. And over modbus we control individual HW.

Do you think, in our case we need to use async server?

[dhoomakethu]

Technically that should work, but you will have to make sure your serial line terminations are well in place for the serial lines to work. Your client code deals only with one slave in a while loop do you have another client which does the read transactions for the another slave as well or are you using the same client to transact with the other client. If you are using two clients , You will have to make sure only one client access the serial line at a time as per the modbus/rs485 specs. otherwise the result is unpredictable something in the lines of what you are seeing here. Using an async client is also advisable if you are running repeated transaction at a set interval .

[picarro-cvala]

@dhoomakethu do you mean by async will work?

[dhoomakethu]

@picarro-cvala see my comment above

[picarro-cvala]

@dhoomakethu I have added some more code as below for slave 2. but problem is reproducible if we talk with just one slave and 2nd slave will report error.

I am not using two client. only one client. Agree with you, if more than one client talks on RTU it will be unpredictable behavior.

import sys
import time
from struct import pack, unpack
from pymodbus.client.sync import ModbusSerialClient as ModbusClient

com_setting = {"port": "COM2", "baudrate": 9600, "timeout": 3}
client = ModbusClient(method='rtu', **com_setting)
client.connect()
print("Connected to server")

while True:
try:
time.sleep(1)
#First slave read
result = client.read_input_registers(1,12, unit=1)
values = pack(">12H", *result.registers)
print("Slave1:(Timestamp , H2O2 , H2O , Cavity Pressure)")
print unpack(">12sfff", values)

#Second slave read
result = client.read_input_registers(1,12, unit=2)
values = pack(">12H", *result.registers)
print("Slave 2:(Timestamp , H2O2 , H2O , Cavity Pressure)")
print unpack(">12sfff", values)

    print("-------------------------------------------------------------------------------")
except Exception, e:
    print "Error in reading Modbus: %s " % e
    print("-------------------------------------------------------------------------------")

client.close()

[dhoomakethu]

@picarro-cvala I believe the problem is with the servers , could you please post your server code here (for both servers) ?

[picarro-cvala]

@dhoomakethu yes, error is at server side. In my first message I have describe problem and it is at server side. Also shared exact method where this problem will happen. I think any server example code will work to reproduce problem, for example "https://pymodbus.readthedocs.io/en/latest/examples/synchronous-server.html". Just make one change that give slave id and make context as below:

#1 for slave 1 and 2 for slave 2
self.slaveid = 1
store = ModbusSlaveContext(
di = ModbusSequentialDataBlock(0, [17]*100),
co = ModbusSequentialDataBlock(0, [17]*100),
hr = ModbusSequentialDataBlock(0, [17]*100),
ir = ModbusSequentialDataBlock(0, [17]*100))
ModbusServerContext(slaves={self.slaveid:store}, single=False)

Problem Location: transaction.py file ModbusRtuFramer->processIncomingPacket(self, data, callback). It will fail in self.checkFrame() method as it will assume response for other slave as request from client, and try to calculate CRC as request.

#-----------------------------------------------------------------------#
#Public Member Functions
#-----------------------------------------------------------------------#
def processIncomingPacket(self, data, callback):
''' The new packet processing pattern

This takes in a new request packet, adds it to the current
packet stream, and performs framing on it. That is, checks
for complete messages, and once found, will process all that
exist.  This handles the case when we read N + 1 or 1 / N
messages at a time instead of 1.

The processed and decoded messages are pushed to the callback
function to process and send.

:param data: The new packet data
:param callback: The function to send results to
'''
self.addToFrame(data)
while True:
    if self.isFrameReady():
        if self.checkFrame():
            self._process(callback)
        else:
            # Could be an error response
            if len(self.__buffer):
                # Possible error ???
               self._process(callback, error=True)
    else:
        if len(self.__buffer):
            # Possible error ???
            if self.__header.get('len', 0) < 2:
                self._process(callback, error=True)
        break

[dhoomakethu]

@picarro-cvala Could you please use dev branch with the fix and see if that fixes the issue or You can use this tar file
pymodbus-1.4.0rc1.tar.gz

[picarro-cvala]

@dhoomakethu sure I will try to use dev branch. Meanwhile can you please share code changes for this issue in dev branch (I would like to do code change walk through).

[dhoomakethu]

@piccaro-cvala please refer the commit 5f0d2ed

[picarro-cvala]

@dhoomakethu I did code walk through, and I think change looks good. I will try to use dev branch and verify on real master and slave environment.

[rsfellers]

@dhoomakethu I tested dev branch on real HW and looks good. But I see one problem in the fix. With this commit you are overwriting one existing functionality, which is I think for master request message.

File name: sync.py method ModbusBaseRequestHandler.execute(). With implemented change it will never go into execute method and it will not go into NoSuchSlaveException exception case where it throw exception if slave is missing and ignore_missing_slaves is false.

def execute(self, request):
    ''' The callback to call with the resulting message

    :param request: The decoded request message
    '''
    try:
        context = self.server.context[request.unit_id]
        response = request.execute(context)
    except NoSuchSlaveException as ex:
        _logger.debug("requested slave does not exist: %s" % request.unit_id )
        if self.server.ignore_missing_slaves:
            return # the client will simply timeout waiting for a response
        response = request.doException(merror.GatewayNoResponse)
    except Exception as ex:
        _logger.debug("Datastore unable to fulfill request: %s; %s", ex, traceback.format_exc() )
        response = request.doException(merror.SlaveFailure)
    response.transaction_id = request.transaction_id
    response.unit_id = request.unit_id
    self.send(response)

[picarro-cvala]

@dhoomakethu above comment was mine. By mistakenly I posted comment with my teammate account.

[dhoomakethu]

@picarro-cvala those line become irrelevant now, the modbus exception code GatewayNoResponse in the exception block response = request.doException(merror.GatewayNoResponse) is only applicable for gateway's which is generally a TCP device and is as per modbus spec. Refer section 7 MODBUS Exception Responses. One of the four scenarios read

 If the server receives the request, but detects a communication error (parity, LRC,
CRC, ...), no response is returned. The client program will eventually process a
timeout condition for the request.

The changes are specifically meant for RTU slaves/servers. If you a ModbusTcpServer, you will still see the modbus error being returned. For RTU's we just don't send any response in line with the specs.

[picarro-cvala]

@dhoomakethu sounds good. I have one more question: By any chance do you know when this bug fix will be releasing?

[dhoomakethu]

@picarro-cvala as soon as #209 is closed.

[picarro-cvala]

@dhoomakethu just request: if it will take more than one week, then it is possible to create patch for this fix?

[dhoomakethu]

@picarro-cvala there is not definite timeline , I can create pre-release on github if you would like that. If you want to have it on PyPI then you will have to wait till all the action items are closed

[dhoomakethu]

@picarro-cvala There is a new pre release available 1.4.0rc2 both on PyPI and Github with the fix to this issue. I will close this issue as of now, Please feel free to re-open if you still think the issue is not fixed. Sorry for the delay with the release.
pip install pymodbus==1.4.0rc2

[picarro-cvala]

@dhoomakethu Sounds good.