Addresses a handful of bugs with the new device types

- Fixes an issue where compound devices wouldn't be labeled properly - Fixes an issue where the stray device task was scanning a bridge every time it onboarded, even if there were no strays - Fixes an issue where refresh on devices could fail early in driver start by accidentally passing in an API Key string instead of an API Rest Client instance to a function - Fixes an issue where multi-service devices (Buttons, Motion Sensors, Contact Sensors) could sometimes fail to emit attributes if their init handler ran before their parent bridge's network connection was open. Move `battery` above refresh in profile for motion Remove debug logic from tamper emit Remove unused Rooms API calls Replace strings with consts in API methods Add missing early return in button attribute handler Early return in device discovery for unsupported buttons Move debug logs to correct function Update category for single button profile Fix duped line from bad rerere rebase Finish SSE event stream generalizations Tidy up docs/logs that still assume lights-only Generalize stray device handling Normalize module structures
SmartThingsCommunity · May 28, 2024 · 05ffd46 · 05ffd46
1 parent a3be7e3
commit 05ffd46
Show file tree

Hide file tree

Showing 31 changed files with 525 additions and 522 deletions.
diff --git a/drivers/SmartThings/philips-hue/PROGRESS.md b/drivers/SmartThings/philips-hue/PROGRESS.md
@@ -8,35 +8,35 @@ Tracking TODOs for this refactor in this file; this is mostly to allow for creat
 
 #### Tasks
 
-- [x] Convert supported resources map to a map of handlers instead of boolean ✅ 2024-04-16
-- [x] Move handlers to their own file ✅ 2024-04-16
-- [x] Rename the "light_state_disco_cache" key to be service type agnostic ✅ 2024-04-16
-- [x] Update the `discovered_device_callback` function to allow for other device types ✅ 2024-04-16
+- [x] Convert supported resources map to a map of handlers instead of boolean
+- [x] Move handlers to their own file
+- [x] Rename the "light_state_disco_cache" key to be service type agnostic
+- [x] Update the `discovered_device_callback` function to allow for other device types
 
 ### Capability Handlers
 
 #### Tasks
 
-- [x] Create a table of handlers for dispatching refreshes by device type ✅ 2024-04-17
-- [x] Fix `refresh_all_for_bridge` to remove assumptions that all child devices are lights ✅ 2024-04-17
+- [x] Create a table of handlers for dispatching refreshes by device type
+- [x] Fix `refresh_all_for_bridge` to remove assumptions that all child devices are lights
 
 ### Driver (init.lua) Refactors
 
 #### Tasks
 
-- [x] Extract lifecycle handlers to their own module(s) ✅ 2024-04-22
-  - [x] 2024-04-18 Update: Initial code review missed that `is_*_bridge` and `is_*_light` calls in `utils` were implemented such that the check for light was based on failing the check for bridge. So those need to be fixed as well.  ✅ 2024-04-22
-- [x] Extract attribute event emitters to their own module(s) ✅ 2024-04-17
-- [ ] Refactor Stray Light Handler to be a general Stray Device Handler
-- [ ] Refactor SSE `onmessage` callback to remove light-specific assumptions
-  - [ ] `update` messages are hard coded to emit light events with no checks
-  - [ ] `add` message handling rejects non-light devices instead of being written to be extensible
+- [x] Extract lifecycle handlers to their own module(s)
+  - [x] 2024-04-18 Update: Initial code review missed that `is_*_bridge` and `is_*_light` calls in `utils` were implemented such that the check for light was based on failing the check for bridge. So those need to be fixed as well.
+- [x] Extract attribute event emitters to their own module(s)
+- [x] Refactor Stray Light Handler to be a general Stray Device Handler
+- [x] Refactor SSE `onmessage` callback to remove light-specific assumptions
+  - [x] `update` messages are hard coded to emit light events with no checks
+  - [x] `add` message handling rejects non-light devices instead of being written to be extensible
 
 ### Miscellaneous/Custodial
 
 #### Tasks
 
 - [ ] Refactor fresh handlers to be a single generic refresh handler, which is only possible once all of the above is complete.
