Skip to content

Adding support for Lutron covers#11602

Merged
pvizeli merged 3 commits into
home-assistant:devfrom
nickovs:lutron-covers
Jan 13, 2018
Merged

Adding support for Lutron covers#11602
pvizeli merged 3 commits into
home-assistant:devfrom
nickovs:lutron-covers

Conversation

@nickovs
Copy link
Copy Markdown
Contributor

@nickovs nickovs commented Jan 12, 2018

Description:

This patch adds support for controlling roller shades through the cover component on the Lutron platform.

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

Configuration

As with the Lutron light component, no configuration is necessary (or available) as shades are detected automatically.

Checklist:

  • The code change is tested and works locally.

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.

"""Call when forcing a refresh of the device."""
# Reading the property (rather than last_level()) fetchs value
level = self._lutron_device.level
_LOGGER.debug("Lutron ID: %d updated to %f", self._lutron_device.id, level)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (83 > 79 characters)

@nickovs
Copy link
Copy Markdown
Contributor Author

nickovs commented Jan 12, 2018

Note that the Lutron platform does not currently have any unit tests and was already explicitly excluded in .coveragerc. As a result I have not provided unit tests for this new code. I will take a look at putting together a separate PR to provide test coverage for the whole Lutron platform but that is a much, much larger exercise than this modest extension to the existing functionality since it requires mocking most of the pylutron library.

Copy link
Copy Markdown
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

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

Store the level as self.level and work with that on properties. You do actual IO inside properties and that is not allow.

@nickovs
Copy link
Copy Markdown
Contributor Author

nickovs commented Jan 13, 2018

@pvizeli Incorrect. No IO is done in the properties; they only ever call the last_level() function of the output object, which is fetching the cached level information (see https://github.com/thecynic/pylutron/blob/master/pylutron/__init__.py#L477 for the definition). This code works the same way as the existing light/lutron.py code. Do you really think that a second layer of caching is necessary?

@pvizeli pvizeli merged commit 3e43f4e into home-assistant:dev Jan 13, 2018
def update(self):
"""Call when forcing a refresh of the device."""
# Reading the property (rather than last_level()) fetchs value
level = self._lutron_device.level
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This doesn't seem to do anything.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It force the update on client library

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

devs.append(dev)

add_devices(devs, True)
return True
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nothing is checking this return value.

def device_state_attributes(self):
"""Return the state attributes."""
attr = {}
attr['Lutron Integration ID'] = self._lutron_device.id
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

State attributes should be lowercase snakecase.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I named it this way for consistency with the light\lutron.py component. If snakecase is the policy then we should change it in both places but for the purpose of this new code I though it best not to make too many changes to what was already there in case it broke existing deployments.

@balloob balloob mentioned this pull request Jan 26, 2018
@home-assistant home-assistant locked and limited conversation to collaborators May 29, 2018
@ghost ghost removed the platform: cover.lutron label Mar 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants