sampsyo / hass-smartthinq

Home Assistant component for LG SmartThinQ HVAC devices
MIT License
285 stars 98 forks source link

Integrate climate with the smartthinq platform. #27

Closed dermotduffy closed 5 years ago

dermotduffy commented 5 years ago

Caution: I can only partially test this, as I do not have any LG HVAC gear. It appears to do the right thing as far as the point of device creation logic -- and the right warning messages appear to get printed. This will support both the old style (printing warning messages) and the new style.

You may want to test this yourself.

Also took the liberty of updating the README. Happy to remove the dishwasher image if you'd rather keep it clean.

dermotduffy commented 5 years ago

Thanks for the great suggestions. All implemented.

sampsyo commented 5 years ago

Looking great; thank you!!

I gave this a shot, and it looks like we might have one small thing to clean up: the component fails to load if the smartthinq config doesn't exist at the top level. It would be great to fail more gracefully when this happens so it's still possible to use the climate-specific config instead (although it's depreciated). Specifically, I get this message in my log at startup:

Traceback (most recent call last):
  File "/Users/asampson/Library/Python/3.7/lib/python/site-packages/homeassistant/setup.py", line 172, in _async_setup_component
    component.setup, hass, processed_config  # type: ignore
  File "/usr/local/Cellar/python/3.7.4/Frameworks/Python.framework/Versions/3.7/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/Users/asampson/.homeassistant/custom_components/smartthinq/__init__.py", line 37, in setup
    refresh_token = config[DOMAIN].get(CONF_TOKEN)
KeyError: 'smartthinq'

And also:

2019-08-18 22:37:33 ERROR (MainThread) [homeassistant.setup] Unable to prepare setup for platform smartthinq.climate: Unable to set up component.

And the UI reports a message like this:

The following components and platforms could not be set up:

smartthinq smartthinq.climate

Please check your config.

So maybe we just have to be tolerant of the missing config[DOMAIN]?

dermotduffy commented 5 years ago

Hmm. Is it possible you are testing the version without this PR? I specifically tested the configuration you describe. Here it is working:

2019-08-18 19:47:47 WARNING (SyncWorker_32) [custom_components.smartthinq] Direct use of the smartthinq components without a toplevel smartthinq platform configuration is deprecated. Please use a top-level smartthinq platform instead. Please see https://github.com/sampsyo/hass-smartthinq/blob/master/README.md . Configuration mapping:
        climate.refresh_token -> smartthinq.token
        climate.country -> smartthinq.region
        climate.language -> smartthinq.language

Configuration used:

#smartthinq:
#  token: !secret smartthinq_token
#  region: US
#  language: en-US

climate:
  - platform: smartthinq
    refresh_token: !secret smartthinq_token
    country: US
    language: en-US

The other clue is that your stacktrace line numbers don't match my master repo (and this PR). With this PR, prior to the code shown in your stacktrace, is this block:

 57     if DOMAIN not in config:
 58         LOGGER.warning(DEPRECATION_WARNING)
 59         return True

... i.e. it should not get as far as the stacktrace shows.

Let me know if there's any chance you didn't patch in this full PR when you ran that test?

sampsyo commented 5 years ago

Aha; you're so right! Sorry about that.

There's another problem, however—here's the traceback:

Traceback (most recent call last):
  File "/Users/asampson/Library/Python/3.7/lib/python/site-packages/homeassistant/helpers/entity_platform.py", line 149, in _async_setup_platform
    await asyncio.wait_for(asyncio.shield(task), SLOW_SETUP_MAX_WAIT)
  File "/usr/local/Cellar/python/3.7.4/Frameworks/Python.framework/Versions/3.7/lib/python3.7/asyncio/tasks.py", line 442, in wait_for
    return fut.result()
  File "/usr/local/Cellar/python/3.7.4/Frameworks/Python.framework/Versions/3.7/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/Users/asampson/.homeassistant/custom_components/smartthinq/climate.py", line 56, in setup_platform
    hass.data[CONF_REGION]
KeyError: 'region'

This happens if neither method specifies a country code. Maybe that line needs to use a .get() as well?

dermotduffy commented 5 years ago

My goodness, sorry about that. We'll get there in the end ... please take another look.

sampsyo commented 5 years ago

Wonderful! That did it. Thanks!!