Skip to content

Commit

Permalink
[snmp] Set thing only online on valid response (openhab#8759)
Browse files Browse the repository at this point in the history 
* [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 <[email protected]>
  • Loading branch information
@Hilbrand
Hilbrand authored Oct 25, 2020
1 parent eb15a33 commit e3b1d04
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 9 deletions.
5 changes: 4 additions & 1 deletion ...enhab.binding.snmp/src/main/java/org/openhab/binding/snmp/internal/SnmpTargetHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 -> {
Expand Down Expand Up @@ -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);
Expand Down
17 changes: 14 additions & 3 deletions ...g.snmp/src/test/java/org/openhab/binding/snmp/internal/AbstractSnmpTargetHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -165,7 +172,7 @@ protected void setup(ChannelTypeUID channelTypeUID, SnmpChannelMode channelMode,
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");

Expand Down Expand Up @@ -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()));
}
}
4 changes: 4 additions & 0 deletions ...b.binding.snmp/src/test/java/org/openhab/binding/snmp/internal/SnmpTargetHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -118,6 +121,7 @@ public void testCancelingAsyncRequest() {

thingHandler.onResponse(event);
assertEquals(1, source.cancelCallCounter);
verifyStatus(ThingStatus.ONLINE);
}

class SnmpMock extends Snmp {
Expand Down
2 changes: 2 additions & 0 deletions ...enhab.binding.snmp/src/test/java/org/openhab/binding/snmp/internal/StringChannelTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}
18 changes: 13 additions & 5 deletions ...enhab.binding.snmp/src/test/java/org/openhab/binding/snmp/internal/SwitchChannelTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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);
}
}

0 comments on commit e3b1d04

Please sign in to comment.