Better support for covers

[EnricoBilla]

I'm editing the way covers are integrated. As it is written now it takes for granted that every cover has a datapoint to set a specific position, which is not always the case; so I want to generalize it a little bit more.

[Tho85]

Maybe we should implement a method in fah/devices/fah_cover.py like this:

class FahCover(FahDevice):
    def supports_position(self):
        """Returns true if cover supports position"""
        return PID_SET_ABSOLUTE_POSITION_BLINDS in self._datapoints

and check for that in cover.py in the supported_features function?

[EnricoBilla]

I was thinking of something similar, I'll implement it!

[Tho85]

@EnricoBilla Looks good. Could you give me a hint when you're functionally complete with this PR? I'd like to add a test for it, based on the config snippet you sent me. Or maybe you could add one?

[EnricoBilla]

@Tho85 I'll add tests later, before merging to master. First I want to get my covers to work, but I have to troubleshoot it more. For now my covers are initialized correctly (I can see attributes correctly set in HA) but after initialization they become unavailable

[EnricoBilla]

Note that here I'm not 100% sure that state=1 means the cover is closed, it seems to be right but I had strange result when testing it. If someone could test it a little bit more I would be happy

[Tho85]

That's a better solution. I can confirm that it works perfectly with my 1-gang and 4-gang blind actuators. :+1: Actually, I accidentially included that datapoint already in my test fixtures: https://github.com/jheling/freeathome/blob/master/freeathome/tests/fixtures/1013_update_closed.xml#L11

[EnricoBilla]

@Tho85 Thanks, that's perfect. I'll add tests later and this will be ready to merge I think

[EnricoBilla]

I was trying to run tests, but everytime I launch pytest tests/ it fails because the import inside the test files are on an upper level directory. @Tho85, how did you ran tests?

[Tho85]

I always run them from the freeathome directory like this: python3.8 -m pytest --log-level=DEBUG tests/

I'm not a Python expert, so maybe there's a better way.

[EnricoBilla]

Okay the tests ran. I also found an error in the cover test, in fact as you can see the cover was asserted not close while in reality it was, given the state is 1.

[Tho85]

Nice, the get-master-message.py definitely needed an overhaul. Have a look at #77 where I've basically moved its functionality into a service. So everyone with a running HA instance can get a dump without having a working Python installation.

I mainly used the get-master-message.py for updates that were sent after the initial configuration, e.g. when hitting a switch in the wall. It looks like that got removed, right?

[EnricoBilla]

@Tho85 daamn, I didn't think of that. I used get-master-message.py only to get the config xml; can confirm it doesn't work the way it did. Gonna fix it! I've just seen that new service, very useful for end users

[Tho85]

Maybe we should get rid of it altogether and add an additional service for that? The MQTT integration has a dump service that listens for a specified amount of time and dumps all received messages (see docs). We could implement something similar. I will have a go at it tonight.

[EnricoBilla]

I think a service is more useful, I updated the get-master-message.py to make it work with the problem of duplicate attributes

[EnricoBilla]

Ookay, I added tests for the cover, all of them are passing, I also reverted back get-master-message.py since in future this will become a service in HA. I think this is ready to be merged; if someone could test it one more time to make sure it's working correctly it would be perfect

[Tho85]

I tested your branch, and I must correct myself regarding the value 1=closed. It looks like it is more nuanced than that, and the failure in the test wasn't actually a failure... I observe the following:

• When opening the cover: state=2. When opening movement stops, state=0
• When closing the cover: state=3. When closing movement stops, state=1

In other terms:

direction = state & 0x01
moving = state & 0x02

So your PR changes the behaviour of covers, because now all my covers are reported as closed with a position of 20%, whereas before everything above 0% was considered as open.

Would it be possible to keep the existing logic if the cover supports reporting a position, and only consider the state when it doesn't?

[Tho85]

Tests for your device look good, but I had minor issues with the pytest.mark.asyncio. I got the following message (tons of them) when running the tests:

tests/test_binary_sensor.py:224
  /workspaces/config/custom_components/freeathome/tests/test_binary_sensor.py:224: PytestUnknownMarkWarning: Unknown pytest.mark.asyncio - is this a typo?  You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/stable/mark.html
    @pytest.mark.asyncio

Looks like we have to add pytest-asyncio==0.14.0 to requirements.txt, and add a tests/pytest.ini with the following content:

[pytest]
markers =
  asyncio: Async

Then the messages disappear. Also, it looks like it is not needed to mark all methods, but set a global marker in each file, at the top:

import pytest
pytestmark = pytest.mark.asyncio

[Tho85]

And BTW, I just added a freeathome.monitor service in #77 and removed the get-master-message.py there.

[EnricoBilla]

Okay, I get what you're saying. Since my covers only supports moving but not setting a position the state=1 happens only when the cover is closed. That's a thing to fix, the other problem is that now I'm not sure what the state will be if I stop my cover halfway

[Tho85]

I've added a compromise as a PR to your repo, have a look: https://github.com/EnricoBilla/freeathome/pull/1

[EnricoBilla]

Looks like we have to add pytest-asyncio==0.14.0 to requirements.txt

Uhm yes, I totally forgot to add it to the requirements. For me it was working well even without the ini file, but that's a minor thing to add. As for the marker, way better a one liner at the top instead of a line for each method

[Tho85]

Looks good from my perspective :+1:

[EnricoBilla]

I did some testing and I found that you are right, even on my devices state=1 doesn't mean the cover is closed, it does mean the cover has been stopped after a closing movement. I thought of keeping track of how many seconds the cover has been closing; but that would mean having to configure a total closing time for each cover etc. that implies a lot of other complications. Now I'm searching a cleaner way to detect whether the cover is closed or open...

[Tho85]

According to the developer docs covers may return None for is_closed if the state is unknown. So maybe the cover could be implemented like this:

 def is_cover_closed(self):
    """ Return if the cover is closed   """
    if self.supports_position():
        return int(self.position) == 0

    if self.state != '1'
        return False

    return None # for clarity

The rest could be done through an automation and a python script. If the cover is closing for x seconds, manually set the cover state to closed through a python script.

[EnricoBilla]

From what I've seen returning False to is_closed implies the cover is open, so the third and second to last lines should be removed. I will test and commit it. Thanks, I didn't know the status could be edited from an external python script 👍🏼

[EnricoBilla]

That's all I think

[EnricoBilla]

@jheling This PR could be merged as far as I'm concerned

[Tho85]

@EnricoBilla There are conflicts, the PR needs a rebase / master merge

[EnricoBilla]

@Tho85 Ahh damn, there wasn't any conflict last week, maybe they appeared after he merged your PRs. Thanks for having told me cause I didn't look at it

[jheling]

I'm new in resolving conflicts on github. Yesterday I tried to use github desktop to resolve this case, but the user interface is not very inituitive for me.

[jheling]

I did another try. Can you check if it is ok now?

[EnricoBilla]

From a quick look it seems okay to me, but I can't look at it better in these days