[DNM] Gree Integration rewrite#373
Draft
p-monteiro wants to merge 84 commits intoRobHofmann:masterfrom
Draft
Conversation
- Change config flow to: be multi step and allow setting the device features; allow to reconfigure an entry - Separate HA and Device API logic: device API is now declarative and handles device behaviour. HA entities expose the device - Implement a coordinator - Async device communication - More error handling
- Move consts around - Proper error report on config_flow - Proper translations on config_flow - Configuration of timeout - Fetch device info during binding
Contributor
Author
|
@RobHofmann I was adding the temperature step back, but I'm not sure of its utility. |
Contributor
|
@p-monteiro |
Contributor
Author
|
Hi @domialex, that's fine. I'm more interested in the real use case for it. Should it be locked to integers? I don't think any device supports half degrees or even 0.1 steps, as is the lower limit for the current slider... |
Contributor
Author
|
@RobHofmann I don't mind breaking it up, but I'm not sure how much can be separated. It mainly touches these points:
Do you have any suggestions on how to go about it? |
Contributor
|
It’s essentially a full rewrite, and I don’t have the time to test or review all the changes in detail. @RobHofmann I don't recommend breaking the PR into smaller pieces since this is a rewrite, but I do recommend making a beta release instead and see how it goes. Put the release as a "pre-release", HACS should handle the rest. |
Owner
|
@domialex Thats fair. @p-monteiro do you know if this is a breaking release (will entities be recreated or have different names for example)? And I need some insights on feature parity with the current version. I will do some testing on my side when we clean the above things up. |
Contributor
Author
|
@RobHofmann This is very much a breaking release, because configs cannot be migrated (change in Domain). I didn't know HACS supported pre-preleases; that would be sweet because it's missing testing. @domialex, I am trying to support it as best as I can, from what I gather in this PR, VRF is an issue, and discovery seems to fail in encryption v1 (?). I need people to test and send logs, because my multisplit is working well, as is. I'd love to make more meaningful logs, but I'm not sure how, because when it fails, it basically gives an exception communicating with the device, probably because a bad request was made. But the device is a bit of a black box. Regarding feature regressions, I don't think there are any more. Some things might be configured differently during the flow, but the end result should be good. |
|
I'm getting the following error: I ran a git pull to ensure I had the latest, on the gree-rewrite branch, and then restarted HA. When I download the logs from Home Assistant rather than just clicking on the error binding, I get: Let me know if I can get further logs |
Contributor
Author
|
@ChillingSilence Thanks for the report, will look into it. Do you know the correct encryption version of your device? |
I don't sorry @p-monteiro but if you have a link to how I can extract that info, I've tried the luc10/gree-api-client but I don't believe that tells me the encryption version: I have two of these devices, both showing the same symptoms. Is there a way for me to extract that info from the app? |
Contributor
Author
Not that I am aware of. You can try manually adding the device using that key and testing both versions. |
|
I tried it again after doing a git pull, then restarted HA: I tried initially to autodiscover. That failed, so I used the output from the gree-api-client: |
Contributor
Author
|
@ChillingSilence I suspected it was happening, but you are NOT on the latest commit. Try updating to the latest commit on the correct branch. If you are on it, check the folders, delete the 2 pycache folders inside the integration and restart HA |
Do not error if the key is wrong. If so, try the generic keys first before erroring.
Allows for less parameter repetition in API calls., which also use JSON dict for payloads up until being sent.
Also disallow the list of default broadcast addresses and use only the one provided by HA. Revise this if people report problems.
|
OK progress, just pulled the latest changes, now on commit c827321 |
Contributor
Author
|
@ChillingSilence, Your device doesn't really want to play along with the fetch_device_info() for getting the firmware version, etc. I allow binding to occur even with an error in fetch info; let's see if the binding fails now. Please try again in alpha.81 and provide the log, even if the bind is successful |
|
@p-monteiro Hope this helps, using 5e3c35f, tried auto-add, then manually adding with IP+MAC, then manually specifying the encryption key on Auto Encryption Version. |
Contributor
Author
|
That's some really odd behaviour there @ChillingSilence. Just so we are on the same page:
|
|
Home Assistant is an x86_64 HAOS install. The network is a flat 192.168.22.0/24 with an Edgerouter-X giving out DHCP and a Ubiquiti U6-LR. There's no firewall or anything on the LAN, the router is doing NAT only and the WiFi is bridging to the LAN. Other things that use broadcasting are fine, like Chromecast etc I've just checked now. It seems the device doesn't work on the other public version either. It responds to pings from within the HA "Terminal & SSH" addon, and also from another workstation on my network. It is a relatively new AC purchase / install / unit so I wonder if it's perhaps just a new type? I found this repo originally and this PR seemed to be getting the most love do honestly I started here. Trying the other primary public repo now though and sadly also no joy. The app shows the following, in case it's helpful: Ran nmap and it showed all TCP ports closed, but it does respond to pings... Confirmed it's the right device too, the arp table matches the DHCP lease which matches the details from luc10/gree-api-client |
Contributor
Author
|
@ChillingSilence ahh, alright. I was under the assumption it was working in the "master" version, and I was actually losing my head, because nothing is functionally different here. So if that is not the case, we shall move to another kind of troubleshooting. I'm not sure the device information is very useful for Gree. I have the same model ID and version as you. However, my firmware is in the range v1.XX and yours is at v3.XX Netcat shows no output, correct? Or does it error? I think I read somewhere that Gree uses MQTT in some units, so maybe newer firmwares dropped the legacy UDP protocol we are using here. I will dig around. Can you send the complete model of your indoor unit? |
|
Ahh my apologies @p-monteiro I had only just tried it now with the master version so didn't know it wasn't behaving there otherwise. My bad. Netcat shows now output, correct. No errors, just, nothing. I have 2x Gree Kingfisher GWH24ATEXF units. I can confirm they work on the WiFi, and I can disconnect my phone to use 4G and still control them... So they're talking somehow. I wonder if I can packet capture from the router 🤔 |
Contributor
Author
|
@ChillingSilence, packet capture would be the best way if you actually know how to do it. I'm not good at it. |
|
It looks like it's still doing stuff on UDP port 7000? All the five packets are pretty much the same Is this helpful as a starting point thought @p-monteiro ? |
Contributor
Author
|
@ChillingSilence It's performing the normal Also, I would prefer not to pollute this PR with this since it is unrelated to it. Open a new issue, and we can continue discussing there. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[DO NOT MERGE]
⚠️ATTENTION ⚠
This PR changes the domain of the integration, so current users will lose their device configs!
The config schema also changed, so verify your YAML if you use it to configure the devices.
This PR introduces a rewrite of this integration. The main goal of this was to better align the integration with the Home Assistant (HA) development workflow with two main changes:
Other changes that are present in this PR:
I want full feature parity with the current integration and iron out some bugs before this is ready to merge (if you so wish). Currently missing and wishlist:
As you can see, there are a lot of changes, and I understand if this does not go through as is.
I am creating the PR mainly to gather feedback and track progress since most features are now present in this PR.
Don't treat this PR as blocking. I am tracking new changes and integrating them as they are committed.
Also, since this is a big rewrite, I wonder if we should use it to bump the major version (4?) and possibly change the domain to not replace the official integration (as I saw in the bug reports that it might impose some problems).