From 4c8f3a67cab91088be90dd761b2fb6db5061afb3 Mon Sep 17 00:00:00 2001 From: Stefan Feilmeier Date: Thu, 24 Aug 2023 01:41:28 +0200 Subject: [PATCH] ModbusBridge: Fix ClassCastException to Register[] (#2320) This fixes the issue reported in https://community.openems.io/t/modbus-tcp-gerat-mit-eclipse-debuggen/1839/2 * Add JUnit test that shows previous implementation error * Parse FC4 response to Register[] Tested on private setup --- ...sk.java => AbstractReadRegistersTask.java} | 16 ++--- .../modbus/api/task/FC3ReadRegistersTask.java | 6 +- .../api/task/FC4ReadInputRegistersTask.java | 16 +++-- .../bridge/modbus/DummyModbusComponent.java | 4 +- .../element/SignedDoublewordElementTest.java | 1 - .../api/element/StringWordElementTest.java | 3 - .../modbus/api/task/FC1ReadCoilsTaskTest.java | 23 ++++++- .../api/task/FC2ReadInputsTaskTest.java | 23 ++++++- .../api/task/FC3ReadRegistersTaskTest.java | 27 ++++++-- .../task/FC4ReadInputRegistersTaskTest.java | 63 +++++++++++++++++-- 10 files changed, 149 insertions(+), 33 deletions(-) rename io.openems.edge.bridge.modbus/src/io/openems/edge/bridge/modbus/api/task/{AbstractReadInputRegistersTask.java => AbstractReadRegistersTask.java} (61%) diff --git a/io.openems.edge.bridge.modbus/src/io/openems/edge/bridge/modbus/api/task/AbstractReadInputRegistersTask.java b/io.openems.edge.bridge.modbus/src/io/openems/edge/bridge/modbus/api/task/AbstractReadRegistersTask.java similarity index 61% rename from io.openems.edge.bridge.modbus/src/io/openems/edge/bridge/modbus/api/task/AbstractReadInputRegistersTask.java rename to io.openems.edge.bridge.modbus/src/io/openems/edge/bridge/modbus/api/task/AbstractReadRegistersTask.java index 939fad4ddfb..936971a7464 100644 --- a/io.openems.edge.bridge.modbus/src/io/openems/edge/bridge/modbus/api/task/AbstractReadInputRegistersTask.java +++ b/io.openems.edge.bridge.modbus/src/io/openems/edge/bridge/modbus/api/task/AbstractReadRegistersTask.java @@ -4,7 +4,7 @@ import com.ghgande.j2mod.modbus.msg.ModbusRequest; import com.ghgande.j2mod.modbus.msg.ModbusResponse; -import com.ghgande.j2mod.modbus.procimg.InputRegister; +import com.ghgande.j2mod.modbus.procimg.Register; import io.openems.common.exceptions.OpenemsException; import io.openems.edge.bridge.modbus.api.element.ModbusElement; @@ -12,23 +12,25 @@ import io.openems.edge.common.taskmanager.Priority; @SuppressWarnings("rawtypes") -public abstract class AbstractReadInputRegistersTask - extends AbstractReadTask { +public abstract class AbstractReadRegistersTask // + extends AbstractReadTask { - public AbstractReadInputRegistersTask(String name, Class responseClazz, int startAddress, - Priority priority, ModbusElement... elements) { + public AbstractReadRegistersTask(String name, Class responseClazz, int startAddress, Priority priority, + ModbusElement... elements) { super(name, responseClazz, ModbusRegisterElement.class, startAddress, priority, elements); } @SuppressWarnings("unchecked") @Override - protected void handleResponse(ModbusRegisterElement element, int position, InputRegister[] response) + protected final void handleResponse(ModbusRegisterElement element, int position, Register[] response) throws OpenemsException { element.setInputValue(Arrays.copyOfRange(response, position, position + element.length)); } @Override - protected int calculateNextPosition(ModbusElement modbusElement, int position) { + protected final int calculateNextPosition(ModbusElement modbusElement, int position) { return position + modbusElement.length; } } \ No newline at end of file diff --git a/io.openems.edge.bridge.modbus/src/io/openems/edge/bridge/modbus/api/task/FC3ReadRegistersTask.java b/io.openems.edge.bridge.modbus/src/io/openems/edge/bridge/modbus/api/task/FC3ReadRegistersTask.java index 8b32d90e58f..d1d104a28fd 100644 --- a/io.openems.edge.bridge.modbus/src/io/openems/edge/bridge/modbus/api/task/FC3ReadRegistersTask.java +++ b/io.openems.edge.bridge.modbus/src/io/openems/edge/bridge/modbus/api/task/FC3ReadRegistersTask.java @@ -2,7 +2,7 @@ import com.ghgande.j2mod.modbus.msg.ReadMultipleRegistersRequest; import com.ghgande.j2mod.modbus.msg.ReadMultipleRegistersResponse; -import com.ghgande.j2mod.modbus.procimg.InputRegister; +import com.ghgande.j2mod.modbus.procimg.Register; import io.openems.common.exceptions.OpenemsException; import io.openems.edge.bridge.modbus.api.ModbusUtils; @@ -14,7 +14,7 @@ * (http://www.simplymodbus.ca/FC03.htm). */ public class FC3ReadRegistersTask - extends AbstractReadInputRegistersTask { + extends AbstractReadRegistersTask { public FC3ReadRegistersTask(int startAddress, Priority priority, ModbusElement... elements) { super("FC3ReadHoldingRegisters", ReadMultipleRegistersResponse.class, startAddress, priority, elements); @@ -26,7 +26,7 @@ protected ReadMultipleRegistersRequest createModbusRequest() { } @Override - protected InputRegister[] parseResponse(ReadMultipleRegistersResponse response) throws OpenemsException { + protected Register[] parseResponse(ReadMultipleRegistersResponse response) throws OpenemsException { return response.getRegisters(); } diff --git a/io.openems.edge.bridge.modbus/src/io/openems/edge/bridge/modbus/api/task/FC4ReadInputRegistersTask.java b/io.openems.edge.bridge.modbus/src/io/openems/edge/bridge/modbus/api/task/FC4ReadInputRegistersTask.java index 568971022b7..e05190fd759 100644 --- a/io.openems.edge.bridge.modbus/src/io/openems/edge/bridge/modbus/api/task/FC4ReadInputRegistersTask.java +++ b/io.openems.edge.bridge.modbus/src/io/openems/edge/bridge/modbus/api/task/FC4ReadInputRegistersTask.java @@ -1,8 +1,11 @@ package io.openems.edge.bridge.modbus.api.task; +import java.util.stream.Stream; + import com.ghgande.j2mod.modbus.msg.ReadInputRegistersRequest; import com.ghgande.j2mod.modbus.msg.ReadInputRegistersResponse; -import com.ghgande.j2mod.modbus.procimg.InputRegister; +import com.ghgande.j2mod.modbus.procimg.Register; +import com.ghgande.j2mod.modbus.procimg.SimpleRegister; import io.openems.common.exceptions.OpenemsException; import io.openems.edge.bridge.modbus.api.ModbusUtils; @@ -14,7 +17,7 @@ * (http://www.simplymodbus.ca/FC04.htm). */ public class FC4ReadInputRegistersTask - extends AbstractReadInputRegistersTask { + extends AbstractReadRegistersTask { public FC4ReadInputRegistersTask(int startAddress, Priority priority, ModbusElement... elements) { super("FC4ReadInputRegisters", ReadInputRegistersResponse.class, startAddress, priority, elements); @@ -26,8 +29,13 @@ protected ReadInputRegistersRequest createModbusRequest() { } @Override - protected InputRegister[] parseResponse(ReadInputRegistersResponse response) throws OpenemsException { - return response.getRegisters(); + protected Register[] parseResponse(ReadInputRegistersResponse response) throws OpenemsException { + return Stream.of(response.getRegisters()) // + .map(r -> { + var bs = r.toBytes(); + return new SimpleRegister(bs[0], bs[1]); + }) // + .toArray(Register[]::new); } @Override diff --git a/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/DummyModbusComponent.java b/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/DummyModbusComponent.java index 48120802422..f0b8ee7d3e6 100644 --- a/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/DummyModbusComponent.java +++ b/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/DummyModbusComponent.java @@ -4,8 +4,8 @@ import io.openems.common.exceptions.OpenemsException; import io.openems.common.utils.ConfigUtils; +import io.openems.edge.bridge.modbus.api.AbstractModbusBridge; import io.openems.edge.bridge.modbus.api.AbstractOpenemsModbusComponent; -import io.openems.edge.bridge.modbus.api.BridgeModbus; import io.openems.edge.bridge.modbus.api.ModbusComponent; import io.openems.edge.bridge.modbus.api.ModbusProtocol; import io.openems.edge.bridge.modbus.test.DummyModbusBridge; @@ -29,7 +29,7 @@ public DummyModbusComponent(String id, String bridgeId) throws OpenemsException this(id, new DummyModbusBridge(bridgeId), DEFAULT_UNIT_ID, new io.openems.edge.common.channel.ChannelId[0]); } - public DummyModbusComponent(String id, BridgeModbus bridge, int unitId, + public DummyModbusComponent(String id, AbstractModbusBridge bridge, int unitId, io.openems.edge.common.channel.ChannelId[] additionalChannelIds) throws OpenemsException { super(// OpenemsComponent.ChannelId.values(), // diff --git a/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/element/SignedDoublewordElementTest.java b/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/element/SignedDoublewordElementTest.java index eee2d847bb3..96272d628dd 100644 --- a/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/element/SignedDoublewordElementTest.java +++ b/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/element/SignedDoublewordElementTest.java @@ -37,7 +37,6 @@ public void testReadBigEndianLswMsw() throws OpenemsException { new SimpleRegister((byte) 0xAB, (byte) 0xCD), // new SimpleRegister((byte) 0x12, (byte) 0x34) // }); - System.out.println(sut.channel.getNextValue().get()); assertEquals(0x1234_ABCDL, sut.channel.getNextValue().get()); } diff --git a/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/element/StringWordElementTest.java b/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/element/StringWordElementTest.java index 80881998f4f..99d1d15c658 100644 --- a/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/element/StringWordElementTest.java +++ b/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/element/StringWordElementTest.java @@ -73,9 +73,6 @@ public void testWriteBigEndian() throws IllegalArgumentException, OpenemsNamedEx var sut = new ModbusTest.FC16WriteRegisters<>(new StringWordElement(0, 6), STRING); sut.channel.setNextWriteValueFromObject("OpenEMS"); var registers = sut.element.getNextWriteValueAndReset(); - for (var reg : registers) { - System.out.println(Integer.toHexString(reg.toBytes()[0]) + " " + Integer.toHexString(reg.toBytes()[1])); - } assertEquals(6, registers.length); assertArrayEquals(new byte[] { (byte) 0x4F, (byte) 0x70 }, registers[0].toBytes()); assertArrayEquals(new byte[] { (byte) 0x65, (byte) 0x6E }, registers[1].toBytes()); diff --git a/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/task/FC1ReadCoilsTaskTest.java b/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/task/FC1ReadCoilsTaskTest.java index 9bccb7feb3f..596edf36f38 100644 --- a/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/task/FC1ReadCoilsTaskTest.java +++ b/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/task/FC1ReadCoilsTaskTest.java @@ -1,6 +1,10 @@ package io.openems.edge.bridge.modbus.api.task; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +import java.util.concurrent.atomic.AtomicReference; +import java.util.stream.IntStream; import org.junit.Test; @@ -15,9 +19,14 @@ public class FC1ReadCoilsTaskTest { @Test - public void testToLogMessage() throws OpenemsException { + public void test() throws OpenemsException { var component = new DummyModbusComponent(); - var task = new FC1ReadCoilsTask(10, Priority.HIGH, new CoilElement(10), new CoilElement(11)); + var value = new AtomicReference(); + var element10 = new CoilElement(10); + element10.onUpdateCallback(v -> value.set(v)); + var element11 = new CoilElement(11); + element11.onUpdateCallback(v -> value.set(v)); + var task = new FC1ReadCoilsTask(10, Priority.HIGH, element10, element11); task.setParent(component); var request = task.createModbusRequest(); var response = (ReadCoilsResponse) request.getResponse(); @@ -26,5 +35,15 @@ public void testToLogMessage() throws OpenemsException { assertEquals("FC1ReadCoils [device0;unitid=1;priority=HIGH;ref=10/0xa;length=2;response=10]", task.toLogMessage(LogVerbosity.READS_AND_WRITES_VERBOSE, request, response)); + + var coils = response.getCoils(); + var values = IntStream.range(0, coils.size()) // + .mapToObj(i -> Boolean.valueOf(coils.getBit(i))) // + .toArray(Boolean[]::new); + assertNull(value.get()); + task.handleResponse(element10, 0, values); + assertEquals(Boolean.valueOf(true), value.get()); + task.handleResponse(element11, 1, values); + assertEquals(Boolean.valueOf(false), value.get()); } } diff --git a/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/task/FC2ReadInputsTaskTest.java b/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/task/FC2ReadInputsTaskTest.java index 10dd5f3585e..d5fce0d6b82 100644 --- a/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/task/FC2ReadInputsTaskTest.java +++ b/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/task/FC2ReadInputsTaskTest.java @@ -1,6 +1,10 @@ package io.openems.edge.bridge.modbus.api.task; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +import java.util.concurrent.atomic.AtomicReference; +import java.util.stream.IntStream; import org.junit.Test; @@ -15,9 +19,14 @@ public class FC2ReadInputsTaskTest { @Test - public void testToLogMessage() throws OpenemsException { + public void test() throws OpenemsException { var component = new DummyModbusComponent(); - var task = new FC2ReadInputsTask(10, Priority.HIGH, new CoilElement(10), new CoilElement(11)); + var value = new AtomicReference(); + var element10 = new CoilElement(10); + element10.onUpdateCallback(v -> value.set(v)); + var element11 = new CoilElement(11); + element11.onUpdateCallback(v -> value.set(v)); + var task = new FC2ReadInputsTask(10, Priority.HIGH, element10, element11); task.setParent(component); var request = task.createModbusRequest(); var response = (ReadInputDiscretesResponse) request.getResponse(); @@ -26,5 +35,15 @@ public void testToLogMessage() throws OpenemsException { assertEquals("FC2ReadCoils [device0;unitid=1;priority=HIGH;ref=10/0xa;length=2;response=10]", task.toLogMessage(LogVerbosity.READS_AND_WRITES_VERBOSE, request, response)); + + var discretes = response.getDiscretes(); + var values = IntStream.range(0, discretes.size()) // + .mapToObj(i -> Boolean.valueOf(discretes.getBit(i))) // + .toArray(Boolean[]::new); + assertNull(value.get()); + task.handleResponse(element10, 0, values); + assertEquals(Boolean.valueOf(true), value.get()); + task.handleResponse(element11, 1, values); + assertEquals(Boolean.valueOf(false), value.get()); } } diff --git a/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/task/FC3ReadRegistersTaskTest.java b/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/task/FC3ReadRegistersTaskTest.java index 061a2590d59..0edc51a082a 100644 --- a/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/task/FC3ReadRegistersTaskTest.java +++ b/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/task/FC3ReadRegistersTaskTest.java @@ -1,6 +1,9 @@ package io.openems.edge.bridge.modbus.api.task; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +import java.util.concurrent.atomic.AtomicReference; import org.junit.Test; @@ -17,15 +20,31 @@ public class FC3ReadRegistersTaskTest { @Test - public void testToLogMessage() throws OpenemsException { + public void test() throws OpenemsException { var component = new DummyModbusComponent(); - var task = new FC3ReadRegistersTask(20, Priority.LOW, new UnsignedDoublewordElement(20)); + var value = new AtomicReference(); + var element20 = new UnsignedDoublewordElement(20); + element20.onUpdateCallback(v -> value.set(v)); + var element22 = new UnsignedDoublewordElement(22); + element22.onUpdateCallback(v -> value.set(v)); + + var task = new FC3ReadRegistersTask(20, Priority.LOW, element20, element22); task.setParent(component); var request = task.createModbusRequest(); var response = (ReadMultipleRegistersResponse) request.getResponse(); - response.setRegisters(new Register[] { new SimpleRegister(100), new SimpleRegister(200) }); + response.setRegisters(new Register[] { // + new SimpleRegister(100), new SimpleRegister(200), // + new SimpleRegister(300), new SimpleRegister(400) }); - assertEquals("FC3ReadHoldingRegisters [device0;unitid=1;priority=LOW;ref=20/0x14;length=2;response=0064 00c8]", + assertEquals( + "FC3ReadHoldingRegisters [device0;unitid=1;priority=LOW;ref=20/0x14;length=4;response=0064 00c8 012c 0190]", task.toLogMessage(LogVerbosity.READS_AND_WRITES_VERBOSE, request, response)); + + var registers = task.parseResponse(response); + assertNull(value.get()); + task.handleResponse(element20, 0, registers); + assertEquals(Long.valueOf(6553800), value.get()); + task.handleResponse(element22, 2, registers); + assertEquals(Long.valueOf(19661200), value.get()); } } diff --git a/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/task/FC4ReadInputRegistersTaskTest.java b/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/task/FC4ReadInputRegistersTaskTest.java index 1bc27a12d6e..04a63b525ce 100644 --- a/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/task/FC4ReadInputRegistersTaskTest.java +++ b/io.openems.edge.bridge.modbus/test/io/openems/edge/bridge/modbus/api/task/FC4ReadInputRegistersTaskTest.java @@ -1,11 +1,15 @@ package io.openems.edge.bridge.modbus.api.task; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +import java.util.concurrent.atomic.AtomicReference; import org.junit.Test; +import com.ghgande.j2mod.modbus.procimg.InputRegister; import com.ghgande.j2mod.modbus.procimg.Register; -import com.ghgande.j2mod.modbus.procimg.SimpleRegister; +import com.ghgande.j2mod.modbus.procimg.SimpleInputRegister; import io.openems.common.exceptions.OpenemsException; import io.openems.edge.bridge.modbus.DummyModbusComponent; @@ -15,16 +19,65 @@ public class FC4ReadInputRegistersTaskTest { + /** + * Custom implementation that only implements {@link InputRegister} and not + * {@link Register}. This is the difference to {@link SimpleInputRegister}. + */ + public static class MyInputRegister implements InputRegister { + + private final SimpleInputRegister delegate; + + public MyInputRegister(int value) { + this.delegate = new SimpleInputRegister(value); + } + + @Override + public int getValue() { + return this.delegate.getValue(); + } + + @Override + public int toUnsignedShort() { + return this.delegate.toUnsignedShort(); + } + + @Override + public short toShort() { + return this.delegate.toShort(); + } + + @Override + public byte[] toBytes() { + return this.delegate.toBytes(); + } + + } + @Test - public void testToLogMessage() throws OpenemsException { + public void test() throws OpenemsException { var component = new DummyModbusComponent(); - var task = new FC4ReadInputRegistersTask(20, Priority.LOW, new UnsignedDoublewordElement(20)); + var value = new AtomicReference(); + var element20 = new UnsignedDoublewordElement(20); + element20.onUpdateCallback(v -> value.set(v)); + var element22 = new UnsignedDoublewordElement(22); + element22.onUpdateCallback(v -> value.set(v)); + var task = new FC4ReadInputRegistersTask(20, Priority.LOW, element20, element22); task.setParent(component); var request = task.createModbusRequest(); var response = request.getResponse(); - response.setRegisters(new Register[] { new SimpleRegister(987), new SimpleRegister(654) }); + response.setRegisters(new InputRegister[] { // + new MyInputRegister(987), new MyInputRegister(654), // + new MyInputRegister(321), new MyInputRegister(0) }); - assertEquals("FC4ReadInputRegisters [device0;unitid=1;priority=LOW;ref=20/0x14;length=2;response=03db 028e]", + assertEquals( + "FC4ReadInputRegisters [device0;unitid=1;priority=LOW;ref=20/0x14;length=4;response=03db 028e 0141 0000]", task.toLogMessage(LogVerbosity.READS_AND_WRITES_VERBOSE, request, response)); + + var registers = task.parseResponse(response); + assertNull(value.get()); + task.handleResponse(element20, 0, registers); + assertEquals(Long.valueOf(64684686), value.get()); + task.handleResponse(element22, 2, registers); + assertEquals(Long.valueOf(21037056), value.get()); } }