SmartThingsCommunity
Matter-Window-Covering: Fix refresh bug
When the device card for a window covering device is refreshed, the shade level displays as 0%. This is due to the CurrentLevel attribute, which is not a required cluster for the window covering device type, overwriting the windowShadeLevel capability after it is set by CurrentPositionLiftPercent100ths. This change removes the CurrentLevel attribute from the matter-window-covering driver.
These changes were tested using the VDA Window Covering virtual device by verifying that all capabilities display correctly, all commands function correctly, and that upon a refresh the shadeLevel is not reset back to 0%.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Test Results
64 files ±0 396 suites ±0 0s :stopwatch: ±0s 1 937 tests - 1 1 937 :white_check_mark: - 1 0 :zzz: ±0 0 :x: ±0 3 361 runs - 1 3 361 :white_check_mark: - 1 0 :zzz: ±0 0 :x: ±0
Results for commit 333380cd. ± Comparison against base commit 534b87c7.
This pull request removes 1 test.
LevelControl CurrentLevel handler
:recycle: This comment has been updated with latest results.
| File | Coverage | |
|---|---|---|
| All files | 95% |
:white_check_mark: |
| /home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-window-covering/src/init.lua | 95% |
:white_check_mark: |
Minimum allowed coverage is 90%
Generated by :monkey: cobertura-action against 333380cdcf4daefc3f2869fb96ee013173a6a35b
What testing have you done to make sure that things are still working correctly?
What testing have you done to make sure that things are still working correctly?
Hey Harrison, I realized that I never responded to your comment, but I updated the description of this PR with some info on how this was tested!
This changes makes sense to me, as the WindowCovering device type does not specify a requirement for the LevelControl cluster, and the CurrentPositionLiftPercent100ths attribute should be used to handle the window "level" setting. So I think this change makes sense.
However, did this originate just from VDA testing or do you know if this has been seen on devices? I just want to make sure this is safe to remove and no devices are actually using levelControl. Based on a quick git blame, it looks like the LevelControl cluster handler has been in the driver since it was originally released, so it seems to me it is just an artifact of an early implementation of the driver/device type that is no longer needed.
This changes makes sense to me, as the WindowCovering device type does not specify a requirement for the LevelControl cluster, and the
CurrentPositionLiftPercent100thsattribute should be used to handle the window "level" setting. So I think this change makes sense.However, did this originate just from VDA testing or do you know if this has been seen on devices? I just want to make sure this is safe to remove and no devices are actually using levelControl. Based on a quick git blame, it looks like the LevelControl cluster handler has been in the driver since it was originally released, so it seems to me it is just an artifact of an early implementation of the driver/device type that is no longer needed.
This originated from the VDA, which sends the CurrentLevel attribute, always with a value of 0. This would override the value of CurrentPositionLiftPercent100ths after a refresh. This has only been tested with the VDA - do you know of anyone with a physical window covering device that we could test this with? (The one I have is an Eve device but it doesn't work)
After some consideration, this PR will be closed in favor of updating the VDA Window Covering device type to not send the CurrentLevel attribute. Implementing this driver change would be a risk because there is a possibility that there is a device out there that uses this attribute. Unless we see a report of a real device with this issue, we shouldn't make this driver change.