Hue lan integration tests review

drivers/SmartThings/philips-hue/src/handlers/attribute_emitters.lua

@@ -60,67 +60,84 @@ local function _emit_light_events_inner(light_device, light_repr)
       if light_repr.color_temperature.mirek_valid then
         mirek = light_repr.color_temperature.mirek
       end
-      local mirek_schema = light_repr.color_temperature.mirek_schema or {
-        mirek_minimum = Consts.DEFAULT_MIN_MIREK,
-        mirek_maximum = Consts.DEFAULT_MAX_MIREK
-      }
-
       -- See note in `src/handlers/lifecycle_handlers/light.lua` about min/max relationship
       -- if the below is not intuitive.
       local color_temp_range = light_device:get_latest_state("main", capabilities.colorTemperature.ID, capabilities.colorTemperature.colorTemperatureRange.NAME);
       local min_kelvin = (color_temp_range and color_temp_range.minimum)
-      local api_min_kelvin = math.floor(utils.mirek_to_kelvin(mirek_schema.mirek_maximum, Consts.KELVIN_STEP_SIZE) or Consts.MIN_TEMP_KELVIN_COLOR_AMBIANCE)
       local max_kelvin = (color_temp_range and color_temp_range.maximum)
-      local api_max_kelvin = math.floor(utils.mirek_to_kelvin(mirek_schema.mirek_minimum, Consts.KELVIN_STEP_SIZE) or Consts.MAX_TEMP_KELVIN)
 
-      local update_range = false
-      if min_kelvin ~= api_min_kelvin then
-        update_range = true
-        min_kelvin = api_min_kelvin
-        light_device:set_field(Fields.MIN_KELVIN, min_kelvin, { persist = true })
-      end
+      -- Updates (e.g. over SSE) usually carry no `mirek_schema`; keep the range learned from the
+      -- full resource rather than resetting it to the defaults.
+      local mirek_schema = light_repr.color_temperature.mirek_schema
+      if mirek_schema or not (min_kelvin and max_kelvin) then
+        mirek_schema = mirek_schema or {
+          mirek_minimum = Consts.DEFAULT_MIN_MIREK,
+          mirek_maximum = Consts.DEFAULT_MAX_MIREK
+        }
+        local api_min_kelvin = math.floor(utils.mirek_to_kelvin(mirek_schema.mirek_maximum, Consts.KELVIN_STEP_SIZE) or Consts.MIN_TEMP_KELVIN_COLOR_AMBIANCE)
+        local api_max_kelvin = math.floor(utils.mirek_to_kelvin(mirek_schema.mirek_minimum, Consts.KELVIN_STEP_SIZE) or Consts.MAX_TEMP_KELVIN)
+
+        local update_range = false
+        if min_kelvin ~= api_min_kelvin then
+          update_range = true
+          min_kelvin = api_min_kelvin
+          light_device:set_field(Fields.MIN_KELVIN, min_kelvin, { persist = true })
+        end
 
-      if max_kelvin ~= api_max_kelvin then
-        update_range = true
-        max_kelvin = api_max_kelvin
-        light_device:set_field(Fields.MAX_KELVIN, max_kelvin, { persist = true })
-      end
+        if max_kelvin ~= api_max_kelvin then
+          update_range = true
+          max_kelvin = api_max_kelvin
+          light_device:set_field(Fields.MAX_KELVIN, max_kelvin, { persist = true })
+        end
 
-      if update_range then
-        light_device.log.debug(st_utils.stringify_table({ minimum = min_kelvin, maximum = max_kelvin }, "updating color temp range"));
-        light_device:emit_event(capabilities.colorTemperature.colorTemperatureRange({ minimum = min_kelvin, maximum = max_kelvin }))
-      else
-        light_device.log.debug(st_utils.stringify_table(color_temp_range, "color temp range unchanged"));
+        if update_range then
+          light_device.log.debug(st_utils.stringify_table({ minimum = min_kelvin, maximum = max_kelvin }, "updating color temp range"));
+          light_device:emit_event(capabilities.colorTemperature.colorTemperatureRange({ minimum = min_kelvin, maximum = max_kelvin }))
+        else
+          light_device.log.debug(st_utils.stringify_table(color_temp_range, "color temp range unchanged"));
+        end
       end
 
-      -- local min =  or Consts.MIN_TEMP_KELVIN_WHITE_AMBIANCE
-      local kelvin = math.floor(
-        st_utils.clamp_value(utils.mirek_to_kelvin(mirek), min_kelvin, max_kelvin)
-      )
-      if utils.is_nan(kelvin) then
-        light_device.log.warn(
-          string.format(
-            "Non numeric value %s computed for colorTemperature Attribute Event, ignoring.",
-            kelvin
-          )
-        )
+      -- In xy colour mode the bridge reports `mirek_valid = false` (and `mirek = null`); there is
+      -- no colour temperature to report then.
+      if light_repr.color_temperature.mirek_valid == false then
+        light_device.log.debug("Light is in colour mode, not emitting a colour temperature")
       else
