Skip to content

Support for new velux api, added cover.velux#18738

Merged
rytilahti merged 17 commits into
home-assistant:devfrom
Julius2342:new-velux-api
Feb 1, 2019
Merged

Support for new velux api, added cover.velux#18738
rytilahti merged 17 commits into
home-assistant:devfrom
Julius2342:new-velux-api

Conversation

@Julius2342
Copy link
Copy Markdown
Contributor

@Julius2342 Julius2342 commented Nov 27, 2018

Description:

Upgraded pyvlx to version 0.2.*. Users will need to upgrade to a recent firmware version. Old and new firmwareversions are not compatible (both use a completely different API).

Added support for controlling Velux windows one by one.

Configuration should stay the same. Component will fetch the available devices/nodes from API.

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#7867

EDIT: Within the changelog of the release we should point to the page where the new Firmware may be downloaded: https://www.velux.com/api/klf200

@WoodsterDK
Copy link
Copy Markdown

Hi I noticed one issue:

async def async_open_cover(self, **kwargs):
    """Open the cover."""
    await self.node.close()

should be async def async_open_cover(self, **kwargs):
"""Open the cover."""
await self.node.open()

And

def current_cover_position(self):
    """Return the current position of the cover."""
    return 0

can be changed to support position query:

def current_cover_position(self):
    """Return the current position of the cover."""
    return self.node.current_position

@Julius2342
Copy link
Copy Markdown
Contributor Author

@WoodsterDK ty for you feedback! i already spotted the s/close/open/ - thing, but did not yet commit. I'm currently working on retrieving the current position from various feedback/status messages, but this still takes some iterations. I will post an update here if i have a working prototype :)

@WoodsterDK
Copy link
Copy Markdown

@Julius2342 no problem - I'm up for testing it, when you have something ready.
Let me know if you need some help.

Comment thread homeassistant/components/cover/velux.py Outdated
if ATTR_POSITION in kwargs:
position_percent = 100 - kwargs[ATTR_POSITION]
from pyvlx import Position
await self.node.set_position(Position(position_percent=position_percent))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (85 > 79 characters)

@Julius2342 Julius2342 changed the title [WIP] Support for new velux api, added cover.velux Support for new velux api, added cover.velux Dec 15, 2018
@Julius2342
Copy link
Copy Markdown
Contributor Author

@WoodsterDK ^^

@WoodsterDK
Copy link
Copy Markdown

Hi,
The covers are not being inserted as entities.

  • I think there might be an issue if no scenes are present
  • I have seen an "out of index"
    image

@Julius2342
Copy link
Copy Markdown
Contributor Author

Julius2342 commented Dec 19, 2018

@WoodsterDK : do you get any error when initializing the the component? It should load all entities automatically on startup:

    async def async_start(self):
        """Start velux component."""
        await self.pyvlx.load_scenes()
        await self.pyvlx.load_nodes()

@Julius2342
Copy link
Copy Markdown
Contributor Author

@WoodsterDK : may you retry?

@WoodsterDK
Copy link
Copy Markdown

WoodsterDK commented Dec 19, 2018

@WoodsterDK : may you retry?

@Julius2342 Will test tomorrow...
Could it be caused by a combination with the switches I have? - the nodeIDs for my covers might not come in a numerical order due to the switches.
Do you lookup in the node array according to the nodeID or by an index found by searching for the nodeID?

@Julius2342
Copy link
Copy Markdown
Contributor Author

Julius2342 commented Dec 19, 2018

@WoodsterDK : When retrieving the scenes, my old implementation was waiting for the last FrameGetSceneListNotification with remaining_scenes==0. If in your implementation no scenes are configured, this frame is never sent, the whole thing runs into a timeout and an exception is raised that the scene list was not retrieved.

@Julius2342
Copy link
Copy Markdown
Contributor Author

@WoodsterDK : And you are right, there was another bug with accessing the nodes via index-id instead of node_id (which makes more sense).

@WoodsterDK
Copy link
Copy Markdown

@WoodsterDK : And you are right, there was another bug with accessing the nodes via index-id instead of node_id (which makes more sense).

OK - just tried running with the updated library, and my covers seems to added as entities - GREAT.
I can control them, but it seems the position is not being correctly reflected.

