Cover fake position persistent #213

rospogrigio · 2020-12-03T13:54:34Z

Introduced storing and retrieving of cover last position, in order to make it persistent after rebooting/restarting HA.
It assumes the cover to be fully closed at the first startup, however after a full opening it will be coherent with the actual cover position.
This allows to get rid of the "half range" weird behavior.
Implemented for issue #175, but it should fix also issue #186, and probably #207.

…positioning

rospogrigio · 2020-12-03T14:03:24Z

@postlund , I tried to implement what we agreed in issue #175. I need your review of the code, in particular for 2 aspects:

I initially thought to use a unique json file for all covers, but was a bit afraid of concurrency since they can stop together (I have a group template) and I was fearing they might end with wrong values... is it a real risk? If it is not, I can easily change the code to support a single file (which I would prefer)
I don't know if it is correct in terms of asyncio, since it is doing I/O in the end, so please correct me if I should call the methods in some asyncio-safe way (probably async_add_executor_job is needed?)
Thank you

postlund · 2020-12-03T14:27:42Z

@rospogrigio I don't have time to do a full review now, can do that later. But doing I/O like that is a problem. Home Assistant can already save previous state for you using RestoreEntity. You can change our base class to do so. An example of how to use this is here:

https://aarongodfrey.dev/programming/restoring-an-entity-in-home-assistant/

rospogrigio · 2020-12-03T16:04:07Z

@postlund thank you, I read the link you posted but I can't even inherit the RestoreEntity class, so I'll wait for your support. Also, I don't know whether we should inherit LocaltuyaCover or the LocaltuyaEntity... will wait for your advice when you have time, thank you

TheDukeSr · 2020-12-03T19:39:47Z

Just to make sure I test correctly.

Do I need to completely UNinstall, REinstall (or even change the repo in Hacs?) local_tuya?
I just replaced the \custom_components\localtuya folder this time.

Is it still needed to half the opening time in config (my cover opening is 30 secs, but in previous config I had to input 15)?

rospogrigio · 2020-12-03T21:09:32Z

It should be fine to just replace the folder (actually, only cover.py has changed so you could just replace that one).
And no, it is no longer needed to behalf your opening time, now it is handled as you expected it.

rospogrigio · 2020-12-04T00:06:57Z

@postlund OK I think I have understood how it works, get ready for a review... 😉

rospogrigio · 2020-12-04T10:34:19Z

@postlund OK I tried it and it seems to work partially. It is working if I restart HA, but if I reboot the Raspberry it loses the stored status.
I seem to understand that the status is stored in /config/.storage/core.restore_state , and if I look in there after a reboot, the "current_position" attribute is not set. Am I missing something? It seems it is stated also in this thread:
https://community.home-assistant.io/t/how-to-setup-restore-of-light-switches-template-switches-and-input-boolean-after-restart-of-home-assistant/212185/4

tom_I: Restore only works across home assistant restarts. Not host reboots.

if this is the case, then we'd better make asyncio-safe the storing on a file, since for example I reboot the host once a day (I actually leave it off overnight). Your thoughts? Thank you

rospogrigio · 2020-12-08T14:29:39Z

Hi @postlund I just saw you were back, hope everything is fine... do you have any comment/suggestion about this? Thank you...

TheDukeSr · 2020-12-08T16:18:34Z

It's not working correctly.
My cover takes 30secs to open en 30 secs to close.
So my Config is set to On_Off_Stop / Fake / 30 Sec.

-- Clean Hass start ---

Status: Slider is at 0%
I move the slider to 30%, the cover is opening for 10 secs and stops at 30%. That is PERFECT!
Then I click on close in Hass and Cover is closing, but Slider stays at 30% and Close button keeps being activated.- NOT Good.

2 problems:
+/- 10 secs after clicking the Close button the slider should have moved to 0%
And also there should have been a STOP command after 10 secs.

Only when Clicking STOP manually, the slider moves to 0%.

Hope this helps, logs available if needed.

rospogrigio · 2020-12-09T08:31:49Z

Actually this behavior is correct.

the slider position is updated only when the cover stops
the device has a timeout, usually quite long (for mine, it is 2 mins even if my opening time is 25 secs) because it does not know which is the shutter it is operating, and how long it takes for a full opening. So, if you just wait you will see that in approx. 2 mins the device will automatically stop, and the slider will be updated
this behavior shall not be modified, because it is the only way it is possible to recover from a situation of misalignment between the HA status and the actual cover position (i.e., HA starts at 0% but the device is fully open: in this case you just press the "open" button and when the timeout elapses, HA and cover position are coherent).
Hope this helps...

