Major refactor of cover platform and config flow support tweaks for cover platform

[cwttdb70]

This is a major rework and refactor of cover.py.
Depends on #6
Should be merged after #8 for the cleanest merging.

See https://github.com/postlund/localtuya-homeassistant/pull/1 for previous conversation with @postlund on this PR.

A few things to note about my pushed code:

1. This is a a pretty major rework of cover.py. I refactored almost all of the functions to align with Home Assistant best practices for devices generally and for covers specifically, per https://developers.home-assistant.io/docs/core/entity/cover/ and https://developers.home-assistant.io/docs/asyncio_working_with_async/.

2. None of my "set" functions (open, close, set position, etc.) in cover.py call anything in init.py other than set_value. As rospogrigio mentioned, and with which I completely agree, pytuya should be a minimal library to communicate with Tuya devices. Handling of specific device characteristics, attributes, functions, etc., should be done on localtuya's end.

3. Here's my lovelace code which shows the blind using a few different card types (some stock, some custom).

- entities:
          - buttons_position: left
            entity: cover.test1007
            invert_percentage: true
            name: Test Blind
            title_position: bottom
        title: Test Blind
        type: 'custom:shutter-card'
      - entities:
          - entity: cover.test1007
            icon: 'mdi:blinds'
            name: Left Blind
            type: 'custom:slider-entity-row'
          - type: divider
          - entity: cover.test1007
            icon: 'mdi:blinds'
            name: Left Blind
        full_row: true
        show_header_toggle: false
        title: Test Blind
        type: entities

And that gets you:

(Note: The reason the top card shows 81% and the bottom card shows 80% is because the bottom card rounds to the nearest 10%).

4. There are a couple FIXME/TODO:

• I still want to think about whether it's possible during config_flow to only allow the user to select from the dps list for the following characteristics: GET_POSITION_KEY, SET_POSITION_KEY, COVER_COMMAND_KEY, LAST_MOVEMENT_KEY. Entering an integer with no list to choose from is not terrible, but it's not optimal. As a stopgap measure, perhaps we print the dps data in the config_flow setup screen so that people can refer to it:

• I need to add code to validate that OPEN_CMD, CLOSE_CMD, and STOP_CMD are all in lowercase ("open" vs "OPEN" or "Open"--only works when all lowercase). Or, alternatively, let people use whatever they want in config and then parse it as lowercase in cover.py. Or both. Easy fix but not a big deal and will do it later.

• When you hit one button on the lovelace UI (such as open) and then VERY QUICKLY (within less than a second or so) hit another button (such as stop), the second command sometimes does not register. This might just be a limitation of Tuya devices--they don't like getting inundated with rapid connections.

• When you open or close the blind, and then hit stop, the current_cover_position immediately updates to the correct value. However, when you set a specific position, and the cover opens/closes to that position, the current_cover_position does not update until the next update call after the opening or closing is complete, which can be as long as 30 seconds, but is usually closer to 15 seconds.

[rospogrigio]

Regarding keeping track of opening percentage, I had thought a lot about it and I think it's not a good idea, or at least I would make it optional. This because if you have a cover (like mine) that has also physical buttons (which I believe is the most common situation) there is no way for the device to know if a manual opening or closing has occurred, and for how long. The positioning info is unreliable for these devices, as a consequence.

[cwttdb70]

Regarding keeping track of opening percentage, I had thought a lot about it and I think it's not a good idea, or at least I would make it optional. This because if you have a cover (like mine) that has also physical buttons (which I believe is the most common situation) there is no way for the device to know if a manual opening or closing has occurred, and for how long. The positioning info is unreliable for these devices, as a consequence.

Hi @rospogrigio, thanks for the note. I'm curious to see how your device compares to mine. This will help us think of the best way to approach this situation. Can you run the following two scenarios for me:

Scenario A

1. Using the physical buttons, start closing your cover device (by close, I mean cover up your window with the cover)

2. Wait until the cover is completely covering your window and the cover movement has stopped

3. Run tuya-cli get --id YOURID --key YOURKEY --ip YOURID --protocol-version YOURPROTOCOLVERSION --all

4. Reply to this message with A.3 Results: PASTEOUTPUTHERE

5. Using the physical buttons, start opening your cover device

6. After 3 seconds, use the physical button to stop the opening

7. Run tuya-cli get --id YOURID --key YOURKEY --ip YOURID --protocol-version YOURPROTOCOLVERSION --all

8. Reply to this message with A.7 Results: PASTEOUTPUTHERE

Scenario B

1. Using Home Assistant, start closing your cover device (by close, I mean cover up your window with the cover)

2. Wait until the cover is completely covering your window and the cover movement has stopped

3. Run tuya-cli get --id YOURID --key YOURKEY --ip YOURID --protocol-version YOURPROTOCOLVERSION --all

4. Reply to this message with B.3 Results: PASTEOUTPUTHERE

5. Using Home Assistant start opening your cover device

6. After 3 seconds, use Home Assistant to stop the opening of the cover device

7. Run tuya-cli get --id YOURID --key YOURKEY --ip YOURID --protocol-version YOURPROTOCOLVERSION --all

8. Reply to this message with B.7 Results: PASTEOUTPUTHERE

[rospogrigio]

OK I'll try it but first I have to fix tuya-cli because it has never worked with my cover devices, but now I should know how to fix it.
Will keep you posted.

Edit: no, I actually can't understand how to fix tuya-cli, it goes over my skills. It's the same old "json data unvalid" error, however, that tuyapi still does not handle correctly.
Anything else you'd like to check?

[gd71]

Hello, I'm using 3 cover switches with the current implementation of local tuya.
(https://www.amazon.fr/gp/product/B07PNV9XKQ/ref=ppx_yo_dt_b_asin_title_o07_s00?ie=UTF8&psc=1)
on any scenario described above, the result of the command is:
{ devId: '27751547fcf5c48598b8', dps: { '1': 'stop', '9': 15, '101': true } }
So I guess the remark from @rospogrigio is relevant.
dps 1 is for the well known commands ('on','off','stop' in my case), 9 is for the trip time (15 seconds here) I configured via the smart app then 101 looks to me for the backlight.
Not very skilled on debugging/developing on such kind devices, but at least I'm at your disposal for some tests.
Keep up your great job!

[cwttdb70]

@rospogrigio, I forgot that tuya-cli does not work with your device. Can you get me the output of those 4 scenarios above (A.3, A.7, B.3, B.7) using a different method. Instead of using tuya-cli, let's use localtuya's debug output. In pytuya/__init__.py, ensure that you have the the two log.debug('decrypted result=%r', result) lines in your file and not commented out.

elif result.startswith(PROTOCOL_VERSION_BYTES_31):
            # got an encrypted payload, happens occasionally
            # expect resulting json to look similar to:: {"devId":"ID","dps":{"1":true,"2":0},"t":EPOCH_SECS,"s":3_DIGIT_NUM}
            # NOTE dps.2 may or may not be present
            result = result[len(PROTOCOL_VERSION_BYTES_31):]  # remove version header
            result = result[16:]  # remove (what I'm guessing, but not confirmed is) 16-bytes of MD5 hexdigest of payload
            cipher = AESCipher(self.local_key)
            result = cipher.decrypt(result)
            log.debug('decrypted result=%r', result)
            if not isinstance(result, str):
                result = result.decode()
            result = json.loads(result)
        elif self.version == 3.3: 
            cipher = AESCipher(self.local_key)
            result = cipher.decrypt(result, False)
            log.debug('decrypted result=%r', result)

Then, enable debug logging in configuration.yaml:

#Logging
logger:
  default: info
  logs:
    custom_components.localtuya: debug

Now, run the 4 scenarios above (A.3, A.7, B.3, B.7), and after each one, wait until the next decrypted_result posts to the Home Assistant log, and then grab that output.

[cwttdb70]

Hello, I'm using 3 cover switches with the current implementation of local tuya.
(https://www.amazon.fr/gp/product/B07PNV9XKQ/ref=ppx_yo_dt_b_asin_title_o07_s00?ie=UTF8&psc=1)
on any scenario described above, the result of the command is:
{ devId: '27751547fcf5c48598b8', dps: { '1': 'stop', '9': 15, '101': true } }
So I guess the remark from @rospogrigio is relevant.
dps 1 is for the well known commands ('on','off','stop' in my case), 9 is for the trip time (15 seconds here) I configured via the smart app then 101 looks to me for the backlight.
Not very skilled on debugging/developing on such kind devices, but at least I'm at your disposal for some tests.
Keep up your great job!

Thank you for this, it's very helpful. As you said, your cover device opens/closes based on a set amount of time. It does not report current position, nor let you set current position. It's entirely time based.

That's very different from my device, which does not use timers. Rather, it reports the current position ('3') and lets me set the current position directly ('2').

  dps: {
    '1': 'open',
    '2': 51,
    '3': 51,
    '5': false,
    '7': 'closing',
    '8': 'cancel',
    '9': 0,
    '10': 0,
    '11': 38610
  }

I think that @rospogrigio's device is similar to yours, as his commands are also on off and stop (whereas mine are open close and stop).

However, I want to wait until @rospogrigio provides his output for those 4 scenarios just to be certain.

If @rospogrigio's output looks similar to yours, then I think a potential solution is as follows:

1. Add a new configuration option to device setup called Operation Mode with the available options as Timed and Position.
2. Add another new configuration to device setup called Timer Key which is the dps key for the timer, which in your case is 9.
3. If you set Position, then everything operates exactly as this PR currently stands.
4. If you set Timed, then you don't need to enter GET_POSITION_KEY or SET_POSITION_KEY in the device configuration (or you can enter them, but localtuya will simply ignore them). On the back-end, set position is disabled.
5. open_cover and close_cover get the timer from the Timer Key and run for that amount of time, or until stop is called.

@rospogrigio I know you used to have some code for setting an actual position of time-based cover devices, but I am struggling to understand how it works in practice:

def set_cover_position(self, **kwargs):
        """Move the cover to a specific position."""
        newpos = float(kwargs["position"])
        currpos = self.current_cover_position
        posdiff = abs(newpos - currpos)
#       25 sec corrisponde alla chiusura/apertura completa
        mydelay = posdiff / 2.0
        if newpos > currpos:
            self.open_cover()
        else:
            self.close_cover()
        sleep( mydelay )
        self.stop_cover()
        self._position = 50  # newpos

Can you explain how this code worked in conjunction with your device. It's confusing to me, because you're setting the self._position=50 after each call of this function, so obviously that is a proxy for device position since you can't get it directly from the device. However, when you run set_cover_position, you would always be comparing the new position to 50, right? I'm not quite sure how this works. What if the cover's actual position is currently 10, and you use this code to set_cover_position to 50? It looks to me like nothing would happen, since newpos and currpos would both be 50, posdiff would be zero, self.close_cover() would be called but mydelay would be 0 so the function would then immediately call self.stop_cover().

(Sidenote, but does calling sleep() here pause execution of all localtuya code? I know that Python supports multithreading, but I don't think we're multithreading here.)

[cwttdb70]

My general thoughts are that if a cover device doesn't technically support setting the position, then we probably shouldn't include a hacked together attempt to set the position.

And on that train of thought, if a device does not support setting the position, we should ensure that SUPPORT_SET_POSITION is not sent to Home Assistant for the device. That might mean that we need to dynamically set the device's supported features based on whether the user sets the new Operation Mode in the initial device config to Timed or Position.

Those are my thoughts on the matter. @postlund, curious to hear your thoughts (on how we should approach cover devices that don't technically report the current position or let you set the new position).

[postlund]

I tend to agree with you @andrewmeepos. If we can't provide a reliable position it's better if we don't. The user will know this. Since Home Assistant provides a template for covers, users can toll their own solution:

https://www.home-assistant.io/integrations/cover.template/#position_template

[postlund]

And yeah, we must not return SUPPORT_SET_POSITION in case we don't support it. But that should be rather simple if we depend on an optional DPS value (no value = not supported).

[gd71]

please note that the cover device is managing the time by itself. Could be a workaround to finally set position to 50 in order to let the widget always proposing close and open actions as HASS does not know the real position.
Fully shared, if the device is not in a situation to feedback the position then don't try to guess it, there will be more issues than benefits

[rospogrigio]

@andrewmeepos , I will try to get the output you need as soon as I can.
Regarding the code for positioning, I knew this question would come, sooner or later... Well, it's a kind of a hack actually. The idea behind it is to allow a partial opening or closing of a desired amount, regardless of the current position, which shall be known a priori by the user. It just doubles the posDiff and sends it as a positioning command. To better explain, the cover believes it is always at 50, and you can give positioning commands like 100 (fully open) or 0 (fully close). Setting 75 would open half way, 25 would close it half way (the 25 difference from 50 is doubled). Obviously this movement is effective only if the user is aware of the current position. I just added it because if I wanted, let's say, to open it just a 10% more than it is now, I just needed to set 55, instead of issuing an open+stop series of commands.
It is not of vital importance, if you want to get rid of this little hack. Hope it's clearer now, though.
Bye!

[rospogrigio]

Here is the output:
DEBUG:localtuya.pytuya:decrypted result='{"dps":{"1":"stop","101":false},"t":1599985046}'

I only get the status (1) and the dps 101, which is the backlight status. I'd leave the behavior configurable, with options Position/Hacked position/None.
Your thoughts?

[gd71]

@rospogrigio , I wonder whether the hacked position could be achieved through the template cover, https://www.home-assistant.io/integrations/cover.template/#position_template as suggested by @postlund and script for setting position. But I'm very new to HA and not yet comfortable with these features

[cwttdb70]

I'd leave the behavior configurable, with options Position/Hacked position/None.`

I'm not sure. I might be ok with including "Hacked Position" as an option, as long as we very clearly explain what it is that "Hacked Position" does. However, it is definitely improper Home Assistant behavior. Would we also have to add a configuration option for time to open/close? I'm leaning towards not including it, but I could be convinced either way. I recognize that excluding this option would be unfortunate for those with devices that don't support set position.

I have no experience with position_template, so I can't comment on that.

What do others think? @postlund?

[gd71]

In my case, the trip-time is at dps 9 and most probably can be set by HA. I guess this can be detected automatically.
If you want to implement the "hacked position", my guess is that it is up to the user to set the trip-time. My current tests on current implementation is not accurate for me as it looks like 25s is hardcoded. Also, as you mention, we must be sure that HA continue working when sleep instruction is ongoing

[cwttdb70]

I would like to propose that we proceed with merging this pull request. However, I see that there are conflicts. Can someone explain how I go about resolving these conflicts? I'm a little confused.

@rospogrigio, after this is merged you can open a new pull request which adds the functionality to use the "Hacked Position" open, and we can continue the discussion there.

[rospogrigio]

@andrewmeepos , I would like to merge #11 and #12 before getting into this. Can you try to merge from #11 and fix the conflicts from there? If you still have problems I can help but give me some time.

[cwttdb70]

@andrewmeepos , I would like to merge #11 and #12 before getting into this. Can you try to merge from #11 and fix the conflicts from there? If you still have problems I can help but give me some time.

Yes, i'll start with #11 and build this back on top of it. Will give this a spin tonight.

[postlund]

You can always just "push -f" to your branch to start over. There's really no need to close the PR.

[cwttdb70]

@postlund Ok, finally figured out how to rebase and force push. Anyways, the one error holding me up on this is below. I get the error every time I finish setting up a cover device from the config flow. Everything was working prior to merging my code to the latest master, so I must've done something wrong when updating my code. Haven't been able to get to the bottom of this yet.

2020-09-16 01:17:29 ERROR (MainThread) [homeassistant.components.cover] Error while setting up localtuya platform for cover
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/helpers/entity_platform.py", line 184, in _async_setup_platform
    await asyncio.shield(task)
  File "/config/custom_components/localtuya/cover.py", line 111, in async_setup_entry
    TuyaCache(device, config_entry.data[CONF_FRIENDLY_NAME]),
KeyError: 'friendly_name'`

[rospogrigio]

As I wrote in #19, I'm getting the same error so that's not your fault. Will try to fix it when I get the time.

[cwttdb70]

Do you get this error when setting up switches too, or just covers?

[cwttdb70]

@postlund I've incorporated most of your changes. I had a few questions about your feedback, which i've added as comments. I have not yet pushed the updated PR, as I want to resolve my several questions with your feedback before doing so.

[postlund]

@andrewmeepos I think/hope I answered the questions now.

[cwttdb70]

@postlund Ok, I think I updated everything. Take a look and let me know.

[cwttdb70]

Would be great to get this merged so I can stop resolving conflicts and/or rebasing over and over 😬

[postlund]

A few small things left but otherwise it's good!

[cwttdb70]

@postlund I believe I incorporated all of your comments

[postlund]

Added two minor things I found after looking at everybody again. After that I'm all for merging 👍

[postlund]

I thought once more about these. You can use self.dps(...) to fetch the value (forgot about that one for a second). So basically:

self._last_movement = self.dps(str._config.get(CONF_LAST_MOVEMENT))

This will log a warning if the requested DP does not exist.

[cwttdb70]

@postlund good to merge?

[postlund]

@andrewmeepos Yeah! 👍

[cwttdb70]

@andrewmeepos Yeah! 👍

@postlund I tested it and it had some issues, but I just fixed it.

For the record, this is the line that didn't work:
self._last_movement = self.dps(str._config.get(CONF_LAST_MOVEMENT))

Changed it to this and it works:
self._last_movement = self.dps(str(self._config.get(CONF_LAST_MOVEMENT)))

Just need to fix conflicts with master, remove my internal logging, and push.

[cwttdb70]

@postlund @rospogrigio ok, this should be good to merge

[rospogrigio]

Sorry @andrewmeepos but I still could not test this and I must ask you to suspend working on this until we have merged #40 because I would really have that sorted before proceeding, and I don't want to waste your time by having you solve conflicts over and over again. Once #40 is done, I promise this will be the next PR to go in.
Just one comment: I could not test it but I see you have removed the is_opening() and is_closing() methods, but I quite remember that they need to be defined for my covers to work (I think they are needed for the icons to change when opening/closing). Let me know if your covers behave the same.
Sorry about the mess, please wait #40 before proceeding, thank you.

[rospogrigio]

Totally rewritten in #60, closing PR.