Furthermore it could be nice to add the support of the feature cover.stop_cover in the component.

If I look in the log every 10 second there is this:
image
Even if there is no movement of the covers - do you poll for states from the products, or is it only the internal values that are being "broadcastet"?

@Julius2342
Copy link
Copy Markdown
Contributor Author

what do you mean with not being correctly reflected. . I'dont know what the correct correlation is.

WRT cover.stop, do you know which Frame to send?

No i do not poll. I activated the house-monitor. During movement they broadcast their position from time to time. Does this stop after the covers reached their final position?

@WoodsterDK
Copy link
Copy Markdown

@Julius2342 Great work - a couple of things:

  • notification of position seems to be working, if the slider is being used.
  • stop works
  • position is being messed up when the movement is stopped by pressing "Stop"

@jpearce73
Copy link
Copy Markdown

Thanks for this work, I'd love to try it out.
I just upgraded homeassistant to 0.85.1. I found the components/velux.py and discovered its still the old version. Where do I learn how to pull pre-release change set like this?

@apeeters
Copy link
Copy Markdown
Contributor

I tested this branch and it works works perfectly. Thanks for the great work! Can't wait to have it merged :)

@WoodsterDK
Copy link
Copy Markdown

@Julius2342 have you had a chance to look at:

notification of position seems to be working, if the slider is being used.
stop works
position is being messed up when the movement is stopped by pressing "Stop"

@Julius2342
Copy link
Copy Markdown
Contributor Author

@WoodsterDK : Yes, started a small refactoring but then I was interrupted. Will try to continue soon.

@apeeters
Copy link
Copy Markdown
Contributor

@WoodsterDK I did not experience that problem when pressing stop.

@WoodsterDK
Copy link
Copy Markdown

@apeeters if you operate the cover, press stop close the ui card and open it again, the slider is positioned in a wrong position

@apeeters
Copy link
Copy Markdown
Contributor

@WoodsterDK @Julius2342 I was able to reproduce this. It does not always occur and is caused by negative position values. Initially all my covers (closed) have position -23, after moving and stopping I sometimes get -5.

@l-mb
Copy link
Copy Markdown
Contributor

l-mb commented Jan 23, 2019

Very interested in seeing this merged, let me know if I can test via a beta channel/repo for hass.io.

@Julius2342
Copy link
Copy Markdown
Contributor Author

@WoodsterDK may you retry?

@WoodsterDK
Copy link
Copy Markdown

@Julius2342 Sure thing - will see if I can fit in try today

@callback
def async_register_callbacks(self):
"""Register callbacks to update hass after device was changed."""
async def after_update_callback(device):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Question, should the async def be outside of the def? I'm not 100% familiar with this yet.
example: cover.template.pyL201

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that's okay, as it is just passed as a callback (which I assume gets awaited later on). The @callback marks this as something that does not do I/O (for optimization), see: https://developers.home-assistant.io/docs/en/asyncio_categorizing_functions.html#the-callback-function

@WoodsterDK
Copy link
Copy Markdown

@WoodsterDK may you retry?

@Julius2342 - just tested, with the reference to 'pyvlx==0.2.8' it seems to be working as expected... COOL :)

@rytilahti
Copy link
Copy Markdown
Member

Added the 'breaking change' label as according to the initial descriptions this will be such for existing users.

Comment thread homeassistant/components/velux.py Outdated
host=host,
password=password)
password=password,
log_frames=True)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does this option do as it sounds like a debugging option, and why is it wanted here? Otherwise (considering the reports that this is working on real devices) this should be ready to be merged.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I removed this parameter!

@Julius2342
Copy link
Copy Markdown
Contributor Author

@rytilahti : I addressed your issue.

@rytilahti
Copy link
Copy Markdown
Member

Thanks! 👍 I bolded the need to mention this in the changelog (inside description), as I don't think we have a label to highlight specific parts at the moment.

@rytilahti rytilahti merged commit 7479410 into home-assistant:dev Feb 1, 2019
@ghost ghost removed the in progress label Feb 1, 2019
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Good! Just a small comment.


async def async_set_cover_position(self, **kwargs):
"""Move the cover to a specific position."""
if ATTR_POSITION in kwargs:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is already validated by the service schema.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants