Skip to content

Matter Switch move button and switch initialization to doConfigure #2041

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Matter Switch move button and switch initialization to doConfigure #2041

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
146 changes: 77 additions & 69 deletions drivers/SmartThings/matter-switch/src/init.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,19 +44,12 @@ local SWITCH_LEVEL_LIGHTING_MIN = 1
local CURRENT_HUESAT_ATTR_MIN = 0
local CURRENT_HUESAT_ATTR_MAX = 254

local SWITCH_INITIALIZED = "__switch_intialized"
-- COMPONENT_TO_ENDPOINT_MAP is here only to preserve the endpoint mapping for
-- COMPONENT_TO_ENDPOINT_MAP is here to preserve the endpoint mapping for
-- devices that were joined to this driver as MCD devices before the transition
-- to join all matter-switch devices as parent-child. This value will only exist
-- in the device table for devices that joined prior to this transition, and it
-- will not be set for new devices.
-- to join switch devices as parent-child. This value will exist in the device
-- table for devices that joined prior to this transition, and is also used for
-- button devices that require component mapping.
local COMPONENT_TO_ENDPOINT_MAP = "__component_to_endpoint_map"
-- COMPONENT_TO_ENDPOINT_MAP_BUTTON is for devices with button endpoints, to
-- preserve the MCD functionality for button devices from the matter-button
-- driver after it was merged into the matter-switch driver. Note that devices
-- containing both button endpoints and switch endpoints will use this field
-- rather than COMPONENT_TO_ENDPOINT_MAP.
local COMPONENT_TO_ENDPOINT_MAP_BUTTON = "__component_to_endpoint_map_button"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What as the original intent behind using a different field specifically for the button MCD devices? Was it because of the check in init that essentially bypassed MCD devices? That sounds right to me but I just want to make sure there wasn't another intent

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's right. We didn't want to mess with the original field because there are probably still old switch MCD devices out there that we didn't want to re-configure as parent-child. But if the init logic is in doConfigure then we wouldn't need to worry about that.

local ENERGY_MANAGEMENT_ENDPOINT = "__energy_management_endpoint"
local IS_PARENT_CHILD_DEVICE = "__is_parent_child_device"
local COLOR_TEMP_BOUND_RECEIVED_KELVIN = "__colorTemp_bound_received_kelvin"
Expand All @@ -67,6 +60,12 @@ local LEVEL_BOUND_RECEIVED = "__level_bound_received"
local LEVEL_MIN = "__level_min"
local LEVEL_MAX = "__level_max"
local COLOR_MODE = "__color_mode"

local updated_fields = {
{ field_name = "__component_to_endpoint_map_button", updated_field_name = COMPONENT_TO_ENDPOINT_MAP },
{ field_name = "__switch_intialized", updated_field_name = nil }
}

local HUE_SAT_COLOR_MODE = clusters.ColorControl.types.ColorMode.CURRENT_HUE_AND_CURRENT_SATURATION
local X_Y_COLOR_MODE = clusters.ColorControl.types.ColorMode.CURRENTX_AND_CURRENTY

Expand Down Expand Up @@ -292,8 +291,6 @@ local HELD_THRESHOLD = 1
-- this is the number of buttons for which we have a static profile already made
local STATIC_BUTTON_PROFILE_SUPPORTED = {1, 2, 3, 4, 5, 6, 7, 8}

local BUTTON_DEVICE_PROFILED = "__button_device_profiled"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should also set this field to nil since we have that handy function to remove these en masse

Copy link
Contributor Author

@nickolas-deboom nickolas-deboom Apr 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field was not persisted so we should be good there 👍


