From 222fccdbff9f9f8244d9c7731e8fcab26c9bcf74 Mon Sep 17 00:00:00 2001 From: David Goodyear Date: Wed, 19 Feb 2025 12:17:50 +0000 Subject: [PATCH] [linktap] Bugfix Issue 18076 (#18090) [linktap] Bugfix Issue 18076 Signed-off-by: David Goodyear --- .../internal/LinkTapBridgeHandler.java | 145 ++++++++++++++---- .../linktap/internal/LinkTapHandler.java | 6 + .../internal/PollingDeviceHandler.java | 24 ++- .../internal/TransactionProcessor.java | 12 +- .../frames/GatewayDeviceResponse.java | 2 +- .../protocol/frames/GatewayEndDevListReq.java | 2 +- .../TransientCommunicationIssueException.java | 7 +- .../linktap/protocol/http/WebServerApi.java | 10 +- .../resources/OH-INF/i18n/linktap.properties | 4 + 9 files changed, 174 insertions(+), 38 deletions(-) diff --git a/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/internal/LinkTapBridgeHandler.java b/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/internal/LinkTapBridgeHandler.java index 521ca7526b7..731255c0e0f 100644 --- a/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/internal/LinkTapBridgeHandler.java +++ b/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/internal/LinkTapBridgeHandler.java @@ -13,9 +13,13 @@ package org.openhab.binding.linktap.internal; import static org.openhab.binding.linktap.internal.LinkTapBindingConstants.*; +import static org.openhab.binding.linktap.internal.TransactionProcessor.MAX_COMMAND_RETRIES; +import static org.openhab.binding.linktap.protocol.frames.GatewayDeviceResponse.ResultStatus.RET_GATEWAY_BUSY; +import static org.openhab.binding.linktap.protocol.frames.GatewayDeviceResponse.ResultStatus.RET_GW_INTERNAL_ERR; import static org.openhab.binding.linktap.protocol.frames.TLGatewayFrame.*; import static org.openhab.binding.linktap.protocol.frames.ValidationError.Cause.BUG; import static org.openhab.binding.linktap.protocol.frames.ValidationError.Cause.USER; +import static org.openhab.binding.linktap.protocol.http.TransientCommunicationIssueException.TransientExecptionDefinitions.GATEWAY_BUSY; import java.io.IOException; import java.net.InetAddress; @@ -105,6 +109,7 @@ public class LinkTapBridgeHandler extends BaseBridgeHandler { private volatile LinkTapBridgeConfiguration config = new LinkTapBridgeConfiguration(); private volatile long lastGwCommandRecvTs = 0L; private volatile long lastMdnsScanMillis = -1L; + private volatile boolean readZeroDevices = false; private String bridgeKey = ""; private IHttpClientProvider httpClientProvider; @@ -143,7 +148,7 @@ public class LinkTapBridgeHandler extends BaseBridgeHandler { cancelGwPolling(); backgroundGwPollingScheduler = scheduler.scheduleWithFixedDelay(() -> { if (lastGwCommandRecvTs + 120000 < System.currentTimeMillis()) { - getGatewayConfiguration(); + getGatewayConfiguration(false); } }, 5000, 120000, TimeUnit.MILLISECONDS); } @@ -227,7 +232,15 @@ public class LinkTapBridgeHandler extends BaseBridgeHandler { return true; } - public void getGatewayConfiguration() { + public boolean getGatewayConfigurationFreshCheck() { + readZeroDevices = false; + return getGatewayConfiguration(true); + } + + public boolean getGatewayConfiguration(final boolean forceFreshRead) { + if (forceFreshRead) { + lastGetConfigCache.invalidateValue(); + } String resp = ""; synchronized (getConfigLock) { resp = lastGetConfigCache.getValue(); @@ -251,13 +264,17 @@ public class LinkTapBridgeHandler extends BaseBridgeHandler { } lastGetConfigCache.putValue(resp); } - } final GatewayConfigResp gwConfig = LinkTapBindingConstants.GSON.fromJson(resp, GatewayConfigResp.class); if (gwConfig == null) { - return; + return false; } + + if (gwConfig.isRetryableError()) { + return false; + } + currentGwId = gwConfig.gatewayId; final String version = gwConfig.version; @@ -269,7 +286,6 @@ public class LinkTapBridgeHandler extends BaseBridgeHandler { final Map props = editProperties(); props.put(BRIDGE_PROP_GW_VER, version); updateProperties(props); - return; } if (!volUnit.equals(editProperties().get(BRIDGE_PROP_VOL_UNIT))) { final Map props = editProperties(); @@ -285,6 +301,20 @@ public class LinkTapBridgeHandler extends BaseBridgeHandler { } } + // Filter out the processing where we receive just a single response containing no device definitions, ensure + // this is confirmed by a second poll. + if (devIds.length == 0 || devNames.length == 0) { + if (!readZeroDevices) { + logger.trace("Detected ZERO devices in Gateway from CMD 16"); + readZeroDevices = true; + lastGetConfigCache.invalidateValue(); + return false; // Don't process the potentially incorrect data + } + logger.debug("Confirmed ZERO devices in Gateway from CMD 16"); + } else { + readZeroDevices = false; + } + boolean updatedDeviceInfo = devIds.length != discoveredDevices.size(); for (int i = 0; i < devIds.length; ++i) { @@ -298,19 +328,41 @@ public class LinkTapBridgeHandler extends BaseBridgeHandler { handlers.forEach(x -> x.handleMetadataRetrieved(this)); - if (updatedDeviceInfo) { - this.scheduler.execute(() -> { - for (Thing el : getThing().getThings()) { - final ThingHandler th = el.getHandler(); - if (th instanceof IBridgeData bridgeData) { + final boolean forceDeviceInit = updatedDeviceInfo; + this.scheduler.execute(() -> { + for (Thing el : getThing().getThings()) { + final ThingHandler th = el.getHandler(); + if (th instanceof IBridgeData bridgeData) { + if (forceDeviceInit || ThingStatus.OFFLINE.equals(th.getThing().getStatus())) { bridgeData.handleBridgeDataUpdated(); } } - }); - } + } + }); + + return true; } - public String sendApiRequest(final TLGatewayFrame req) { + public String sendApiRequest(final TLGatewayFrame request) { + int triesLeft = MAX_COMMAND_RETRIES; + int retry = 0; + while (triesLeft > 0) { + try { + return sendSingleApiRequest(request); + } catch (TransientCommunicationIssueException tcie) { + --triesLeft; + try { + Thread.sleep(1000L * retry); + } catch (InterruptedException ie) { + return ""; + } + ++retry; + } + } + return ""; + } + + public String sendSingleApiRequest(final TLGatewayFrame req) throws TransientCommunicationIssueException { final UUID uid = UUID.randomUUID(); final WebServerApi api = WebServerApi.getInstance(); @@ -329,17 +381,26 @@ public class LinkTapBridgeHandler extends BaseBridgeHandler { logger.debug("{} = APP BRIDGE -> GW -> Request {}", uid, reqData); final String respData = api.sendRequest(host, reqData); logger.debug("{} = APP BRIDGE -> GW -> Response {}", uid, respData); - final TLGatewayFrame gwResponseFrame = LinkTapBindingConstants.GSON.fromJson(respData, - TLGatewayFrame.class); - if (confirmGateway && gwResponseFrame != null && !gwResponseFrame.gatewayId.equals(req.gatewayId)) { - logger.warn("{}", getLocalizedText("warning.response-from-wrong-gw-id", uid, req.gatewayId, - gwResponseFrame.gatewayId)); - return ""; - } - if (gwResponseFrame != null && req.command != gwResponseFrame.command) { - logger.warn("{}", - getLocalizedText("warning.incorrect-cmd-resp", uid, req.command, gwResponseFrame.command)); - return ""; + final GatewayDeviceResponse gwResponseFrame = LinkTapBindingConstants.GSON.fromJson(respData, + GatewayDeviceResponse.class); + + if (gwResponseFrame != null) { + if (confirmGateway && !gwResponseFrame.gatewayId.equals(req.gatewayId)) { + logger.warn("{}", getLocalizedText("warning.response-from-wrong-gw-id", uid, req.gatewayId, + gwResponseFrame.gatewayId)); + return ""; + } + + if (RET_GW_INTERNAL_ERR.equals(gwResponseFrame.getRes()) + || RET_GATEWAY_BUSY.equals(gwResponseFrame.getRes())) { + throw new TransientCommunicationIssueException(GATEWAY_BUSY); + } + + if (req.command != gwResponseFrame.command) { + logger.warn("{}", + getLocalizedText("warning.incorrect-cmd-resp", uid, req.command, gwResponseFrame.command)); + return ""; + } } return respData; } catch (NotTapLinkGatewayException e) { @@ -385,7 +446,11 @@ public class LinkTapBridgeHandler extends BaseBridgeHandler { } } - getGatewayConfiguration(); + if (!getGatewayConfiguration(true)) { + logger.debug("{}", getLocalizedText("bridge.info.awaiting-init")); + scheduleReconnect(); + return; + } // Update the GW ID -> this bridge lookup GW_ID_LOOKUP.registerItem(currentGwId, this, () -> { @@ -418,13 +483,33 @@ public class LinkTapBridgeHandler extends BaseBridgeHandler { final Optional servletEpOpt = (!servletEp.isEmpty()) ? Optional.of(servletEp) : Optional.empty(); api.configureBridge(hostname, Optional.of(config.enableMDNS), Optional.of(config.enableJSONComms), servletEpOpt); - updateStatus(ThingStatus.ONLINE); if (Thread.currentThread().isInterrupted()) { return; } + + // Ensure we have a response with data in if not schedule a reconnect in 15 seconds, theres no reason + // for a gateway with no devices. + if (!getGatewayConfigurationFreshCheck()) { + logger.debug("{}", getLocalizedText("bridge.info.awaiting-init")); + scheduleReconnect(); + return; + } + + updateStatus(ThingStatus.ONLINE); startGwPolling(); connectRepair = null; + // Force all child things run their init sequences to ensure they are registered by the + // device ID. + this.scheduler.execute(() -> { + for (Thing el : getThing().getThings()) { + final ThingHandler th = el.getHandler(); + if (th instanceof IBridgeData bridgeData) { + bridgeData.handleBridgeDataUpdated(); + } + } + }); + final Firmware firmware = new Firmware(getThing().getProperties().get(BRIDGE_PROP_GW_VER)); if (!firmware.supportsLocalConfig()) { logger.warn("{}", getLocalizedText("warning.fw-update-local-config", getThing().getLabel(), @@ -622,13 +707,17 @@ public class LinkTapBridgeHandler extends BaseBridgeHandler { } if (fullScanRequired) { logger.trace("The configured devices have changed a full scan should be run"); - scheduler.execute(this::getGatewayConfiguration); + scheduler.execute(() -> { + getGatewayConfiguration(true); + }); } } @Override public void childHandlerDisposed(ThingHandler childHandler, Thing childThing) { - scheduler.execute(this::getGatewayConfiguration); + scheduler.execute(() -> { + getGatewayConfiguration(false); + }); super.childHandlerDisposed(childHandler, childThing); } } diff --git a/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/internal/LinkTapHandler.java b/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/internal/LinkTapHandler.java index c4bcdb0148b..1df6b2229fe 100644 --- a/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/internal/LinkTapHandler.java +++ b/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/internal/LinkTapHandler.java @@ -445,6 +445,12 @@ public class LinkTapHandler extends PollingDeviceHandler { @Override public void handleBridgeDataUpdated() { switch (getThing().getStatus()) { + case ONLINE: + if (!initPending) { + logger.trace("Handling new bridge data for {} not required already online and processed", + getThing().getLabel()); + return; + } case OFFLINE: case UNKNOWN: logger.trace("Handling new bridge data for {}", getThing().getLabel()); diff --git a/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/internal/PollingDeviceHandler.java b/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/internal/PollingDeviceHandler.java index be0b10cd9bd..6c8c54c9e7d 100644 --- a/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/internal/PollingDeviceHandler.java +++ b/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/internal/PollingDeviceHandler.java @@ -56,6 +56,8 @@ import org.slf4j.LoggerFactory; @NonNullByDefault public abstract class PollingDeviceHandler extends BaseThingHandler implements IBridgeData { + protected boolean initPending = true; + protected static final String MARKER_INVALID_DEVICE_KEY = "---INVALID---"; private final Logger logger = LoggerFactory.getLogger(PollingDeviceHandler.class); @@ -154,6 +156,7 @@ public abstract class PollingDeviceHandler extends BaseThingHandler implements I } protected void initAfterBridge(final LinkTapBridgeHandler bridge) { + initPending = true; String deviceId = getValidatedIdString(); if (MARKER_INVALID_DEVICE_KEY.equals(deviceId)) { updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, @@ -167,19 +170,36 @@ public abstract class PollingDeviceHandler extends BaseThingHandler implements I registeredDeviceId = deviceId; } + // This can be called, and then the bridge data gets updated boolean knownToBridge = bridge.getDiscoveredDevices().anyMatch(x -> deviceId.equals(x.deviceId)); if (knownToBridge) { - updateStatus(ThingStatus.ONLINE); + if (!ThingStatus.ONLINE.equals(getThing().getStatus())) { + updateStatus(ThingStatus.ONLINE); + } registerDevice(); scheduleInitialPoll(); scheduler.execute(this::runStartInit); startStatusPolling(); + initPending = false; } else { updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, getLocalizedText("polling-device.error.unknown-device-id")); } } + @Override + protected void updateStatus(ThingStatus status, ThingStatusDetail statusDetail, @Nullable String description) { + super.updateStatus(status, statusDetail, description); + scheduler.execute(() -> { + if (initPending && ThingStatus.ONLINE.equals(status)) { + final BridgeHandler handler = getBridgeHandler(); + if (handler instanceof LinkTapBridgeHandler linkTapBridge) { + initAfterBridge(linkTapBridge); + } + } + }); + } + protected abstract void runStartInit(); protected abstract void registerDevice(); @@ -247,6 +267,7 @@ public abstract class PollingDeviceHandler extends BaseThingHandler implements I final LinkTapDeviceMetadata metadata = vesyncBridgeHandler.getDeviceLookup().get(devId); if (metadata != null) { + logger.trace("Using matched Address ID (dev_id) {}", metadata.deviceId); return metadata.deviceId; } } @@ -266,6 +287,7 @@ public abstract class PollingDeviceHandler extends BaseThingHandler implements I return MARKER_INVALID_DEVICE_KEY; } + logger.trace("Using matched Address ID (dev_name) {}", matchedAddressIds[0]); return matchedAddressIds[0]; } } diff --git a/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/internal/TransactionProcessor.java b/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/internal/TransactionProcessor.java index ea2512c7673..aeaf25b6205 100644 --- a/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/internal/TransactionProcessor.java +++ b/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/internal/TransactionProcessor.java @@ -62,7 +62,7 @@ public final class TransactionProcessor { // As the Gateway is an embedded device, private static final WebServerApi API = WebServerApi.getInstance(); - private static final int MAX_COMMAND_RETRIES = 3; + static final int MAX_COMMAND_RETRIES = 3; private static final TransactionProcessor INSTANCE = new TransactionProcessor(); private final Logger logger = LoggerFactory.getLogger(TransactionProcessor.class); @@ -207,6 +207,14 @@ public final class TransactionProcessor { try { return sendSingleRequest(handler, request); } catch (TransientCommunicationIssueException tcie) { + // Only retry a water timer status read once, with a 3 second delay + if (request.command == CMD_UPDATE_WATER_TIMER_STATUS) { + if (retry > 1) { + return ""; + } else { + retry = 3; + } + } --triesLeft; try { Thread.sleep(1000L * retry); @@ -286,7 +294,7 @@ public final class TransactionProcessor { case RET_GATEWAY_BUSY: case RET_GW_INTERNAL_ERR: logger.trace("The request can be re-tried"); - break; + throw new TransientCommunicationIssueException(GATEWAY_BUSY); case RET_CONFLICT_WATER_PLAN: logger.trace("Gateway rejected command due to water plan conflict"); break; diff --git a/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/protocol/frames/GatewayDeviceResponse.java b/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/protocol/frames/GatewayDeviceResponse.java index f31d3a98453..a9068b177c7 100644 --- a/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/protocol/frames/GatewayDeviceResponse.java +++ b/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/protocol/frames/GatewayDeviceResponse.java @@ -57,7 +57,7 @@ public class GatewayDeviceResponse extends TLGatewayFrame { public boolean isRetryableError() { switch (getRes()) { - case RET_CONFLICT_WATER_PLAN: // Conflict with watering plan + case RET_GATEWAY_BUSY: // Gateway is busy (likely booting up) case RET_GW_INTERNAL_ERR: // Gateway internal error return true; default: diff --git a/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/protocol/frames/GatewayEndDevListReq.java b/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/protocol/frames/GatewayEndDevListReq.java index 72632e8a525..9d3773eabae 100644 --- a/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/protocol/frames/GatewayEndDevListReq.java +++ b/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/protocol/frames/GatewayEndDevListReq.java @@ -29,7 +29,7 @@ import com.google.gson.annotations.SerializedName; * @author David Goodyear - Initial contribution */ @NonNullByDefault -public class GatewayEndDevListReq extends TLGatewayFrame { +public class GatewayEndDevListReq extends GatewayDeviceResponse { protected static final Pattern FULL_DEVICE_ID_PATTERN = Pattern.compile("[a-zA-Z0-9]{20}"); diff --git a/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/protocol/http/TransientCommunicationIssueException.java b/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/protocol/http/TransientCommunicationIssueException.java index 86992390c8f..109f0a4c2fc 100644 --- a/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/protocol/http/TransientCommunicationIssueException.java +++ b/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/protocol/http/TransientCommunicationIssueException.java @@ -63,7 +63,12 @@ public class TransientCommunicationIssueException extends I18Exception { /** * COMMUNICATIONS_LOST */ - COMMUNICATIONS_LOST("Communications Lost", "exception.communications-lost"); + COMMUNICATIONS_LOST("Communications Lost", "exception.communications-lost"), + + /** + * GATEWAY_BUSY + */ + GATEWAY_BUSY("Gateway Busy", "exception.gateway-busy"); private final String description; private final String i18Key; diff --git a/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/protocol/http/WebServerApi.java b/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/protocol/http/WebServerApi.java index 023788f9b4a..ebb11b9e9ed 100644 --- a/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/protocol/http/WebServerApi.java +++ b/bundles/org.openhab.binding.linktap/src/main/java/org/openhab/binding/linktap/protocol/http/WebServerApi.java @@ -18,6 +18,7 @@ import static org.openhab.binding.linktap.protocol.http.TransientCommunicationIs import java.net.HttpURLConnection; import java.net.InetAddress; +import java.net.SocketException; import java.net.SocketTimeoutException; import java.net.URLDecoder; import java.net.URLEncoder; @@ -166,7 +167,8 @@ public final class WebServerApi { throw new TransientCommunicationIssueException(COMMUNICATIONS_LOST); } catch (ExecutionException e) { final Throwable t = e.getCause(); - if (t instanceof UnknownHostException || t instanceof SocketTimeoutException) { + if (t instanceof UnknownHostException || t instanceof SocketTimeoutException + || t instanceof SocketException) { throw new TransientCommunicationIssueException(HOST_UNREACHABLE); } else if (t instanceof SSLHandshakeException) { throw new NotTapLinkGatewayException(UNEXPECTED_HTTPS); @@ -457,7 +459,7 @@ public final class WebServerApi { final Throwable t = e.getCause(); if (t instanceof UnknownHostException) { throw new TransientCommunicationIssueException(HOST_NOT_RESOLVED); - } else if (t instanceof SocketTimeoutException) { + } else if (t instanceof SocketTimeoutException || t instanceof SocketException) { throw new TransientCommunicationIssueException(HOST_UNREACHABLE); } else if (t instanceof SSLHandshakeException) { throw new NotTapLinkGatewayException(UNEXPECTED_HTTPS); @@ -490,7 +492,7 @@ public final class WebServerApi { final Throwable t = e.getCause(); if (t instanceof UnknownHostException) { throw new TransientCommunicationIssueException(HOST_NOT_RESOLVED); - } else if (t instanceof SocketTimeoutException) { + } else if (t instanceof SocketTimeoutException || t instanceof SocketException) { throw new TransientCommunicationIssueException(HOST_UNREACHABLE); } else if (t instanceof SSLHandshakeException) { throw new NotTapLinkGatewayException(UNEXPECTED_HTTPS); @@ -560,7 +562,7 @@ public final class WebServerApi { throw new TransientCommunicationIssueException(HOST_NOT_RESOLVED); } catch (ExecutionException e) { final Throwable t = e.getCause(); - if (t instanceof UnknownHostException) { + if (t instanceof UnknownHostException || t instanceof SocketException) { throw new TransientCommunicationIssueException(HOST_NOT_RESOLVED); } else if (t instanceof SSLHandshakeException) { throw new NotTapLinkGatewayException(UNEXPECTED_HTTPS); diff --git a/bundles/org.openhab.binding.linktap/src/main/resources/OH-INF/i18n/linktap.properties b/bundles/org.openhab.binding.linktap/src/main/resources/OH-INF/i18n/linktap.properties index 29cd79bf376..1e203ed0efb 100644 --- a/bundles/org.openhab.binding.linktap/src/main/resources/OH-INF/i18n/linktap.properties +++ b/bundles/org.openhab.binding.linktap/src/main/resources/OH-INF/i18n/linktap.properties @@ -250,6 +250,10 @@ channel-type.linktap.waterSkipFutureRain.description = Future rainfall calculate channel-type.linktap.waterSkipPrevRain.label = Watering Skipped Previous Rain channel-type.linktap.waterSkipPrevRain.description = Previous rainfall calculated when watering was skipped +# informative messages + +bridge.info.awaiting-init = Awaiting Gateway initialisation + # errors bridge.error.host-not-found = Hostname / IP cannot be found