Add Lutron fan platform #43915
Add Lutron fan platform #43915
Conversation
This is a first, untested stab based upon my understanding of pylutron and interaction with devices using ipython + pylutron. I'm not certain about how I'm registering the devices to hass.
|
Hey there @JonGilmore, mind taking a look at this pull request as its been labeled with an integration ( |
|
just some immediate feedback on this (while we wait for your upstream change to be merged/released in pylutron). I had a PR in the past that did this same thing, but found it a little weird because HA didn't have the concept of a fan with the number of speeds that Lutron has. The speeds that were supported were if I recall correctly. It was for this reason that I closed the previous PR. I didnt want to make a decision that would force all users of this integration to not be able to use one fan speed via HA :P |
|
I've figured out some of the testing and structure changes that I need to make. Looking at the Fan Template, it seems like HA actually accepts an arbitrary list of speeds. Later, after reading many of the fan speed PRs. Nevermind, I see the problem. I just wish my fans acted like fans and not dimmers. It looks like I need to split my work out to a distinct |
|
Also, the proposed changes to pylutron that I made do not affect this directly since, as far as RR2 is concerned, a fan controller is a dimmer with a level range of 0 - 100. It actually only translates to OFF (0), LOW (1-25), MEDIUM (26-50), MEDIUM-HIGH (51-75), and HIGH (76-100).... roughly. It's actually a float and there's weird fractional cutoffs, but if you truncate to an int, it's accurate. |
- Removed MEDIUM_HIGH speed until architectural adjustments are made
|
|
||
| def to_hass_speed(speed: int) -> str: | ||
| """Convert the given Lutron (0.0-100.0) light level to Home Assistant (0-255).""" | ||
| return VALUE_TO_SPEED[int(speed)] |
There was a problem hiding this comment.
The comment implies that it can take any speed from 0 to 100 and convert it to a home assistant value, however my read of VALUE_TO_SPEED is that it only works on discrete values. Are there some limitations on what is returned by last_level() that makes this discrete?
Also, I am not sure that SPEED_OFF can ever be looked up given that int(speed) can never return None. Am I missing how this works?
| SPEED_HIGH: FAN_HIGH, | ||
| } | ||
|
|
||
| FAN_SPEEDS = [SPEED_OFF, SPEED_LOW, SPEED_MEDIUM, SPEED_HIGH] |
There was a problem hiding this comment.
could be SPEED_TO_VALUE.keys() to get fancy
|
|
||
| def set_speed(self, speed: str) -> None: | ||
| """Set the speed of the fan.""" | ||
| self._lutron_device.level = to_lutron_speed(speed) |
There was a problem hiding this comment.
It looks like speed may be None due to the case where self._prev_speed is already None and turn_on and the speed arg to turn_on is None. It looks like to_lutron_speed won't work when invoked with None
Is the intent that the check on line 97 self._prev_speed == 0: should be not self._prev_speed to handle the None case?
| else: | ||
| new_speed = self._prev_speed | ||
|
|
||
| self._prev_speed = new_speed |
There was a problem hiding this comment.
Should self._prev_speed be updated in set_speed or turn_off also? It seems like it is updated as a side effect of calling the self.speed (but not is_on which seems to be the primary "state") also but this is a bit hard to reason about. Perhaps its worth a comment explaining the rationale.
|
Apologies, I reviewed this (just eager to help with the backlog) then saw it was a draft... |
| return SUPPORT_SET_SPEED | ||
|
|
||
| @property | ||
| def speed(self) -> Optional[str]: |
There was a problem hiding this comment.
In another review in HA it came up that @Property methods should not be mutating state or logging etc. I think the preservation of _prev_speed logic should move somewhere else.
I think this can happen two ways: Either use sets it from set_speed or when the device is updated (e.g. from the self._lutron_device.subscribe callback to _update_callback which means last_level has been updated. Maybe want to update _prev_speed directly when that happens?
| if speed is not None: | ||
| new_speed = speed | ||
| elif not self._prev_speed: | ||
| new_speed = SPEED_MEDIUM |
There was a problem hiding this comment.
I think this might be equivalent to setting SPEED_MEDIUM as the default value in the constructor so that _prev_speed is never None to simplify this case.
|
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. |
|
The fan entity model has changed to allow more speeds: |
|
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. |
This is a first, untested stab based upon my understanding of pylutron and interaction with devices using ipython + pylutron. I'm not certain about how I'm registering the devices to hass. I welcome feedback on style, approach, or anything.
Breaking change
Previously, Lutron Fans were treated like any other dimmer. Now the fan objects will be correctly recognized as ceiling fans with discrete speeds. If user automations previously relied on the dimmer functionality, those automations would break.
Proposed change
This change creates the discrete states for a four-speed fan controller and fan entity type.
Type of change
Additional information
TODO: I'll file an issue and update this section.
Checklist
TODO: I'm not sure how to test this locally
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: