Support Closure device type#2875
Conversation
|
Duplicate profile check: Warning - duplicate profiles detected. |
|
Invitation URL: |
Test Results 72 files 509 suites 0s ⏱️ Results for commit 68d4763. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 68d4763 |
nickolas-deboom
force-pushed
the
feature/closure-device-type
branch
2 times, most recently
from
611fc50 to
7a0bf8b
Compare
nickolas-deboom
force-pushed
the
feature/closure-device-type
branch
3 times, most recently
from
b497ce6 to
3e00e9d
Compare
nickolas-deboom
force-pushed
the
feature/closure-device-type
branch
from
542367b to
44c80b1
Compare
|
@nickolas-deboom would it make more sense to implement this closure logic as a subdriver? That way, we would overwrite the capability handlers rather than add a bunch of else's to the handlers |
nickolas-deboom
force-pushed
the
feature/closure-device-type
branch
from
44c80b1 to
50c21ea
Compare
I think that might be a good idea |
nickolas-deboom
force-pushed
the
feature/closure-device-type
branch
2 times, most recently
from
494dd19 to
f33d782
Compare
nickolas-deboom
force-pushed
the
feature/closure-device-type
branch
from
4b6eba2 to
84e38ff
Compare
| local MatterDriver = require "st.matter.driver" | ||
| local version = require "version" | ||
|
|
||
| -- TODO: change this to < 20 once the lua libs have been updated for hub-core 61 |
There was a problem hiding this comment.
Can we make a ticket to track this?
There was a problem hiding this comment.
This wasn't a permanent todo as this is already done as of 61.2 so I will update this in a commit within this PR.
| optional: true | ||
| categories: | ||
| - name: Blind | ||
| - id: windowShade4 |
There was a problem hiding this comment.
Why are there multiple components in each of these profiles, and why are there 4 in each? Is 4 an arbitrarily selection?
There was a problem hiding this comment.
Right, 4 is arbitrary since i wasn't sure how many closure panels to typically expect (the spec just says 0+). The example devices I've seen so far have only had up to 2 so I think 4 should be good however I will define this as a constant as you suggested in another comment
There was a problem hiding this comment.
To clarify, a closure consists of 1 endpoint implementing the ClosureControl cluster and then any number of endpoints of the Closure Panel device type that each implement the ClosureDimension cluster. So the number of components in these profiles that are used would reflect the number of ClosureDimension endpoints.
| clusters.ClosureDimension = require "embedded_clusters.ClosureDimension" | ||
| end | ||
|
|
||
| local mock_device = test.mock_device.build_test_matter_device( |
There was a problem hiding this comment.
These test cases only cover the covering type - could we add some coverage for the door/garage/gate types as well? I know that currently we would expect these to generally behave the same since the attribute handler just switches between windowShade and doorControl when checking what the device supports here, but I think it'd be good to at least have a couple door-specific unit tests to make sure this parity is maintained.
| for _, ep in ipairs(eps) do | ||
| if ep ~= 0 then | ||
| table.insert(result, ep) | ||
| if #result >= 4 then break end |
There was a problem hiding this comment.
I'm not sure where this 4 value comes from - could you define it as a constant somewhere for documentation purposes?
| -- Single ClosureDimension: enable the capability on the main component. | ||
| local found_main = false | ||
| for _, entry in ipairs(optional_caps) do | ||
| if entry[1] == "main" then |
There was a problem hiding this comment.
this might be cleaner to do something like this where you define a main_component_capabilities table earlier in the function and then insert it into the table at the end, that way you can just consistently add to the main capabilities without needing to find them within this optional caps struct. You can just define main_component_capabilities up on line 143 and then insert into it directly for the battery capability checking in line 144-148, and then insert directly in this handling as well.
Then at the end of the function, just push the main_component_capabilities into the optional_caps if it is not null.
| if attr.value == 0x0C then | ||
| match_profile(device, battery_support.BATTERY_PERCENTAGE) | ||
| if is_closure then | ||
| device:set_field(CLOSURE_BATTERY_SUPPORT, battery_support.BATTERY_PERCENTAGE, {persist = true}) |
There was a problem hiding this comment.
Could we just pass the battery support value in the match_profile_for_closure function, like what is done for the match_profile function?
Furthermore, could we have the match_profile function itself handle both closures and other types? You could just add a line at the top of the match_profile function that calls match_profile_for_closure if the device has a ClosureControl ep. It would essentially just be centralizing the logic in this function to the match_profile function itself, which I think would be a little cleaner.
|
@hcarter-775 I made an update to move the closure handling to a subdriver in 541b896. Lemmeknow how that looks |
3 participants
Check all that apply
Type of Change
Checklist
Description of Change
Add support for the Closure device type.
Note that these changes were originally in #2751, but that PR was based on a branch that would migrate the matter-window-covering driver to matter-switch, which might be done at some point but is a much larger scope. In the interest of supporting this device type sooner, this PR implements this code in matter-window-covering.
Summary of Completed Tests
Tested with VDA Closure and CHIP example app.