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

Don't wait until final position of Velux cover is reached #21558

Merged
merged 1 commit into from Mar 10, 2019

Conversation

Projects
None yet
4 participants
@Julius2342
Copy link
Contributor

commented Mar 1, 2019

Description:

  • Bump version to latest version of pyvlx: 0.2.10. Library more failure tolerant, when detecting an unsupported device.
  • When calling API (e.g. run scene, change position) don't wait until device has reached target position (This caused HASS to be flaky while the device was moving)
  • Support for vertical and horizontal awnings.
@elupus

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

This seem strange to me. But there might be some background reason. If this really is needed add them as jobs instead. That way you have control of exception stack/cancelation.

But why can't we have long running services?

@Julius2342

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

within the library, window.open() returns when the window is open.

(Internally, you set a command on the bus, you first get a CommandSendConfirmationStatus when the API accepted the command and a FrameSessionFinishedNotification when the device has reached the target position. The default is to wait for the latter. )

@Julius2342

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

@elupus : But I still wonder why HASS gets flaky when I do asyncio.Event().wait(). Any ideas?

@elupus

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

Do you have a reference to what you mean by flaky?

@Julius2342

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

Flaky as in "not responding".

Usecase 1

I have the following configuration:

binary_sensor:
- platform: knx
  name: Button1
  address: '5/0/8'
  automation:
    - action:
         - entity_id: scene.bath_open  # A Velux scene, opening the window.
           service: scene.turn_on

- platform: knx
  name: Button2
  address: '5/0/10'
  automation:
    - counter: 1
      hook: 'on'
      action:
        - entity_id: light.bath
          service: homeassistant.turn_on

When first press Button1 and while the windows open Button2, the lights will only switch on after the movement finished.

Usecase 2

Another user reported that with his configuration:

cover:
  - platform: group
    name: "Family Room Window Shades"
    entities:
      - cover.family_room_shade_1
      - cover.family_room_shade_2
      - cover.family_room_shade_3
      - cover.family_room_shade_4
      - cover.family_room_shade_5

the covers open "one by one" after triggering the group. With my change, all shades open simultanously.

@elupus

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2019

@elupus

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2019

Not sure this is the issue, but.. do_api_call(self) can't really run reliably in parallel. If a new call comes in while awaiting send or reply. The internal state will get broken. That really need a refactor to support case with multiple calls in parallel. (it should probably have asyncio.Lock()) until then.

You should be able to reproduce by hacking up something that does:
asyncio.wait(do_api_call(longrunningcmd), do_api_call(shortrunningcmd_with_small_start_delay))

The short command should finish first, even if the long running starts first. Right now this won't work with the single global wait event.

It could be the culprit, but not fully sure.

@elupus

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2019

So given above, I think the change in this pull is correct. Also makes more sense to be able to abort the long running command by requesting a new position.

@Julius2342

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

@elupus : requesting a new position as well as stop() works. The underlaying library can handle arbitrary api calls in parallel.

@elupus

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2019

The wait on the event will wake up wrongly if two requests are waiting at the same time I think. But as I said I still think change here make sence.

@fabaff fabaff changed the title Velux component: don't wait until final position is reached. Don't wait until final position of Velux cover is reached Mar 7, 2019

@robbiet480 robbiet480 merged commit 6f77d9b into home-assistant:dev Mar 10, 2019

4 checks passed

Hound No violations found. Woof!
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.002%) to 92.748%
Details

@ghost ghost removed the in progress label Mar 10, 2019

@balloob balloob referenced this pull request Mar 20, 2019

Merged

0.90.0 #22216

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.