-- [ ] Update all doc strings that claim we only support bridges and lights
-- [ ] Update any dangling utility methods/variables/symbols that use "light" when they should use "device"
-- [ ] Normalize modules to all use `<dir>/init.lua` instead of `<mod>.lua` as a sibling to `<dir>`.
+- [x] Update all doc strings that claim we only support bridges and lights
+- [x] Update any dangling utility methods/variables/symbols that use "light" when they should use "device"
+- [x] Normalize modules to all use `<dir>/init.lua` instead of `<mod>.lua` as a sibling to `<dir>`.
diff --git a/drivers/SmartThings/philips-hue/profiles/motion-sensor.yml b/drivers/SmartThings/philips-hue/profiles/motion-sensor.yml
@@ -4,12 +4,12 @@ components:
   capabilities:
   - id: motionSensor
     version: 1
-  - id: battery
-    version: 1
   - id: illuminanceMeasurement
     version: 1
   - id: temperatureMeasurement
     version: 1
+  - id: battery
+    version: 1
   - id: refresh
     version: 1
   categories:

diff --git a/drivers/SmartThings/philips-hue/profiles/single-button.yml b/drivers/SmartThings/philips-hue/profiles/single-button.yml
@@ -9,4 +9,4 @@ components:
       - id: refresh
         version: 1
     categories:
-    - name: RemoteController
+    - name: Button
diff --git a/drivers/SmartThings/philips-hue/src/disco/button.lua b/drivers/SmartThings/philips-hue/src/disco/button.lua
@@ -7,6 +7,7 @@ local HueDeviceTypes = require "hue_device_types"
 ---@class DiscoveredButtonHandler: DiscoveredChildDeviceHandler
 local M = {}
 
+-- TODO This should be generalizable to all "sensors", including buttons.
 ---@param driver HueDriver
 ---@param api_instance PhilipsHueApi
 ---@param device_service_info HueDeviceInfo
@@ -15,6 +16,7 @@ local M = {}
 ---@return table<string,any>? description nil on error
 ---@return string? err nil on success
 local function _do_update(driver, api_instance, device_service_info, bridge_network_id, cache)
+  log.debug("------------ _do_update")
   local rid_by_rtype = {}
   local button_services = {}
   local num_buttons = 0
@@ -34,7 +36,8 @@ local function _do_update(driver, api_instance, device_service_info, bridge_netw
     parent_device_id = bridge_device.id,
     hue_device_id = device_service_info.id,
     hue_device_data = device_service_info,
-    num_buttons = num_buttons
+    num_buttons = num_buttons,
+    sensor_list = { power_id = HueDeviceTypes.DEVICE_POWER }
   }
 
   for _, button_rid in ipairs(button_services) do
