Tahoma dimmable light #36258

vlebourl · 2020-05-29T12:10:05Z

Breaking change

Proposed change

Add support for dimmable lights in Tahoma.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Example entry for `configuration.yaml`:

# Example configuration.yaml

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

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

Documentation added/updated for www.home-assistant.io

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

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

vlebourl · 2020-05-29T13:10:14Z

I don't understand the logic, got asked to use f-strings in another PR, so I made the change here as well, but pylint wants the % formatting in logging functions. If someone can explain, I'd be happy to know why this is required. Cheers

tetienne · 2020-06-02T18:17:01Z

@vlebourl It's about performance. For simple applications with few log, you don't care. But you log a lot, it can have an impact. You can see a discussion here: pylint-dev/pylint#2395

stale · 2020-07-03T04:11:10Z

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

stale · 2020-08-02T17:02:33Z

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

bdraco · 2020-09-04T14:30:03Z

homeassistant/components/tahoma/light.py

+    def brightness(self) -> int:
+        """Return the brightness of this light between 0..255."""
+        _LOGGER.debug("[THM] Called to get brightness %s", self._brightness)
+        return int(self._brightness * (255 / 100))


Suggested change

return int(self._brightness * (255 / 100))

return round(self._brightness * (255 / 100))

bdraco · 2020-09-04T14:30:18Z

homeassistant/components/tahoma/light.py

+        self._skip_update = True
+
+        if ATTR_BRIGHTNESS in kwargs:
+            self._brightness = int(float(kwargs[ATTR_BRIGHTNESS]) / 255 * 100)


Suggested change

self._brightness = int(float(kwargs[ATTR_BRIGHTNESS]) / 255 * 100)

self._brightness = round(float(kwargs[ATTR_BRIGHTNESS]) / 255 * 100)

bdraco · 2020-09-04T14:32:19Z

homeassistant/components/tahoma/light.py

+        super().__init__(tahoma_device, controller)
+        self._skip_update = False
+        self._effect = None
+        self._state = STATE_UNKNOWN


Suggested change

self._state = STATE_UNKNOWN

self._state = None

entity.py will set this to STATE_UNKNOWN if its none
state = STATE_UNKNOWN if state is None else str(state)

vlebourl · 2020-09-04T14:32:26Z

Hi, I actually forgot about that PR... I think we should close it as we're working on a full rewrite of the component with @iMicknl and @tetienne. Do you agree?

bdraco · 2020-09-04T14:33:33Z

homeassistant/components/tahoma/light.py

+        self._brightness = 0
+        if self.tahoma_device.type == "io:DimmableLightIOComponent":
+            self._type = "io"
+            self._unique_id = self.tahoma_device.url


This doesn't appear to meet the unique id requirements here https://developers.home-assistant.io/docs/entity_registry_index/#unique-id-requirements

We should use something that is stable for the life of the device or remove it.

bdraco · 2020-09-04T14:33:58Z

homeassistant/components/tahoma/light.py

+
+    async def async_turn_on(self, **kwargs) -> None:
+        """Turn the light on."""
+        _LOGGER.debug("[THM] Called to turn on (%s, %s)", kwargs, self._brightness)


Suggested change

_LOGGER.debug("[THM] Called to turn on (%s, %s)", kwargs, self._brightness)

_LOGGER.debug("Called to turn on (%s, %s)", kwargs, self._brightness)

The name of the module is already logged

bdraco · 2020-09-04T14:34:15Z

homeassistant/components/tahoma/light.py

+            self._skip_update = False
+            return
+
+        _LOGGER.debug("[THM] Updating state...")


Suggested change

_LOGGER.debug("[THM] Updating state...")

_LOGGER.debug("Updating state...")

bdraco · 2020-09-04T14:34:25Z

homeassistant/components/tahoma/light.py

+    @property
+    def is_on(self) -> bool:
+        """Return true if light is on."""
+        _LOGGER.debug("[THM] Called to check is on %s", self._state)


Suggested change

_LOGGER.debug("[THM] Called to check is on %s", self._state)

_LOGGER.debug("Called to check is on %s", self._state)

bdraco · 2020-09-04T14:34:55Z

homeassistant/components/tahoma/light.py

+    devices = []
+
+    for device in hass.data[TAHOMA_DOMAIN]["devices"]["light"]:
+        devices.append(TahomaLight(device, controller))
+
+    async_add_entities(devices, True)


Suggested change

devices = []

for device in hass.data[TAHOMA_DOMAIN]["devices"]["light"]:

devices.append(TahomaLight(device, controller))

async_add_entities(devices, True)

entities = []

for device in hass.data[TAHOMA_DOMAIN]["devices"]["light"]:

entities.append(TahomaLight(device, controller))

async_add_entities(entities, True)

These are entities

iMicknl · 2020-09-04T14:35:04Z

Hi, I actually forgot about that PR... I think we should close it as we're working on a full rewrite of the component with @iMicknl and @tetienne. Do you agree?

@vlebourl I think it is best to close this one, I didn't know this one was open.. :(
This will also make our new core PR a bit smaller since it doesn't need to include the light integration.

