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

Adding support for Plum Lightpad #16576

Merged
merged 60 commits into from Dec 14, 2018

Conversation

@ColinHarrington
Copy link
Contributor

ColinHarrington commented Sep 12, 2018

Description:

Plum Lightpad support. https://plumlife.com/
These devices are Configurable/Dimmable Wifi Lightswitches that support

  • Multi-touch Guestures
  • Motion Sensor
  • Energy Meter
  • RGB+W Glow Ring
  • Wifi & Bluetooth

There are Phone Apps for iOS & Android. They are Cloud registered but local interaction is available.

Screenshot with 3 of these devices (controlling 2 lights):
https://imgur.com/a/ZZGGffT

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

Example entry for configuration.yaml (if applicable):

plum_lightpad:
  username: <bob@example.com>
  password: "<YOUR_PASSWORD_HERE>"

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

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

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

ColinHarrington and others added some commits Mar 28, 2018

Used Const values
is_on is a bool
@homeassistant

This comment has been minimized.

Copy link

homeassistant commented Sep 12, 2018

Hi @ColinHarrington,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!


hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, cleanup)

cloud_web_sesison = async_create_clientsession(hass, verify_ssl=True)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 16, 2018

Member

Use async_get_clientsession.

await discovery.async_load_platform(
hass, 'binary_sensor', DOMAIN, discovered=device, hass_config=conf)

device_web_session = async_create_clientsession(hass, verify_ssl=False)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 16, 2018

Member

See above.


async def new_load(device):
"""Load light and sensor platforms when LogicalLoad is detected."""
await discovery.async_load_platform(

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 16, 2018

Member

Use asyncio.wait.

tasks = []
tasks.append(hass.async_create_task(
    discovery.async_load_platform(...)))
tasks.append(...)
if tasks:
    await asyncio.wait(tasks)

It's more efficient to only await once.

This comment has been minimized.

@ColinHarrington

ColinHarrington Oct 16, 2018

Contributor

new_load and new_lightpad is called several times when a new device is discovered - even the discovery process is asynchronus - it's whenever a new device is found on the network so we don't know how many devices will be found ahead of time.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 16, 2018

Member

Does this affect my proposed change?

This comment has been minimized.

@ColinHarrington

ColinHarrington Oct 16, 2018

Contributor

There is only ever one device passed to the new_load or new_lightpad methods and they happen at different times

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 16, 2018

Member

Maybe I was unclear in my proposal. I'd like us to wait for the two platform discovery tasks here in new_load. We shouldn't wait for all four tasks in both new_load and new_lightpad at the same time.

This comment has been minimized.

@ColinHarrington

ColinHarrington Oct 17, 2018

Contributor

Oh I see what you mean, i misunderstood what you meant. Thank you for being patient with me :-) Is this any better?