-- Some switches will send a MultiPressComplete event as part of a long press sequence. Normally the driver will create a
-- button capability event on receipt of MultiPressComplete, but in this case that would result in an extra event because
-- the "held" capability event is generated when the LongPress event is received. The IGNORE_NEXT_MPC flag is used
Expand Down Expand Up @@ -406,6 +403,7 @@ local function device_type_supports_button_switch_combination(device, endpoint_i
end

local function get_first_non_zero_endpoint(endpoints)
table.sort(endpoints)
for _,ep in ipairs(endpoints) do
if ep ~= 0 then -- 0 is the matter RootNode endpoint
return ep
Expand All @@ -425,8 +423,6 @@ local function find_default_endpoint(device)

local switch_eps = device:get_endpoints(clusters.OnOff.ID)
local button_eps = device:get_endpoints(clusters.Switch.ID, {feature_bitmap=clusters.Switch.types.SwitchFeature.MOMENTARY_SWITCH})
table.sort(switch_eps)
table.sort(button_eps)

-- Return the first switch endpoint as the default endpoint if no button endpoints are present
if #button_eps == 0 and #switch_eps > 0 then
Expand Down Expand Up @@ -456,15 +452,15 @@ local function find_default_endpoint(device)
end

local function component_to_endpoint(device, component)
local map = device:get_field(COMPONENT_TO_ENDPOINT_MAP_BUTTON) or device:get_field(COMPONENT_TO_ENDPOINT_MAP) or {}
local map = device:get_field(COMPONENT_TO_ENDPOINT_MAP) or {}
if map[component] then
return map[component]
end
return find_default_endpoint(device)
end

local function endpoint_to_component(device, ep)
local map = device:get_field(COMPONENT_TO_ENDPOINT_MAP_BUTTON) or device:get_field(COMPONENT_TO_ENDPOINT_MAP) or {}
local map = device:get_field(COMPONENT_TO_ENDPOINT_MAP) or {}
for component, endpoint in pairs(map) do
if endpoint == ep then
return component
Expand All @@ -473,6 +469,17 @@ local function endpoint_to_component(device, ep)
return "main"
end

local function check_field_name_updates(device)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a very critical function. It definitely looks fine to me, but it may be best to make this functionality as explicit as possible. I think being able to reason about this very clearly is important.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think there is any risk that the command to copy the old field to the new field could fail, and then the next line would set the old field to nil? I kind of doubt that it could happen but I suppose it's possible

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose maybe that could happen? but a driver crash wouldn't cause that, so like lua itself and the lua runtime would have to fail. That's not something we're really capable of worrying about I don't think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're probably right.

for _, field in ipairs(updated_fields) do
if device:get_field(field.field_name) then
if field.updated_field_name ~= nil then
device:set_field(field.updated_field_name, device:get_field(field.field_name), {persist = true})
end
device:set_field(field.field_name, nil)
end
end
end

local function assign_child_profile(device, child_ep)
local profile

Expand Down Expand Up @@ -512,41 +519,6 @@ local function assign_child_profile(device, child_ep)
return profile or "switch-binary"
end

local function do_configure(driver, device)
if device:get_field(BUTTON_DEVICE_PROFILED) then
return
end
local fan_eps = device:get_endpoints(clusters.FanControl.ID)
local level_eps = device:get_endpoints(clusters.LevelControl.ID)
local energy_eps = embedded_cluster_utils.get_endpoints(device, clusters.ElectricalEnergyMeasurement.ID)
local power_eps = embedded_cluster_utils.get_endpoints(device, clusters.ElectricalPowerMeasurement.ID)
local valve_eps = embedded_cluster_utils.get_endpoints(device, clusters.ValveConfigurationAndControl.ID)
local profile_name = nil
local level_support = ""
if #level_eps > 0 then
level_support = "-level"
end
if #energy_eps > 0 and #power_eps > 0 then
profile_name = "plug" .. level_support .. "-power-energy-powerConsumption"
elseif #energy_eps > 0 then
profile_name = "plug" .. level_support .. "-energy-powerConsumption"
elseif #power_eps > 0 then
profile_name = "plug" .. level_support .. "-power"
elseif #valve_eps > 0 then
profile_name = "water-valve"
if #embedded_cluster_utils.get_endpoints(device, clusters.ValveConfigurationAndControl.ID,
{feature_bitmap = clusters.ValveConfigurationAndControl.types.Feature.LEVEL}) > 0 then
profile_name = profile_name .. "-level"
end
elseif #fan_eps > 0 then
profile_name = "light-color-level-fan"
end

if profile_name then
device:try_update_metadata({ profile = profile_name })
end
end

local function configure_buttons(device)
if device.network_type == device_lib.NETWORK_TYPE_CHILD then
return
Expand Down Expand Up @@ -603,7 +575,7 @@ local function build_button_component_map(device, main_endpoint, button_eps)
component_map[button_component] = ep
end
end
device:set_field(COMPONENT_TO_ENDPOINT_MAP_BUTTON, component_map, {persist = true})
device:set_field(COMPONENT_TO_ENDPOINT_MAP, component_map, {persist = true})
end

local function build_button_profile(device, main_endpoint, num_button_eps)
Expand All @@ -613,13 +585,10 @@ local function build_button_profile(device, main_endpoint, num_button_eps)
end
local battery_supported = #device:get_endpoints(clusters.PowerSource.ID, {feature_bitmap = clusters.PowerSource.types.PowerSourceFeature.BATTERY}) > 0
if battery_supported then -- battery profiles are configured later, in power_source_attribute_list_handler
local attribute_list_read = im.InteractionRequest(im.InteractionRequest.RequestType.READ, {})
attribute_list_read:merge(clusters.PowerSource.attributes.AttributeList:read())
device:send(attribute_list_read)
device:send(clusters.PowerSource.attributes.AttributeList:read(device))
else
device:try_update_metadata({profile = profile_name})
end
device:set_field(BUTTON_DEVICE_PROFILED, true)
end