-        local last_kelvin_setpoint = light_device:get_field(Fields.COLOR_TEMP_SETPOINT)
-        if
-          last_kelvin_setpoint ~= nil and
-          last_kelvin_setpoint >= utils.mirek_to_kelvin(mirek + 1) and
-          last_kelvin_setpoint <= utils.mirek_to_kelvin(mirek - 1)
-        then
-          kelvin = last_kelvin_setpoint;
+        -- local min =  or Consts.MIN_TEMP_KELVIN_WHITE_AMBIANCE
+        local kelvin = math.floor(
+          st_utils.clamp_value(utils.mirek_to_kelvin(mirek), min_kelvin, max_kelvin)
+        )
+        if utils.is_nan(kelvin) then
+          light_device.log.warn(
+            string.format(
+              "Non numeric value %s computed for colorTemperature Attribute Event, ignoring.",
+              kelvin
+            )
+          )
+        else
+          local last_kelvin_setpoint = light_device:get_field(Fields.COLOR_TEMP_SETPOINT)
+          if
+            last_kelvin_setpoint ~= nil and
+            last_kelvin_setpoint >= utils.mirek_to_kelvin(mirek + 1) and
+            last_kelvin_setpoint <= utils.mirek_to_kelvin(mirek - 1)
+          then
+            kelvin = last_kelvin_setpoint;
+          end
+          light_device:emit_event(capabilities.colorTemperature.colorTemperature(kelvin))
         end
-        light_device:emit_event(capabilities.colorTemperature.colorTemperature(kelvin))
       end
       light_device:set_field(Fields.COLOR_TEMP_SETPOINT, nil);
     end
 
     if light_repr.color then
-      light_device:set_field(Fields.GAMUT, light_repr.color.gamut, { persist = true })
-      local r, g, b = HueColorUtils.safe_xy_to_rgb(light_repr.color.xy, light_repr.color.gamut)
+      -- Updates (e.g. over SSE) usually carry only `xy`; keep the gamut learned from the full resource.
+      local gamut = light_repr.color.gamut
+      if gamut ~= nil then
+        light_device:set_field(Fields.GAMUT, gamut, { persist = true })
+      else
+        gamut = light_device:get_field(Fields.GAMUT)
+      end
+      local r, g, b = HueColorUtils.safe_xy_to_rgb(light_repr.color.xy, gamut)
       local hue, sat, _ = st_utils.rgb_to_hsv(r, g, b)
       -- We sent a command where hue == 100 and wrapped the value to 0, reverse that here
       if light_device:get_field(Fields.WRAPPED_HUE) == true and (hue + .05 >= 1 or hue - .05 <= 0) then

drivers/SmartThings/philips-hue/src/handlers/grouped_light_commands.lua

@@ -11,6 +11,23 @@ local utils = require "utils"
 ---@class GroupedLightCommandHandlers
 local GroupedLightCommandHandlers = {}
 
+--- Log the error from a failed request, and any errors the bridge returned in its response.
+---@param resp table?
+---@param err string?
+---@param action_desc string
+local function log_response_errors(resp, err, action_desc)
+  if err ~= nil then
+    log.error_with({ hub_logs = true }, "Error performing " .. action_desc .. ": " .. err)
+  end
+
+  if resp and resp.errors and #resp.errors > 0 then
+    for _, error in ipairs(resp.errors) do
+      log.error_with({ hub_logs = true },
+        "Error returned in Hue response for " .. action_desc .. ": " .. tostring(error.description))
+    end
+  end
+end
+
 ---@param driver HueDriver
 ---@param bridge_device HueBridgeDevice
 ---@param group table
@@ -33,15 +50,7 @@ local function do_switch_action(driver, bridge_device, group, args)
   end
 
   local resp, err = hue_api:set_grouped_light_on_state(grouped_light_id, on)
-  if not resp or (resp.errors and #resp.errors == 0) then
-    if err ~= nil then
-      log.error_with({ hub_logs = true }, "Error performing on/off action: " .. err)
-    elseif resp and #resp.errors > 0 then
-      for _, error in ipairs(resp.errors) do
-        log.error_with({ hub_logs = true }, "Error returned in Hue response: " .. error.description)
-      end
-    end
-  end
+  log_response_errors(resp, err, "on/off action")
 end
 
 ---@param driver HueDriver
@@ -65,29 +74,22 @@ local function do_switch_level_action(driver, bridge_device, group, args)
     return
   end
 
-  -- An individual command checks the state of the device before doing this.
-  -- It is probably not worth iterating through all the devices to check their state.
-  local resp, err = hue_api:set_grouped_light_on_state(grouped_light_id, true)
-  if not resp or (resp.errors and #resp.errors == 0) then
-    if err ~= nil then
-      log.error_with({ hub_logs = true }, "Error performing on/off action: " .. err)
-    elseif resp and #resp.errors > 0 then
-      for _, error in ipairs(resp.errors) do
-        log.error_with({ hub_logs = true }, "Error returned in Hue response: " .. error.description)
-      end
+  -- Only turn the group on first if one of its lights is off. A light whose state hasn't been
+  -- reported yet counts as off: setting the brightness of a light that is off doesn't turn it on.
+  local any_off = false
+  for _, device in ipairs(group.devices or {}) do
+    if device:get_field(Fields.SWITCH_STATE) ~= "on" then
+      any_off = true
+      break
     end
   end
+  if any_off then
+    local resp, err = hue_api:set_grouped_light_on_state(grouped_light_id, true)
+    log_response_errors(resp, err, "on/off action")
+  end
 
   local resp, err = hue_api:set_grouped_light_level(grouped_light_id, level)
-  if not resp or (resp.errors and #resp.errors == 0) then
-    if err ~= nil then
-      log.error_with({ hub_logs = true }, "Error performing switch level action: " .. err)
-    elseif resp and #resp.errors > 0 then
-      for _, error in ipairs(resp.errors) do
-        log.error_with({ hub_logs = true }, "Error returned in Hue response: " .. error.description)
-      end
-    end
-  end
+  log_response_errors(resp, err, "switch level action")
 end
 
 
@@ -121,15 +123,7 @@ local function do_color_action(driver, bridge_device, group, args, aux)
   local xy = HueColorUtils.safe_rgb_to_xy(red, green, blue, aux[Fields.GAMUT])
 
   local resp, err = hue_api:set_grouped_light_color_xy(grouped_light_id, xy)
-  if not resp or (resp.errors and #resp.errors == 0) then
-    if err ~= nil then
-      log.error_with({ hub_logs = true }, "Error performing color action: " .. err)
-    elseif resp and #resp.errors > 0 then
-      for _, error in ipairs(resp.errors) do
-        log.error_with({ hub_logs = true }, "Error returned in Hue response: " .. error.description)
-      end
-    end
-  end
+  log_response_errors(resp, err, "color action")
 end
 
 ---@param driver HueDriver
@@ -192,15 +186,7 @@ local function do_color_temp_action(driver, bridge_device, group, args, aux)
 
   local resp, err = hue_api:set_grouped_light_color_temp(grouped_light_id, mirek)
 
-  if not resp or (resp.errors and #resp.errors == 0) then
-    if err ~= nil then
-      log.error_with({ hub_logs = true }, "Error performing color temp action: " .. err)
-    elseif resp and #resp.errors > 0 then
-      for _, error in ipairs(resp.errors) do
-        log.error_with({ hub_logs = true }, "Error returned in Hue response: " .. error.description)
-      end
-    end
-  end
+  log_response_errors(resp, err, "color temp action")
   grouped_utils.set_field_on_group_devices(group, Fields.COLOR_TEMP_SETPOINT, clamped_kelvin);
 end
 

drivers/SmartThings/philips-hue/src/handlers/lifecycle_handlers/light.lua

@@ -227,6 +227,9 @@ function LightLifecycleHandlers.init(driver, device)
   if not svc_rids_for_device[device_light_resource_id] then
     svc_rids_for_device[device_light_resource_id] = HueDeviceTypes.LIGHT
   end
+  if hue_device_id ~= nil then
+    driver.services_for_device_rid[hue_device_id] = svc_rids_for_device
+  end
   device:set_field(Fields._INIT, true, { persist = false })
   device:emit_event(capabilities.switchLevel.levelRange({ minimum = 1, maximum = 100 }))
 

drivers/SmartThings/philips-hue/src/test/fixtures/capabilities/samsungim.hueSyncMode.json

@@ -0,0 +1,30 @@
+{
+    "id": "samsungim.hueSyncMode",
+    "version": 1,
+    "status": "proposed",
+    "name": "Hue Sync Mode",
+    "ephemeral": false,
+    "attributes": {
+        "mode": {
+            "schema": {
+                "type": "object",
+                "properties": {
+                    "value": {
+                        "type": "string",
+                        "title": "Hue Sync mode",
+                        "enum": [
+                            "normal",
+                            "streaming"
+                        ]
+                    }
+                },
+                "additionalProperties": false,
+                "required": [
+                    "value"
+                ]
+            },
+            "enumCommands": []
+        }
+    },
+    "commands": {}
+}