From dd0968b7b1841a204a54a515a82f40ad49eb4571 Mon Sep 17 00:00:00 2001 From: Catalin Sanda Date: Sat, 26 Aug 2023 20:41:54 +0300 Subject: [PATCH] Fix for #28 - Implement poor man's state machine for logging and disable connection related error reporting when the logger (or inverter) didn't respond properly after 3 tries. --- .../internal/SolarmanLoggerHandler.java | 31 +++++++++++++---- .../modbus/SolarmanLoggerConnection.java | 23 ++++++++----- .../internal/modbus/SolarmanV5Protocol.java | 33 +++++++++++-------- .../solarman/internal/state/LoggerState.java | 30 +++++++++++++++++ .../updater/SolarmanChannelUpdater.java | 29 ++++++++-------- .../modbus/SolarmanV5ProtocolTest.java | 17 +++++----- 6 files changed, 110 insertions(+), 53 deletions(-) create mode 100644 src/main/java/org/openhab/binding/solarman/internal/state/LoggerState.java diff --git a/src/main/java/org/openhab/binding/solarman/internal/SolarmanLoggerHandler.java b/src/main/java/org/openhab/binding/solarman/internal/SolarmanLoggerHandler.java index 7f32dfb..134caf8 100644 --- a/src/main/java/org/openhab/binding/solarman/internal/SolarmanLoggerHandler.java +++ b/src/main/java/org/openhab/binding/solarman/internal/SolarmanLoggerHandler.java @@ -32,6 +32,7 @@ import org.openhab.binding.solarman.internal.defmodel.Validation; import org.openhab.binding.solarman.internal.modbus.SolarmanLoggerConnector; import org.openhab.binding.solarman.internal.modbus.SolarmanV5Protocol; +import org.openhab.binding.solarman.internal.state.LoggerState; import org.openhab.binding.solarman.internal.updater.SolarmanChannelUpdater; import org.openhab.core.thing.*; import org.openhab.core.thing.binding.BaseThingHandler; @@ -54,6 +55,7 @@ public class SolarmanLoggerHandler extends BaseThingHandler { private final DefinitionParser definitionParser; private final SolarmanChannelManager solarmanChannelManager; + private final LoggerState loggerState; @Nullable private volatile ScheduledFuture scheduledFuture; @@ -61,6 +63,7 @@ public SolarmanLoggerHandler(Thing thing) { super(thing); this.definitionParser = new DefinitionParser(); this.solarmanChannelManager = new SolarmanChannelManager(); + this.loggerState = new LoggerState(); } @Override @@ -113,16 +116,30 @@ public void initialize() { ); SolarmanChannelUpdater solarmanChannelUpdater = new SolarmanChannelUpdater( - this::updateStatus, this::updateState ); - scheduledFuture = scheduler.scheduleAtFixedRate( - () -> solarmanChannelUpdater.fetchDataFromLogger( - mergedRequests, - solarmanLoggerConnector, - solarmanV5Protocol, - paramToChannelMapping), + scheduledFuture = scheduler.scheduleAtFixedRate(() -> { + boolean fetchSuccessful = solarmanChannelUpdater.fetchDataFromLogger( + mergedRequests, + solarmanLoggerConnector, + solarmanV5Protocol, + paramToChannelMapping, + loggerState); + + if (fetchSuccessful) { + updateStatus(ThingStatus.ONLINE); + loggerState.setOnline(); + } else { + updateStatus(ThingStatus.OFFLINE); + loggerState.setPossiblyOffline(); + } + + if (loggerState.isJustBecameOffline()) { + logger.info("Assuming logger is OFFLINE after {} failed requests. Disabling connection error logging until it becomes available again", + LoggerState.NO_FAILED_REQUESTS); + } + }, 0, config.refreshInterval, TimeUnit.SECONDS ); } diff --git a/src/main/java/org/openhab/binding/solarman/internal/modbus/SolarmanLoggerConnection.java b/src/main/java/org/openhab/binding/solarman/internal/modbus/SolarmanLoggerConnection.java index a8253e3..9876b9b 100644 --- a/src/main/java/org/openhab/binding/solarman/internal/modbus/SolarmanLoggerConnection.java +++ b/src/main/java/org/openhab/binding/solarman/internal/modbus/SolarmanLoggerConnection.java @@ -19,11 +19,12 @@ public SolarmanLoggerConnection(String hostName, int port) { sockaddr = new InetSocketAddress(hostName, port); } - public byte[] sendRequest(byte[] reqFrame) { + public byte[] sendRequest(byte[] reqFrame, Boolean allowLogging) { // Will not be used by multiple threads, so not bothering making it thread safe for now if (socket == null) { - if ((socket = connectSocket()) == null) { - LOGGER.info("Error creating socket"); + if ((socket = connectSocket(allowLogging)) == null) { + if (allowLogging) + LOGGER.info("Error creating socket"); return new byte[0]; } } @@ -32,7 +33,8 @@ public byte[] sendRequest(byte[] reqFrame) { LOGGER.debug("Request frame: " + bytesToHex(reqFrame)); socket.getOutputStream().write(reqFrame); } catch (IOException e) { - LOGGER.info("Unable to send frame to logger", e); + if (allowLogging) + LOGGER.info("Unable to send frame to logger", e); return new byte[0]; } @@ -45,7 +47,8 @@ public byte[] sendRequest(byte[] reqFrame) { try { int bytesRead = socket.getInputStream().read(buffer); if (bytesRead < 0) { - LOGGER.info("No data received"); + if (allowLogging) + LOGGER.info("No data received"); } else { byte[] data = Arrays.copyOfRange(buffer, 0, bytesRead); if (LOGGER.isDebugEnabled()) @@ -54,11 +57,12 @@ public byte[] sendRequest(byte[] reqFrame) { } } catch (SocketTimeoutException e) { LOGGER.debug("Connection timeout", e); - if (attempts == 0) { + if (attempts == 0 && allowLogging) { LOGGER.info("Too many connection timeouts"); } } catch (IOException e) { - LOGGER.info("Connection error", e); + if (allowLogging) + LOGGER.info("Connection error", e); } } @@ -70,7 +74,7 @@ private static String bytesToHex(byte[] bytes) { .collect(Collectors.joining()); } - private Socket connectSocket() { + private Socket connectSocket(Boolean allowLogging) { try { Socket clientSocket = new Socket(); @@ -79,7 +83,8 @@ private Socket connectSocket() { return clientSocket; } catch (IOException e) { - LOGGER.error("Could not open socket on IP " + sockaddr.toString(), e); + if (allowLogging) + LOGGER.error("Could not open socket on IP " + sockaddr.toString(), e); return null; } } diff --git a/src/main/java/org/openhab/binding/solarman/internal/modbus/SolarmanV5Protocol.java b/src/main/java/org/openhab/binding/solarman/internal/modbus/SolarmanV5Protocol.java index a9248c0..f80bb95 100644 --- a/src/main/java/org/openhab/binding/solarman/internal/modbus/SolarmanV5Protocol.java +++ b/src/main/java/org/openhab/binding/solarman/internal/modbus/SolarmanV5Protocol.java @@ -23,12 +23,12 @@ public SolarmanV5Protocol(SolarmanLoggerConfiguration solarmanLoggerConfiguratio this.solarmanLoggerConfiguration = solarmanLoggerConfiguration; } - public Map readRegisters(SolarmanLoggerConnection solarmanLoggerConnection, byte mbFunctionCode, int firstReg, int lastReg) { + public Map readRegisters(SolarmanLoggerConnection solarmanLoggerConnection, byte mbFunctionCode, int firstReg, int lastReg, Boolean allowLogging) { byte[] solarmanV5Frame = buildSolarmanV5Frame(mbFunctionCode, firstReg, lastReg); - byte[] respFrame = solarmanLoggerConnection.sendRequest(solarmanV5Frame); + byte[] respFrame = solarmanLoggerConnection.sendRequest(solarmanV5Frame, allowLogging); if (respFrame.length > 0) { - byte[] modbusRespFrame = extractModbusResponseFrame(respFrame, solarmanV5Frame); - return parseModbusReadHoldingRegistersResponse(modbusRespFrame, firstReg, lastReg); + byte[] modbusRespFrame = extractModbusResponseFrame(respFrame, solarmanV5Frame, allowLogging); + return parseModbusReadHoldingRegistersResponse(modbusRespFrame, firstReg, lastReg, allowLogging); } else { return Collections.emptyMap(); } @@ -153,12 +153,13 @@ protected byte[] buildModbusReadHoldingRegistersRequestFrame(byte slaveId, byte return ByteBuffer.allocate(req.length + crc.length).put(req).put(crc).array(); } - protected Map parseModbusReadHoldingRegistersResponse(byte[] frame, int firstReg, int lastReg) { + protected Map parseModbusReadHoldingRegistersResponse(byte[] frame, int firstReg, int lastReg, Boolean allowLogging) { int regCount = lastReg - firstReg + 1; Map registers = new HashMap<>(); int expectedFrameDataLen = 2 + 1 + regCount * 2; if (frame == null || frame.length < expectedFrameDataLen + 2) { - LOGGER.error("Modbus frame is too short or empty"); + if (allowLogging) + LOGGER.error("Modbus frame is too short or empty"); return registers; } @@ -167,7 +168,8 @@ protected Map parseModbusReadHoldingRegistersResponse(byte[] fr int expectedCrc = CRC16Modbus.calculate(Arrays.copyOfRange(frame, 0, expectedFrameDataLen)); if (actualCrc != expectedCrc) { - LOGGER.error(String.format("Modbus frame crc is not valid. Expected %04x, got %04x", expectedCrc, actualCrc)); + if (allowLogging) + LOGGER.error(String.format("Modbus frame crc is not valid. Expected %04x, got %04x", expectedCrc, actualCrc)); return registers; } @@ -181,21 +183,26 @@ protected Map parseModbusReadHoldingRegistersResponse(byte[] fr return registers; } - protected byte[] extractModbusResponseFrame(byte[] responseFrame, byte[] requestFrame) { - if (responseFrame == null) { - LOGGER.error("No response frame"); + protected byte[] extractModbusResponseFrame(byte[] responseFrame, byte[] requestFrame, Boolean allowLogging) { + if (responseFrame == null || responseFrame.length == 0) { + if (allowLogging) + LOGGER.error("No response frame"); return null; } else if (responseFrame.length == 29) { + parseResponseErrorCode(responseFrame, requestFrame); return null; } else if (responseFrame.length < (29 + 4)) { - LOGGER.error("Response frame is too short"); + if (allowLogging) + LOGGER.error("Response frame is too short"); return null; } else if (responseFrame[0] != (byte) 0xA5) { - LOGGER.error("Response frame has invalid starting byte"); + if (allowLogging) + LOGGER.error("Response frame has invalid starting byte"); return null; } else if (responseFrame[responseFrame.length - 1] != (byte) 0x15) { - LOGGER.error("Response frame has invalid ending byte"); + if (allowLogging) + LOGGER.error("Response frame has invalid ending byte"); return null; } diff --git a/src/main/java/org/openhab/binding/solarman/internal/state/LoggerState.java b/src/main/java/org/openhab/binding/solarman/internal/state/LoggerState.java new file mode 100644 index 0000000..42edcca --- /dev/null +++ b/src/main/java/org/openhab/binding/solarman/internal/state/LoggerState.java @@ -0,0 +1,30 @@ +package org.openhab.binding.solarman.internal.state; + +public class LoggerState { + public final static Integer NO_FAILED_REQUESTS = 3; + private State state = State.ONLINE; // Let's assume we're online initially + private int offlineTryCount = 0; + + public void setOnline() { + state = State.ONLINE; + offlineTryCount = 0; + } + + public void setPossiblyOffline() { + state = ++offlineTryCount < NO_FAILED_REQUESTS ? State.LIMBO : State.OFFLINE; + } + + public boolean isOffline() { + return state == State.OFFLINE; + } + + public boolean isJustBecameOffline() { + return state == State.OFFLINE && offlineTryCount == NO_FAILED_REQUESTS; + } + + public enum State { + ONLINE, + LIMBO, + OFFLINE, + } +} diff --git a/src/main/java/org/openhab/binding/solarman/internal/updater/SolarmanChannelUpdater.java b/src/main/java/org/openhab/binding/solarman/internal/updater/SolarmanChannelUpdater.java index 5fa4cf2..f295181 100644 --- a/src/main/java/org/openhab/binding/solarman/internal/updater/SolarmanChannelUpdater.java +++ b/src/main/java/org/openhab/binding/solarman/internal/updater/SolarmanChannelUpdater.java @@ -8,6 +8,7 @@ import org.openhab.binding.solarman.internal.modbus.SolarmanLoggerConnection; import org.openhab.binding.solarman.internal.modbus.SolarmanLoggerConnector; import org.openhab.binding.solarman.internal.modbus.SolarmanV5Protocol; +import org.openhab.binding.solarman.internal.state.LoggerState; import org.openhab.binding.solarman.internal.typeprovider.ChannelUtils; import org.openhab.binding.solarman.internal.util.StreamUtils; import org.openhab.core.library.types.DateTimeType; @@ -15,7 +16,6 @@ import org.openhab.core.library.types.QuantityType; import org.openhab.core.library.types.StringType; import org.openhab.core.thing.ChannelUID; -import org.openhab.core.thing.ThingStatus; import org.openhab.core.types.State; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -41,18 +41,17 @@ public class SolarmanChannelUpdater { private final Logger LOGGER = LoggerFactory.getLogger(SolarmanChannelUpdater.class); - private final StatusUpdater statusUpdater; private final StateUpdater stateUpdater; - public SolarmanChannelUpdater(StatusUpdater statusUpdater, StateUpdater stateUpdater) { - this.statusUpdater = statusUpdater; + public SolarmanChannelUpdater(StateUpdater stateUpdater) { this.stateUpdater = stateUpdater; } - public void fetchDataFromLogger(List requests, - SolarmanLoggerConnector solarmanLoggerConnector, - SolarmanV5Protocol solarmanV5Protocol, - Map paramToChannelMapping) { + public boolean fetchDataFromLogger(List requests, + SolarmanLoggerConnector solarmanLoggerConnector, + SolarmanV5Protocol solarmanV5Protocol, + Map paramToChannelMapping, + LoggerState loggerState) { try (SolarmanLoggerConnection solarmanLoggerConnection = solarmanLoggerConnector.createConnection()) { LOGGER.debug("Fetching data from logger"); @@ -61,15 +60,18 @@ public void fetchDataFromLogger(List requests, .map(request -> solarmanV5Protocol.readRegisters(solarmanLoggerConnection, (byte) request.getMbFunctioncode().intValue(), request.getStart(), - request.getEnd()) + request.getEnd(), + !loggerState.isOffline()) ) .reduce(new HashMap<>(), this::mergeMaps); - updateChannelsForReadRegisters(paramToChannelMapping, readRegistersMap); + if (!readRegistersMap.isEmpty()) + updateChannelsForReadRegisters(paramToChannelMapping, readRegistersMap); - statusUpdater.updateStatus(readRegistersMap.isEmpty() ? ThingStatus.OFFLINE : ThingStatus.ONLINE); + return !readRegistersMap.isEmpty(); } catch (Exception e) { LOGGER.error("Error invoking handler", e); + return false; } } @@ -219,11 +221,6 @@ private enum ValueType { UNSIGNED, SIGNED } - @FunctionalInterface - public interface StatusUpdater { - void updateStatus(ThingStatus thingStatus); - } - @FunctionalInterface public interface StateUpdater { void updateState(ChannelUID channelUID, State state); diff --git a/src/test/java/org/openhab/binding/solarman/internal/modbus/SolarmanV5ProtocolTest.java b/src/test/java/org/openhab/binding/solarman/internal/modbus/SolarmanV5ProtocolTest.java index 4c9a3c4..b4d2179 100644 --- a/src/test/java/org/openhab/binding/solarman/internal/modbus/SolarmanV5ProtocolTest.java +++ b/src/test/java/org/openhab/binding/solarman/internal/modbus/SolarmanV5ProtocolTest.java @@ -1,6 +1,7 @@ package org.openhab.binding.solarman.internal.modbus; import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.any; import static org.mockito.Mockito.when; @@ -47,11 +48,11 @@ void testbuildSolarmanV5Frame() { @Test void testReadRegister0x01() { // given - when(solarmanLoggerConnection.sendRequest(any())).thenReturn( + when(solarmanLoggerConnection.sendRequest(any(), eq(true))).thenReturn( hexStringToByteArray("a5000000000000000000000000000000000000000000000000010301000ac84300000015")); // when - Map regValues = solarmanV5Protocol.readRegisters(solarmanLoggerConnection, (byte) 0x03, 1, 1); + Map regValues = solarmanV5Protocol.readRegisters(solarmanLoggerConnection, (byte) 0x03, 1, 1, true); // then assertEquals(1, regValues.size()); @@ -62,11 +63,11 @@ void testReadRegister0x01() { @Test void testReadRegisters0x02_0x03() { // given - when(solarmanLoggerConnection.sendRequest(any())).thenReturn( + when(solarmanLoggerConnection.sendRequest(any(), eq(true))).thenReturn( hexStringToByteArray("a5000000000000000000000000000000000000000000000000010302000a000b13f600000015")); // when - Map regValues = solarmanV5Protocol.readRegisters(solarmanLoggerConnection, (byte) 0x03, 2, 3); + Map regValues = solarmanV5Protocol.readRegisters(solarmanLoggerConnection, (byte) 0x03, 2, 3, true); // then assertEquals(2, regValues.size()); @@ -79,11 +80,11 @@ void testReadRegisters0x02_0x03() { @Test void testReadRegisterSUN_10K_SG04LP3_EU_part1() { // given - when(solarmanLoggerConnection.sendRequest(any())).thenReturn(hexStringToByteArray( + when(solarmanLoggerConnection.sendRequest(any(), eq(true))).thenReturn(hexStringToByteArray( "a53b0010150007482ee38d020121d0060091010000403e486301032800ffffff160a12162420ffffffffffffffffffffffffffffffffffff0001ffff0001ffff000003e81fa45115")); // when - Map regValues = solarmanV5Protocol.readRegisters(solarmanLoggerConnection, (byte) 0x03, 0x3c, 0x4f); + Map regValues = solarmanV5Protocol.readRegisters(solarmanLoggerConnection, (byte) 0x03, 0x3c, 0x4f, true); // then assertEquals(20, regValues.size()); @@ -96,11 +97,11 @@ void testReadRegisterSUN_10K_SG04LP3_EU_part1() { @Test void testReadRegisterSUN_10K_SG04LP3_EU_part2() { // given - when(solarmanLoggerConnection.sendRequest(any())).thenReturn(hexStringToByteArray( + when(solarmanLoggerConnection.sendRequest(any(), eq(true))).thenReturn(hexStringToByteArray( "a5330010150008482ee38d020122d0060091010000403e486301032000010000ffffffffffff0001ffffffffffffffffffff0000ffff0011ffffffff3a005715")); // when - Map regValues = solarmanV5Protocol.readRegisters(solarmanLoggerConnection, (byte) 0x03, 0x50, 0x5f); + Map regValues = solarmanV5Protocol.readRegisters(solarmanLoggerConnection, (byte) 0x03, 0x50, 0x5f, true); // then assertEquals(16, regValues.size());