dlarrick / hass-kumo

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

feat: update to add kumo station outdoor temperature sensor #67

Closed brgaulin closed 2 years ago

brgaulin commented 2 years ago

Rough draft to pair with https://github.com/dlarrick/pykumo/pull/17

This PR adds support for outdoor temperature from the Kumo Station if one is equipped in your system.

Concerns I still need to address: [] Is the way we are starting up 2 domains now ok? Need to read up on platforms vs domains vs integrations [] Can we abstract the update command to be once for the station, and update X sensors, in case we add more in the future

dlarrick commented 2 years ago

Yes, those are the right questions :-) . I don't know the answer, which is why I've never done this for the sensor-like attributes of the indoor units. But examining some of the other integrations to see how they're organized would be a good path forward. For example, Ecobee is similar in that it has one cloud account with multiple indoor thermostats each of which can have remote sensors; it creates sensor entities for those as well as exposing a thermostat.

brgaulin commented 2 years ago

@dlarrick I've updated this PR quite substantially. Some important notes:

The overall startup flow, is to run __init__.py's async_setup_entry, and that has a call to load the platforms, the async_setup_entry in climate.py and sensors.py get executed, and the entities are populated.

The coordinator pattern seems to be the best practice for sharing data between entities / handling polling. Only one coordinator is created for each kumo unit and it handles the polling interval.

This appears to be working ok for my installation, and I'll keep this PR install going and report any issues.

Apologies that it is quite a large PR, and definitely open to any feedback, nits, or changes you think would improve the experience. Most of this PR is restructuring code, and doesn't make changes to the implementation of the climate class, hopefully it is welcome =)

dlarrick commented 2 years ago

Wow, some significant work here. Thanks for this! It's going to take me some time to go through it and your related changes in pykumo.

brgaulin commented 2 years ago

All good, and I'll beta test it in the meantime =) Also, small world. Taking a look at your repositories as I was bouncing around we may be in the same area haha

dlarrick commented 2 years ago

Please update & refresh now that pykumo 0.2.0 is out with the underlying feature.

brgaulin commented 2 years ago

I probably shouldn't have pushed that up just yet. Getting an exception:

Setup failed for kumo: Unable to import component: cannot import name 'PyKumoBase' from 'pykumo' (/usr/local/lib/python3.9/site-packages/pykumo/__init__.py)
dlarrick commented 2 years ago

I probably shouldn't have pushed that up just yet. Getting an exception:

Setup failed for kumo: Unable to import component: cannot import name 'PyKumoBase' from 'pykumo' (/usr/local/lib/python3.9/site-packages/pykumo/__init__.py)

Ah, I removed the export of PyKumoBase class since I didn't really see the point of exporting it. But I see your code here uses it. If you can get by without referencing the base class that might be better. Or if really need be we can make a new pykumo release that exports that again.

brgaulin commented 2 years ago

Ah that would do it. I'm not too familiar with Python, but I feel this might break some type safety, as the coordinator knows it is getting an object that is at least PyKumoBase but could be much more than that.

If it is possible to do something like, KumoDataUpdateCoordinator<PyKumo> and KumoDataUpdateCoordinator<PyKumoStation> we could maybe work around it. But I think it would still require exposing the interface of PyKumoBase to ensure the type safety

brgaulin commented 2 years ago

With the linked PR, it seems like this PR for hass is working again. But do let me know if you would prefer a refactor on what the coordinator depends on

dmcc commented 2 years ago

Hey, was just curious about the status of this PR?

brgaulin commented 2 years ago

I've been using this branch for a few months now and it seems stable. I think it should be safe to merge

dlarrick commented 2 years ago

Sorry guys, life gets in the way sometimes. I'll try to find some time to review, merge, and make a release this weekend.

brgaulin commented 2 years ago

No worries @dlarrick my long trial with the code has worked well :)

The only gotcha, but I assume this is present in the current version. Is that sometimes the updates don't work / are slow. Ex. I change the unit to on. Then after 2-3s it shows as on and I change the set temp. It will fail to set the temp unless I wait about 30s between commands(to the same unit)

dlarrick commented 2 years ago

Please note that I used squash-and-merge to avoid polluting the git history. So if your fork shows weird history and/or conflicts, that's why.

dlarrick commented 2 years ago

Hmm, everything looked fine on my dev setup but on my real system I am seeing:

2022-06-18 09:33:10 WARNING (MainThread) [pykumo.py_kumo_base] Error issuing request http://192.168.1.113/api: Blocking calls must be done in the executor or a separate thread; Use `await hass.async_add_executor_job()`; at custom_components/kumo/climate.py, line 198: success = self._pykumo.update_status()

I'll try to fix this up and push a new beta.

brgaulin commented 2 years ago

Oh weird. I don't have any of those errors in my logs. But more async sounds good to me

dlarrick commented 2 years ago

Updating to latest HA (2022.6.5 -> 2022.6.6) fixed it. Weird.

brgaulin commented 2 years ago

I'm on 2022.6.1. Very weird.