andrewleech / ha_magiqtouch

Home Assistant (HACS) integration to control Seeley MagiQtouch heating and cooling units.
MIT License
24 stars 6 forks source link

Add initial functionality #5

Closed epetousis closed 1 year ago

epetousis commented 2 years ago

I've put this one off for a while, but I'm committed to getting this integration to a baseline "working" state.

So far we have:

Ideally I'd like to clean the code up a bit, but I think for the time being it's worth just getting the damn changes merged. There's an issue relating to botocore that causes Home Assistant to break sometimes, which isn't resolved by this pull request. I've opened an issue for it in #4. I believe switching to program mode is something that the API is capable of doing, so might be worth adding. I'm not quite sure if there are enough Home Assistant modes for this feature to fit in (currently the "auto" mode is being used for automatic cooling).

andrewleech commented 1 year ago

A huge thanks for the solid work you've done here, sorry it's taken me so long to get to it.

I've recently been starting to use my system for the season which has given opportunity to test this, follow the code through and re-aquatint myself with it.

I've got a couple of minor comments but in general this looks really well organised and clean, a huge step forward.

I'm pretty happy to merge as-is and add a few little things on to myself; I've fixed the text during initial login, made fan-only mode work (mostly) for me and extended some of the logging a little.

I think more might want to be done with the refresh; I've noticed when changing settings they often revert a second or so later, before going through properly ~10sec later. This is kind of expected as it takes time the for settings to go through and be updated on the system so the immediate refresh when changing anything still returns the previous setting, then it's up to the 10sec update rate to pull through the new setting. I'd like to trial setting a much faster poll interval for maybe 15sec after any change to pull updates quicker, before reverting to slower updates to pick up manual changes made elsewhere.

Long term it might be nice to be "smarter", caching and showing the "expected" new setting during this fast update rate period until it's detected that the change has gone through, with timeout in case it never goes through properly.

That's work for the future though; for now let me know if you've done more locally you'd like to add here before I merge, else I'll merge as-is and add my little changes in top!

Thanks again!

epetousis commented 1 year ago

I'd like to trial setting a much faster poll interval for maybe 15sec after any change to pull updates quicker, before reverting to slower updates to pick up manual changes made elsewhere.

This would be great, it was something I had in mind but didn't end up implementing myself. I think the refresh co-ordinator was supposed to have some sort of helper for this?

Long term it might be nice to be "smarter", caching and showing the "expected" new setting during this fast update rate period until it's detected that the change has gone through, with timeout in case it never goes through properly.

I like this idea too, would probably allow the client to be more of a good citizen.

for now let me know if you've done more locally you'd like to add here before I merge

Totally good on my end, I haven't touched the code since I submitted the pull request (I didn't end up taking another stab at #4). Appreciate you reviewing the changes!

andrewleech commented 1 year ago

I'm hopeful to have #4 fixed too but including a copy of some of those deps in the repo, rather than needing them installed.

Others (like structured _config) are still set as dependencies that need HA to issuer them because they have binary modules that would need to be kept in sync with whatever version of python HA is using whereas botocore etc are pure python so safe to include.

I haven't had a chance to clean up my git tree and push it all, hopefully get that sorted in the next couple of days!