[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 <hilbrand@h72.nl>

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 4d37c589e3e8f57229c5c04cfbfadb47420adbe6..1e35357fff0a82e853b4dc39057f525fa0dac2ce 100644 (file)
--- 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.Thing;
 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 class SnmpTargetHandler extends BaseThingHandler implements ResponseListe
             return;
         }
         timeoutCounter = 0;
+        if (ThingHandlerHelper.isHandlerInitialized(this)) {
+            updateStatus(ThingStatus.ONLINE);
+        }
         logger.trace("{} received {}", thing.getUID(), response);
 
         response.getVariableBindings().forEach(variable -> {
@@ -423,7 +427,6 @@ public class SnmpTargetHandler extends BaseThingHandler implements ResponseListe
         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 883dbc069606bb1ee95d38055ed58666a3ac5596..01f059515aeffa28b558268e72e65e6aa62aa09d 100644 (file)
--- 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.HashMap;
 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 @@ public abstract class AbstractSnmpTargetHandlerTest extends JavaTest {
     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 @@ public abstract class AbstractSnmpTargetHandlerTest extends JavaTest {
             String onValue, String offValue, String exceptionValue) {
         Map<String, Object> channelConfig = new HashMap<>();
         Map<String, Object> thingConfig = new HashMap<>();
-        MockitoAnnotations.initMocks(this);
+        mocks = MockitoAnnotations.openMocks(this);
 
         thingConfig.put("hostname", "localhost");
 
@@ -206,6 +213,10 @@ public abstract class AbstractSnmpTargetHandlerTest extends JavaTest {
 
         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 16b82435edee4bc032ba0aadfd711cf66a5242ff..adfe632a1a378f45e70e9554033c4ecfc7cec9b2 100644 (file)
--- 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.junit.jupiter.api.Test;
 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 class SnmpTargetHandlerTest extends AbstractSnmpTargetHandlerTest {
                 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 class SnmpTargetHandlerTest extends AbstractSnmpTargetHandlerTest {
         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 class SnmpTargetHandlerTest extends AbstractSnmpTargetHandlerTest {
 
         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 89dac6c4956b63feaa2b7f2c10eca09ca86b4bbf..07c81453bc131b4caf3d6a227796e69d69edd7e3 100644 (file)
--- 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 java.util.Collections;
 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 class StringChannelTest extends AbstractSnmpTargetHandlerTest {
         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 4ac15b5b8c30d32ab79e2d6086d9f305680716cb..4e40cc78c5d5c365f3a512a6d7409d88375275ff 100644 (file)
--- 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 java.util.Collections;
 
 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 class SwitchChannelTest extends AbstractSnmpTargetHandlerTest {
                 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 class SwitchChannelTest extends AbstractSnmpTargetHandlerTest {
                 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 class SwitchChannelTest extends AbstractSnmpTargetHandlerTest {
                 .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 class SwitchChannelTest extends AbstractSnmpTargetHandlerTest {
         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 class SwitchChannelTest extends AbstractSnmpTargetHandlerTest {
                 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 class SwitchChannelTest extends AbstractSnmpTargetHandlerTest {
                 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);
     }
 }