SmartThingsCommunity
Fix an issue that windowShade value is incorrect
Mamaba Curtain controller sometimes sends operational status event twice when it stops moving. At that time, old code is not working correctly. So This patch maps each status to each single event. The mapping is as follows.
- Position ==100 : Open
- Position == 0 : Closed
- 0 < Position < 100 : Partially Open
- Operational Status == Opening : Opening
- Operational Status == Closing : Closing
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
| File | Coverage | |
|---|---|---|
| All files | 71% |
:x: |
| /home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-window-covering/src/matter-window-covering-position-updates-while-moving/init.lua | 36% |
:x: |
| /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 8753f8c8093c0244713f9720a2f34b0f096517ad
Test Results
64 files ±0 396 suites ±0 0s :stopwatch: ±0s 1 938 tests ±0 1 938 :white_check_mark: ±0 0 :zzz: ±0 0 :x: ±0 3 362 runs ±0 3 362 :white_check_mark: ±0 0 :zzz: ±0 0 :x: ±0
Results for commit 8753f8c8. ± Comparison against base commit 534b87c7.
:recycle: This comment has been updated with latest results.
This change, like you said, maps the status of the window covering to one single event. However, this will remove our "Opening..." and "Closing..." commands, as expected per the change, but is this really what we want? Couldn't we just add extra handling for if an event is sent twice based on time or previous state check logic?
One question: what happens in the Mamaba Curtain case when it sends the Operational State twice?
@hcarter-775
However, this will remove our "Opening..." and "Closing..." commands, as expected per the change.
Almost of window covering send Operational Status Event once at the Operating Start and End. so when window covering starts to open, driver receive moving event and driver can change the window covering status to opening or closing. Opening and Closing state are not going to be removed.
One question: what happens in the Mamaba Curtain case when it sends the Operational State twice?
The is_moving value and event_state value are set to incorrect values that do not match the situation.
The reason I mention the Opening... and Closing... statements disappearing is because when I ran the VDA device with this PR's changes, those statements disappeared. So I was not sure if this was expected behavior.
Thanks @hcarter-775 ! I assume we would want to maintain the behavior of displaying Opening, Closing, etc. We could move this to a sub driver if this change is specifically needed just for this device.
@ctowns, @hcarter-775 I don't understand what you said When cannot we see the opening and closing with this PR? Could you share the use case?
@ctowns, @hcarter-775 I don't understand what you said When cannot we see the opening and closing with this PR? Could you share the use case?
Hey @HunsupJung, @ctowns was expressing that right now the location of this code change isdrivers/SmartThings/matter-window-covering/src/init.lua and needs to be changed to a sub-driver. Currently how it is proposed this means that the changes being adding would affect all devices. Our belief is that this change should only be introduced to help support the Mamaba Curtain, which means that a new sub-driver should be introduced which will be a new file path. Please check out this page for reference. https://developer.smartthings.com/docs/edge-device-drivers/driver.html#subdrivers
@HunsupJung please make that change, and provide updated testing results with the device integration.
@ctowns Please feel free to chime in if I am mistaken.
During CN WWST for YeeLight & OVRIBO, and also a issues raised from a solution company called Longin.Link
Some device does not send operational status, some does not send initial operational status or CurrentPositionLiftPercent100ths, some does not send final operational status.
For these situations , ST all show incorrect windowShade status now.
And all the three vendors says their device works fine with Apple or google.
I think this PR will make great help for the issues mentioned above and we will also test here.
@hcarter-775 I'm so sorry for the reply late. and thank you for your explanation. As I said at slack, the situation which you said above can occurs. but it's only VDA case and a window covering device connected by DIRIGERA Hub (IKEA). So if you cover those case, we need to separate it to sub driver to support VDA and DIRIGERA.
The following link is the test video. https://smartthings.atlassian.net/browse/IOTE-4253
@lelandblue @ctowns @hcarter-775 @nickolas-deboom If there's no problem, could you approve this PR? :)
Hi @HunsupJung , the changes look good to me. I also tested them with the VDA and everything is working great. I'm not certain but it appears that this even fixes an issue I saw earlier with the VDA Window Covering device where the pause button did not work.
It still seems strange to me that a separate sub driver is needed just for the VDA. Are we certain that the VDA shouldn't be updated instead of the driver? If there is already consensus that this is the best way, I will approve the PR, but that said, it would be best to wait to merge this PR until Cooper returns from vacation tomorrow and can verify that his feedback has been addressed as well.
Also, there are some warning in the luacheck check that need to be resolved too.
Hi @HunsupJung can you handle the warnings that the Luacheck is generating? Otherwise it looks good to me!
@nickolas-deboom I don't think this PR is just for the VDA, I think it is for the Mamaba window covering device, and the VDA happens to have similar behaviors to the Mamaba device that we noted.
@nickolas-deboom @hcarter-775 Thank you for your review~! I'll fix the warnings that the Luacheck is generating.
@nickolas-deboom I don't think this PR is just for the VDA, I think it is for the Mamaba window covering device, and the VDA happens to have similar behaviors to the Mamaba device that we noted.
Ok gotcha!
Hello @HunsupJung, I may have missed something in these exchanges, but if the functionality you want has been moved to a sub-driver, shouldn't the regular init.lua function be reverted to how it used to be? I see there is a lot of removed code and I'm not sure if it should be removed.
@hcarter-775 At first, I updated regular init.lua file for most devices, including Mamaba. And I made the sub driver for a rare case that works like VDA.
The link I shared is the result of test by regular init.lua. https://smartthings.atlassian.net/browse/IOTE-4253
These are my comments from the link you shared, organized by device.
Mamaba curtain: shows the previous value when opening/closing until the update is complete. Why is this? Was it doing this before?
Mamaba blinds: When changing values to open, the UI reads "open" for a short moment before switching to "opening..." (1:13). When changing values to closed, the UI reads "partially open" fora short moment before switching to closed (1:26). Why does this occur? Was this happening before? Can we fix this? And at (1:40) we see the same thing I noted about Mamaba curtain. Why does this occur? Can we not fix this?
Zemismart Curtain: at (1:57) the UI reads closing... correctly, but then switches to "closed" before it is actually closed. Why does this occur? Was this happening before? Can we fix this? At (2:02) we see the same issue when opening, where it switches to "open" way before it is open. AT (2:18) we see the same issues.
Zemismart Bridged Curtain: I understand that the UI was not working before the patch, so no comments here. Is there any documentation about this issue though? It seems pretty extreme from a UI standpoint.
Zemismart Blind: At (3:38) we see the same issue I mentioned with the Zemismart Curtain.
So all that said:
- Why do some devices show the previous value when opening/closing until the update is complete? Was it doing this before, and can this be fixed?
- Some devices switch to "closed" or "open" before they should. Why is this? Was this happening before, and can this be fixed?
- Is there anything that can be done about the Zemismart Bridged Curtain for the future?
Were all the issues I'm mentioning happening before?
@hcarter-775 First of all, I'm sorry it took so long to answer. I have to reset the test environment for an answer and it takes half a day and the answer has been delayed due to Matter 1.3 preparation and door lock preparation over the past time.
Mamaba curtain: shows the previous value when opening/closing until the update is complete. Why is this? Was it doing this before?
When user click open or close, Mamaba send an opening event and when this operation is done, Mamaba send a closed event and 0% event. so this is normal operating.
Mamaba blinds: When changing values to open, the UI reads "open" for a short moment before switching to "opening..." (1:13).
I think it was device side issue. today I tested it and there's no same issue.
When changing values to closed, the UI reads "partially open" for a short moment before switching to closed (1:26). Why does this occur? Was this happening before? Can we fix this?
This issue has also been fixed.
And at (1:40) we see the same thing I noted about Mamaba curtain. Why does this occur? Can we not fix this?
I think this issue was not same with Mamaba curtain. this issue was same with other Mamaba blinds issues and has been fixed.
Zemismart Curtain: at (1:57) the UI reads closing... correctly, but then switches to "closed" before it is actually closed. Why does this occur? Was this happening before? Can we fix this? At (2:02) we see the same issue when opening, where it switches to "open" way before it is open. AT (2:18) we see the same issues.
Zemismart send 0% value and closed event. So the result value changes even before the operation is over.
2024-10-07_13:05:45.367 | I | dev_cmd_bs | [edi] handled device command: 5068ce5b-de84-4be6-bda1-deb14695e65b.main:windowShade.close("[]")
2024-10-07_13:05:46.574 | I | edi | <Matter Window Covering (6441e3f4)> <MatterDevice: 5068ce5b-de84-4be6-bda1-deb14695e65b [9F68F465BB4B164F-DC49B643D45BD1BF] (Zemismart MT01 Slide Curtain)> emitting event: {"attribute_id":"windowShade","capability_id":"windowShade","component_id":"main","state":{"value":"closing"}}
2024-10-07_13:05:46.779 | I | edi | <Matter Window Covering (6441e3f4)> <MatterDevice: 5068ce5b-de84-4be6-bda1-deb14695e65b [9F68F465BB4B164F-DC49B643D45BD1BF] (Zemismart MT01 Slide Curtain)> emitting event: {"attribute_id":"shadeLevel","capability_id":"windowShadeLevel","component_id":"main","state":{"value":0}}
2024-10-07_13:05:46.813 | I | edi | <Matter Window Covering (6441e3f4)> <MatterDevice: 5068ce5b-de84-4be6-bda1-deb14695e65b [9F68F465BB4B164F-DC49B643D45BD1BF] (Zemismart MT01 Slide Curtain)> emitting event: {"attribute_id":"windowShade","capability_id":"windowShade","component_id":"main","state":{"value":"closed"}}
(I omitted some part except important log.)
Zemismart Bridged Curtain: I understand that the UI was not working before the patch, so no comments here. Is there any documentation about this issue though? It seems pretty extreme from a UI standpoint.
I think we need to deal with this issue anew. I don't have any other tickets yet, and I will upload them again after this PR is merged.
Zemismart Blind: At (3:38) we see the same issue I mentioned with the Zemismart Curtain.
It is the same as the answer to the Zemismart Curtain.
@hcarter-775 @nickolas-deboom Thank you for all your reviews. I think all modification is updated. If there are any other problems, please let me know.
I raised a PR that use TargetPosition and CurrentPosition to decide the status of window covering, which avoid considering the order of attribute (current position and operational status) reports from devices, thereby simplifying the code. Please help to check. https://github.com/SmartThingsCommunity/SmartThingsEdgeDrivers/pull/1679/commits/447740d35102bfcb323773e2c028f9f0df87fb3f
According to Matter specification, the TargetPosition should be reported immediately after the open/close command is issued, and once the device has completed its movement, it should then report the CurrentPosition.
It should also be noted that according to the Matter specification, if a device supports the CurrentPosition attribute, it is required to support the TargetPosition attribute as well. Consequently, the code should also be applicable to devices that have previously passed WWST.
Hi @HunsupJung would you mind looking at @pInksenberg 's commit? I believe it may be a much simpler solution for the issue @nickolas-deboom mentioned here than creating a new subdriver for this one possibility.
There could be a problem creating a new subdriver for special situations, as bridged devices lack a PID and therefore cannot be matched with the subdriver.
@pInksenberg
First of all, thank you for your suggestion.
Considering the correlation between the target position value and the current position value in Matter Spec, I think it is the perfectly correct way.
However, in SmartThings, the value actually applied to capability is current position and the current position value may not match the actual device properly due to network error.
For example, some devices continue to send the current position value during opening / closing.
In this device, let's suppose the device is opening.
While shadeLevel is updated to 96% > 98% > 100%, update to 100% may fail and remain at 98%.
And since the driver has received 100% event, the value of windowShade will try to update to open.
After that the shadeLevel value is 98%, but the windowShade value is open.
(96% success > 98% success > 100% fail > open success)
So the Suwon team conclude to decide that the windowShade only by the current position value.
Of course, as an exception, if the operation value changes to opening / closing, we are updating it.
In addition, there is a problem with your commit.
If the target position value and the current position value come to driver at the same time, the windowShade is not updated.
For example, when windowShade is open and driver send a close command, the problem that shadeLevel is 0% and windowShade is open occurs.
This occurs when the target position value and the current position value come at the same time.
(I confirmed that this commit causes the problem.)
There could be a problem creating a new subdriver for special situations, as bridged devices lack a PID and therefore cannot be matched with the subdriver.
The basic policy is to determine the windowShade with only one attribute without comparing the two attributes.
Therefore, it is correct to apply the situation that cannot be solved by the subdriver as a basic policy.
@hcarter-775 Currently, this issue has been raised by users since about three months ago and has kept them waiting for a really long time. I think there could be a better code than the one I implemented. However, I don't think it's a good idea to keep waiting for users without putting in the current PR.
First of all, I think we need to solve this issue by merging the current PR. And then If there is a better and more efficient code in the future, I don't think it's too late to merge it then. so please approve it.