All the comments from @bdraco are probably already fixed in the refactor https://github.com/iMicknl/ha-tahoma. I will double check, to not waste his review.

bdraco · 2020-09-04T14:35:21Z

homeassistant/components/tahoma/light.py

+
+    async def async_turn_off(self, **kwargs) -> None:
+        """Turn the light off."""
+        _LOGGER.debug("[THM] Called to turn off")


Suggested change

_LOGGER.debug("[THM] Called to turn off")

_LOGGER.debug("Called to turn off")

__name__ will already be in the logger

bdraco

Thanks for adding this. Please see comments above

stale · 2020-10-04T15:03:51Z

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

vlebourl · 2020-10-06T06:49:10Z

Let's close this one as we are submitting a bigger update to this component: #41267

vlebourl added 2 commits May 29, 2020 13:47

Added light entity with support for dimmable light.

28f2684

applied black styling

d271c02

homeassistant added the cla-signed label May 29, 2020

project-bot bot added this to Needs review in Dev May 29, 2020

probot-home-assistant bot added integration: tahoma new-feature new-platform labels May 29, 2020

vlebourl added 6 commits May 29, 2020 14:16

applied black styling

6464242

applied lint fixes

f70ffb5

applied isort fixes

617d0fc

applied isort fixes

ef7932d

applied isort fixes

32ba84d

applied pylint fixes

7f56189

stale bot added the stale label Jul 3, 2020

MartinHjelmare changed the title ~~Tahoma dimable light~~ Tahoma dimmable light Jul 3, 2020

stale bot removed the stale label Jul 3, 2020

This was referenced Jul 6, 2020

Refactor constants and strings iMicknl/ha-tahoma#110

Closed

Add support for more sensors & basic Siren (switch) support iMicknl/ha-tahoma#84

Merged

stale bot added the stale label Aug 2, 2020

stale bot closed this Aug 9, 2020

Dev automation moved this from Needs review to Cancelled Aug 9, 2020

MartinHjelmare reopened this Aug 9, 2020

Dev automation moved this from Cancelled to Incoming Aug 9, 2020

stale bot removed the stale label Aug 9, 2020

MartinHjelmare moved this from Incoming to Needs review in Dev Aug 16, 2020

bdraco reviewed Sep 4, 2020

View reviewed changes

bdraco requested changes Sep 4, 2020

View reviewed changes

Dev automation moved this from Needs review to Review in progress Sep 4, 2020

stale bot added the stale label Oct 4, 2020

vlebourl closed this Oct 6, 2020

Dev automation moved this from Review in progress to Cancelled Oct 6, 2020

vlebourl deleted the TahomaDimableLight branch May 11, 2021 07:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tahoma dimmable light #36258

Tahoma dimmable light #36258

vlebourl commented May 29, 2020 •

edited

vlebourl commented May 29, 2020

tetienne commented Jun 2, 2020

stale bot commented Jul 3, 2020

stale bot commented Aug 2, 2020

bdraco Sep 4, 2020

bdraco Sep 4, 2020

bdraco Sep 4, 2020

vlebourl commented Sep 4, 2020 •

edited

bdraco Sep 4, 2020

bdraco Sep 4, 2020

bdraco Sep 4, 2020

bdraco Sep 4, 2020

bdraco Sep 4, 2020

iMicknl commented Sep 4, 2020

bdraco Sep 4, 2020

bdraco left a comment

stale bot commented Oct 4, 2020

vlebourl commented Oct 6, 2020

	return int(self._brightness * (255 / 100))
	return round(self._brightness * (255 / 100))

	self._brightness = int(float(kwargs[ATTR_BRIGHTNESS]) / 255 * 100)
	self._brightness = round(float(kwargs[ATTR_BRIGHTNESS]) / 255 * 100)

	_LOGGER.debug("[THM] Called to turn on (%s, %s)", kwargs, self._brightness)
	_LOGGER.debug("Called to turn on (%s, %s)", kwargs, self._brightness)

	_LOGGER.debug("[THM] Updating state...")
	_LOGGER.debug("Updating state...")

	_LOGGER.debug("[THM] Called to check is on %s", self._state)
	_LOGGER.debug("Called to check is on %s", self._state)

	_LOGGER.debug("[THM] Called to turn off")
	_LOGGER.debug("Called to turn off")

Tahoma dimmable light #36258

Tahoma dimmable light #36258

Conversation

vlebourl commented May 29, 2020 • edited

Breaking change

Proposed change

Type of change

Example entry for configuration.yaml:

Additional information

Checklist

vlebourl commented May 29, 2020

tetienne commented Jun 2, 2020

stale bot commented Jul 3, 2020

stale bot commented Aug 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlebourl commented Sep 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iMicknl commented Sep 4, 2020

Choose a reason for hiding this comment

bdraco left a comment

Choose a reason for hiding this comment

stale bot commented Oct 4, 2020

vlebourl commented Oct 6, 2020

vlebourl commented May 29, 2020 •

edited

Example entry for `configuration.yaml`:

vlebourl commented Sep 4, 2020 •

edited