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.

Sign up for GitHub

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

Jump to bottom

Add support for Opple light #16765

Merged

balloob merged 5 commits into home-assistant:dev from jedmeng:opple-platform

Sep 25, 2018

Contributor

jedmeng commented Sep 21, 2018 •

edited

Description:

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

Example entry for `configuration.yaml` (if applicable):

light:
  - platform: opple
    name: LIGHT_NAME
    host: IP_ADDRESS

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:

Documentation added/updated in home-assistant.io

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.


          Add support for Opple light

ee60d37

homeassistant added cla-signed new-platform platform: light.opple labels

ghost added the in progress label

jedmeng mentioned this pull request

Add doc for Opple light home-assistant/home-assistant.io#6328

Merged

2 tasks


          Update docstring

a82eb24

homeassistant added the cla-signed label

MartinHjelmare reviewed

View reviewed changes

Member

MartinHjelmare left a comment

Please add unique_id property and return the mac address. This will make the integration more user friendly.

homeassistant/components/light/opple.py Outdated

		})


		def setup_platform(hass, config, add_devices, discovery_info=None):

Member

MartinHjelmare Sep 25, 2018

Rename add_devices to add_entities.

homeassistant/components/light/opple.py Outdated

+              def setup_platform(hass, config, add_devices, discovery_info=None):
+                  """Set up Opple light platform."""
+                  name = config.get('name')
+                  host = config.get('host')

Member

MartinHjelmare Sep 25, 2018

Use dict[key] for required config keys.

Member

MartinHjelmare Sep 25, 2018

Use the constants used in the config schema.

homeassistant/components/light/opple.py Outdated

+                      self._brightness = None
+                      self._color_temp = None
+                      _LOGGER.debug("init light %s %s", self._device.ip, self._device.mac)

Member

MartinHjelmare Sep 25, 2018

Capitalize start word of log messages.

homeassistant/components/light/opple.py Outdated

+                      """Instruct the light to turn on."""
+                      _LOGGER.debug("Turn on light %s %s", self._device.ip, kwargs)
+                      if not self.is_on:
+                          self._is_on = self._device.power_on = True

Member

MartinHjelmare Sep 25, 2018

Let update change state holding instance attributes. It will be called directly after the method call.

Contributor Author

jedmeng Sep 25, 2018

I'm sorry I didn't catch your meaning

Member

MartinHjelmare Sep 25, 2018

Change the line to:

self._device.power_on = True

is_on will be updated in update.

homeassistant/components/light/opple.py Outdated

+                      self._brightness = self._device.brightness
+                      self._color_temp = self._device.color_temperature
+                      if not self.available:

Member

MartinHjelmare Sep 25, 2018

Only log one message when the device changes availability state, not on each update if state is the same.

Member

MartinHjelmare commented Sep 25, 2018

The config section in the PR description is not accurate.

jedmeng added 2 commits

September 25, 2018 20:33


          review fix

55e9bc1


          review fix

0f8b1d7

MartinHjelmare reviewed

View reviewed changes

homeassistant/components/light/opple.py

+                          return
+                      if not self.available:
+                          _LOGGER.debug("Light %s is offline", self._device.ip)

Member

MartinHjelmare Sep 25, 2018

We could consider raising the logging level to warning, depending on how often we think this will occur.

Contributor Author

jedmeng Sep 25, 2018

When people turn off their light through a wall switch, the light goes offline, so it's pretty common. So I think "debug level" is just fine.

homeassistant/components/light/opple.py Outdated

+              def setup_platform(hass, config, add_entities, discovery_info=None):
+                  """Set up Opple light platform."""
+                  name = config['name']

Member

MartinHjelmare Sep 25, 2018

Use CONF_NAME.

homeassistant/components/light/opple.py Outdated

+              def setup_platform(hass, config, add_entities, discovery_info=None):
+                  """Set up Opple light platform."""
+                  name = config['name']
+                  host = config['host']

Member

MartinHjelmare Sep 25, 2018

Use CONF_HOST.

homeassistant/components/light/opple.py Outdated

+                      self._brightness = None
+                      self._color_temp = None
+                      _LOGGER.debug("Init light %s %s", self._device.ip, self._device.mac)

Member

MartinHjelmare Sep 25, 2018

Please move this to setup_platform.


          review fix

369fa56

MartinHjelmare approved these changes

View reviewed changes

Member

MartinHjelmare left a comment

Looks good! Can be merged when build passes.

balloob merged commit 8e31168 into home-assistant:dev

ghost removed the in progress label

balloob mentioned this pull request

Merged

home-assistant locked and limited conversation to collaborators

ghost added integration: opple and removed platform: light.opple labels

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