Skip to content

Add fan support to lutron_caseta#24770

Closed
djj211 wants to merge 12 commits into
home-assistant:devfrom
djj211:lutron-caseta-fan-support
Closed

Add fan support to lutron_caseta#24770
djj211 wants to merge 12 commits into
home-assistant:devfrom
djj211:lutron-caseta-fan-support

Conversation

@djj211
Copy link
Copy Markdown
Contributor

@djj211 djj211 commented Jun 26, 2019

Breaking Change:

Description:

Adding the lutron caseta fan controller to a smartbridge will break the lutron_caset component. That is really a pylutron_caseta issue and not home assistant issue. This pull request simply adds support for the fan controls based on changes made in a separate commit to the pylutron_caseta repository.
gurumitts/pylutron-caseta#34

Additional work is likely needed to up the version of pylutron_caseta and etc in pypi.

Related issue (if applicable): fixes #

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

Example entry for configuration.yaml (if applicable):

lutron_caseta:
  host: 192.168.1.1
  keyfile: ./config/lutron/caseta.key
  certfile: ./config/lutron/caseta.crt
  ca_certs: ./config/lutron/caseta-bridge.crt

Checklist:

  • [ X] The code change is tested and works locally.
  • [X ] Local tests pass with tox. Your PR cannot be merged unless tests pass
  • [ X] There is no commented out code in this PR.
  • [ X] I have followed the development checklist

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

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

  • [ X] The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • [X ] New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

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

…were required of pylutron_caseta to get this to work. Those changes are in a separate pull request against that repository.
@homeassistant
Copy link
Copy Markdown
Contributor

Hi @djj211,

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!

djj211 added a commit to djj211/home-assistant.io that referenced this pull request Jul 4, 2019
Updated for new config point to work with my new pylutron_caseta mods and also support for the new caseta fan.

home-assistant/core#24770
gurumitts/pylutron-caseta#34
@mdonoughe
Copy link
Copy Markdown
Contributor

We may have some trouble merging this. I had opened a pull request for Entity Registry support, but pylutron_caseta hasn't released since I added the code to expose the device serial numbers. Although I have access to push to that repository, I think only @gurumitts can release the Python package and I haven't heard from him.

@MartinHjelmare MartinHjelmare changed the title Added fan support to the lutron_caseta component. Add fan support to lutron_caseta Jul 7, 2019

_LOGGER = logging.getLogger(__name__)

LUTRON_SPEED_OFF = 'Off'
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.

Only speeds in the base fan component are allowed to be returned in the speed list or accepted as set speed argument.

We can build two dicts to map between home assistant speeds and device speeds. Make a best effort. It might not be a perfect map.

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.

@MartinHjelmare I added the two dictionaries to map between speeds and devices (modeled off some other fan integrations). The base fan component does not have a "MediumHigh" field to map to. Not sure if you prefer that I take the approach that I did, or if you prefer that I add "mediumhigh" to the base component?

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.

We can't add new speeds without approval in an issue in our architecture repo.

Please remove all speeds not currently in the base fan component.

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.

@MartinHjelmare Understood. I will open an issue. I removed the speed not in the base fan component.

@djj211
Copy link
Copy Markdown
Contributor Author

djj211 commented Jul 12, 2019

We may have some trouble merging this. I had opened a pull request for Entity Registry support, but pylutron_caseta hasn't released since I added the code to expose the device serial numbers. Although I have access to push to that repository, I think only @gurumitts can release the Python package and I haven't heard from him.

@mdonoughe That is what I feared. Has any body else thought about taking ownership of the repository or starting their own api library based on this in the pypi?

devs.append(dev)

async_add_entities(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. We can remove the statement.

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.

@MartinHjelmare return value removed.

@property
def speed_list(self) -> list:
"""Get the list of available speeds."""
return [SPEED_OFF, SPEED_LOW, SPEED_MEDIUM, SPEED_HIGH]
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.

We can extract this list of speeds to a module level constant which we return here.

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.

@MartinHjelmare list of speeds extracted to module level constant.

@MartinHjelmare
Copy link
Copy Markdown
Member

Looks good! Now we only need to wait for the library PR to be merged and a new release made, right?

We're trying to decrease our open PR buffer. If you don't think the release will happen in the near future, please close this PR and you can open this PR again yourself when the release has been made.

@djj211
Copy link
Copy Markdown
Contributor Author

djj211 commented Jul 29, 2019

@MartinHjelmare Looks like the changes to pylutron_caseta may not be merged any time soon. I'll see what happens in that regards. In the meantime I will close this PR and reopen once ready.

@djj211 djj211 closed this Jul 29, 2019
@cryptk
Copy link
Copy Markdown
Contributor

cryptk commented Aug 8, 2019

I know this is currently closed, but I just wanted to check if there is any chance that home-assistant would be willing to take stewardship of the pylutron_caseta code.

I built a customized docker image for my RPi4 k3s cluster that has home-assistant with the updated pylutron_caseta with fan support, and added in a custom component to replace the built-in lutron_caseta component with the modifications from this PR and everything seems to be working flawlessly. I even have automations built around controlling my office fan based on the room temperature and a presence sensor.

It would be a shame for this functionality to not be available to the wider home-assistant community just because the maintainer of pylutron_caseta seems to have lost interest... the code is all here, ready to go, and working great.

@frenck
Copy link
Copy Markdown
Member

frenck commented Aug 8, 2019

@cryptk As seen in the above comments, this depends on the upstream library to adopt this functionality. As mentioned here.

Furthermore, Home Assistant depends on contributions to the project. Feel free to contribute a solution.

@home-assistant home-assistant locked and limited conversation to collaborators Aug 8, 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.

6 participants