From 66225e849ce6fb77310e016c7c03247e6f1c37cd Mon Sep 17 00:00:00 2001 From: Nagy Attila Gabor Date: Tue, 30 Jun 2020 23:22:51 +0200 Subject: [PATCH] [modbus] Fixed caching in ModbusPollerThingHandler Signed-off-by: Nagy Attila Gabor --- .../handler/ModbusPollerThingHandler.java | 64 +++++++++++++++---- .../modbus/tests/ModbusDataHandlerTest.java | 4 +- 2 files changed, 55 insertions(+), 13 deletions(-) diff --git a/bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/handler/ModbusPollerThingHandler.java b/bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/handler/ModbusPollerThingHandler.java index 38b3493ec9980..dff050d7feacf 100644 --- a/bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/handler/ModbusPollerThingHandler.java +++ b/bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/handler/ModbusPollerThingHandler.java @@ -66,16 +66,15 @@ public class ModbusPollerThingHandler extends BaseBridgeHandler { private class ReadCallbackDelegator implements ModbusReadCallback, ModbusFailureCallback { - private volatile @Nullable AtomicStampedValue lastResult; + private volatile @Nullable AtomicStampedValue lastResult; - @Override - public synchronized void handle(AsyncModbusReadResult result) { + public synchronized void handleResult(PollResult result) { // Ignore all incoming data and errors if configuration is not correct if (hasConfigurationError() || disposed) { return; } if (config.getCacheMillis() >= 0) { - AtomicStampedValue localLastResult = this.lastResult; + AtomicStampedValue localLastResult = this.lastResult; if (localLastResult == null) { this.lastResult = new AtomicStampedValue<>(System.currentTimeMillis(), result); } else { @@ -84,16 +83,25 @@ public synchronized void handle(AsyncModbusReadResult result) { } } logger.debug("Thing {} received response {}", thing.getUID(), result); - childCallbacks.forEach(handler -> handler.onReadResult(result)); - resetCommunicationError(); + notifyChildren(result); + if (result.failure != null) { + Exception error = result.failure.getCause(); + assert error != null; + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, + String.format("Error with read: %s: %s", error.getClass().getName(), error.getMessage())); + } else { + resetCommunicationError(); + } + } + + @Override + public synchronized void handle(AsyncModbusReadResult result) { + handleResult(new PollResult(result)); } @Override public synchronized void handle(AsyncModbusFailure failure) { - Exception error = failure.getCause(); - assert error != null; - updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, - String.format("Error with read: %s: %s", error.getClass().getName(), error.getMessage())); + handleResult(new PollResult(failure)); } private void resetCommunicationError() { @@ -115,11 +123,25 @@ public boolean updateChildrenWithOldData(long oldestStamp) { return Optional.ofNullable(this.lastResult).map(result -> result.copyIfStampAfter(oldestStamp)) .map(result -> { logger.debug("Thing {} reusing cached data: {}", thing.getUID(), result.getValue()); - childCallbacks.forEach(handler -> handler.onReadResult(result.getValue())); + notifyChildren(result.getValue()); return true; }).orElse(false); } + private void notifyChildren(PollResult pollResult) { + @Nullable + AsyncModbusReadResult result = pollResult.result; + @Nullable + AsyncModbusFailure failure = pollResult.failure; + childCallbacks.forEach(handler -> { + if (result != null) { + handler.onReadResult(result); + } else if (failure != null) { + handler.handleReadError(failure); + } + }); + } + /** * Rest data caches */ @@ -155,6 +177,26 @@ public ModbusPollerReadRequest(ModbusPollerConfiguration config, } } + /** + * Immutable data object to cache the results of a poll request + */ + private class PollResult { + @Nullable + public final AsyncModbusReadResult result; + @Nullable + public final AsyncModbusFailure failure; + + PollResult(AsyncModbusReadResult result) { + this.result = result; + this.failure = null; + } + + PollResult(AsyncModbusFailure failure) { + this.result = null; + this.failure = failure; + } + } + private final Logger logger = LoggerFactory.getLogger(ModbusPollerThingHandler.class); @NonNullByDefault({}) diff --git a/itests/org.openhab.binding.modbus.tests/src/main/java/org/openhab/binding/modbus/tests/ModbusDataHandlerTest.java b/itests/org.openhab.binding.modbus.tests/src/main/java/org/openhab/binding/modbus/tests/ModbusDataHandlerTest.java index 8ba667f5f2004..7b3e4cab07f85 100644 --- a/itests/org.openhab.binding.modbus.tests/src/main/java/org/openhab/binding/modbus/tests/ModbusDataHandlerTest.java +++ b/itests/org.openhab.binding.modbus.tests/src/main/java/org/openhab/binding/modbus/tests/ModbusDataHandlerTest.java @@ -14,8 +14,8 @@ import static org.hamcrest.CoreMatchers.*; import static org.junit.Assert.*; +import static org.mockito.ArgumentMatchers.*; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.notNull; import static org.mockito.Mockito.*; import static org.openhab.binding.modbus.internal.ModbusBindingConstantsInternal.*; @@ -778,7 +778,7 @@ public void testRefreshOnData() throws InterruptedException { builder -> builder.withConfiguration(dataConfig), bundleContext); assertThat(dataHandler.getThing().getStatus(), is(equalTo(ThingStatus.ONLINE))); - verify(comms, never()).submitOneTimePoll(request, notNull(), notNull()); + verify(comms, never()).submitOneTimePoll(eq(request), notNull(), notNull()); // Reset initial REFRESH commands to data thing channels from the Core reset(poller.getHandler()); dataHandler.handleCommand(Mockito.mock(ChannelUID.class), RefreshType.REFRESH);