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

Window covering now support position and tilt position in % #40

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

smodard
Copy link

@smodard smodard commented Sep 15, 2024

  1. The configuration still do not ask for all group addresses. I did not find how to change that.
  2. The cursor for setting the (tilt) position should start from the bottom to mimic the store position on the window. using the invert check box is working but not the widget.
  3. I added the DPT5.001 as a real percentage

Thanks for taking this PR into consideration to improve window covering.

I do use this PR in my homey pro and can now handle all my sun blinds as they should (using the 2 cursors only)

@ttherbrink
Copy link
Contributor

Thanks for your work.
To answer your questions:

  1. To add the group adresses during pairing, You can add them to the pairing view here: https://github.com/athombv/org.knx/blob/master/.homeycompose/drivers/pair/select_groupaddresses/index.html
  2. I do not really understand what you mean. Could you elaborate more?

It would be nice if you could add a check during the onInit() that checks if the new adresses are filled and according to that registers the capability listeners. When people migrate from a previous version they would get new UI elements that do not work as the adreses are not filled in.

As for the DPT17 change: Could you explain why this change is needed?

If there are any questions feel free to ask.

@smodard
Copy link
Author

smodard commented Sep 16, 2024

  1. For the DPT17, with your code you always receive 0 as scenario value. The change is implemented on my Homey PRO and all is working with my already installed knx participants. I do now receive 0 to 63 as scenario value
  2. For a blind, we only need 2 (2 more for status) address groups, 1 to set the store position (0-100%) and 1 to set the slat position (0-100%). As those are cursors, the best would be to have a cursor starting from up to bottom (mimicking the store position on the window : 0% is up and 100% when fully down). Same for slat/tilt.

@ttherbrink
Copy link
Contributor

  1. Thanks! makes sense.
  2. If i understand correctly this is more a UI problem right? You can see it as x% opened instead of closed. At this moment this behaviour is universal between all blinds within homey, So it is beneficial for the users of multiple brands to keep the same behaviour.

@smodard
Copy link
Author

smodard commented Sep 16, 2024

I added the fields for configuration.
You ask to check capabilities in onInit() method depending on address configuration but looking at all the other devices, you do not do it either. So why me ?
The only thing that I should know is how to remove the up/stop/down UI screen of the window covering device.

Everything is currently being tested in my Homet PRO and all works fine.

PS : When too much KNX telegrams, the Homey seems to loose the LAN connection to the KNX IP gateway.

@ttherbrink
Copy link
Contributor

The reason why i ask this is because of migration of other users.
As capabilities are only added when the device is paired so existing devices will not get your newly added features.
Adding and removing capabilities will also remove or add the UI components.
For example:

If (this.settings.ga_slat_position ==! '' && !this.hasCapability(windowcoverings_tilt_set)) {
this.addCapability(windowcoverings_tilt_set); 
}
If (this.settings.ga_slat_position === '' && this.hasCapability(windowcoverings_tilt_set)) {
this.removeCapability(windowcoverings_tilt_set); 
}

PS: If you might know when or why this connection drop happens please let me know. We have a limited setup so not that many telegrams

@ttherbrink ttherbrink self-requested a review December 3, 2024 15:11
Copy link
Contributor

@ttherbrink ttherbrink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please adress the above comments for this PR to be merged

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

Successfully merging this pull request may close these issues.

2 participants