TheDukeSr · 2020-12-11T22:02:40Z

Actually this behavior is correct.

the device has a timeout, usually quite long (for mine, it is 2 mins even if my opening time is 25 secs) because it does not know which is the shutter it is operating, and how long it takes for a full opening. So, if you just wait you will see that in approx. 2 mins the device will automatically stop, and the slider will be updated
Hope this helps...
Hi,

Thnx for the explanation, but one thing I don't understand.

because it does not know which is the shutter it is operating, and how long it takes for a full opening

But It does know how long it takes to fully open, it is set in the cfg "Full opening time in secs (fake mode only)"?!
I hope this link to picture works ;)

So why wait for a long 2 minutes timeout, if the max timeout is allready known in de config settings?
If you use this setting as Max timeout, then it would work in any condition good, failed, reset, and unknown.
Maybe i'm missing something here? But it seems to me a STOP command after this cfg max time setting is allways correct and saves waiting for a unneeded long timeout.

ps. Not all covers have a timeout, mine does not. It just stops the motor because there's a adjustable microswitch inside.
So the manual switch on my wall keeps now Blinking on Open or Close until a STOP command is given. The motor does not communicate to the switch that there's some sort of end or timeout.

rospogrigio · 2020-12-12T14:26:20Z

I'm surprised: I always tend to think that all devices behave like mine, but I am usually proven to be wrong, as in this case. 😆
Actually, also my cover stops when it reaches the end due to an internal microswitch, but the wall switch remains to open, until the timeout that comes 2 mins later makes it change to stop. The stop command is done internally, it's not coming from the software (be it localtuya, tuya or the Tuya App).
Anyway, you do have a point when you say that we could just launch the stop command after a full opening time elapses, instead of waiting the internal timeout... I will code it soon, thank you for the suggestion!

…out equal to full opening time

rospogrigio · 2020-12-12T15:01:10Z

@TheDukeSr I've done it, can you please give it a go and report feedback? PLEASE NOTE: I changed the name of the positioning from "fake" to "timed", since now I like this naming more: so please update your YAML file if you are using it, or recreate the Integration if you went with the config flow. Thank you!

rospogrigio · 2020-12-14T06:56:50Z

Mmm, I noticed that now you can't stop the movement until it has reached the timeout, need to fix this so please wait...

rospogrigio · 2020-12-14T13:29:43Z

@TheDukeSr now it should work as expected, give it a go and report feedback, tranks!

TheDukeSr · 2020-12-14T14:20:29Z

@TheDukeSr now it should work as expected, give it a go and report feedback, tranks!

@rospogrigio Hi, I'll test it tomorow (15-12-2020) and report back here asap.

rospogrigio · 2020-12-15T09:06:07Z

@postlund may I ask you a review on this? Also, since as I reported the RestoreEntity is not effective in case the HA server is rebooted, would the I/O operation being invoked with async_create_task (so it is parallelized) be a solution?
Thank you

TheDukeSr · 2020-12-15T11:23:50Z

@TheDukeSr now it should work as expected, give it a go and report feedback, tranks!

@rospogrigio I'm sorry to say but there's still much going wrong.

Isn't there a way to commonucate directly like discord, teamspeak..? (Dutch or English).
Github is nice for development, but communication.. just hate it.. arrggg (could not even find a private message option here. pfff) , It takes me 1 hour to explain something here that can be done in 5 minutes in a live chat.

rospogrigio · 2020-12-15T11:25:58Z

You can send me a private message on https://community.home-assistant.io/ 😉

postlund · 2020-12-15T11:39:44Z

@rospogrigio Sorry, forgot about this one! I don't really accept the fact that there's a difference between a restart of Home Assistant and the system itself. Home Assistant (core) does not make any distinction of that. I had to take a look at the code to see how it works.

The states are stored in core.restore_statesand are periodically saved every 15 minutes and whenever Home Assistant is stopped. I suspect that the states are not written down properly when restarting the system. I'm not sure how you restart? If you shut down Home Assistant properly, verify that the state has been written down to core.restore_states and then reboot. Does it still not work?

Creating a new task does not make anything run in parallel, like you would expect with regular multithreading. In asyncio there's only one thread and one thing running at once. So you are never allowed to do potentially blocking I/O in async context as the blocks everything. You need to put that kind of work in an executor (loop.run_in_executor), which runs the code outside of async context using a pool of "regular" threads. The result is dispatched back to async context when done. I however expect Home Assistant to have some helpers for this. Otherwise there are libraries dealing with this.

I however would like to not do this at all, but rather rely on RestoreEntity and report potential problems back to core to have them fixed. Much rather than work around the problems. Some problems I see with current solution is that:

We don't consider wearing (when dealing with flash memory), which RestoreEntity at least do to some extent.
We use one file per device, which potentially clutters the file system.
Files are never removed, even if a device is removed. That is bad practice: we must clean up after ourselves.
Mentioned issue with sync code in async context.

rospogrigio · 2020-12-15T11:46:05Z

Thank you! Well, to answer your question: I just switch off the system (the smart plug that provides power has a switch-off timing). But still the problem might occur if there is a power cut and the status was not saved... is there any way to force updating core.restore_states ? Anyway, I get that this is a bit of a corner case (power cut with cover movement occurred less than 15 minutes ago).

TheDukeSr · 2020-12-15T11:48:05Z

You can send me a private message on https://community.home-assistant.io/ 😉

Nope ;-(

New user, so my rights there are limited and can't send messages.... OMG..
I realy want to help you guys with this local tuya project, but the systems wont let me ;-)
You're maybe able to send me a messy on that site, so i can reply? user TheDukeSr

postlund · 2020-12-15T12:50:40Z

@rospogrigio Do you turn off Home Assistant by just cutting power?! That is not a good idea. Things like this can certainly happen because of that and it's also a big risk breaking your storage medium. Shut down properly instead.

You can always lose data due to an accidental power cut. That's an edge case that we should not solve, it's a problem for Hone Assistant itself. If everyone runs around trying to fix every little detail, it just becomes a mess. We should restore if possible and be robust, providing as good fallback if we can't. But we should not mess with the internals of Home Assistant.

rospogrigio · 2020-12-16T13:46:23Z

@postlund Yeah, I was not scared for the storage medium since I only use solid state devices but I agree with you and I moved to a proper shutdown,
Can you review this PR now? I think also other platforms might benefit from inheriting RestoreEntity now.

postlund · 2020-12-16T22:02:59Z

custom_components/localtuya/common.py


    async def async_added_to_hass(self):
        """Subscribe localtuya events."""
        await super().async_added_to_hass()

        self.debug("Adding %s with configuration: %s", self.entity_id, self._config)

+        state = await self.async_get_last_state()
+        if state:
+            self._stored_state = state


Do we really have to store it in self._stored_state, can't we just pass the value as an argument to status_restored?

postlund · 2020-12-16T22:06:58Z

custom_components/localtuya/cover.py

