Matter Camera: Only update profile if capabilities have changed #2686
Conversation
|
|
||
| if camera_utils.optional_capabilities_list_changed(optional_supported_component_capabilities, device.profile.components) then | ||
| device:try_update_metadata({profile = "camera", optional_component_capabilities = optional_supported_component_capabilities}) | ||
| if #doorbell_endpoints > 0 then |
There was a problem hiding this comment.
button config can be put behind the optional_capabilities_list_changed gate so it doesn't run each time CameraAvStreamManagement.AttributeList is received.
Test Results 71 files 480 suites 0s ⏱️ Results for commit 7f067b7. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 7f067b7 |
drivers/SmartThings/matter-switch/src/sub_drivers/camera/camera_utils/utils.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/sub_drivers/camera/camera_utils/utils.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/sub_drivers/camera/camera_utils/utils.lua
Outdated
Show resolved
Hide resolved
| -- Check if all components in new_optional_capabilities exist in prev with same capabilities | ||
| for comp_name, cap_list in pairs(new_optional_capabilities) do | ||
| local prev_cap_list = prev_optional_capabilities[comp_name] | ||
| if prev_cap_list == nil then | ||
| return true | ||
| end | ||
| if #cap_list ~= #prev_cap_list then | ||
| return true | ||
| end | ||
| for i, cap in ipairs(cap_list) do | ||
| if cap ~= prev_cap_list[i] then | ||
| return true | ||
| end | ||
| end | ||
| end | ||
| for _, capability in pairs(prev_optional_capabilities or {}) do | ||
| if not switch_utils.tbl_contains(optional_capabilities, capability) then | ||
|
|
||
| -- Check if all components in prev exist in new_optional_capabilities | ||
| for comp_name, _ in pairs(prev_optional_capabilities) do | ||
| if new_optional_capabilities[comp_name] == nil then | ||
| return true | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
It might make sense to consolidate this... like create a size count when first initializing one of these "normalized tables", and then when checking one of these paths, make sure the size of the other was fully checked. If not, this means the other was bigger/smaller than the other and thus not everything is the same.
There was a problem hiding this comment.
In the latest commit I decided to format the components and capabilities from the profile from the device object into the same format as our optional component/capabilities list that gets formed in match_profile. The downside of this is that we can't index into these lists as easily and instead have to iterate through them. So basically the new algorithm is:
- Reformat
previous_component_capability_list - Iterate through each list and check for any components and capabilities that don't exist in the other list.
I think that overall it's easier to follow now but let me know what you think!
There was a problem hiding this comment.
Hm, I actually think I like the first version better, since this significantly cuts down checks ( O(n) vs O(n^2), give or take). At the same time, the lists we're looking through aren't all that big I suppose.
Still, I may suggest changing the formatting back to creating an explicit table and then just checking whether the key is nil or not.
Also, though the code is much cleaner now with your helper function (nice) this still includes a lot of double checking, since we check in one direction and then the other. Cutting the n^2 down to n will help with this, but I think we can go further. Specifically, I think we can do something like this:
local component_sizes = {}
for component_name, component in pairs(previous_component_capability_list or {}) do
component_sizes[component_name] = 0
for _, capability in pairs(component.capabilities or {}) do
... make the table that we can key off of later ...
component_sizes[component_name] = component_sizes[component_name] + 1 -- also, increment the number of capabilities found
end
end
local number_of_components_counted = 0
for component_name, component in pairs(new_component_capability_list or {}) do
number_of_components_counted = number_of_components_counted + 1 -- see how many components are in this new list
local capabilities_counted_for_component = 0
for _, capability in pairs(component.capabilities or {}) do
.... do the checking of keys ....
capabilities_counted_for_component = capabilities_counted_for_component + 1
end
if capabilities_counted_for_component ~= #component_sizes[component_name] then return false end -- make sure the number of capabilities just checked matches the number of capabilities in this list. If it doesn't, these aren't the same.
end
if number_of_components_counted ~= #component_sizes then return false end -- same idea as above for caps on a comp, but for the components.
I think something like this can keep the total looping down to once per list, by using a hash map + a use of the comp/cap lengths
There was a problem hiding this comment.
The algorithm you suggested is quite nice, however I think we need to check the actual capability IDs rather than just the number of capabilities per component, right? (and maybe we can do that with the code you suggested?). However, given the small size of the lists as well as the infrequency that this code will run, the difference in the computation complexity seems pretty negligible and my preference leans toward the current implementation, since it feels a bit simpler and takes the cap ID's into account. Although, when I think about it I kind of doubt that the capability list would change but stay at the same number of capabilities, so either implementation is probably just fine.
There was a problem hiding this comment.
@nickolas-deboom , this idea does take capability ids into account. I have that part in the little comment here .... do the checking of keys .... where I meant check the capability we're looking at now exists in the table we just made, aka the key. So the primary thing it does is check capabilities.
The added thing this version does is that it also checks sizes to make sure everything was actually checked, so we don't have to check both sides explicitly.
There was a problem hiding this comment.
Ah, thank you! I was misreading it. I will go ahead and implement this change in that case.
There was a problem hiding this comment.
Ok, made this change in 784195c. Lemme know if that looks ok to you!
There was a problem hiding this comment.
Thanks for making these changes Nick! Looks really nice, gonna take a last pass at it
drivers/SmartThings/matter-switch/src/sub_drivers/camera/camera_utils/utils.lua
Outdated
Show resolved
Hide resolved
nickolas-deboom
force-pushed
the
fix/matter-camera-dont-update-profile-unless-optional-capabilities-list-changes
branch
from
f5572eb to
73fa6d3
Compare
The `optional_capabilities_list_changed` function was not properly comparing the new set of supported capabilities with the current set, meaning that a profile update was occurring each time `camera_av_stream_management_attribute_list_handler` runs. This commit fixes `optional_capabilities_list_changed` and provides new test cases to ensure that the extraneous profile updates no longer occur.
nickolas-deboom
force-pushed
the
fix/matter-camera-dont-update-profile-unless-optional-capabilities-list-changes
branch
from
784195c to
7f067b7
Compare
| local component_name = new_component_capabilities[1] | ||
| local capability_list = new_component_capabilities[2] | ||
|
|
||
| number_of_components_counted = number_of_components_counted + 1 |
There was a problem hiding this comment.
I think, since this list is actually just a simple table that we know the structure of, we can just do
local new_list_num_components = #new_component_capability_list
and similar for the capabilities:
local new_list_num_capabilities_in_component = #capability_list
There was a problem hiding this comment.
or maybe we don't even need to define them as variables in this case, and just call this directly as needed
There was a problem hiding this comment.
I tried that but apparently it doesn't work for keyed tables. I didn't try very hard tho, so lemme see if it'll work
There was a problem hiding this comment.
ah my bad, that's true. I forgot about that.
There was a problem hiding this comment.
actually, ins't the capability_list a basic table? That one might work
|
|
||
| number_of_components_counted = number_of_components_counted + 1 | ||
|
|
||
| if not previous_capability_map[component_name] then |
There was a problem hiding this comment.
nit but can we do if <something> == nil then in these cases? Idk, since we're talking about whether these were found this structuring seems more natural? Just throwing it out there, what do you think
The
optional_capabilities_list_changedfunction was not properly comparing the new set of supported capabilities with the current set, meaning that a profile update was occurring each timecamera_av_stream_management_attribute_list_handlerruns. This commit fixesoptional_capabilities_list_changedand provides new test cases to ensure that the extraneous profile updates no longer occur.