From 91c008a06c04778a23c77d7d06b5fa0c88c83904 Mon Sep 17 00:00:00 2001 From: Andrew Fiddian-Green Date: Thu, 27 Jul 2023 13:55:17 +0100 Subject: [PATCH] [hue] Fix reconnection, parallel commands, trigger channels, and light level formula (#15169) * [hue] post merge tweaks Signed-off-by: Andrew Fiddian-Green * [hue] abandon internal restart Signed-off-by: Andrew Fiddian-Green * [hue] remove externalRestartScheduled flag Signed-off-by: Andrew Fiddian-Green * [hue] serialize PUT calls Signed-off-by: Andrew Fiddian-Green * [hue] GET requests shall not activate trigger channels Signed-off-by: Andrew Fiddian-Green * [hue] fix LightLevel formula Signed-off-by: Andrew Fiddian-Green * [hue] fix Button DTO null error Signed-off-by: Andrew Fiddian-Green --------- Signed-off-by: Andrew Fiddian-Green (cherry picked from commit ae36108cbd360cc7f68ef75d0f41e390cd37959e) --- .../org.openhab.binding.hue/doc/readme_v2.md | 4 +- .../hue/internal/connection/Clip2Bridge.java | 108 +++--------------- .../hue/internal/dto/clip2/Button.java | 19 +-- .../hue/internal/dto/clip2/LightLevel.java | 9 +- .../hue/internal/dto/clip2/Resource.java | 5 - .../internal/handler/Clip2BridgeHandler.java | 21 ++-- .../internal/handler/Clip2ThingHandler.java | 8 +- 7 files changed, 50 insertions(+), 124 deletions(-) diff --git a/bundles/org.openhab.binding.hue/doc/readme_v2.md b/bundles/org.openhab.binding.hue/doc/readme_v2.md index b8929df4fe9..2bec43af0e1 100644 --- a/bundles/org.openhab.binding.hue/doc/readme_v2.md +++ b/bundles/org.openhab.binding.hue/doc/readme_v2.md @@ -69,8 +69,8 @@ Device things support some of the following channels: | dynamics | Number:Time | Sets the duration of dynamic transitions between light states. (Advanced) | | alert | String | Allows setting an alert on a light e.g. flashing them. (Advanced) | | effect | String | Allows setting an effect on a light e.g. 'candle' effect. (Advanced) | -| button-last-event | Number | Informs which button was last pressed in the device. (Trigger Channel) | -| rotary-steps | Number | Informs about the number of rotary steps of the last rotary dial movement. (Trigger Channel) | +| button-last-event | (String) | Informs which button was last pressed in the device. (Trigger Channel) | +| rotary-steps | (String) | Informs about the number of rotary steps of the last rotary dial movement. (Trigger Channel) | | motion | Switch | Shows if motion has been detected by the sensor. (Read Only) | | motion-enabled | Switch | Supports enabling / disabling the motion sensor. (Advanced) | | light-level | Number:Illuminance | Shows the current light level measured by the sensor. (Read Only) | diff --git a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/connection/Clip2Bridge.java b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/connection/Clip2Bridge.java index c1b5a680414..794f8676cbc 100644 --- a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/connection/Clip2Bridge.java +++ b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/connection/Clip2Bridge.java @@ -416,7 +416,6 @@ public class Clip2Bridge implements Closeable { private static final int CHECK_ALIVE_SECONDS = 300; private static final int REQUEST_INTERVAL_MILLISECS = 50; private static final int MAX_CONCURRENT_STREAMS = 3; - private static final int RESTART_AFTER_SECONDS = 5; private static final ResourceReference BRIDGE = new ResourceReference().setType(ResourceType.BRIDGE); @@ -463,15 +462,12 @@ public class Clip2Bridge implements Closeable { private final Semaphore streamMutex = new Semaphore(MAX_CONCURRENT_STREAMS, true); private boolean closing; - private boolean internalRestartScheduled; - private boolean externalRestartScheduled; private State onlineState = State.CLOSED; private Optional lastRequestTime = Optional.empty(); private Instant sessionExpireTime = Instant.MAX; private @Nullable Session http2Session; private @Nullable Future checkAliveTask; - private @Nullable Future internalRestartTask; private Map> fatalErrorTasks = new ConcurrentHashMap<>(); /** @@ -481,14 +477,20 @@ public class Clip2Bridge implements Closeable { * @param bridgeHandler the bridge handler. * @param hostName the host name (ip address) of the Hue bridge * @param applicationKey the application key. + * @throws ApiException if unable to open Jetty HTTP/2 client. */ public Clip2Bridge(HttpClientFactory httpClientFactory, Clip2BridgeHandler bridgeHandler, String hostName, - String applicationKey) { + String applicationKey) throws ApiException { LOGGER.debug("Clip2Bridge()"); httpClient = httpClientFactory.getCommonHttpClient(); http2Client = httpClientFactory.createHttp2Client("hue-clip2", httpClient.getSslContextFactory()); http2Client.setConnectTimeout(Clip2Bridge.TIMEOUT_SECONDS * 1000); http2Client.setIdleTimeout(-1); + try { + http2Client.start(); + } catch (Exception e) { + throw new ApiException("Error starting HTTP/2 client", e); + } this.bridgeHandler = bridgeHandler; this.hostName = hostName; this.applicationKey = applicationKey; @@ -541,9 +543,11 @@ public class Clip2Bridge implements Closeable { @Override public void close() { closing = true; - externalRestartScheduled = false; - internalRestartScheduled = false; close2(); + try { + http2Client.stop(); + } catch (Exception e) { + } } /** @@ -552,26 +556,15 @@ public class Clip2Bridge implements Closeable { private void close2() { synchronized (this) { LOGGER.debug("close2()"); - boolean notifyHandler = onlineState == State.ACTIVE && !internalRestartScheduled - && !externalRestartScheduled && !closing; + boolean notifyHandler = onlineState == State.ACTIVE && !closing; onlineState = State.CLOSED; synchronized (fatalErrorTasks) { fatalErrorTasks.values().forEach(task -> cancelTask(task, true)); fatalErrorTasks.clear(); } - if (!internalRestartScheduled) { - // don't close the task if a restart is current - cancelTask(internalRestartTask, true); - internalRestartTask = null; - } cancelTask(checkAliveTask, true); checkAliveTask = null; closeSession(); - try { - http2Client.stop(); - } catch (Exception e) { - // ignore - } if (notifyHandler) { bridgeHandler.onConnectionOffline(); } @@ -584,7 +577,7 @@ public class Clip2Bridge implements Closeable { private void closeSession() { LOGGER.debug("closeSession()"); Session session = http2Session; - if (Objects.nonNull(session)) { + if (Objects.nonNull(session) && !session.isClosed()) { session.close(0, null, Callback.NOOP); } http2Session = null; @@ -599,24 +592,13 @@ public class Clip2Bridge implements Closeable { * @param cause the exception that caused the error. */ private synchronized void fatalError(Object listener, Http2Exception cause) { - if (externalRestartScheduled || internalRestartScheduled || onlineState == State.CLOSED || closing) { + if (onlineState == State.CLOSED || closing) { return; } String causeId = listener.getClass().getSimpleName(); if (listener instanceof ContentStreamListenerAdapter) { // on GET / PUT requests the caller handles errors and closes the stream; the session is still OK LOGGER.debug("fatalError() {} {} ignoring", causeId, cause.error); - } else if (cause.error == Http2Error.GO_AWAY) { - LOGGER.debug("fatalError() {} {} scheduling reconnect", causeId, cause.error); - - // schedule task to open again - internalRestartScheduled = true; - cancelTask(internalRestartTask, false); - internalRestartTask = bridgeHandler.getScheduler().schedule( - () -> internalRestart(onlineState == State.ACTIVE), RESTART_AFTER_SECONDS, TimeUnit.SECONDS); - - // force close immediately to be clean when internalRestart() starts - close2(); } else { if (LOGGER.isDebugEnabled()) { LOGGER.debug("fatalError() {} {} closing", causeId, cause.error, cause); @@ -659,7 +641,6 @@ public class Clip2Bridge implements Closeable { * @throws InterruptedException */ public Resources getResources(ResourceReference reference) throws ApiException, InterruptedException { - sleepDuringRestart(); if (onlineState == State.CLOSED) { throw new ApiException("getResources() offline"); } @@ -735,30 +716,6 @@ public class Clip2Bridge implements Closeable { return Objects.isNull(id) || id.isEmpty() ? url : url + "/" + id; } - /** - * Restart the session. - * - * @param active boolean that selects whether to restart in active or passive mode. - */ - private void internalRestart(boolean active) { - try { - openPassive(); - if (active) { - openActive(); - } - internalRestartScheduled = false; - } catch (ApiException e) { - if (LOGGER.isDebugEnabled()) { - LOGGER.debug("internalRestart() failed", e); - } else { - LOGGER.warn("Scheduled reconnection task failed."); - } - internalRestartScheduled = false; - close2(); - } catch (InterruptedException e) { - } - } - /** * The event stream calls this method when it has received text data. It parses the text as JSON into a list of * Event entries, converts the list of events to a list of resources, and forwards that list to the bridge @@ -892,11 +849,6 @@ public class Clip2Bridge implements Closeable { synchronized (this) { LOGGER.debug("openPassive()"); onlineState = State.CLOSED; - try { - http2Client.start(); - } catch (Exception e) { - throw new ApiException("Error starting HTTP/2 client", e); - } openSession(); openCheckAliveTask(); onlineState = State.PASSIVE; @@ -965,14 +917,14 @@ public class Clip2Bridge implements Closeable { } /** - * Use an HTTP/2 PUT command to send a resource to the server. + * Use an HTTP/2 PUT command to send a resource to the server. Note: the Hue bridge server can sometimes get + * confused by parallel PUT commands, so use 'synchronized' to prevent that. * * @param resource the resource to put. * @throws ApiException if something fails. * @throws InterruptedException */ - public void putResource(Resource resource) throws ApiException, InterruptedException { - sleepDuringRestart(); + public synchronized void putResource(Resource resource) throws ApiException, InterruptedException { if (onlineState == State.CLOSED) { return; } @@ -1067,32 +1019,6 @@ public class Clip2Bridge implements Closeable { throw new HttpUnauthorizedException("Application key registration failed"); } - public void setExternalRestartScheduled() { - externalRestartScheduled = true; - internalRestartScheduled = false; - cancelTask(internalRestartTask, false); - internalRestartTask = null; - close2(); - } - - /** - * Sleep the caller during any period when the connection is restarting. - * - * @throws ApiException if anything failed. - * @throws InterruptedException - */ - private void sleepDuringRestart() throws ApiException, InterruptedException { - Future restartTask = this.internalRestartTask; - if (Objects.nonNull(restartTask)) { - try { - restartTask.get(RESTART_AFTER_SECONDS * 2, TimeUnit.SECONDS); - } catch (ExecutionException | TimeoutException e) { - throw new ApiException("sleepDuringRestart() error", e); - } - } - internalRestartScheduled = false; - } - /** * Test the Hue Bridge connection state by attempting to connect and trying to execute a basic command that requires * authentication. diff --git a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/dto/clip2/Button.java b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/dto/clip2/Button.java index 7f8da6cc5ac..b6688da2d5b 100644 --- a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/dto/clip2/Button.java +++ b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/dto/clip2/Button.java @@ -12,10 +12,11 @@ */ package org.openhab.binding.hue.internal.dto.clip2; +import java.util.Objects; + import org.eclipse.jdt.annotation.NonNullByDefault; +import org.eclipse.jdt.annotation.Nullable; import org.openhab.binding.hue.internal.dto.clip2.enums.ButtonEventType; -import org.openhab.core.library.types.StringType; -import org.openhab.core.types.State; import com.google.gson.annotations.SerializedName; @@ -26,17 +27,17 @@ import com.google.gson.annotations.SerializedName; */ @NonNullByDefault public class Button { - private @NonNullByDefault({}) @SerializedName("last_event") String lastEvent; + private @Nullable @SerializedName("last_event") String lastEvent; /** * @return the last button event as an enum. - * @throws IllegalArgumentException if lastEvent is bad. + * @throws IllegalArgumentException if lastEvent is null or bad. */ public ButtonEventType getLastEvent() throws IllegalArgumentException { - return ButtonEventType.valueOf(lastEvent.toUpperCase()); - } - - public State getLastEventState() { - return new StringType(getLastEvent().name()); + String lastEvent = this.lastEvent; + if (Objects.nonNull(lastEvent)) { + return ButtonEventType.valueOf(lastEvent.toUpperCase()); + } + throw new IllegalArgumentException("lastEvent field is null"); } } diff --git a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/dto/clip2/LightLevel.java b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/dto/clip2/LightLevel.java index 6d327bb33be..1c975bedb7e 100644 --- a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/dto/clip2/LightLevel.java +++ b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/dto/clip2/LightLevel.java @@ -40,16 +40,15 @@ public class LightLevel { } /** - * Raw sensor light level is '10000 * log10(lux) + 1' so apply the inverse formula to convert to Lux. + * Raw sensor light level formula is '10000 * log10(lux + 1)' so apply the inverse formula to convert back to Lux. + * NOTE: the Philips/Signify API documentation quotes the formula as '10000 * log10(lux) + 1', however this code + * author thinks that that formula is wrong since zero Lux would cause a log10(0) negative infinity overflow! * * @return a QuantityType with light level in Lux, or UNDEF. */ public State getLightLevelState() { if (lightLevelValid) { - double rawLightLevel = lightLevel; - if (rawLightLevel > 1f) { - return new QuantityType<>(Math.pow(10f, (rawLightLevel - 1f) / 10000f), Units.LUX); - } + return new QuantityType<>(Math.pow(10f, (double) lightLevel / 10000f) - 1f, Units.LUX); } return UnDefType.UNDEF; } diff --git a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/dto/clip2/Resource.java b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/dto/clip2/Resource.java index 20dd1e7beb8..44ef31e889e 100644 --- a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/dto/clip2/Resource.java +++ b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/dto/clip2/Resource.java @@ -196,11 +196,6 @@ public class Resource { return UnDefType.NULL; } - public State getButtonLastEventState() { - Button button = this.button; - return Objects.nonNull(button) ? button.getLastEventState() : UnDefType.NULL; - } - public List getChildren() { List children = this.children; return Objects.nonNull(children) ? children : List.of(); diff --git a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2BridgeHandler.java b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2BridgeHandler.java index 31b4a4f69fe..d7903bce6b2 100644 --- a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2BridgeHandler.java +++ b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2BridgeHandler.java @@ -486,7 +486,15 @@ public class Clip2BridgeHandler extends BaseBridgeHandler { String applicationKey = config.applicationKey; applicationKey = Objects.nonNull(applicationKey) ? applicationKey : ""; - clip2Bridge = new Clip2Bridge(httpClientFactory, this, ipAddress, applicationKey); + + try { + clip2Bridge = new Clip2Bridge(httpClientFactory, this, ipAddress, applicationKey); + } catch (ApiException e) { + logger.trace("initializeAssets() communication error on '{}'", ipAddress, e); + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, + "@text/offline.api2.comm-error.exception [\"" + e.getMessage() + "\"]"); + return; + } assetsLoaded = true; } @@ -499,14 +507,9 @@ public class Clip2BridgeHandler extends BaseBridgeHandler { */ public void onConnectionOffline() { if (assetsLoaded) { - try { - getClip2Bridge().setExternalRestartScheduled(); - cancelTask(checkConnectionTask, false); - checkConnectionTask = scheduler.schedule(() -> checkConnection(), RECONNECT_DELAY_SECONDS, - TimeUnit.SECONDS); - } catch (AssetNotLoadedException e) { - // should never occur - } + cancelTask(checkConnectionTask, false); + checkConnectionTask = scheduler.schedule(() -> checkConnection(), RECONNECT_DELAY_SECONDS, + TimeUnit.SECONDS); } } diff --git a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2ThingHandler.java b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2ThingHandler.java index 350e6387c21..00975e55d2c 100644 --- a/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2ThingHandler.java +++ b/bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2ThingHandler.java @@ -783,9 +783,10 @@ public class Clip2ThingHandler extends BaseThingHandler { if (fullUpdate) { addSupportedChannel(CHANNEL_2_BUTTON_LAST_EVENT); controlIds.put(resource.getId(), resource.getControlId()); + } else { + State buttonState = resource.getButtonEventState(controlIds); + updateState(CHANNEL_2_BUTTON_LAST_EVENT, buttonState, fullUpdate); } - State buttonState = resource.getButtonEventState(controlIds); - updateState(CHANNEL_2_BUTTON_LAST_EVENT, buttonState, fullUpdate); break; case DEVICE_POWER: @@ -828,8 +829,9 @@ public class Clip2ThingHandler extends BaseThingHandler { case RELATIVE_ROTARY: if (fullUpdate) { addSupportedChannel(CHANNEL_2_ROTARY_STEPS); + } else { + updateState(CHANNEL_2_ROTARY_STEPS, resource.getRotaryStepsState(), fullUpdate); } - updateState(CHANNEL_2_ROTARY_STEPS, resource.getRotaryStepsState(), fullUpdate); break; case TEMPERATURE: