-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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.
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
add set fan_min_on_time thermostat service #2159
Conversation
@@ -21,14 +21,15 @@ class DemoThermostat(ThermostatDevice): | |||
"""Representation of a demo thermostat.""" | |||
|
|||
def __init__(self, name, target_temperature, unit_of_measurement, | |||
away, current_temperature, is_fan_on): | |||
away, current_temperature, is_fan_on, fan_min_on_time = None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 71: E251 unexpected spaces around keyword / parameter equals
- 73: E251 unexpected spaces around keyword / parameter equals
I wonder if this is a common service that all thermostats should have or that this is something that we should only add to the ecobee. I'm leaning to the latter. |
@balloob I agree with you. I just couldn't figure out how to expose that service just on the ecobee thermostat. Some guidance would be appreciated :) |
@@ -37,6 +38,7 @@ | |||
ATTR_CURRENT_TEMPERATURE = "current_temperature" | |||
ATTR_AWAY_MODE = "away_mode" | |||
ATTR_FAN = "fan" | |||
ATTR_FAN_MIN_ON_TIME = "minutes" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have the same value as the key for fan_min_on_time
in the state attributes for the entity that have this attribute, to make it intuitive how to call the service.
Since this new service modifies a state attribute you should update the dict in the state helper to support scenes. |
@balloob and @MartinHjelmare can you please review this again. I've made it so this service is exposed only to the ecobee thermostat and I updated the dictionary accordingly. Cheers, |
add_devices(devices) | ||
|
||
def fan_min_on_time_set_service(service): | ||
"""Set temperature on the target thermostats.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the docstring. Also consider changing the name of the serivce function to set_fan_min_on_time_service
.
One minor comment. Otherwise, this looks good to me. |
Looks good! 🐬 |
Description:
Related issue (if applicable): #
Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#
Example entry for
configuration.yaml
(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
If code communicates with devices:
tox
run successfully. Your PR cannot be merged unless tests passREQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.If the code does not interact with devices:
tox
run successfully. Your PR cannot be merged unless tests passThis is to create a service that allows you to set the minimum fan run time per hour as per ecobee's api: https://www.ecobee.com/home/developer/api/documentation/v1/objects/Settings.shtml , under
fanMinOnTime
. The nest seems to have similar functionality but I wasn't able to nail it down. Wasn't sure how to add a thermostat service just to the ecobee component so I added it to the thermostat component. Any feedback is appreciated.Still waiting on nkgilley/python-ecobee-api#6 to be merged before updating the
components/ecobee.py
requirements. Also trying to determine what documentation needs to be updated.Want to get this PR out there to get some feedback on the added functionality before updating the documentation.