Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

is_open / is_closed not correctly reported if position is inverted #257

Closed
2 tasks done
SoftXperience opened this issue Jan 24, 2020 · 22 comments · Fixed by #259
Closed
2 tasks done

is_open / is_closed not correctly reported if position is inverted #257

SoftXperience opened this issue Jan 24, 2020 · 22 comments · Fixed by #259

Comments

@SoftXperience
Copy link

Description of problem:
The cover device directs the is_open and is_closed methods to the TravelCalculator. The problem is, that the TravelCalculator doesn't know about inverted position. So, is_open returns false if the position is 0. But with inverted position 0 is open.

I'm not sure where to fix it... should we change the TravelCalculator to correctly report open/closed? Or should we change the cover device which could check itself if current_position == open_position (which is correctly stored in the device).

Please give an advice and I could submit a PR

  • using xknx standalone
  • using Home-Assistant knx plugin

Version information:

  • xknx / Home-Assistant release with the issue: 0.104.2
  • last working xknx / Home-Assistant release (if known):

KNX installation:
Using a cover actuator from Jung.

Problem-relevant xknx.yaml or configuration.yaml entries (fill out even if it seems unimportant):

  • platform: knx
    name: bath
    move_long_address: '8/2/1'
    move_short_address: '8/1/1'
    position_address: '8/3/1'
    position_state_address: '8/4/1'
    invert_position: true

Traceback (if applicable):

@farmio
Copy link
Member

farmio commented Jan 26, 2020

Thanks for pointing this out.
I'd just

def is_open(self):
        """Return if cover is open."""
        return self.travelcalculator.is_open() ^ self.invert_position

