[DNM] Gree Integration rewrite#373
Conversation
|
@RobHofmann I was adding the temperature step back, but I'm not sure of its utility. |
|
@p-monteiro |
|
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... |
|
@p-monteiro I'm getting an error when trying to auto discovery (vrf). |
|
@meirlo Thanks for the heads-up. Interestingly, I believe that bit is the same as the current version. |
|
here is the same operation with the pack dump: |
|
@meirlo Seems like it is obtaining a device key, and that key is not decrypting the pack correctly. I'll have to check in more detail. If you can debug further since I don't have a VRF, it'll be helpful. |
|
@RobHofmann @meirlo @domialex I'd like your input on some decisions that are blocking this.
|
the encryption key should the same for all the sub unit as it calculated only with the mac address (without the sub_mac_addr). update: I'm not sure if that's the only issue. as I don't have much time to test that currently. |
|
@p-monteiro |
|
@meirlo Good catch, I also noticed the subList request uses the default key and not the specific device key, so I also changed to that. Now I'm not sure if this works because the device key is actually the generic one, or the sublist requires the generic one. |
|
I pushed the domain change, beware! |
|
@RobHofmann Ah ok, I'll add it back. Would a bool for "Supports half increments" be better? |
I would leave the steps option. people can then choose how to do this (even 0.1's are theoretically possible). |
|
@RobHofmann I re-added the temperature step, but I limited it to 0.5 increments as the API only supports half ºC and integer ºF. A warning is logged if a non-integer ºF is set, since it is rounded to the nearest integer. |
Sorry it took me a while, quite busy lately. here is the log: Please let me know if you have an idea where the issue is, or need me to debug further. |
|
@meirlo Thanks for your patience with us. |
|
here is the next error :) BTW, I see that this integration does not override the official integration anymore and also the Gree logo is missing. |
That is intended. Once we go through with this PR, we can PR the logo on the official custom integration brands. About the error, as predicted, the device was added, but now it is failing to get the status, which is weird since the log shows that it receives the status correctly, and there is no exception logged. I'll check later. Update: Actually, I see that some important fields are blank. Do you happen to know if some of the properties are given by the main device while others are given by the subdevices? That can explain the problem. Also, can you provide screenshots of how the official app organizes the settings for the VRF? |
The office AC is the only standalone one and is not part of the vrf system. The living room is the main one, meaning it controls the cool/heat setting for all the vrf ac (since there only a single outdoor unit, it's impossible to set some indoor units to cool and some to heating), it is possible to change this setting for other ac units, but it will revert to Fan mode if that's conflict with the main ac. Here are some screenshots from the bedroom ac: 
Please let me know if you want a screenshot from a specific ac/page |
|
@meirlo Thanks for the screenshots, I'm having a bit of a hard time understanding the options, not gonna lie. More importantly, how would you expect the controls to work in Home Assistant in terms of devices and the controls they provide? |
|
@p-monteiro does it looks different on your ac? In home assistant, I believe each unit needs to be a separate device, with its own settings. similar to how it shows in the gree+ app. The limitation of the mode that must be set on the main unit first, can be ignored. if one want to sync between the mode in all ac it can be done via automation, no need to over complicate the integration. |
|
@meirlo The thing is, at first glance, the subdevices API does not provide some of the parameters that you want (they came empty in the status pack, but it needs to be further investigated), so maybe the official app sends commands to both the subunit and the main unit when you change the mode on the subunit. If that's the case, it can actually be easier to make the subunits override the main one, or just omit the properties altogether. I just pushed a commit, please try it, and check if the main device can be discovered, added and it works. Ignore the subunits for a moment hehe |
|
This might need some architectural changes in the way we communicate with the device (possibly to communicate with the main and sub devices for getting and setting the status) |
|
@meirlo Another thing, in a VRF system all units are the same as the main one right? So the capabilities of them all are the same. |
|
I'm getting an error when trying to add the entity without the sub unit For the second question, the indoor units are not always identical. For example, my living room ac is a different one than the rest. Can you please elaborate which parameters are missing? |
|
@meirlo I'm having a hard time debugging the issue, sorry. Please try the latest version and provide the full debug log, not only the exception. |
|
I've been using this for some time now to control my AC without any problem. It would be nice to have more people trying out the latest commit to find bugs or missing features so this can be merged, including people with VRF. |
|
@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 ? |
|
@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. |
|
@RobHofmann I'm down to get this merged into a different branch and published as an alpha/beta version. Fixes can then be made against that. I'll let you decide whatever you deem fit for the project. Remember, this is a breaking update. Refer to the original PR message (#373 (comment)) |
|
Hey! Im unable to discover my devices through this integration. I've created a fix for this on my own version yesterday: Can you please include this in your setup too? As for your comment on YAML: I'm not sure. I'm not using it anymore. But theres a big reason why I could see people wanting to use this: Configuration as Code --> Easier to save in a repository. Same with Infrastructure as Code. This also means you can version your configuration. Reason why i'm currently not using it anymore is because I need to test the config flow for the ingration, but for other components I use YAML as long as I can :). But tbh i'm not sure how other people look at this. PS. I've started testing your release just now. |
|
@RobHofmann Hi, thanks for the notes. Keep testing and let me know whatever you need. As for the YAML, that's a good and valid point, though HA backups should address the issue. I'm just more comfortable following the official guidelines. But your call. |
You need to brief me on the validation part here. No hurry, let me know when you get it in :).
Backups would be nice, but for a docker setup this is not a feature. I do have backups in place, but its all "self managed". I trust more in plain code "backups" to a repository where I can also use things like branching (and test on my dev machine etc). So in short: i assume powerusers have more benefit from the YAML than "regular" users. Normally i'd absolutely go "home assistant native", but so far I'm not really seeing solutions accros the board. This is why I want to keep the options open. For us it's easy to maintain YAML for now. If these challenges get solved in the future we can always drop it. |
It's just HACS and Hassfest validation to ensure integration quality.
I also have backups configured for my Docker instance, and they are automatic through the Backups page inside HA, but I do understand where you come from. That's fine, really. It requires a bit more code to do proper config validation, but that's all. It's done and working. |
While including this, I noticed that only the target host might be needed. @RobHofmann and anyone else, can you confirm if you can discover your devices when enabling the respective adapters in HA configuration? |
Can you explain what your doing here? Im unsure what you are looking for here. I dont have any network configuration in my HomeAssistant at all. In the past I did use macvlan, but i switched to docker networks due to their more secure nature. In my cross VLAN solution, its only needed to pass networks you want to scan (since scanning cross vlan is not a default option) cross VLAN. For the same subnet, it wouldn't require input. |
|
@RobHofmann Oh, so your HA Docker container is only connected to a single bridged Docker network? No macvlan or host networking, and you have no adapter directly connecting HA to the VLAN, right? In your solution, does cross-VLAN broadcast work? Or is it the case that if a cross-VLAN is specified, it targets its individual hosts? |
|
@RobHofmann Did you manage to test this in some capacity? |
|
Any chances to published this as alpha/beta reliese? |
|
Hmm im not sure, theres no option; i assume because its not a branch in my repo. I guess we need to PR this to a branch from main in my repo and from there create a release? PS. I have tested it, but I'm using a very limited set of options on my devices. So I think we need to scale up our testing audience. |
|
I still have this PR marked as a draft. |
|
@RobHofmann You need to click pre-release in a github release. |
|
Created branch @domialex i tried creating a pre-release, but theres no option to select the branch of @p-monteiro. So i assume i need to pull it in to this pre-release branch and from there create the pre-release. |
|
@RobHofmann Yes, you can only create releases or perform your workflows on your branches. This work is still not merged into one. I'll make the PR to your new branch today. |
|
Because of the new branches and all the changes, merging this will break the commit history between the old and new files, since they are shown as new rather than modified old ones. I tried to do a manual rebase, but the changes are so complex now that it is unfeasible. |
|
Also, @RobHofmann your version bump and release workflows need a bit of tweaking because they assume the releases are from the "master" branch |
|
I've created a pre-release. Let me know if it's good now. |
|
That looks good, will test. Maybe if/when it reaches a stable release, use the summary from the PR for the changelog in addition to the generated one. |
|
Also, check the validations as your Repo has some missing things |
Done, fixed :) |
7 participants
Caution
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 no longer overwrites the official integration.
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:
Breaking changes
Features
DataUpdateCoordinatorGreeDeviceabstraction for device logicRestoreEntitysupport inClimateEntityImprovements
Fixes
Known problems:
As you can see, there are a lot of changes, and I understand if this does not go through as is.