Update profile selection logic to support plug/button devices #1799
Conversation
nickolas-deboom
requested review from
ctowns,
z-michel,
lelandblue and
hcarter-775
|
Duplicate profile check: Passed - no duplicate profiles detected. |
Test Results 64 files 402 suites 0s ⏱️ Results for commit 5e12292. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 5e12292 |
|
Channel deleted. |
| if check_fingerprint_profile_overrides(device) then | ||
| if #button_eps > 0 then | ||
| configure_buttons(device) | ||
| end | ||
| return | ||
| end |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
and more than that, devices that don't use switch level. sorry, missed that other difference on my first pass
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 then | ||
| configure_buttons(device) | ||
| end | ||
| return |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I agree with Cooper here- this may be a good time to refactor this function a little bit.
There was a problem hiding this comment.
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?
…thub.com:SmartThingsCommunity/SmartThingsEdgeDrivers into matter-switch-add-profile-for-button-plug-device
nickolas-deboom
changed the title
…thub.com:SmartThingsCommunity/SmartThingsEdgeDrivers into matter-switch-add-profile-for-button-plug-device
nickolas-deboom
deleted the
matter-switch-add-profile-for-button-plug-device
branch
Type of Change
Checklist
Description of Change
Add a new profile and handling in the matter-switch driver for NEO plug + button device (see WWSTCERT-4757).
Summary of Completed Tests