search

SmartThingsCommunity / SmartThingsEdgeDrivers

Apache License 2.0
256 stars 445 forks source link

Fix a part of mis-implementation for window covering device type #1447

Closed HunsupJung closed 1 month ago

HunsupJung commented 2 months ago

Previously, the window covering driver was modified because the windowshade value was sometimes wrong. And there was a part of mis-implementation. So the issue was reported as below. https://github.com/SmartThingsCommunity/SmartThingsEdgeDrivers/issues/1436

This is a patch that fix the issue.

github-actions[bot] commented 2 months ago

Channel deleted.

github-actions[bot] commented 2 months ago

Test Results

   60 files  ±0    377 suites  ±0   0s :stopwatch: ±0s 1 821 tests +2  1 821 :white_check_mark: +2  0 :zzz: ±0  0 :x: ±0  3 159 runs  +2  3 159 :white_check_mark: +2  0 :zzz: ±0  0 :x: ±0 

Results for commit 13b60935. ± Comparison against base commit 154dd38a.

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

github-actions[bot] commented 2 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 13b60935a70265bba76a7dfa69fed4f06931f346

HunsupJung commented 2 months ago

Please refer to following link for detail situation. https://smartthings.atlassian.net/wiki/spaces/STHK/pages/3268051961/Matter+window+covering+device+type+issue

nickolas-deboom commented 2 months ago

I looked into this reported bug as well and came to a similar conclusion as you did and I made a slightly different update here. I think I will close that PR, because your changes also include an update to current_status_handler which I think are needed as well.

However, I might leave my PR up to at least include the test case updates. I added two test cases which exercise two scenarios: (1) adjusting the shadeLevel by > 2%, which works correctly on the main branch and with these changes, and (2) adjusting the shadeLevel by <= 2%, which on the main branch does not update the status to "Partially Open" but instead gets stuck on "Closing"/"Opening".

Edit: After some discussion, the driver changes made under PR 1442 were reverted and the test case updates will be merged into this branch.

HunsupJung commented 2 months ago

@nickolas-deboom Thank you for additional patch.