local function build_child_switch_profiles(driver, device, main_endpoint)
Expand Down Expand Up @@ -683,13 +652,15 @@ local function handle_light_switch_with_onOff_server_clusters(device, main_endpo
end

local function initialize_buttons_and_switches(driver, device, main_endpoint)
local profile_found = false
local button_eps = device:get_endpoints(clusters.Switch.ID, {feature_bitmap=clusters.Switch.types.SwitchFeature.MOMENTARY_SWITCH})
if tbl_contains(STATIC_BUTTON_PROFILE_SUPPORTED, #button_eps) then
build_button_profile(device, main_endpoint, #button_eps)
-- All button endpoints found will be added as additional components in the profile containing the main_endpoint.
-- The resulting endpoint to component map is saved in the COMPONENT_TO_ENDPOINT_MAP_BUTTON field
-- The resulting endpoint to component map is saved in the COMPONENT_TO_ENDPOINT_MAP field
build_button_component_map(device, main_endpoint, button_eps)
configure_buttons(device)
profile_found = true
end

-- Without support for bindings, only clusters that are implemented as server are counted. This count is handled
Expand All @@ -701,9 +672,9 @@ local function initialize_buttons_and_switches(driver, device, main_endpoint)
-- Note: since their device type isn't supported, these devices join as a matter-thing.
if num_switch_server_eps > 0 and detect_matter_thing(device) then
handle_light_switch_with_onOff_server_clusters(device, main_endpoint)
profile_found = true
end

device:set_field(SWITCH_INITIALIZED, true, {persist = true})
return profile_found
end

local function detect_bridge(device)
Expand All @@ -721,20 +692,13 @@ local function device_init(driver, device)
if device.network_type ~= device_lib.NETWORK_TYPE_MATTER then
return
end

check_field_name_updates(device)
device:set_component_to_endpoint_fn(component_to_endpoint)
device:set_endpoint_to_component_fn(endpoint_to_component)

local main_endpoint = find_default_endpoint(device)
if not device:get_field(COMPONENT_TO_ENDPOINT_MAP) and -- this field is only set for old MCD devices. See comments in the field def.
not device:get_field(SWITCH_INITIALIZED) and
not detect_bridge(device) then
-- initialize the main device card with buttons if applicable, and create child devices as needed for multi-switch devices.
initialize_buttons_and_switches(driver, device, main_endpoint)
end
if device:get_field(IS_PARENT_CHILD_DEVICE) then
device:set_find_child(find_child)
end
local main_endpoint = find_default_endpoint(device)
-- ensure subscription to all endpoint attributes- including those mapped to child devices
for _, ep in ipairs(device.endpoints) do
if ep.endpoint_id ~= main_endpoint then
Expand All @@ -756,6 +720,50 @@ local function device_init(driver, device)
device:subscribe()
end

local function do_configure(driver, device)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to put this in doConfigure instead of added? I think that we have effectively treated these functions as nearly interchangable "one-time configuration" functions, but it may be worth setting a precedent with this change since this is our biggest and most used driver. Any thoughts on which lifecycle event we should use for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some calls in device_init that are required to run before initialize_buttons_and_switches (device:set_component_to_endpoint_fn(component_to_endpoint) for example), so I think doConfigure is the best place for this code since it will run after device_init (while added precedes device_init)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further, since doConfigure is tied to whether or not the provisioning state has been changed to provisioned while added is also related to a driver change, doConfigure is the more appropriate place for a "true" one time handling like device profiling.

if detect_bridge(device) then
return
end
local main_endpoint = find_default_endpoint(device)
-- initialize the main device card with buttons if applicable, and create child devices as needed for multi-switch devices.
local profile_found = initialize_buttons_and_switches(driver, device, main_endpoint)
if device:get_field(IS_PARENT_CHILD_DEVICE) then
device:set_find_child(find_child)
end
if profile_found then
return
end

local fan_eps = device:get_endpoints(clusters.FanControl.ID)
local level_eps = device:get_endpoints(clusters.LevelControl.ID)
local energy_eps = embedded_cluster_utils.get_endpoints(device, clusters.ElectricalEnergyMeasurement.ID)
local power_eps = embedded_cluster_utils.get_endpoints(device, clusters.ElectricalPowerMeasurement.ID)
local valve_eps = embedded_cluster_utils.get_endpoints(device, clusters.ValveConfigurationAndControl.ID)
local profile_name = nil
local level_support = ""
if #level_eps > 0 then
level_support = "-level"
end
if #energy_eps > 0 and #power_eps > 0 then
profile_name = "plug" .. level_support .. "-power-energy-powerConsumption"
elseif #energy_eps > 0 then
profile_name = "plug" .. level_support .. "-energy-powerConsumption"
elseif #power_eps > 0 then
profile_name = "plug" .. level_support .. "-power"
elseif #valve_eps > 0 then
profile_name = "water-valve"
if #embedded_cluster_utils.get_endpoints(device, clusters.ValveConfigurationAndControl.ID,
{feature_bitmap = clusters.ValveConfigurationAndControl.types.Feature.LEVEL}) > 0 then
profile_name = profile_name .. "-level"
end
elseif #fan_eps > 0 then
profile_name = "light-color-level-fan"
end
if profile_name then
device:try_update_metadata({ profile = profile_name })
end
end

local function device_removed(driver, device)
log.info("device removed")
delete_import_poll_schedule(device)
Expand Down
Loading
Loading