async def new_lightpad(device):
"""Load light and binary sensor platforms when Lightpad detected."""
await discovery.async_load_platform(

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 16, 2018

Member

See above.


if 'lpid' in discovery_info:
lightpad = plum.get_lightpad(discovery_info['lpid'])
async_add_entities([

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 16, 2018

Member

Add all entities in one call to async_add_entities. So first collect the entities in a list in the two if blocks and then if there are entities in the list call async_add_entities.

This comment has been minimized.

@ColinHarrington

ColinHarrington Oct 16, 2018

Contributor

We won't have a list of all the entities to add at once, they are added as they are discovered. Should we only be calling async_setup_platform once? If so, then I need to go back to some other event when devices are discovered after setup.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 16, 2018

Member

No don't change anything but the code within async_setup_platform, ie make sure we only call async_add_entities once for each call to async_setup_platform.

Show resolved Hide resolved homeassistant/components/light/plum_lightpad.py
def __init__(self, lightpad):
"""Initialize the light."""
self._lightpad = lightpad
self._name = lightpad.friendly_name + " Glow Ring"

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 16, 2018

Member

Use new style string formatting instead of string concatenation.

Show resolved Hide resolved homeassistant/components/light/plum_lightpad.py
@ColinHarrington
Copy link
Contributor

ColinHarrington left a comment

I updated the PR with requested changes. Based on previous review comments, async_setup_platform gets called with discovery info when we receive it. It doesn't always come at once - there is a periodic heartbeat that these devices send out so it happens asynchronusly when the datagram discovery routine receives a reply.

Show resolved Hide resolved homeassistant/components/light/plum_lightpad.py

if 'lpid' in discovery_info:
lightpad = plum.get_lightpad(discovery_info['lpid'])
async_add_entities([

This comment has been minimized.

@ColinHarrington

ColinHarrington Oct 16, 2018

Contributor

We won't have a list of all the entities to add at once, they are added as they are discovered. Should we only be calling async_setup_platform once? If so, then I need to go back to some other event when devices are discovered after setup.

Show resolved Hide resolved homeassistant/components/light/plum_lightpad.py
def turn_on(self, **kwargs):
"""Turn the light on."""
if ATTR_BRIGHTNESS in kwargs:
self._brightness = kwargs[ATTR_BRIGHTNESS]

This comment has been minimized.

@ColinHarrington

ColinHarrington Oct 16, 2018

Contributor

Good call, I missed this one.

Show resolved Hide resolved homeassistant/components/plum_lightpad.py
def __init__(self, lightpad):
"""Initialize the light."""
self._lightpad = lightpad
self._name = "%s Glow Ring" % lightpad.friendly_name

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 16, 2018

Member

We prefer new style string formatting over old style.
https://pyformat.info/

@frenck frenck removed the docs-missing label Oct 16, 2018

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Almost done. Please also remove the platforms besides light before we merge. The other platforms should come in separate PRs.

Show resolved Hide resolved homeassistant/components/plum_lightpad.py
self._red = lightpad.glow_color['red']
self._green = lightpad.glow_color['green']
self._blue = lightpad.glow_color['blue']
self._white = lightpad.glow_color['white']

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 17, 2018

Member

self._white is never accessed only assigned.

This comment has been minimized.

@ColinHarrington

ColinHarrington Oct 17, 2018

Contributor

Good call. This was leftover from earlier - I'll remove it. There are actual RGBW values that you can throw at the device, but it's not really accurate and gave ugly results. RGB worked for home-assistant,

{"glowIntensity": kwargs[ATTR_BRIGHTNESS]})
elif ATTR_HS_COLOR in kwargs:
hs_color = kwargs[ATTR_HS_COLOR]
self._red, self._green, self._blue = \

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 17, 2018

Member

Probably store red green and blue in local variables only and let the callback update the instance attributes.

ColinHarrington added some commits Oct 17, 2018

Used local variables for GlowRing colors & added a setter for the hs_…
…color property to keep the values in sync
Support for Plum Lightpad switches.
For more details about this component, please refer to the documentation at
https://home-assistant.io/components/plum_lightpad

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 17, 2018

Member

Each platform should have its own docs page. Look at other platforms for the correct url format. We will refer to the component page from the platform page.

return color_util.color_RGB_to_hs(self._red, self._green, self._blue)

@hs_color.setter
def hs_color(self, value):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 17, 2018

Member

The setter is never used. Please remove it.

@aric89

This comment has been minimized.

Copy link

aric89 commented Nov 10, 2018

Is this implemented yet? I tried to set it up in my config yaml but didn't verify successfully.

@balloob balloob merged commit 9c62149 into home-assistant:dev Dec 14, 2018

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA

@wafflebot wafflebot bot removed the in progress label Dec 14, 2018

mxworm added a commit to mxworm/home-assistant that referenced this pull request Dec 14, 2018

Merge branch 'dev' into current
* dev: (88 commits)
  huawei_lte: Fetch only required data (home-assistant#17618)
  Adding support for Plum Lightpad (home-assistant#16576)
  Fix restore state for manual alarm control panel (home-assistant#19284)
  Set InsteonEntity name to be combo of description and address. (home-assistant#17262)
  Device config for Fibaro hub integration (home-assistant#19171)
  Add air pollutants component (home-assistant#18707)
  Updated ELIQ Online sensor to async API (home-assistant#19248)
  Set unavailable when unreachable (home-assistant#19012)
  Make variable `entity_id` available to value_template for MQTT binary sensor (home-assistant#19195)
  home-assistant#17333: update to use DOMAIN constants and standards. (home-assistant#19242)
  Fix race in entity_platform.async_add_entities (home-assistant#19222)
  Fix race in entity_platform.async_add_entities (home-assistant#19222)
  Bumped version to 0.84.2
  Fix call to super() (home-assistant#19279)
  Fix OwnTracks deadlocking (home-assistant#19260)
  Fix list (fixes home-assistant#19235) (home-assistant#19258)
  Add automation and script events to logbook filter events (home-assistant#19253)
  Bump aioasuswrt (home-assistant#19229)
  Review comments
  Move check to websocket
  ...

dshokouhi added a commit to dshokouhi/home-assistant that referenced this pull request Dec 25, 2018

Adding support for Plum Lightpad (home-assistant#16576)
* Adding basic Plum Lightpad support - https://plumlife.com/

* Used Const values
is_on is a bool

* Added LightpadPowerMeter Sensor to the plum_lightpad platform

* Moved to async setup, Introduced a PlumManager, events, subscription, Light and Power meter working

* Added PlumMotionSensor

* Added Glow Ring support

* Updated plum library and re-normalized

* set the glow-ring's icon

* Naming the glow ring

* Formatting and linting

* Cleaned up a number of linting issues.  Left a number of documentation warnings

* setup_platform migrated to async_setup_platform Plum discovery run as a job

* bumped plumlightpad version

* On shutdown disconnect the telnet session from each plum lightpad

* Cleanup & formatting. Worked on parallell cloud update

* Moved setup from async to non-async

* Utilize async_call_later from the helpers

* Cleanedup and linted, down to documentation & one #TODO

* Remove commented out debug lines

* Fixed Linting issues

* Remove TODO

* Updated comments & fixed Linting issues

* Added plumlightpad to requirements_all.txt

* Fixing imports with isort

* Added components to .coveragerc

* Added PLUM_DATA constant for accessing hass.data[PLUM_DATA]

* used dictionary syntax vs get(...) for config

* Linting needed an additonal line

* Fully async_setup now. removed @callback utilize bus events for detecting new devices found.

* Upgraded to plumlightpad 0.0.10

* Removed extra unused PLATFORM_SCHEMA declarations

* Moved listener attachment to `async_added_to_hass` and removed unused properties & device_state_attributes

* Utilized Discovery when devices were located

* Linting and cleanup

* used `hass.async_create_task` instead of `hass.async_add_job` per Martin

* Removed redundant criteria in if block

* Without discovery info, there is no need to setup

* Better state management and async on/off for Glow Ring

* renamed async_set_config back to set_config, fixed cleanup callback and Plum Initialization

* Fixed flake8 linting issues

* plumlightpad package update

* Add 'motion' device_class to Motion Sensor

* Fixed last known Linting issue

* let homeassistant handle setting the brightness state

* String formatting vs concatenation

* use shared aiohttp session from homeassistant

* Updating to use new formatting style

* looks like @cleanup isn't neccesary

* ditch the serial awaits

* Ensure async_add_entities is only called once per async_setup_platform

* Creating tasks to wait for vs coroutines

* Remove unused white component in the GlowRing

* Used local variables for GlowRing colors & added a setter for the hs_color property to keep the values in sync

* Linted and added docstring

* Update the documentation path to point to the component page

* Removed the extra sensor and binary_sensor platforms as requested. (To be added in later PRs)

* Update plum_lightpad.py

* Update plum_lightpad.py

@balloob balloob referenced this pull request Jan 10, 2019

Merged

0.85.0 #19897

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