dlarrick / hass-kumo

Home Assistant module interfacing with Mitsubishi mini-split units
MIT License
97 stars 21 forks source link

async causing intermittent auth issues #56

Closed danielgoepp closed 3 years ago

danielgoepp commented 3 years ago

I finally got down to finding exactly where the problem is that I have experienced (and have seen others are experiencing too). I have tried to resolve, but am not familiar enough with the asyncio implementation in HA core to fix it. I am going to continue to try though. I am posting here with my findings so far, and will update when I find more. The first thing is that when testing the missing IP addresses issue (not with this integration, with kumo cloud), I keep deleting and adding back the kumo integration. I get about a 50/50 success rate here, and it would error with:

Screen Shot 2021-08-27 at 5 54 29 PM

But, this was a misleading error. I am able to test via curl, pykumo and the web app, all worked fine. Also, even though this throws an error, the cache file was getting created! Clearly auth is working, something else is going on. I put some logging into the code and found where this is really happening. If the result from the try_setup is false it just assumes invalid auth and raises an error (lines 38 and 39 in config_flow.py). I'm not really sure yet what the problem is, but it's not an auth issue, it's a sync issue.

omriasta commented 3 years ago

This only exists in my fork...didn't really finalize everything on that end. I'll clean it up more before submitting a PR

dlarrick commented 3 years ago

Just to let you guys know... I am going to be off the grid for about a week. So I'm not ignoring you on this config stuff, which looks like a good idea. I'll be back on Labor Day and should have some time that week to review & merge.

danielgoepp commented 3 years ago

@dlarrick enjoy your time off!

danielgoepp commented 3 years ago

@omriasta I'm pretty sure it's both yours and the original. I reverted back and still couldn't auth. I checked the code and I see the same await lines and the same results test. The way I restored my production instance was actually manually putting the configs back in. I think I learned my lesson and have a separate QA container for testing now :)

omriasta commented 3 years ago

As a side note @dlarrick , when editing the config flow, if the cache file returned is missing IP addresses, I have forced prefer_cache to true since it doesn't make sense to have this as false if the cache file is missing IP addresses. Also on my last update to my fork I also save the cache file during setup for when this happens.

danielgoepp commented 3 years ago

@omriasta thanks for the update. FYI I tested your latest commit, and am getting this. I have not looked into it further yet.

Screen Shot 2021-08-28 at 4 59 01 PM
haqa    | 2021-08-28T20:58:52.362204093Z 2021-08-28 16:58:52 ERROR (MainThread) [custom_components.kumo.config_flow] Unexpected exception
haqa    | 2021-08-28T20:58:52.362743388Z Traceback (most recent call last):
haqa    | 2021-08-28T20:58:52.362815926Z   File "/config/custom_components/kumo/config_flow.py", line 70, in async_step_user
haqa    | 2021-08-28T20:58:52.362871180Z     self.kumo_cache = await self.hass.async_add_executor_job(
haqa    | 2021-08-28T20:58:52.362920190Z   File "/usr/local/lib/python3.9/concurrent/futures/thread.py", line 52, in run
omriasta commented 3 years ago

Yep, as I thought, mistake in the area that grabs the cache file (because I was using your cache I didn't notice it), should be fixed in the last commit

danielgoepp commented 3 years ago

Well I'll be...if that didn't work a charm!

omriasta commented 3 years ago

Yay, do you think that's an acceptable flow? Did you get the screen to assign areas?

danielgoepp commented 3 years ago

I think it is a fantastic improvement! I have several little tweaks / recommendations that I would make. But, I'm going to leave those alone for now. Yes, I did get the prompts for the areas, that is a really nice touch. I'd like to review the code and perhaps see if I can make some of the tweaks I would like directly. You've been doing all the leg work here and it is much appreciated.

omriasta commented 3 years ago

Sounds good, let me know if you need any help with those or when I should submit a PR to @dlarrick

omriasta commented 3 years ago

Also, keep in mind that the cache file will be wiped if you delete the integration and set it up again. I think this should stay that way since it can give a way to users to start fresh when they delete the integration without directly editing files. Also if just one unit changes IP they can now handle that directly through the configure button on the integrations page without setting everything up again. Finally, if you find a better way to handle the field labels for each unit on the IP assignment form let me know as I couldnt find a better way.

danielgoepp commented 3 years ago

I think the only think I would do is run black and isort against it. I know it's annoying, but I'm hoping to work on getting this project accepted as a legit integration, and HA requires them. I have created three patch files for example based on your latest commit.

% black hass-kumo-omriasta
reformatted hass-kumo-omriasta/custom_components/kumo/__init__.py
reformatted hass-kumo-omriasta/custom_components/kumo/config_flow.py
reformatted hass-kumo-omriasta/custom_components/kumo/climate.py
All done! ✨ 🍰 ✨
3 files reformatted, 2 files left unchanged.

% isort hass-kumo-omriasta
Fixing /Users/dang/Documents/Development/hass-kumo-omriasta/custom_components/kumo/__init__.py
Fixing /Users/dang/Documents/Development/hass-kumo-omriasta/custom_components/kumo/climate.py
Fixing /Users/dang/Documents/Development/hass-kumo-omriasta/custom_components/kumo/config_flow.py
Skipped 1 files

isort_patch.diff.txt combined_patch.diff.txt black_patch.diff.txt

omriasta commented 3 years ago

No problem with that. I didn't really write much error checking into my code so others may run into issues which may be hard to troubleshoot but I guess we need to see what comes up. I'll leave it to you if you want to submit the PR instead.

danielgoepp commented 3 years ago

@omriasta - Cool, done.

https://github.com/dlarrick/hass-kumo/pull/57

We'll see when @dlarrick is back what he thinks :)

Thanks again for this!

omriasta commented 3 years ago

@danielgoepp out of curiosity, when I use the webapp and have the dev console open, I can see that the app posts data to the endpoint https://geo-c.kumocloud.com/saveUserData which includes all the unit data (that we see in the kumo_cache.json file). If you right click the request you have an option to "copy as cURL". I assume your's will not have the local ip address in the data that is posted. I'm wondering, if you add the ip address and use curl to post the data, maybe that will "kick" that info to the cloud and you will start getting that in the response cache file?

danielgoepp commented 3 years ago

Since this thread was really about the async / auth issue, I am going to close this out, but will contact you directly with more information about the missing IP address information.