@@ -51,6 +54,8 @@ local function _do_update(driver, api_instance, device_service_info, bridge_netw
       if control_id == 1 then
         button_remote_description.id = button_repr.data[1].id
       end
+
+      button_remote_description.sensor_list[button_id_key] = HueDeviceTypes.BUTTON
     end
   end
 
@@ -87,7 +92,6 @@ function M.update_state_for_all_device_services(driver, api_instance, device_ser
     return
   end
 
-  log.debug("------------ _do_update")
   return _do_update(driver, api_instance, device_service_info.data[1], bridge_network_id, cache)
 end
 
@@ -122,7 +126,7 @@ function M.handle_discovered_device(
       return
     end
 
-    local button_profile_ref = ""
+    local button_profile_ref
     -- For Philips Hue Smart Button or single switch In-Wall Switch module which contains only 1 button
     if button_description.num_buttons == 1 then
       button_profile_ref = "single-button"
@@ -132,6 +136,15 @@ function M.handle_discovered_device(
     -- For Philips Hue Dimmer Remote and Tap Dial, which contains 4 buttons
     elseif button_description.num_buttons == 4 then
       button_profile_ref = "4-button-remote"
+    else
+      log.error(
+        string.format(
+          "Do not currently have a profile for device %s with %s buttons, skipping device create",
+          device_service_info.metadata.name,
+          button_description.num_buttons
+        )
+      )
+      return
     end
 
     local bridge_device = driver:get_device_by_dni(bridge_network_id) or {}

diff --git a/drivers/SmartThings/philips-hue/src/disco/contact.lua b/drivers/SmartThings/philips-hue/src/disco/contact.lua
@@ -1,4 +1,4 @@
-local log = require "log"
+local log = require "logjam"
 local socket = require "cosock".socket
 local st_utils = require "st.utils"
 
@@ -7,6 +7,7 @@ local HueDeviceTypes = require "hue_device_types"
 ---@class DiscoveredContactSensorHandler: DiscoveredChildDeviceHandler
 local M = {}
 
+-- TODO This should be generalizable to all "sensors", including buttons.
 ---@param driver HueDriver
 ---@param api_instance PhilipsHueApi
 ---@param device_service_info HueDeviceInfo
@@ -15,6 +16,7 @@ local M = {}
 ---@return table<string,any>? description nil on error
 ---@return string? err nil on success
 local function _do_update(driver, api_instance, device_service_info, bridge_network_id, cache)
+  log.debug("------------ _do_update")
   local rid_by_rtype = {}
   for _, svc in ipairs(device_service_info.services) do
     rid_by_rtype[svc.rtype] = svc.rid
@@ -55,6 +57,12 @@ local function _do_update(driver, api_instance, device_service_info, bridge_netw
     contact_sensor_description.tamper_reports = tamper.data[1].tamper_reports
   end
 
+  contact_sensor_description.sensor_list = {
+    id = HueDeviceTypes.CONTACT,
+    power_id = HueDeviceTypes.DEVICE_POWER,
+    tamper_id = HueDeviceTypes.TAMPER
+  }
+
   if type(cache) == "table" then
     cache[resource_id] = contact_sensor_description
     if device_service_info.id_v1 then
@@ -80,7 +88,6 @@ function M.update_state_for_all_device_services(driver, api_instance, device_ser
     return
   end
 
-  log.debug("------------ _do_update")
   return _do_update(driver, api_instance, device_service_info.data[1], bridge_network_id, cache)
 end
 

diff --git a/drivers/SmartThings/philips-hue/src/disco/init.lua b/drivers/SmartThings/philips-hue/src/disco/init.lua
@@ -47,44 +47,6 @@ local function is_device_service_supported(svc_info)
   return discovered_device_handlers[svc_info.rtype or ""] ~= nil
 end
 
----@param driver HueDriver
----@param bridge_network_id string
----@param primary_services table<HueDeviceTypes,HueServiceInfo[]> array of services that *can* map to device records. Multi-buttons wouldn't do so, but compound lights would.
----@param device_info table
-local function discovered_device_callback(driver, bridge_network_id, primary_services, device_info)
-  local v1_dni = bridge_network_id .. "/" .. (device_info.id_v1 or "UNKNOWN"):gsub("/lights/", "")
-  local primary_service_type = HueDeviceTypes.determine_main_service_rtype(device_info, primary_services)
-  if not primary_service_type then
-    log.error(
-      string.format(
-        "Couldn't determine primary service type for device %s, unable to join",
-        (device_info.metadata.name)
-      )
-    )
-    return
-  end
-
-  for _, svc_info in ipairs(primary_services[primary_service_type]) do
-    local v2_resource_id = svc_info.rid or ""
-    if driver:get_device_by_dni(v1_dni) or driver.hue_identifier_to_device_record[v2_resource_id] then return end
-  end
-
-  local api_instance = HueDiscovery.disco_api_instances[bridge_network_id]
-  if not api_instance then
-    log.warn("No API instance for bridge_network_id ", bridge_network_id)
-    return
-  end
-
-  HueDiscovery.handle_discovered_child_device(
-    driver,
-    primary_service_type,
-    bridge_network_id,
-    api_instance,
-    primary_services,
-    device_info
-  )
-end
-
 -- "forward declarations"
 ---@param driver HueDriver
 ---@param bridge_ip string
@@ -114,9 +76,7 @@ local function discovered_bridge_callback(driver, bridge_ip, bridge_network_id)
       driver,
       bridge_network_id,
       HueDiscovery.disco_api_instances[bridge_network_id],
-      function(hue_driver, svc_info, device_info)
-        discovered_device_callback(hue_driver, bridge_network_id, svc_info, device_info)
-      end,
+      HueDiscovery.handle_discovered_child_device,
       "[Discovery: " ..
       (known_bridge_device.label or bridge_network_id or known_bridge_device.id or "unknown bridge") ..
       " bridge scan]"
@@ -244,9 +204,7 @@ function HueDiscovery.scan_bridge_and_update_devices(driver, bridge_network_id)
       driver,
       bridge_network_id,
       HueDiscovery.disco_api_instances[bridge_network_id],
-      function(hue_driver, svc_info, device_info)
-        discovered_device_callback(hue_driver, bridge_network_id, svc_info, device_info)
-      end,
+      HueDiscovery.handle_discovered_child_device,
       "[Discovery: " ..
       (known_bridge_device.label or bridge_network_id or known_bridge_device.id or "unknown bridge") ..
       " bridge re-scan]",
@@ -258,7 +216,7 @@ end
 ---@param driver HueDriver
 ---@param bridge_network_id string
 ---@param api_instance PhilipsHueApi
----@param callback fun(driver: HueDriver, svc_info: HueServiceInfo, device_data: table)
+---@param callback fun(driver: HueDriver, bridge_network_id: string, primary_services: table<HueDeviceTypes,HueServiceInfo[]>, device_data: table)
 ---@param log_prefix string?
 ---@param do_delete boolean?
 function HueDiscovery.search_bridge_for_supported_devices(driver, bridge_network_id, api_instance, callback, log_prefix, do_delete)
@@ -282,41 +240,8 @@ function HueDiscovery.search_bridge_for_supported_devices(driver, bridge_network
 
   local device_is_joined_to_bridge = {}
   for _, device_data in ipairs(devices.data or {}) do
-    local primary_device_services = {}
-    for _,
-    svc_info in ipairs(device_data.services or {}) do
-      if is_device_service_supported(svc_info) then
-        driver.services_for_device_rid[device_data.id] = driver.services_for_device_rid[device_data.id] or {}
-        driver.services_for_device_rid[device_data.id][svc_info.rid] = svc_info.rtype
-        if HueDeviceTypes.can_join_device_for_service(svc_info.rtype) then
-          local services_for_type = primary_device_services[svc_info.rtype] or {}
-          table.insert(services_for_type, svc_info)
-          primary_device_services[svc_info.rtype] = services_for_type
-          device_is_joined_to_bridge[device_data.id] = true
-        end
-      end
-    end
-    if next(primary_device_services) then
-      if type(callback) == "function" then
-        log.info_with(
-          { hub_logs = true },
-          string.format(
-            prefix ..
-            "Processing supported services [%s] for Hue device [v2_id: %s | v1_id: %s], with Hue provided name: %s",
-            st_utils.stringify_table(primary_device_services), device_data.id, device_data.id_v1, device_data.metadata.name
-          )
-        )
-        callback(driver, primary_device_services, device_data)
-      else
-        log.warn(
-          prefix .. "Argument passed in `callback` position for "
-          .. "`HueDiscovery.search_bridge_for_supported_devices` is not a function"
-        )
-      end
-    else
-      log.warn(string.format("No primary services for %s", device_data.metadata.name))
-      log.warn(st_utils.stringify_table(device_data.services, "services", true))
-    end
+    device_is_joined_to_bridge[device_data.id] =
+        HueDiscovery.process_device_service(driver, bridge_network_id, device_data, callback, log_prefix)
   end
 
   if do_delete then
@@ -337,12 +262,87 @@ function HueDiscovery.search_bridge_for_supported_devices(driver, bridge_network
 end
 
 ---@param driver HueDriver
----@param primary_service_type HueDeviceTypes
 ---@param bridge_network_id string
----@param api_instance PhilipsHueApi
+---@param device_data HueDeviceInfo
+---@param callback fun(driver: HueDriver, bridge_network_id: string, primary_services: table<HueDeviceTypes,HueServiceInfo[]>, device_data: table, bridge_device: HueBridgeDevice?)?
+---@param log_prefix string?
+---@param bridge_device HueBridgeDevice?
+---@return boolean device_joined_to_bridge true if device was sent through the join process
+function HueDiscovery.process_device_service(driver, bridge_network_id, device_data, callback, log_prefix, bridge_device)
+  local prefix = ""
+  if type(log_prefix) == "string" and #log_prefix > 0 then prefix = log_prefix .. " " end
+  local primary_device_services = {}
+
+  local device_joined_to_bridge = false
+  for _,
+  svc_info in ipairs(device_data.services or {}) do
+    if is_device_service_supported(svc_info) then
+      driver.services_for_device_rid[device_data.id] = driver.services_for_device_rid[device_data.id] or {}
+      driver.services_for_device_rid[device_data.id][svc_info.rid] = svc_info.rtype
+      if HueDeviceTypes.can_join_device_for_service(svc_info.rtype) then
+        local services_for_type = primary_device_services[svc_info.rtype] or {}
+        table.insert(services_for_type, svc_info)
+        primary_device_services[svc_info.rtype] = services_for_type
+        device_joined_to_bridge = true
+      end
+    end
+  end
+  if next(primary_device_services) then
+    if type(callback) == "function" then
+      log.info_with(
+        { hub_logs = true },
+        string.format(
+          prefix ..
+          "Processing supported services [%s] for Hue device [v2_id: %s | v1_id: %s], with Hue provided name: %s",
+          st_utils.stringify_table(primary_device_services), device_data.id, device_data.id_v1, device_data.metadata.name
+        )
+      )
+      callback(driver, bridge_network_id, primary_device_services, device_data, bridge_device)
+    else
+      log.warn(
+        prefix .. "Argument passed in `callback` position for "
+        .. "`HueDiscovery.search_bridge_for_supported_devices` is not a function"
+      )
+    end
+  else
+    log.warn(string.format("No primary services for %s", device_data.metadata.name))
+    log.warn(st_utils.stringify_table(device_data.services, "services", true))
+  end
+
+  return device_joined_to_bridge
+end
+
+---@param driver HueDriver
+---@param bridge_network_id string
 ---@param primary_services table<HueDeviceTypes,HueServiceInfo[]> array of services that *can* map to device records. Multi-buttons wouldn't do so, but compound lights would.
 ---@param device_info table
-function HueDiscovery.handle_discovered_child_device(driver, primary_service_type, bridge_network_id, api_instance, primary_services, device_info)
+---@param bridge_device HueBridgeDevice? If the parent bridge is known, it can be passed in here
+function HueDiscovery.handle_discovered_child_device(driver, bridge_network_id, primary_services, device_info, bridge_device)
+  local v1_dni = bridge_network_id .. "/" .. (device_info.id_v1 or "UNKNOWN"):gsub("/lights/", "")
+  local primary_service_type = HueDeviceTypes.determine_main_service_rtype(device_info, primary_services)
+  if not primary_service_type then
+    log.error(
+      string.format(
+        "Couldn't determine primary service type for device %s, unable to join",
+        (device_info.metadata.name)
+      )
+    )
+    return
+  end
+
+  for _, svc_info in ipairs(primary_services[primary_service_type]) do
+    local v2_resource_id = svc_info.rid or ""
+    if driver:get_device_by_dni(v1_dni) or driver.hue_identifier_to_device_record[v2_resource_id] then return end
+  end
+
+  local api_instance =
+      (bridge_device and bridge_device:get_field(Fields.BRIDGE_API)) or
+      HueDiscovery.disco_api_instances[bridge_network_id]
+  if not api_instance then
+    log.warn("No API instance for bridge_network_id ", bridge_network_id)
+    return
+  end
+
   discovered_device_handlers[primary_service_type].handle_discovered_device(
     driver,
     bridge_network_id,