Skip to content

Multi-Room Support for Greenwave Reality#11364

Merged
pvizeli merged 9 commits into
home-assistant:devfrom
dfiel:dev
Jan 25, 2018
Merged

Multi-Room Support for Greenwave Reality#11364
pvizeli merged 9 commits into
home-assistant:devfrom
dfiel:dev

Conversation

@dfiel
Copy link
Copy Markdown
Contributor

@dfiel dfiel commented Dec 29, 2017 •
edited
Loading

Description:

Added support for gateways with multiple rooms already set up. No longer errors when more than 1 room is defined.

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

Example entry for configuration.yaml (if applicable):

light:
  - platform: greenwave
    host: 192.168.1.97
    version: 3

Checklist:

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

houndci-bot
houndci-bot reviewed Dec 29, 2017
View reviewed changes
Comment thread homeassistant/components/light/greenwave.py Outdated
doc = greenwave.grab_xml(host, token)
add_devices(GreenwaveLight(device, host, token) for device in doc)
for room in doc:
add_devices(GreenwaveLight(device, host, token) for device in room['device'])
Copy link
Copy Markdown

@houndci-bot houndci-bot Dec 29, 2017

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)

@dfiel dfiel requested a review from andrey-git as a code owner December 29, 2017 17:54
@tinloaf
Copy link
Copy Markdown
Contributor

tinloaf commented Dec 29, 2017

Looking at the existing code, it looks like the update logic is not optimal. Am I right assuming that every light will hit the XML API on its update, retrieving all information? I.e., the API will always be called multiple times? I think it would be way better to cache that XML document together with a time stamp somewhere (in hass.data?) and re-use it if the time stamp is not too old. One would have to force an update after service calls which change anything.

I'm not saying that this PR shouldn't be merged without this improvement, I'm just saying that if you feel up to the task… 😉

@dfiel
Copy link
Copy Markdown
Contributor Author

dfiel commented Dec 29, 2017

@tinloaf Yes, your assumption is correct. This is modeled after the android app, but I do have plans to create an update function that updates all bulbs on one XML call. Hopefully in the next update.

@dfiel
Copy link
Copy Markdown
Contributor Author

dfiel commented Jan 18, 2018

Bump? I was hoping this would make it into the .61 release

pvizeli
pvizeli requested changes Jan 22, 2018
View reviewed changes
Comment thread homeassistant/components/light/greenwave.py Outdated
self._state = int(device['state'])
self._brightness = greenwave.hass_brightness(device)
self._online = greenwave.check_online(device)
self._name = device['name']
Copy link
Copy Markdown
Member

@pvizeli pvizeli Jan 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

That is a realy bad runtime. Make a data object like this: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/sensor/yweather.py#L174-L190
On this data object you can map it to a usefuly data format and access without loop.

Your code actual:

  • Hit the device with 10x bulbs it call the gateway 10x times
  • Loop over 1000x

@dfiel dfiel mentioned this pull request Jan 23, 2018
Closed
@amelchio
Copy link
Copy Markdown
Contributor

amelchio commented Jan 23, 2018

@dfiel an inefficient platform is frowned upon because it can slow down all of Home Assistant. What an Android app does is actually irrelevant in this regard.

However, this PR does not introduce the inefficiency but it rather fixes an error (that is even being reported now). So how about we merge this PR if @dfiel promises that the next PR will be to implement the shared state object?

@dfiel
Copy link
Copy Markdown
Contributor Author

dfiel commented Jan 23, 2018

Already working on it :)

@dfiel
Shared State Object
5cbb443 
Shared State Object added, and implemented new function of greenwavereality to abstract complex runtimes.
@dfiel
Copy link
Copy Markdown
Contributor Author

dfiel commented Jan 24, 2018

@pvizeli Does this solve the issue? I believe it does.

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Jan 24, 2018

Yes

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Jan 24, 2018

Please fix lint and we can merge it

@pvizeli pvizeli self-assigned this Jan 24, 2018
@dfiel
Copy link
Copy Markdown
Contributor Author

dfiel commented Jan 24, 2018

@pvizeli Lint issues fixed

@pvizeli pvizeli merged commit caa16da into home-assistant:dev Jan 25, 2018
@balloob balloob mentioned this pull request Jan 26, 2018
Merged
matemaciek pushed a commit to matemaciek/home-assistant that referenced this pull request Jan 27, 2018
@matemaciek-9ld
Merge remote-tracking branch 'upstream/master' into 9ld
4c60a3c 
* upstream/master: (465 commits)
  Update pyhomematic to 0.1.38 (home-assistant#11936)
  Implement Alexa temperature sensors (home-assistant#11930)
  Fixed rfxtrx binary_sensor KeyError on missing optional device_class (home-assistant#11925)
  Allow setting climate devices to AUTO mode via Google Assistant (home-assistant#11923)
  fixes home-assistant#11848 (home-assistant#11915)
  Add "write" service to system_log (home-assistant#11901)
  Update frontend to 20180126.0
  Version 0.62
  Allow separate command and state OIDs and payloads in SNMP switch (home-assistant#11075)
  Add ERC20 tokens to etherscan.io sensor (home-assistant#11916)
  Report scripts and groups as scenes to Alexa (home-assistant#11900)
  Minor fix to configuration validation and related log line. (home-assistant#11898)
  Multi-Room Support for Greenwave Reality (home-assistant#11364)
  Added Xeoma camera platform (home-assistant#11619)
  Improve foscam library exception support (home-assistant#11701)
  Iota wallet (home-assistant#11398)
  New venstar climate component (home-assistant#11639)
  Update python-wink version and multiple wink fixes/updates. (home-assistant#11833)
  Use API to discover Hue if no bridges specified (home-assistant#11909)
  Clarify emulated hue warning (home-assistant#11910)
  ...
@home-assistant home-assistant locked and limited conversation to collaborators May 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@pvizeli pvizeli pvizeli approved these changes

@andrey-git andrey-git Awaiting requested review from andrey-git

+1 more reviewer

@houndci-bot houndci-bot houndci-bot left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@pvizeli pvizeli

Labels

cla-signed integration: greenwave platform requirement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@dfiel @tinloaf @amelchio @pvizeli @houndci-bot @balloobbot @homeassistant