+            # for timed positioning, stop the cover after a full opening timespan
+            # instead of waiting the internal timeout
+            self.hass.async_create_task(
+                self.async_stop_after_timeout(self._config[CONF_SPAN_TIME] + 5)


Magic constant: what does 5 mean? Create a constant for it.

postlund · 2020-12-16T22:07:14Z

custom_components/localtuya/cover.py

+            # for timed positioning, stop the cover after a full opening timespan
+            # instead of waiting the internal timeout
+            self.hass.async_create_task(
+                self.async_stop_after_timeout(self._config[CONF_SPAN_TIME] + 5)


postlund · 2020-12-16T22:10:02Z

custom_components/localtuya/cover.py

+            stored_pos = int(
+                self._stored_state.attributes.get("current_position", "-1")
+            )
+            if stored_pos != -1:


You return the string "-1" if the variable does not exist but compare with an integer here, is that intentional? Maybe just check if not None instead and skip default value?

rospogrigio · 2020-12-21T11:41:42Z

@TheDukeSr can you give me some feedbacks about the current implementation? Let me know...

TheDukeSr · 2020-12-22T20:10:15Z

Hi. I've send you a email. Op ma 21 dec. 2020 12:41 schreef rospogrigio <notifications@github.com>:

…

@TheDukeSr <https://github.com/TheDukeSr> can you give me some feedbacks about the current implementation? You can write me at ***@***.*** if you don't feel comfortable with the PR conversation. Let me know... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#213 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJTZSOGY2RBQ4BTELWSVDWDSV4YALANCNFSM4UMAJWAA> .

rospogrigio · 2020-12-30T14:12:00Z

@postlund I fixed your remarks, can you review this? Then we can merge it and publish a New Year release 😄 , please also send me a summary of the last changes because I lost track... thank you!

postlund · 2020-12-30T14:19:04Z

@rospogrigio Will fix later tonight 👍

postlund · 2020-12-30T18:07:42Z

I guess these are the commits of interest:

$ git log --oneline v3.1.0..upstream/master | grep -v " Bump "
d8de7e3 Merge pull request #255 from rospogrigio/timeout_log
0518f58 Merge branch 'master' into timeout_log
6cc178f Merge pull request #256 from rospogrigio/scene_fix_2
fd404cb Merge branch 'master' into timeout_log
444ec78 Fix review comment.
acf3e6e Merge branch 'master' into scene_fix_2
654c6cd Support adding and removing entities to devices (#191)
41a1e78 Merge branch 'master' into scene_fix_2
8e1e349 Fix HA scene, one more.
3a49403 Use debug message for timeout.
acd70b6 Fix name issue from pylint changes (#252)
52bf9cc Add dependabot config (#249)
f72cbc0 Run pylint from tox (#243)
3cf15f2 Merge pull request #240 from maartenweyns/master
a6e8f28 Fixed editor line indentation
c28fac7 This makes lights smoothly fade from one state to a new state, without the intermediate "turn off"
2d2a621 Merge pull request #238 from rospogrigio/light_fixes
3737dec Merge branch 'master' into light_fixes
d2a307f Merge pull request #239 from rospogrigio/tighter_heartbeat
89dabfc Reduce heartbeat interval.
d0653d4 Fix HA scenes with bulb without color_temp
aeb27a4 Fix improper closing in config flow (#237)
e7d58be Add localtuya.set_dp service (#236)
10e55e6 Add support for passive devices (#171)
1d58abd Update README.md
3b7e806 Update README.md
b50ee8b Properly close discovery listeners (#225)
a74af93 Fix for issue #186 (wrong initialization values for _timer_start and _previous_state)
095f649 Merge pull request #182 from rospogrigio/white_point
3ecae04 overzealous check.
14333b2 Use color picker white point.

Simple summary:

Support adding and removing entities to existing devices (Support adding and removing entities to devices #191)
Reduced heartbeat interval which should reduce amount of "heartbeat failed" errors (Reduce heartbeat interval. #239)
Fix improper closing in config flow that could result in "connection resets" after adding a new device (Fix improper closing in config flow #237)
Add support for passive devices, I.e. connection attempts are now made when discovery messages are received. This is a breaking change as these messages are now mandatory for the integration to function. Ensure UDP broadcasts for port 6666 and 6667 are forwarded to Home Assistant. (Add support for passive devices #171)
Add localtuya.set_dp service (Add localtuya.set_dp service #236)
Properly close discovery listeners which fixes "Address used for discovery is already in use" when trying to add a second device (Properly close discovery listeners #225)

There are a few other things related to light and stuff, but I know to little about that.

* Introduced read/store to JSON file of cover's last position for fake positioning * renamed "fake" positioning to "timed"; introduced timed movement timeout equal to full opening time * Made waiting for timeout for stopping non-blocking * Introduced status storing using RestoreEntity * Fixed postlund's remarks * Updated cover samples in README and info.md

rospogrigio added 2 commits December 3, 2020 14:45

Introduced read/store to JSON file of cover's last position for fake …

f5a49a9

…positioning

Introduced read/store to JSON file of cover's last position for fake …

3526fdc

…positioning

rospogrigio requested a review from postlund December 3, 2020 14:03

This was referenced Dec 3, 2020

Cover Fake Position not working. Value stuck at 50 #175

Closed

[Cover] Connection issue but works when toggling LED #207

Closed

Merge branch 'master' into cover_fake_position_persistent

8e4c8e2

renamed "fake" positioning to "timed"; introduced timed movement time…

5ec9b70

…out equal to full opening time

Made waiting for timeout for stopping non-blocking

16b6859

Merge branch 'master' into cover_fake_position_persistent

3518953

Fixed lint remarks

4a4296e

Merge branch 'master' into cover_fake_position_persistent

fd0f88b

rospogrigio and others added 3 commits December 16, 2020 14:47

Introduced status storing using RestoreEntity

feeb6de

Introduced status storing using RestoreEntity

e6c6a1b

Merge branch 'master' into cover_fake_position_persistent

d175b7f

postlund requested changes Dec 16, 2020

View reviewed changes

Merge branch 'master' into cover_fake_position_persistent

de09977

rospogrigio mentioned this pull request Dec 20, 2020

Update cover fake set position #224

Closed

rospogrigio mentioned this pull request Dec 28, 2020

Energy monitoring not refreshing #87

Closed

rospogrigio and others added 2 commits December 30, 2020 14:13

Merge branch 'master' into cover_fake_position_persistent

5a0ebcc

Fixed postlund's remarks

da6ad53

postlund approved these changes Dec 30, 2020

View reviewed changes

Updated cover samples in README and info.md

42cbb30

rospogrigio merged commit 9bfeb10 into master Dec 30, 2020

rospogrigio deleted the cover_fake_position_persistent branch December 30, 2020 21:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cover fake position persistent #213

Cover fake position persistent #213

rospogrigio commented Dec 3, 2020 •

edited

Loading

rospogrigio commented Dec 3, 2020

postlund commented Dec 3, 2020

rospogrigio commented Dec 3, 2020 •

edited

Loading

TheDukeSr commented Dec 3, 2020 •

edited

Loading

rospogrigio commented Dec 3, 2020

rospogrigio commented Dec 4, 2020

rospogrigio commented Dec 4, 2020

rospogrigio commented Dec 8, 2020

TheDukeSr commented Dec 8, 2020

rospogrigio commented Dec 9, 2020

TheDukeSr commented Dec 11, 2020 •

edited

Loading

rospogrigio commented Dec 12, 2020

rospogrigio commented Dec 12, 2020

rospogrigio commented Dec 14, 2020

rospogrigio commented Dec 14, 2020

TheDukeSr commented Dec 14, 2020 •

edited

Loading

rospogrigio commented Dec 15, 2020

TheDukeSr commented Dec 15, 2020 •

edited

Loading

rospogrigio commented Dec 15, 2020

postlund commented Dec 15, 2020

rospogrigio commented Dec 15, 2020

TheDukeSr commented Dec 15, 2020

postlund commented Dec 15, 2020

rospogrigio commented Dec 16, 2020

postlund Dec 16, 2020

postlund Dec 16, 2020

postlund Dec 16, 2020

postlund Dec 16, 2020

rospogrigio commented Dec 21, 2020 •

edited

Loading

TheDukeSr commented Dec 22, 2020 via email

rospogrigio commented Dec 30, 2020

postlund commented Dec 30, 2020

postlund commented Dec 30, 2020

Cover fake position persistent #213

Cover fake position persistent #213

Conversation

rospogrigio commented Dec 3, 2020 • edited Loading

rospogrigio commented Dec 3, 2020

postlund commented Dec 3, 2020

rospogrigio commented Dec 3, 2020 • edited Loading

TheDukeSr commented Dec 3, 2020 • edited Loading

rospogrigio commented Dec 3, 2020

rospogrigio commented Dec 4, 2020

rospogrigio commented Dec 4, 2020

rospogrigio commented Dec 8, 2020

TheDukeSr commented Dec 8, 2020

rospogrigio commented Dec 9, 2020

TheDukeSr commented Dec 11, 2020 • edited Loading

rospogrigio commented Dec 12, 2020

rospogrigio commented Dec 12, 2020

rospogrigio commented Dec 14, 2020

rospogrigio commented Dec 14, 2020

TheDukeSr commented Dec 14, 2020 • edited Loading

rospogrigio commented Dec 15, 2020

TheDukeSr commented Dec 15, 2020 • edited Loading

rospogrigio commented Dec 15, 2020

postlund commented Dec 15, 2020

rospogrigio commented Dec 15, 2020

TheDukeSr commented Dec 15, 2020

postlund commented Dec 15, 2020

rospogrigio commented Dec 16, 2020

postlund Dec 16, 2020

Choose a reason for hiding this comment

postlund Dec 16, 2020

Choose a reason for hiding this comment

postlund Dec 16, 2020

Choose a reason for hiding this comment

postlund Dec 16, 2020

Choose a reason for hiding this comment

rospogrigio commented Dec 21, 2020 • edited Loading

TheDukeSr commented Dec 22, 2020 via email

rospogrigio commented Dec 30, 2020

postlund commented Dec 30, 2020

postlund commented Dec 30, 2020

rospogrigio commented Dec 3, 2020 •

edited

Loading

rospogrigio commented Dec 3, 2020 •

edited

Loading

TheDukeSr commented Dec 3, 2020 •

edited

Loading

TheDukeSr commented Dec 11, 2020 •

edited

Loading

TheDukeSr commented Dec 14, 2020 •

edited

Loading

TheDukeSr commented Dec 15, 2020 •

edited

Loading

rospogrigio commented Dec 21, 2020 •

edited

Loading