Skip to content
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 Broadlink-wifi climate support #21962

Closed

Conversation

clementTal
Copy link

@clementTal clementTal commented Mar 12, 2019

Description:

The broadlink_thermostat climate platform is a thermostat implemented in Home Assistant. It is used to control wifi thermostat manufactured by broadlink, like Floureon Smart Wi-Fi Programmable Digital Touch Screen Thermostat, Beok TGT70WIFI-EP Smart Wifi Thermostat and SeeSii Programmable Thermostat Heating WiFi

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

previous PR: #15363, #19223

Example entry for configuration.yaml (if applicable):

climate:
  - platform: broadlink
    friendly_name: Thermostat
    mac: "xx:xx:xx:xx:xx"
    host: "xxx.xxx.xxx.xxx"

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

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

  • New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
  • New dependencies are only imported inside functions that use them ([example][ex-import]).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

@clementTal clementTal changed the title update broadlink climate add broadlink climate Mar 12, 2019
@elupus
Copy link
Contributor

elupus commented Mar 12, 2019

Services should go on broadlink domain as per #21465

PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Required(CONF_HOST): cv.string,
vol.Required(CONF_MAC): cv.string,
vol.Required(CONF_FRIENDLY_NAME): cv.string,
Copy link
Member

Choose a reason for hiding this comment

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

The friendly name is required? That is not usual...
I see the library requires it... but it isn't used actually?

image

In the above image, it shows the only part of the code that uses the entity_id?
The variables seem to be unused, however, they are required?

@tsvi
Copy link
Contributor

tsvi commented Mar 13, 2019

We should be careful with the naming here as we already have broadlink RM switch. And since the RM switch can be used as a climate (as I foresee a merge with the smartIr custom component), we might cause problems. Maybe rename the component to broadlink wifi, so we have a clear distinction?

@clementTal
Copy link
Author

@tsvi: not sure about naming problem here. This components is made to control thermostat made by broadlink and rebranded. The smartIr custom components you speak about is a components made to control IR climate. It made it simple than using custom climate component, but do the same. I would prefer having this components named broadlink better than a component that provide the same uses cases as another.

If my component should have another name, it will be named by it's brand name, so I'll need to create a least 3 component (breok, floureon, seesii) with the same code inside.

@tsvi
Copy link
Contributor

tsvi commented Mar 14, 2019

Definitely was not imagining splitting it up.
But maybe rename to Broadlink Wifi Thermostat, as we have #21465 which might create issues. Otherwise we'll need somehow to merge both although they are completely different components.

@fabaff fabaff changed the title add broadlink climate Add Broadlink climate support Mar 19, 2019
@clementTal clementTal changed the title Add Broadlink climate support Add Broadlink-wifi climate support May 13, 2019
Copy link

@fantognazza fantognazza left a comment

Choose a reason for hiding this comment

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

Fix bugs and mistypes

homeassistant/components/broadlink/climate.py Outdated Show resolved Hide resolved
homeassistant/components/broadlink/climate.py Outdated Show resolved Hide resolved

def set_advanced_config(self, advanced_conf):
"""Set advanced configuration."""
self._device.set_advanced_config(self, advanced_conf)

Choose a reason for hiding this comment

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

Suggested change
self._device.set_advanced_config(self, advanced_conf)
self._device.set_advanced_config(advanced_conf)

"""Set automatic schedule."""
self._device.set_schedule(schedules)

def set_advanced_config(self, advanced_conf):

Choose a reason for hiding this comment

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

Suggested change
def set_advanced_config(self, advanced_conf):
def set_advanced_conf(self, advanced_conf):

clementTal and others added 2 commits May 16, 2019 09:48
Co-Authored-By: Francesco Antognazza <francesco.antognazza@gmail.com>
Co-Authored-By: Francesco Antognazza <francesco.antognazza@gmail.com>
@JakubSch
Copy link

Hello guys, I am very sorry for such a dummy question but I am completely new. Could you help me step by step how to install this platform? I am 1 day user of HA :) Shall I create a broadlink/climate.py in custom components or it does not work like that? Thank you very much for help!

@pvizeli
Copy link
Member

pvizeli commented Jul 8, 2019

@MartinHjelmare MartinHjelmare added this to Review in progress in Dev Jul 23, 2019
@MartinHjelmare
Copy link
Member

There hasn't been any movement here for a long time. I'll close now. We're trying to decrease our open PR buffer. Please open a new PR when ready to finish.

Dev automation moved this from Review in progress to Cancelled Aug 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Dev
  
Cancelled
Development

Successfully merging this pull request may close these issues.

None yet

10 participants