[hue] Fix reconnection, parallel commands, trigger channels, and light level formula (#15169)

* [hue] post merge tweaks

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

* [hue] abandon internal restart

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

* [hue] remove externalRestartScheduled flag

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

* [hue] serialize PUT calls

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

* [hue] GET requests shall not activate trigger channels

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

* [hue] fix LightLevel formula

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

* [hue] fix Button DTO null error

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

---------

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

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)                                              |
+| button-last-event     | (String)           | 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)                        |
+| 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)                                                   |

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<Instant> lastRequestTime = Optional.empty();
     private Instant sessionExpireTime = Instant.MAX;
     private @Nullable Session http2Session;
 
     private @Nullable Future<?> checkAliveTask;
-    private @Nullable Future<?> internalRestartTask;
     private Map<Integer, Future<?>> 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
+            boolean notifyHandler = onlineState == State.ACTIVE && !closing;
-                    && !externalRestartScheduled && !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 {
+    public synchronized void putResource(Resource resource) throws ApiException, InterruptedException {
-        sleepDuringRestart();
         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.

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());
+        String lastEvent = this.lastEvent;
-    }
+        if (Objects.nonNull(lastEvent)) {
-
+            return ButtonEventType.valueOf(lastEvent.toUpperCase());
-    public State getLastEventState() {
+        }
-        return new StringType(getLastEvent().name());
+        throw new IllegalArgumentException("lastEvent field is null");
     }
 }

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;
+            return new QuantityType<>(Math.pow(10f, (double) lightLevel / 10000f) - 1f, Units.LUX);
-            if (rawLightLevel > 1f) {
-                return new QuantityType<>(Math.pow(10f, (rawLightLevel - 1f) / 10000f), Units.LUX);
-            }
         }
         return UnDefType.UNDEF;
     }

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<ResourceReference> getChildren() {
         List<ResourceReference> children = this.children;
         return Objects.nonNull(children) ? children : List.of();

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 {
+            cancelTask(checkConnectionTask, false);
-                getClip2Bridge().setExternalRestartScheduled();
+            checkConnectionTask = scheduler.schedule(() -> checkConnection(), RECONNECT_DELAY_SECONDS,
-                cancelTask(checkConnectionTask, false);
+                    TimeUnit.SECONDS);
-                checkConnectionTask = scheduler.schedule(() -> checkConnection(), RECONNECT_DELAY_SECONDS,
-                        TimeUnit.SECONDS);
-            } catch (AssetNotLoadedException e) {
-                // should never occur
-            }
         }
     }
 

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: