Matter Window Covering: Always report accurate capability operational state#3133
Matter Window Covering: Always report accurate capability operational state#3133hcarter-775 wants to merge 3 commits into
Conversation
|
Invitation URL: |
Test Results 73 files 540 suites 0s ⏱️ Results for commit a2ccb07. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 9aaf446 |
| end | ||
|
|
||
| local function is_target_value_reached(current_value, target_value) | ||
| if (target_value and math.abs(current_value - target_value) <= TARGET_REACH_TOLERANCE) then |
There was a problem hiding this comment.
If there is no target value then do we want to return true so that we do emit the status or will we never be in that situation?
There was a problem hiding this comment.
The only cases I can see where there would be no target value would be if
- we reached it already, (handled)
- the timer this value was stored with times out, (handled in the timeout)
- the update came from a third party and we never received a target attribute (kinda handled)
- we received a target attribute report out of order. (handled)
Case 3 is the only one I can see where some mixup may happen might happen, but even then, the idle oprerational status attribute update response should take care of setting this.
Case 4 is the reason I don't think we'd want to default to true, since this very well may happen- timings are finnicky, especially since many times these reports are grouped together into a single response, and so there's no guaranteed ordering there.
cjswedes
left a comment
There was a problem hiding this comment.
By moving this out of the subdriver it now applies to all window coverings. We should consider testing some other models too.
The current VDA does not have the same handling it did at the time this subdriver was introduced. Between the current handling it has and the bridged handling, I believe we have a pretty good variety of devices being checked. To be clear, this is handling that has never worked well either. |
90fa859 to
9aaf446
Compare
61f1750 to
71a36ed
Compare
71a36ed to
09c8f10
Compare
|
matter-window-covering_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against a2ccb07 |
tpmanley
left a comment
There was a problem hiding this comment.
I've done a fair amount of testing of these changes with bridged Ikea shades that support lift (not tilt) and it's working well, including fixing the issue where it would not show "opening" or "closing" the whole time it was moving.
Description of Change
Use target positions and timeouts to ensure that idle states are only emitted when all state is cached properly. In doing this, add subscriptions to the target lift and target tilt attributes, to ensure that proper targets are being tracked at all times, from any source.
Resolves: #3043
I am going to continue with just this PR, I think
Tag-along updates:
Several fields are not needed, so I have removed them to add greater clarity to the updated handling.
CURRENT_LIFT/CURRENT_TILTare fields that store some latest attribute states during runtime. A field is not required for this.REVERSE_POLARITYstored the state of device.preferences.reverse, which is already stored in the data model. Therefore this is not needed. As it is persisted, it needs to be made nil to remove.PRESET_LEVEL_KEYis only ever used (get_field) when the capability state is nil, and is therefore nil in that context as well. So we don't need this.In
info_changed, we should not try to update the device profile on any non-profile update, as the inventories can be synced for many reasons. In fact, the only case this would be required for is a software update, so I have altered the logic to explicitly check for that- this has been validated with identical handling in Matter Switch, and a unit test has been updated to validate this as well.Summary of Completed Tests
Unit tests updated. Tested with VDA Window Covering as well as Bridged Ikea Blinds.
New tests are added to ensure the handlers do what they are expected to do.