steveohara / j2mod

Enhanced Modbus library implemented in the Java programming language
Apache License 2.0
267 stars 111 forks source link

Wrong transaction ID set to request #36

Closed stoorm5 closed 6 years ago

stoorm5 commented 7 years ago

Let's assume to execute the following code:

for (int i = 0; i < 10; i++) {
    ModbusTCPMaster master = new ModbusTCPMaster(slaveAddress,slavePort);
    try {
        master.connect();
    }
    catch (Exception e) {
        logger.error("Error while connecting to slave at {}:{}", slaveAddress, slavePort, e);
        System.exit(-1);
    }
    try {   
        InputRegister[] registers1 = master.readMultipleRegisters(slaveId, 601, 1);
        InputRegister[] registers2 = master.readMultipleRegisters(slaveId, 1001, 1);
    }
    catch (ModbusException e) {
        logger.error("Failure while reading register", e);
    }
    master.disconnect();
}

Here the transaction id associated with the first read request will always be 0, while the transaction id associated to the second read request will be 1 for the first iteration, 3 for the second, and so on. This is due to the fact that, on each iteration, a new ModbusTCPMaster is instantiated and, when invoking readMultipleRegisters(), a new ReadMultipleRegistersRequest object is created too. The newly request object is always initialized with a transaction id equal to 0. Instead, the second readMultipleRegisters() reuses this request object whose transaction id was modified at the end of the previous read operation by ModbusTCPTransaction, setting it to the value of the static property ModbusTransaction.transactionID which is incremented at the end of every read/write operation. I believe that the transaction id associated to the first read in the iteration should also be set to the value in ModbusTransaction.transactionID A possible workaround could be modifying the setRequest method of ModbusTransaction class in the following way:

    public void setRequest(ModbusRequest req) {
        request = req;
        request.setTransactionID(getTransactionID());
    }

In this way, we can always be sure that the transaction id associated to a request will always be equal to the value of ModbusTransaction.transactionID Moreover, a transaction id shaould also be incremented if the current transaction fails.

steveohara commented 7 years ago

Tried your PR but it causes the unit tests to fail

steveohara commented 6 years ago

Without successful unit test I can't add it. Also, time has moved on somewhat