Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Update profile selection logic to support plug/button devices #1799

Closed
wants to merge 8 commits into from
Closed

Update profile selection logic to support plug/button devices #1799

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cf4f01f
Add profile for NEO SmartPlug/Button device
nickolas-deboom Dec 5, 2024
859acba
Merge branch 'main' into matter-switch-add-profile-for-button-plug-de…
nickolas-deboom Dec 6, 2024
6a16137
Refactor device_init handling for button/switch initialization
nickolas-deboom Dec 9, 2024
c1ce7d3
Merge branch 'matter-switch-add-profile-for-button-plug-device' of gi…
nickolas-deboom Dec 9, 2024
30c2255
Adding additional profiles
nickolas-deboom Dec 9, 2024
e418979
Merge branch 'main' into matter-switch-add-profile-for-button-plug-de…
nickolas-deboom Dec 9, 2024
894ca3b
Remove plug-button profile, which is now included in a different PR
nickolas-deboom Dec 9, 2024
5e12292
Merge branch 'matter-switch-add-profile-for-button-plug-device' of gi…
nickolas-deboom Dec 9, 2024
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
18 changes: 18 additions & 0 deletions drivers/SmartThings/matter-switch/profiles/plug-button.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
name: plug-button
components:
- id: main
capabilities:
- id: switch
version: 1
- id: firmwareUpdate
version: 1
- id: refresh
version: 1
categories:
- name: SmartPlug
- id: button
capabilities:
- id: button
version: 1
categories:
- name: RemoteController
23 changes: 23 additions & 0 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 @@ -155,6 +155,10 @@ local child_device_profile_overrides = {
{ vendor_id = 0x1321, product_id = 0x000D, child_profile = "switch-binary" },
}

local fingerprint_profile_overrides = {
{ vendor_id = 0x137F, product_id = 0x027B }, -- NEO Power Plug
}

local detect_matter_thing

local CUMULATIVE_REPORTS_NOT_SUPPORTED = "__cumulative_reports_not_supported"
Expand Down Expand Up @@ -500,6 +504,16 @@ local function find_child(parent, ep_id)
return parent:get_child_by_parent_assigned_key(string.format("%d", ep_id))
end

local function check_fingerprint_profile_overrides(device)
for _, fingerprint in ipairs(fingerprint_profile_overrides) do
if device.manufacturer_info.vendor_id == fingerprint.vendor_id and
device.manufacturer_info.product_id == fingerprint.product_id then
return true
end
end
return false
end

local function initialize_switch(driver, 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})
Expand Down Expand Up @@ -571,6 +585,15 @@ local function initialize_switch(driver, device)
device:set_field(COMPONENT_TO_ENDPOINT_MAP_BUTTON, component_map, {persist = true})
end

-- If there is a custom static profile for the device, configure buttons if needed and
-- then return to prevent profile from being updated.
if check_fingerprint_profile_overrides(device) then
if #button_eps > 0 then
configure_buttons(device)
end
return
Copy link
Contributor

Choose a reason for hiding this comment

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

could we make this an if/else block instead? For readability, I would recommend avoiding early returns in large functions like this if they are not at the top of the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had it this way so that it would configure the buttons and then return right afterwards, to skip the profile selection logic that follows.

Copy link
Contributor

@ctowns ctowns Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

To clarify, I meant could we put this entire override profile section be in an if statement, and then the else case will be the part that handles the logic in the rest of the function?

if profile_overide then
    config buttons and skip profile selection
else
    do the rest of the function, including profile selection
end

this function is getting a little unweidly and could probably use some refactoring in general

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Cooper here- this may be a good time to refactor this function a little bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I mentioned in another comment, I modified is_supported_combination_button_switch_device_type to include the plug device type, and also to return the device type ID so that it can be used in initialize_switch to set the correct profile. I think this simplifies the logic quite a bit. Could you take another look and see if it looks alright?

end
Copy link
Contributor

Choose a reason for hiding this comment

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

this check is to stop the device from configuring to light-level-button? It seems like this should be handled more dynamically, since I can see many more devices being added that use a SmartPlug category rather than a Light, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

and more than that, devices that don't use switch level. sorry, missed that other difference on my first pass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was debating adding this device to is_supported_combination_button_switch_device_type, and then adding a bit of logic to use the plug-button profile rather than light-level-button, but since there is going to be a specific fingerprint added for this device, I decided to add it to fingerprint_profile_overrides and just skip the profile selection logic instead. fingerprint_profile_overrides was planning to be added by a different PR anyway so I thought it might be easier this way, but I would be ok with either option if anyone has an opinion on this!

Adding it to is_supported_combination_button_switch_device_type would have the benefit of supporting devices with a plug and any number of buttons as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I believe the general take on these kinds of questions has been to create the correct profile dynamically (even if there's a fingerprint) since this will future proof this work, will allow non-wwst devices to profile correctly, and will add some more error-prone logic for wwst devices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I updated initialize_switch with this in mind. I expanded is_supported_combination_button_switch_device_type to include the plug device type, so that I could remove the check_fingerprint_overrides function I created earlier. I think this simplifies the logic quite a bit. I also added new profiles with for plugs with 1-8 buttons.


if #button_eps > 0 and is_supported_combination_button_switch_device_type(device, main_endpoint) then
if #button_eps == 1 then
profile_name = "light-level-button"
Expand Down
Loading