steveohara / j2mod

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

Calling the readInputDiscretes() method multiple times under certain conditions on the same ModbusTCPMaster instance resulted in an error. #114

Closed kazuyatada closed 3 years ago

kazuyatada commented 3 years ago

Expected Behavior

It should be possible to read if it is in the range up to the last readable address (65535).

Actual Behavior

Under certain conditions, results will fail even in the range up to the last readable address (65535).

The setReference() method does the comparison: bitCount + ref > 65536. This bitCount is an instance variable. Since ModbusTCPMaster recycles ReadInputDiscretesRequest instances, the second and subsequent requests use the bitCount of the previous one for comparison. In other classes (e.g.: ReadCoilsRequest, ReadInputRegistersRequest), the setReference() method do not make such a comparison, so you probably don't need to make a comparison in ReadInputDiscretesRequest either.

Steps to Reproduce the Problem

ModbusTCPMaster master = null;
try {
    master = new ModbusTCPMaster(address, port);
    master.connect();

    // Request address between 0 and 7.
    BitVector head8 = master.readInputDiscretes(unitId, 0, 8);

    // Request address between 65531 and 65535. However, an incorrect comparison will cause an error: 8 + 65531 > 65536.
    BitVector tail5 = master.readInputDiscretes(unitId, 65531, 5);

} catch (Exception e) {
    e.printStackTrace();
} finally {
    if (master != null) {
        master.disconnect();
    }
}

Result.

java.lang.IllegalArgumentException
    at com.ghgande.j2mod.modbus.msg.ReadInputDiscretesRequest.setReference(ReadInputDiscretesRequest.java:126)
    at com.ghgande.j2mod.modbus.facade.AbstractModbusMaster.readInputDiscretes(AbstractModbusMaster.java:176)

Specifications

steveohara commented 3 years ago

You're right. This is a hangover from the original jmod version but I can't think why the cost of creating request objects was deemed to be so high that it reuses them?? I think I will look to change it so that they are all local instances which will avoid this problem altogether and allow checks to be added to the other request types if needed