From e3b1d04f5cd50f31bf1476ed750e12b3a2ba8aeb Mon Sep 17 00:00:00 2001 From: Hilbrand Bouwkamp Date: Sun, 25 Oct 2020 16:21:37 +0100 Subject: [PATCH] [snmp] Set thing only online on valid response (#8759) * [snmp] Set thing only online on valid response Otherwise it will toggles between online and offline when the call always times-out, which can happen when the device is unreachable (or a wrong ip address configured). Signed-off-by: Hilbrand Bouwkamp --- .../snmp/internal/SnmpTargetHandler.java | 5 ++++- .../AbstractSnmpTargetHandlerTest.java | 17 ++++++++++++++--- .../snmp/internal/SnmpTargetHandlerTest.java | 4 ++++ .../snmp/internal/StringChannelTest.java | 2 ++ .../snmp/internal/SwitchChannelTest.java | 18 +++++++++++++----- 5 files changed, 37 insertions(+), 9 deletions(-) diff --git a/bundles/org.openhab.binding.snmp/src/main/java/org/openhab/binding/snmp/internal/SnmpTargetHandler.java b/bundles/org.openhab.binding.snmp/src/main/java/org/openhab/binding/snmp/internal/SnmpTargetHandler.java index 4d37c589e3e8f..1e35357fff0a8 100644 --- a/bundles/org.openhab.binding.snmp/src/main/java/org/openhab/binding/snmp/internal/SnmpTargetHandler.java +++ b/bundles/org.openhab.binding.snmp/src/main/java/org/openhab/binding/snmp/internal/SnmpTargetHandler.java @@ -40,6 +40,7 @@ import org.openhab.core.thing.ThingStatus; import org.openhab.core.thing.ThingStatusDetail; import org.openhab.core.thing.binding.BaseThingHandler; +import org.openhab.core.thing.util.ThingHandlerHelper; import org.openhab.core.types.Command; import org.openhab.core.types.RefreshType; import org.openhab.core.types.State; @@ -200,6 +201,9 @@ public void onResponse(@Nullable ResponseEvent event) { return; } timeoutCounter = 0; + if (ThingHandlerHelper.isHandlerInitialized(this)) { + updateStatus(ThingStatus.ONLINE); + } logger.trace("{} received {}", thing.getUID(), response); response.getVariableBindings().forEach(variable -> { @@ -423,7 +427,6 @@ private boolean renewTargetAddress() { try { target.setAddress(new UdpAddress(InetAddress.getByName(config.hostname), config.port)); targetAddressString = ((UdpAddress) target.getAddress()).getInetAddress().getHostAddress(); - updateStatus(ThingStatus.ONLINE, ThingStatusDetail.NONE); return true; } catch (UnknownHostException e) { target.setAddress(null); diff --git a/bundles/org.openhab.binding.snmp/src/test/java/org/openhab/binding/snmp/internal/AbstractSnmpTargetHandlerTest.java b/bundles/org.openhab.binding.snmp/src/test/java/org/openhab/binding/snmp/internal/AbstractSnmpTargetHandlerTest.java index 883dbc069606b..01f059515aeff 100644 --- a/bundles/org.openhab.binding.snmp/src/test/java/org/openhab/binding/snmp/internal/AbstractSnmpTargetHandlerTest.java +++ b/bundles/org.openhab.binding.snmp/src/test/java/org/openhab/binding/snmp/internal/AbstractSnmpTargetHandlerTest.java @@ -23,6 +23,7 @@ import java.util.Map; import java.util.Vector; +import org.junit.jupiter.api.AfterEach; import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.MockitoAnnotations; @@ -64,6 +65,12 @@ public abstract class AbstractSnmpTargetHandlerTest extends JavaTest { protected Thing thing; protected SnmpTargetHandler thingHandler; + private AutoCloseable mocks; + + @AfterEach + public void after() throws Exception { + mocks.close(); + } protected VariableBinding handleCommandSwitchChannel(SnmpDatatype datatype, Command command, String onValue, String offValue, boolean refresh) throws IOException { @@ -134,7 +141,7 @@ protected State onResponseSwitchChannel(SnmpChannelMode channelMode, SnmpDatatyp protected void refresh(SnmpChannelMode channelMode, boolean refresh) throws IOException { setup(SnmpBindingConstants.CHANNEL_TYPE_UID_STRING, channelMode); - waitForAssert(() -> assertEquals(ThingStatus.ONLINE, thingHandler.getThing().getStatusInfo().getStatus())); + verifyStatus(ThingStatus.UNKNOWN); verify(snmpService).addCommandResponder(any()); if (refresh) { @@ -165,7 +172,7 @@ protected void setup(ChannelTypeUID channelTypeUID, SnmpChannelMode channelMode, String onValue, String offValue, String exceptionValue) { Map channelConfig = new HashMap<>(); Map thingConfig = new HashMap<>(); - MockitoAnnotations.initMocks(this); + mocks = MockitoAnnotations.openMocks(this); thingConfig.put("hostname", "localhost"); @@ -206,6 +213,10 @@ protected void setup(ChannelTypeUID channelTypeUID, SnmpChannelMode channelMode, thingHandler.initialize(); - waitForAssert(() -> assertEquals(ThingStatus.ONLINE, thingHandler.getThing().getStatusInfo().getStatus())); + verifyStatus(ThingStatus.UNKNOWN); + } + + protected void verifyStatus(ThingStatus status) { + waitForAssert(() -> assertEquals(status, thingHandler.getThing().getStatusInfo().getStatus())); } } diff --git a/bundles/org.openhab.binding.snmp/src/test/java/org/openhab/binding/snmp/internal/SnmpTargetHandlerTest.java b/bundles/org.openhab.binding.snmp/src/test/java/org/openhab/binding/snmp/internal/SnmpTargetHandlerTest.java index 16b82435edee4..adfe632a1a378 100644 --- a/bundles/org.openhab.binding.snmp/src/test/java/org/openhab/binding/snmp/internal/SnmpTargetHandlerTest.java +++ b/bundles/org.openhab.binding.snmp/src/test/java/org/openhab/binding/snmp/internal/SnmpTargetHandlerTest.java @@ -23,6 +23,7 @@ import org.openhab.core.library.types.DecimalType; import org.openhab.core.library.types.OnOffType; import org.openhab.core.library.types.StringType; +import org.openhab.core.thing.ThingStatus; import org.snmp4j.PDU; import org.snmp4j.Snmp; import org.snmp4j.event.ResponseEvent; @@ -62,6 +63,7 @@ public void testChannelsProperlyUpdate() throws IOException { new OctetString("on"), false)); assertNull( onResponseSwitchChannel(SnmpChannelMode.TRAP, SnmpDatatype.INT32, "1", "2", new Integer32(2), false)); + verifyStatus(ThingStatus.ONLINE); } @Test @@ -104,6 +106,7 @@ public void testNumberChannelsProperlyUpdatingFloatValue() throws IOException { ResponseEvent event = new ResponseEvent("test", null, null, responsePDU, null); thingHandler.onResponse(event); verify(thingHandlerCallback, atLeast(1)).stateUpdated(eq(CHANNEL_UID), eq(new DecimalType("12.4"))); + verifyStatus(ThingStatus.ONLINE); } @Test @@ -118,6 +121,7 @@ public void testCancelingAsyncRequest() { thingHandler.onResponse(event); assertEquals(1, source.cancelCallCounter); + verifyStatus(ThingStatus.ONLINE); } class SnmpMock extends Snmp { diff --git a/bundles/org.openhab.binding.snmp/src/test/java/org/openhab/binding/snmp/internal/StringChannelTest.java b/bundles/org.openhab.binding.snmp/src/test/java/org/openhab/binding/snmp/internal/StringChannelTest.java index 89dac6c4956b6..07c81453bc131 100644 --- a/bundles/org.openhab.binding.snmp/src/test/java/org/openhab/binding/snmp/internal/StringChannelTest.java +++ b/bundles/org.openhab.binding.snmp/src/test/java/org/openhab/binding/snmp/internal/StringChannelTest.java @@ -22,6 +22,7 @@ import org.junit.jupiter.api.Test; import org.openhab.core.library.types.DecimalType; import org.openhab.core.library.types.StringType; +import org.openhab.core.thing.ThingStatus; import org.snmp4j.PDU; import org.snmp4j.event.ResponseEvent; import org.snmp4j.smi.IpAddress; @@ -75,5 +76,6 @@ public void testStringChannelsProperlyUpdatingOnHexString() throws IOException { ResponseEvent event = new ResponseEvent("test", null, null, responsePDU, null); thingHandler.onResponse(event); verify(thingHandlerCallback, atLeast(1)).stateUpdated(eq(CHANNEL_UID), eq(new StringType("aa 11 bb"))); + verifyStatus(ThingStatus.ONLINE); } } diff --git a/bundles/org.openhab.binding.snmp/src/test/java/org/openhab/binding/snmp/internal/SwitchChannelTest.java b/bundles/org.openhab.binding.snmp/src/test/java/org/openhab/binding/snmp/internal/SwitchChannelTest.java index 4ac15b5b8c30d..4e40cc78c5d5c 100644 --- a/bundles/org.openhab.binding.snmp/src/test/java/org/openhab/binding/snmp/internal/SwitchChannelTest.java +++ b/bundles/org.openhab.binding.snmp/src/test/java/org/openhab/binding/snmp/internal/SwitchChannelTest.java @@ -21,6 +21,8 @@ import org.junit.jupiter.api.Test; import org.openhab.core.library.types.OnOffType; +import org.openhab.core.thing.ThingStatus; +import org.openhab.core.types.State; import org.openhab.core.types.UnDefType; import org.snmp4j.PDU; import org.snmp4j.event.ResponseEvent; @@ -63,7 +65,7 @@ public void testSwitchChannelsProperlyUpdatingOnValue() throws IOException { Collections.singletonList(new VariableBinding(new OID(TEST_OID), new OctetString("on")))); ResponseEvent event = new ResponseEvent("test", null, null, responsePDU, null); thingHandler.onResponse(event); - verify(thingHandlerCallback, atLeast(1)).stateUpdated(eq(CHANNEL_UID), eq(OnOffType.ON)); + verifyResponse(OnOffType.ON); } @Test @@ -73,7 +75,7 @@ public void testSwitchChannelsProperlyUpdatingOffValue() throws IOException { Collections.singletonList(new VariableBinding(new OID(TEST_OID), new Integer32(3)))); ResponseEvent event = new ResponseEvent("test", null, null, responsePDU, null); thingHandler.onResponse(event); - verify(thingHandlerCallback, atLeast(1)).stateUpdated(eq(CHANNEL_UID), eq(OnOffType.OFF)); + verifyResponse(OnOffType.OFF); } @Test @@ -84,7 +86,7 @@ public void testSwitchChannelsProperlyUpdatingHexValue() throws IOException { .singletonList(new VariableBinding(new OID(TEST_OID), OctetString.fromHexStringPairs("aabb11")))); ResponseEvent event = new ResponseEvent("test", null, null, responsePDU, null); thingHandler.onResponse(event); - verify(thingHandlerCallback, atLeast(1)).stateUpdated(eq(CHANNEL_UID), eq(OnOffType.ON)); + verifyResponse(OnOffType.ON); } @Test @@ -95,6 +97,7 @@ public void testSwitchChannelsIgnoresArbitraryValue() throws IOException { ResponseEvent event = new ResponseEvent("test", null, null, responsePDU, null); thingHandler.onResponse(event); verify(thingHandlerCallback, never()).stateUpdated(eq(CHANNEL_UID), any()); + verifyStatus(ThingStatus.ONLINE); } @Test @@ -104,7 +107,7 @@ public void testSwitchChannelSendsUndefExceptionValue() throws IOException { Collections.singletonList(new VariableBinding(new OID(TEST_OID), Null.noSuchInstance))); ResponseEvent event = new ResponseEvent("test", null, null, responsePDU, null); thingHandler.onResponse(event); - verify(thingHandlerCallback, atLeast(1)).stateUpdated(eq(CHANNEL_UID), eq(UnDefType.UNDEF)); + verifyResponse(UnDefType.UNDEF); } @Test @@ -115,6 +118,11 @@ public void testSwitchChannelSendsConfiguredExceptionValue() throws IOException Collections.singletonList(new VariableBinding(new OID(TEST_OID), Null.noSuchInstance))); ResponseEvent event = new ResponseEvent("test", null, null, responsePDU, null); thingHandler.onResponse(event); - verify(thingHandlerCallback, atLeast(1)).stateUpdated(eq(CHANNEL_UID), eq(OnOffType.OFF)); + verifyResponse(OnOffType.OFF); + } + + private void verifyResponse(State expectedState) { + verify(thingHandlerCallback, atLeast(1)).stateUpdated(eq(CHANNEL_UID), eq(expectedState)); + verifyStatus(ThingStatus.ONLINE); } }