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

[WIP] use v2 names, use state file #91

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

no2chem
Copy link

@no2chem no2chem commented Nov 21, 2020

This PR includes the set of changes necessary to use with with my wideq branch, opened as sampsyo/wideq#132

In particular, the following changes need to be addressed:

  • Support for loading the user number and OAuth root url, which are both necessary in the v2 API.

This is resolved by using the json state file instead of manually transferring just the token to the Home Assistant configuration.yaml file. In theory, we could support both manually inserting the entries, but referring to the JSON file seems to be more robust and user friendly.

  • Changing the name of hard coded values in climate.py to refer to their v2 api (airState) equivalents.

Instead of using these values, the wideq api should provide an api independent method to get the supported operation modes and wind directions

this is marked as a wip because dishwasher support is currently disabled. Hopefully, I can get that working next.

If you would like v2 support, you can try using this branch, and installing the wideq branch here: https://github.com/no2chem/wideq using pip install . in your home-assistant environment.

addresses #87

@sampsyo
Copy link
Owner

sampsyo commented Nov 28, 2020

Hi there! Thanks for getting this started! I'm catching up on the PRs here, and although I know this is still WIP, I wanted to make a couple of notes:

  • I really think we should stick with using Home Assistant's own configuration system rather than including the JSON file that can be serialized from the wideq library. I think this is important for being a "good citizen" of the HA ecosystem here—it would be a bad scene if every integration invented its own configuration storage format! Hopefully, by abiding by the recommended way to store configuration, etc., in HA, we can take advantage of concomitant advantages that come with the HA ecosystem, such as their interactive configuration viewer stuff.
  • We'll need to be careful here about how we transition the integration so it continues to work across changes in the wideq library version. That means either keeping the wideq interface the same or making sure that this integration specifies which version of the library it's supposed to be using.

Thanks again for spearheading the effort!

@jacekpaszkowski
Copy link

jacekpaszkowski commented Dec 8, 2020

Hello @no2chem,

i've tried to use your's PR branch and linked wideq version. No errors in logs, but there are no devices in HA.
I have 3 air conditioners.

Entries from my HA logs:

2020-12-08 22:44:03 WARNING (MainThread) [homeassistant.loader] You are using a custom integration for smartthinq which has not been tested by Home Assistant. This component might cause stability problems, be sure to disable it if you experience issues with Home Assistant.

2020-12-08 22:44:13 INFO (MainThread) [homeassistant.setup] Setting up smartthinq

2020-12-08 22:44:21 INFO (MainThread) [homeassistant.setup] Setup of domain smartthinq took 8.1 seconds

2020-12-08 22:44:31 INFO (MainThread) [homeassistant.components.climate] Setting up climate.smartthinq

2020-12-08 22:44:42 INFO (MainThread) [homeassistant.components.sensor] Setting up sensor.smartthinq

What can i do to investigate this?

Using wideq in command line i'm able to generete wideq_state.json file. But output of ls command is empty.

Using wideq from https://github.com/gladhorn/wideq.git i'm able to list devices using ls command.

@no2chem
Copy link
Author

no2chem commented Dec 9, 2020

@jacekpaszkowski interesting. Are your devices v2? I suspect the issue is due to a TLS issue. Are you located in the US or another region?

@jacekpaszkowski
Copy link

@no2chem i'm quite sure my devices are v2. I have my devices for about 4 months. I'm located in Poland (PL).

I've tested different versions of wideq. Take a look at outputs for my account

https://github.com/sampsyo/wideq
image

https://github.com/no2chem/wideq
image

https://github.com/gladhorn/wideq
image

@@ -56,7 +56,7 @@ def setup_platform(hass, config, add_entities, discovery_info=None):

if dishwashers:
add_entities(dishwashers, True)
return True
return True """
Copy link

Choose a reason for hiding this comment

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

Should this code still be commented?

@mjdegraaff
Copy link

Any news on this pull request. Would be happy to help but I'm no programmer whatsoever...

@beped
Copy link

beped commented Apr 14, 2021

I would love to help. I have a air conditioner v2 here. If there's any test i can do to test, just say so.

@XalaTheShepard
Copy link

I am useless in coding, but sure want to contribute in whichever way possible. I can sure help testing the changes. It is however only possible to test the APIv2 as we do not have APIv1 devices.

@Codex-
Copy link

Codex- commented Apr 27, 2021

I'm able to contribute to the code but realistically we need input from the repository owner before proceeding at this point

@sampsyo
Copy link
Owner

sampsyo commented Apr 28, 2021

We'd love any help we can get!! Carefully figuring out what remains to be done based on the thread would be a great way to start…

@Codex-
Copy link

Codex- commented Apr 28, 2021

I think a good first step to get this back on track would be to add tests for the existing implementation, since I imagine you'd expect both v1 and v2 support to persist?

@sampsyo
Copy link
Owner

sampsyo commented Apr 28, 2021

That's actually the big question: nobody seems to be 100% sure whether we need both v1 and v2 support, or whether we can transition entirely to v2 without dropping support for existing devices. So I think the first thing would be to nail that down—things would certainly be way simpler if we could just support v2.

@Vybo
Copy link

Vybo commented Apr 28, 2021

I was searching for a solution for a few months now and found few alternate forks. I was able to make my v2 devices work with one from marciogranzotto that supports only v2, not v1 devices and it's explicitly state as usable only for v2 things. It's a fork originating from this PR, so I think v1 devices won't work if you support only v2 API. I could be wrong though, so that fork could be in theory used to test it out. (https://github.com/marciogranzotto/hass-smartthinq)

@Codex-
Copy link

Codex- commented Apr 29, 2021

to me, it sounds like people will be stuck with v1 still, so we should adopt a strategy that allows us to support both.

I think starting with some basic testing on the current implementation would go a long way, it sounds painful but this is the way

Once we have comprehensive or at least basic testing over v1, we can safely adopt v2 and pave the way for good support for v3 (if it ever gets done)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants