dlarrick / hass-kumo

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

Configflow - working version #30

Closed omriasta closed 4 years ago

omriasta commented 4 years ago

Moved some of the Constants to the .const file also moved the imports to the start of the files. Tried to keep as close to the original as possible. Still needs errors when can't connect to KumoCloud as well as the thermostat.

omriasta commented 4 years ago

Woops, just noticed that the cached json doesn't work, I left it out of async_setup_entry. for some reason when I try to add it into async_setup_entry the config flow still works but the component can't load after I restart homeassistant.

omriasta commented 4 years ago

Tried to change the config flow form so that the timeouts are floats but for some reason when I change the data schema to floats, it tells me the user input is malformed no matter what I type in the form. Opened a bug in homeassistant: https://github.com/home-assistant/frontend/issues/6119

dlarrick commented 4 years ago

As I feared I had zero time to look at your work this weekend. It sounds like you had some fun and learned some stuff, and made great progress. I will take a look later this week.

omriasta commented 4 years ago

No worries! Regarding the floats, if I set them as strings and then just convert them to floats before saving them it works but haven't updated that yet, want to see if this issue is picked up by home assistant first.

omriasta commented 4 years ago

And yes, finally was able to sit through a quick python intro video and so this is my first actual code writing with some understanding, although homeassistant seems like a pretty complicated place to start ;)

dlarrick commented 4 years ago

I took a read through your changes and in principle have no objections. Thanks for your work here.

It looks like your issue with floats is being looked at by the HA devs, so we should probably wait to see the outcome there before proceeding with either floats or strings for the timeouts.

omriasta commented 4 years ago

Great, I've been working off my fork since last week and haven't noticed any issues at all. That being said, I only have one unit so not sure about more complex settings. Also, maybe for future release, we may want to separate the timeouts and cache settings to an "options" dialog so they can be changed after initial setup.

dlarrick commented 4 years ago

separate the timeouts and cache settings to an "options" dialog

such a dialog might also just offer a button to re-fetch the cache file.

omriasta commented 4 years ago

I think I have the options all setup, will test further this weekend. Regarding re-fetch cache, the config flow options don't really offer an "action button", generally it just stores the data in a config entry and that stays static. So if I created a "refetch cache" field it would be stored in the config entry and there is no way to change that. It would basically refetch cache each time until that field was changed again. I think the way to go on that would be to create a custom service that would do this. Where one would go to the dev tools and simply call the service which would refetch the cache.

dlarrick commented 4 years ago

I'm going to give this a quick try later today (hopefuly), then merge it into master and make a beta release for people to try.

One thing is we'll need to update the README.md to tell people to use config flow, since it's currently talking about config.yaml.

dlarrick commented 4 years ago

Two questions:

  1. A lot of other integrations have an "Options" dialog accessible from their Integrations tile. For Kumo, this would be a good place to correct a changed username or password, or toggle the Prefer Cache option for a quick refresh, or play with the timeout values. Is an options dialog possible to add?
  2. What did you figure out about those timeout values, anyway? 10 seconds is way too long.. my own values that work for me are the defaults of 1.2 and 8 seconds, respectively.

Those two comments aside this seems to work fine. Nice job.

omriasta commented 4 years ago

Sorry, just got a chance to test and commit the flow options. Looking at other options flows, my guess is changing user/password is best done by deleting the integration and readding it again. I did separate out the timeouts and included the workaround for floats in the last commit. This also addresses the issue with the default values which are now set to the ones you suggest. Sorry, below is also a commit for the readme file to include ConfigFlow instructions. Let me know if you need anything else!

dlarrick commented 4 years ago

Nice, approved and merged. I will tag a beta release later this evening.

dlarrick commented 4 years ago

One note: I did a squash-and-merge so please do any further work on a new branch (rather than trying to push more changes to this one).

omriasta commented 4 years ago

Deleted. One thing I noticed is that there is a way to tell homeassistant NOT to load the integration after timeout which is useful if the local unit can't be reached (otherwise homeassistant won't fully start). In addition there should also be a way to tell homeassistant to reload the integration (this would be helpful to avoid restart when changing the timeouts as well as if you want to have the option to make changes to user/pass down the line using options dialog). I saw this on one of the forums but it was way over my head as far as the code goes. Not sure if you would know how to tackle that?

dlarrick commented 4 years ago

I don't have any further info there, no. It may all be changing with 0.111 onward where they declare HA to be "started" before all the components are done initing.

omriasta commented 4 years ago

I checked with 0.111 and for some reason the integration just keeps trying and home assistant does start the web interface but the message that homeassistant is still starting doesn't go away.

dlarrick commented 4 years ago

I'm not seeing any issues with 0.111. I'm making the beta tag this evening.