and same for is_closed in cover.py and leave travelcalculator as is (so we don't have to mess with movement directions and keep the calculator simple).

Edit: Doesn't the travel calculator receive its position information in the same way regardless of inverted True or False? I would think

        position_range_from = 0 if invert_position else 100
        position_range_to = 100 if invert_position else 0
        self.position = RemoteValueScaling(
            xknx,
            group_address_position,
            group_address_position_state,
            device_name=self.name,
            after_update_cb=self.after_update,
            range_from=position_range_from,
            range_to=position_range_to)

should normalize its input.

@SoftXperience
Copy link
Author

I tested it in my home assistant environment. The xknx cover device reports the correct percent value. So it reports 0 if the cover is fully open, 100 if it is fully closed and 20 if it is closed only a small part. So yes, the value is correctly scaled and calculated. Also the slider in the UI works correct to set a new position.

But the TravelCalculator doesn't know about the inverted open/close values. It receives only the travel times. In the init method it is hardcoded to position_closed = 0. So it reports is_closed = true for the value 0. Although it is inverted in my environment.

So I guess your first suggestion would be the simplest one?

@farmio
Copy link
Member

farmio commented Jan 26, 2020

🤔 shouln't 0=open and 100%=closed be standard in KNX?
I just checked for ABB, Gira and MDT actuators.

@SoftXperience
Copy link
Author

Hmm, I don't know. I only have Jung actuators for blinds. Oh and I have an MDT actuator for my marquee and there it is the same, 100% is closed.

@farmio
Copy link
Member

farmio commented Jan 27, 2020

@SoftXperience could you test this branch: https://github.com/farmio/xknx/tree/cover-fix-is-open
I have no blind actuator to test it in the field.

@SoftXperience
Copy link
Author

Hmm... I'm new to the python ecosystem... I have a development environment for homeassistant, bit I believe xknx is not included, isn't it? So is there an easy way to integrate your branch in my test environment? Is it easily explained? Otherwise I have to google around a bit to get it working...

@farmio
Copy link
Member

farmio commented Jan 27, 2020

Yes. See https://xknx.io/home_assistant
It doesn't work with Hassio but with normal home-assistant installation.

Note that you will have to replace "knx" with "xknx" as platform in HA-configuration.

@SoftXperience
Copy link
Author

Ok, I tested your branch. I still have to use the "invert_position=true" option?!

If I do, fully open is reported as position 0 and open. Fully closed is reported as 100 and close. All positions inbetween are reported as open (which I think is correct).

But now the buttons in the lovelace UI are still wrong. The up-button is closing the cover and vice versa.
Also in fully open state only the up-button is enabled.

@farmio
Copy link
Member

farmio commented Jan 28, 2020

Yes. Unfortunately HA defines it contrary to knx so invert_position is still needed.
When you say it reports 0 as open do you mean in ETS?

Can you post your configuration?
Did you get this commit farmio@3cd87f4 ?

@SoftXperience
Copy link
Author

SoftXperience commented Jan 28, 2020

I just pulled and now the buttons have the correct direction (down-arrow closes, up-arrow opens). Sorry, didn't notice that you made further commits meanwhile.

But still one problem: in closed state, only the down-arrow is enabled (and vice versa).

Yes, 0 in ETS and openhab.

- platform: xknx name: work_gable move_long_address: '8/2/11' move_short_address: '8/1/11' position_address: '8/3/11' position_state_address: '8/4/11' invert_position: true

@farmio
Copy link
Member

farmio commented Jan 28, 2020

Ok. could you try this commit without invert_position: farmio@e48c3af
I think it should be good now.
I am constantly confused by the HA-ui slider where left is closed and right is open 🙃

@SoftXperience
Copy link
Author

The buttons (enable/disable) react correct.

But now the position is inverted again. Fully open is correctly reported as "open", but the position is reported as 100 (which leads to the confusing slider). Before the position was correctly shown as 0.

@farmio
Copy link
Member

farmio commented Jan 29, 2020

With invert_position to true or false?

@SoftXperience
Copy link
Author

Still same config with inverted = true

@farmio
Copy link
Member

farmio commented Jan 29, 2020

Good. With invert set to false it should work correct now.
There are still some mixups regarding inversion in the xknx code. I'll try to correct them later today.

@SoftXperience
Copy link
Author

No, I'm sorry.

With inverted=false, fully open blind is in HA:

  • open (correct)
  • position 100 (wrong)
  • down-button enabled (correct)
  • down-button closes (correct)
  • going to position 25 moves blind to 3/4 closed (wrong)

With inverted = true, fully open blind is in HA:

  • closed (wrong)
  • position 0 (correct)
  • down-button disabled (wrong)
  • down-button closes (correct)
  • going to position 25 moves blind to 1/4 closed (correct)

So still messed up... Still think the base version with just inverted close/open states may work correctly?

Another question: I'm not sure if we really need the TravelCalculator... at least my two blinds actuators (Jung, MDT) have group addresses to set the new position and the acuator calculates and controls the travel times. So maybe we just need to write to the position group address (at least if it is set?) and ignore the calculator...

@farmio
Copy link
Member

farmio commented Jan 29, 2020

Not all actuators support position states and we gain finer (calculated) states from actors that only send state when the target position is reached.

Position 100 in HA is meant to be fully open.
If you set position 25 in HA it should be 3/4 closed. The first try with inverted True seems ok to me.

@SoftXperience
Copy link
Author

Hmmm, this is strange... How can I think a cover with 100% does cover nothing? For me this is not logical. Are you sure that it is meant this way? Is it documented somewhere?

So maybe I have to retest your changes because I didn't thought that way.

And I'm not sure how to go on, now because I don't like it that way and maybe have my own fork with that fixed. Which would be annoying in docker environment where my productive system will run...

@farmio
Copy link
Member

farmio commented Feb 2, 2020

Be sure to check out the fork again. I made some more commits since.

I don't know if it is defined in the documentation, but in the code. I have already linked it here: #257 (comment)

I'd stay on normal HA and xknx builds to get updates. If you don't like the slider direction you can always customize a Lovelace card where it is reversed.
The numbers the backendes deal with don't matter imho as long as everything work as supposed.

@SoftXperience
Copy link
Author

Oh sorry, I didn't see the link. Ok, feels still very strange. I used fhem and (currently) openHAB before and none of them use it that way.

Customizing the card is a good idea, but I guess I also have to invert all my scripts and automations.

I'll try to test it again this evening. Your latest changes as well as the current implementation in HA, maybe everything works as intended (although I don't like it)...

@SoftXperience
Copy link
Author

So, to be honest, I'm really confused now... I just retested the current implementation in HA.
blind_closed_dark_in_the_room
This is the state in HA when the blind is fully closed, so that it is dark in the room.#

The buttons of the NOT inverted blink are working correctly (in MY understanding, as MY blinds are moving from top to down to close). So the down button closes the blind. So the down button is disabled correctly, as it is closed. Also the closed state is correclty reported as you can see at the icon.
Only problem is that the slider (and so the position) is reported as 0. So if I set it to position 25, the blind is 3/4 closed which is opposed to what my actuator reports and my understanding. But if this is correct from the viewpoint of HA, it is working as supposed.
And it is confusing, as the integration docs state that if the actuator reports 100% as fully closed, I have to set inverted=true

If I use inverted=true, the buttons and the open/closed state is swapped. Although the position is correct.

So, this was for the current implementation. Now I will test the same with your current branch.

@SoftXperience
Copy link
Author

So with your current branch, the inverted=false mode looks very similiar. But also with wrong (in my opinion) position.
The inverted=true is not working correctly, as I can press the down button in closed state (which will lead to no movement because the actuator knows it is already closed), but the travelcalculator interpolates the time to let the position move from 100 to 0.

But before we further test the implementation, I think we have to clarify what intended behaviour is...
Which position should be open? And if I use invert, what should be inverted? Just the position or also the direction? Also I would like an option to not use the TravelCalculator as my actuators do that on their own and I don't want to confuse the system with additional travel times and needless stop telegrams. 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants