Skip to content

Commit be82a36

Browse files
jsettonjlaur
jsetton
authored and
jlaur
committed
[insteon] Fix device request failure handling (openhab#18087)
Signed-off-by: Jeremy Setton <[email protected]>
1 parent c6a7cec commit be82a36

File tree

4 files changed

+88
-77
lines changed

4 files changed

+88
-77
lines changed

‎bundles/org.openhab.binding.insteon/src/main/java/org/openhab/binding/insteon/internal/device/BaseDevice.java

Lines changed: 79 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ public abstract class BaseDevice<@NonNull T extends DeviceAddress, @NonNull S ex
4343
implements Device {
4444
private static final int DIRECT_ACK_TIMEOUT = 6000; // in milliseconds
4545
private static final int REQUEST_QUEUE_TIMEOUT = 30000; // in milliseconds
46+
private static final int FAILED_REQUEST_THRESHOLD = 5;
4647

4748
protected static enum DeviceStatus {
4849
INITIALIZED,
@@ -63,6 +64,7 @@ protected static enum DeviceStatus {
6364
private Map<Msg, DeviceRequest> requestQueueHash = new HashMap<>();
6465
private @Nullable DeviceFeature featureQueried;
6566
private long pollInterval = -1L; // in milliseconds
67+
private volatile int failedRequestCount = 0;
6668
private volatile long lastRequestQueued = 0L;
6769
private volatile long lastRequestSent = 0L;
6870

@@ -145,6 +147,10 @@ public boolean getFlag(String key, boolean def) {
145147
}
146148
}
147149

150+
public boolean isResponding() {
151+
return failedRequestCount < FAILED_REQUEST_THRESHOLD;
152+
}
153+
148154
public void setModem(@Nullable InsteonModem modem) {
149155
this.modem = modem;
150156
}
@@ -403,10 +409,8 @@ public void resetFeaturesQueryStatus() {
403409
public void handleMessage(Msg msg) {
404410
getFeatures().stream().filter(feature -> feature.handleMessage(msg)).findFirst().ifPresent(feature -> {
405411
logger.trace("handled reply of direct for {}", feature.getName());
406-
// mark feature queried as processed and answered
407-
setFeatureQueried(null);
408-
feature.setQueryMessage(null);
409-
feature.setQueryStatus(QueryStatus.QUERY_ANSWERED);
412+
// notify feature queried was answered
413+
featureQueriedAnswered(feature);
410414
});
411415
}
412416

@@ -527,9 +531,8 @@ private long checkFeatureQueried(long now) {
527531
return now + 1000L; // retry in 1000 ms
528532
}
529533
logger.debug("gave up waiting for {} query to be sent to {}", feature.getName(), address);
530-
// reset feature queried as never queried
531-
feature.setQueryMessage(null);
532-
feature.setQueryStatus(QueryStatus.NEVER_QUERIED);
534+
// notify feature queried failed
535+
featureQueriedFailed(feature);
533536
break;
534537
case QUERY_SENT:
535538
case QUERY_ACKED:
@@ -541,20 +544,61 @@ private long checkFeatureQueried(long now) {
541544
return now + 500L; // retry in 500 ms
542545
}
543546
logger.debug("gave up waiting for {} query reply from {}", feature.getName(), address);
544-
// reset feature queried as never queried
545-
feature.setQueryMessage(null);
546-
feature.setQueryStatus(QueryStatus.NEVER_QUERIED);
547+
// notify feature queried failed
548+
featureQueriedFailed(feature);
547549
break;
548550
default:
549551
logger.debug("unexpected feature {} query status {} for {}", feature.getName(), queryStatus,
550552
address);
553+
// reset feature queried
554+
setFeatureQueried(null);
551555
}
552-
// reset feature queried otheriwse
553-
setFeatureQueried(null);
554556
}
555557
return 0L;
556558
}
557559

560+
/**
561+
* Notifies that the feature queried was answered
562+
*
563+
* @param feature the feature queried
564+
*/
565+
protected void featureQueriedAnswered(DeviceFeature feature) {
566+
// store current failed request count
567+
int prevCount = failedRequestCount;
568+
// reset failed request count
569+
failedRequestCount = 0;
570+
// mark feature queried as processed and answered
571+
setFeatureQueried(null);
572+
feature.setQueryMessage(null);
573+
feature.setQueryStatus(QueryStatus.QUERY_ANSWERED);
574+
// notify status changed if failed request count was above threshold
575+
if (prevCount >= FAILED_REQUEST_THRESHOLD) {
576+
statusChanged();
577+
}
578+
}
579+
580+
/**
581+
* Notifies that the feature queried failed
582+
*
583+
* @param feature the feature queried
584+
*/
585+
protected void featureQueriedFailed(DeviceFeature feature) {
586+
// increase failed request count
587+
failedRequestCount++;
588+
// mark feature queried as processed and never queried
589+
setFeatureQueried(null);
590+
feature.setQueryMessage(null);
591+
feature.setQueryStatus(QueryStatus.NEVER_QUERIED);
592+
// poll feature again if device is responding
593+
if (isResponding()) {
594+
feature.doPoll(0L);
595+
}
596+
// notify status changed if failed request count at threshold
597+
if (failedRequestCount == FAILED_REQUEST_THRESHOLD) {
598+
statusChanged();
599+
}
600+
}
601+
558602
/**
559603
* Notifies that a message request was replied for this device
560604
*
@@ -564,10 +608,17 @@ private long checkFeatureQueried(long now) {
564608
public void requestReplied(Msg msg) {
565609
DeviceFeature feature = getFeatureQueried();
566610
if (feature != null && feature.isMyReply(msg)) {
567-
// mark feature queried as processed and answered
568-
setFeatureQueried(null);
569-
feature.setQueryMessage(null);
570-
feature.setQueryStatus(QueryStatus.QUERY_ANSWERED);
611+
if (msg.isReplyNack()) {
612+
logger.debug("got a reply nack msg: {}", msg);
613+
// notify feature queried failed
614+
featureQueriedFailed(feature);
615+
} else if (!msg.isInsteon()) {
616+
// notify feature queried was answered
617+
featureQueriedAnswered(feature);
618+
} else {
619+
// mark feature queried as acked
620+
feature.setQueryStatus(QueryStatus.QUERY_ACKED);
621+
}
571622
}
572623
}
573624

@@ -588,6 +639,18 @@ public void requestSent(Msg msg, long time) {
588639
}
589640
}
590641

642+
/**
643+
* Notifies that the status has changed for this device
644+
*/
645+
public void statusChanged() {
646+
logger.trace("status for {} has changed", address);
647+
@Nullable
648+
S handler = getHandler();
649+
if (handler != null) {
650+
handler.updateStatus();
651+
}
652+
}
653+
591654
/**
592655
* Refreshes this device
593656
*/

‎bundles/org.openhab.binding.insteon/src/main/java/org/openhab/binding/insteon/internal/device/InsteonDevice.java

Lines changed: 4 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,6 @@
6666
public class InsteonDevice extends BaseDevice<InsteonAddress, InsteonDeviceHandler> {
6767
private static final int BCAST_STATE_TIMEOUT = 2000; // in milliseconds
6868
private static final int DEFAULT_HEARTBEAT_TIMEOUT = 1440; // in minutes
69-
private static final int FAILED_MSG_COUNT_THRESHOLD = 5;
7069

7170
private InsteonEngine engine = InsteonEngine.UNKNOWN;
7271
private LinkDB linkDB;
@@ -76,7 +75,6 @@ public class InsteonDevice extends BaseDevice<InsteonAddress, InsteonDeviceHandl
7675
private Map<Msg, DeviceRequest> deferredQueueHash = new HashMap<>();
7776
private Map<Byte, Long> lastBroadcastReceived = new HashMap<>();
7877
private Map<Integer, GroupMessageStateMachine> groupState = new HashMap<>();
79-
private volatile int failedMsgCount = 0;
8078
private volatile long lastMsgReceived = 0L;
8179

8280
public InsteonDevice() {
@@ -145,10 +143,6 @@ public int getLastMsgValueAsInteger(String type, int group, int defaultValue) {
145143
return Optional.ofNullable(getFeature(type, group)).map(DeviceFeature::getState).orElse(null);
146144
}
147145

148-
public boolean isResponding() {
149-
return failedMsgCount < FAILED_MSG_COUNT_THRESHOLD;
150-
}
151-
152146
public boolean isBatteryPowered() {
153147
return getFlag("batteryPowered", false);
154148
}
@@ -444,35 +438,21 @@ public void handleMessage(Msg msg) {
444438
}
445439
return;
446440
}
447-
// store current responding state
448-
boolean isPrevResponding = isResponding();
449441
// handle message depending if failure report or not
450442
if (msg.isFailureReport()) {
451443
getFeatures().stream().filter(feature -> feature.isMyDirectAckOrNack(msg)).findFirst()
452444
.ifPresent(feature -> {
453445
logger.debug("got a failure report reply of direct for {}", feature.getName());
454-
// increase failed message counter
455-
failedMsgCount++;
456-
// mark feature queried as processed and never queried
457-
setFeatureQueried(null);
458-
feature.setQueryMessage(null);
459-
feature.setQueryStatus(QueryStatus.NEVER_QUERIED);
460-
// poll feature again if device is responding
461-
if (isResponding()) {
462-
feature.doPoll(0L);
463-
}
446+
// notify feature queried failed
447+
featureQueriedFailed(feature);
464448
});
465449
} else {
466450
// update non-status features
467451
getFeatures().stream().filter(feature -> !feature.isStatusFeature() && feature.handleMessage(msg))
468452
.findFirst().ifPresent(feature -> {
469453
logger.trace("handled reply of direct for {}", feature.getName());
470-
// reset failed message counter
471-
failedMsgCount = 0;
472-
// mark feature queried as processed and answered
473-
setFeatureQueried(null);
474-
feature.setQueryMessage(null);
475-
feature.setQueryStatus(QueryStatus.QUERY_ANSWERED);
454+
// notify feature queried was answered
455+
featureQueriedAnswered(feature);
476456
});
477457
// update all status features (e.g. device last update time)
478458
getFeatures().stream().filter(DeviceFeature::isStatusFeature)
@@ -485,10 +465,6 @@ public void handleMessage(Msg msg) {
485465
long delay = msg.isAllLinkBroadcast() && !msg.isAllLinkSuccessReport() && !msg.isReplayed() ? 1500L : 0L;
486466
doPoll(delay);
487467
}
488-
// notify if responding state changed
489-
if (isPrevResponding != isResponding()) {
490-
statusChanged();
491-
}
492468
}
493469

494470
/**
@@ -906,25 +882,6 @@ public void resetHeartbeatMonitor() {
906882
}
907883
}
908884

909-
/**
910-
* Notifies that a message request was replied for this device
911-
*
912-
* @param msg the message received
913-
*/
914-
@Override
915-
public void requestReplied(Msg msg) {
916-
DeviceFeature feature = getFeatureQueried();
917-
if (feature != null && feature.isMyReply(msg)) {
918-
if (msg.isReplyAck()) {
919-
// mark feature queried as acked
920-
feature.setQueryStatus(QueryStatus.QUERY_ACKED);
921-
} else {
922-
logger.debug("got a reply nack msg: {}", msg);
923-
super.requestReplied(msg);
924-
}
925-
}
926-
}
927-
928885
/**
929886
* Notifies that the link db has been updated for this device
930887
*/
@@ -966,18 +923,6 @@ public void propertiesChanged(boolean reset) {
966923
}
967924
}
968925

969-
/**
970-
* Notifies that the status has changed for this device
971-
*/
972-
public void statusChanged() {
973-
logger.trace("status for {} has changed", address);
974-
975-
InsteonDeviceHandler handler = getHandler();
976-
if (handler != null) {
977-
handler.updateStatus();
978-
}
979-
}
980-
981926
/**
982927
* Factory method for creating a InsteonDevice from a device address, modem and cache
983928
*

‎bundles/org.openhab.binding.insteon/src/main/java/org/openhab/binding/insteon/internal/handler/InsteonBaseThingHandler.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -330,8 +330,6 @@ public void refresh() {
330330
updateStatus();
331331
}
332332

333-
public abstract void updateStatus();
334-
335333
public void updateProperties(Device device) {
336334
Map<String, String> properties = editProperties();
337335

‎bundles/org.openhab.binding.insteon/src/main/java/org/openhab/binding/insteon/internal/handler/InsteonThingHandler.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,4 +91,9 @@ public interface InsteonThingHandler extends ThingHandler {
9191
* Refreshes the thing
9292
*/
9393
public void refresh();
94+
95+
/**
96+
* Updates the thing status
97+
*/
98+
public void updateStatus();
9499
}

0 commit comments

Comments
 (0)