search

SmartThingsCommunity / SmartThingsEdgeDrivers

Apache License 2.0
263 stars 452 forks source link

Matter-Window-Covering: Fix refresh bug #1456

Open nickolas-deboom opened 3 months ago

nickolas-deboom commented 3 months ago

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%.

MTR-715

github-actions[bot] commented 3 months ago

Invitation URL: https://bestow-regional.api.smartthings.com/invite/gV2q7N88ej9N

github-actions[bot] commented 3 months ago

Test Results

   63 files    396 suites   0s :stopwatch: 1 935 tests 1 935 :white_check_mark: 0 :zzz: 0 :x: 3 359 runs  3 359 :white_check_mark: 0 :zzz: 0 :x:

Results for commit 86766023.

:recycle: This comment has been updated with latest results.

github-actions[bot] commented 3 months ago

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 867660236f350b6c0f0e274423456ca6f7859150

hcarter-775 commented 3 months ago

What testing have you done to make sure that things are still working correctly?

nickolas-deboom commented 2 months ago

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!

ctowns commented 1 month ago

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.

nickolas-deboom commented 1 month ago

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 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)