Official HomeAssistant integration

[starkillerOG]

Has there already been an attempt at making this into an official HomeAssistant integration?
If not is there a good reason why not to make it an official integration?

I am currently in the process of buying solar pannels for my house and 2 of the 3 quotations I now have include a Goodwe inverter.
If I were to choose for a Goodwe inverter I am more than happy to help out with converting this into an official integration (have done it before a couple of times for other integrations).

[mletenay]

Not really, resp. not anymore.
I had the intention to push in up to HA sources, once it is stable enough.
Which actually already is.
We have it deployed for more than a year without any serious problems and the code was rather stable for most of the time.
Just these past 2 weeks were quite intensive, new inverter families were added and also bunch of new sensors were identified and added.
The goodwe api code was extracted to separate pypi library (not yet used by the plugin at this very moment yet, it is being maintained in parallel).
So to wrap it up, I believe it is ready and any help in this aspect is greatly appreciated, even more if you already have experience with it.

[starkillerOG]

Sounds good.
I found the library here: https://github.com/marcelblijleven/goodwe
Seems like indeed one of the first things that needs to be done is to convert the current custom_component to use the seperate library (will be required by HomeAssistant).
Furthermore a Config Flow needs to be implemented since YAML configuration will not be allowed by HomeAssistant anymore.
I could certainly help with making the Config Flow if you like.
However since I don't have a device yet it will be a bit tricky to test the code.

Are there any plans to use diffrent platforms than only the sensor platform?
(If that is the case, we probably want to implement a DataUpdateCoordinator together with the Config Flow).

Do you have any contact with people working at Goodwe?

[mletenay]

Yes, that's the library. I'm making some final tweaks there and will switch the plugin to it in next few days.
Regarding the config flow, it would be extremely helpful if you can create pull request with that, at this moment I have zero experience with it. I will do the testing, no worry.
No, at the moment I don't have any plans for other platforms, feel free to suggest if you have any idea for improvement.
And lastly no, there's no contact with goodwe. From the knowledge I collected from various PV related forums on net, they were not very keen sharing information. But at this moment (at least for ET/EH based inverters) we know everything it was not know., even without them.

[starkillerOG]

@mletenay I will start working on a config flow.
I see the current YAML options are:

        vol.Required(CONF_IP_ADDRESS): cv.string,
        vol.Optional(CONF_PORT, default=8899): cv.port,
        vol.Optional(CONF_INCLUDE_UNKNOWN_SENSORS, default=False): cv.boolean,
        vol.Optional(CONF_INVERTER_TYPE): cv.string,
        vol.Optional(CONF_COMM_ADDRESS): cv.positive_int,
        vol.Optional(CONF_NETWORK_TIMEOUT, default=2): cv.positive_int,
        vol.Optional(CONF_NETWORK_RETRIES, default=3): cv.positive_int,
        vol.Optional(CONF_SCAN_INTERVAL, default=timedelta(seconds=30)): cv.time_period,
        vol.Optional(CONF_SENSOR_NAME_PREFIX, default="GoodWe"): cv.string,

We will need to reduce this since HomeAssistant dislikes any options that are not striktly needed. It just makes it more complicated for first time users that do not have experiance.

So CONF_NETWORK_TIMEOUT, CONF_NETWORK_RETRIES and CONF_SCAN_INTERVAL need to go and we need to use a sutable default value.
CONF_SENSOR_NAME_PREFIX is not needed since we can rename sensors when setup through config flow.
CONF_INVERTER_TYPE schould be automatically detected during config flow (and then just saved in the config entry)
CONF_INCLUDE_UNKNOWN_SENSORS, I don't think this is really nessesary, if we don't know what the sensors are, we schould not add them. If someone uses it they must know what it is and they schould create an issue at this repo to porperly implement it.

I am not sure about CONF_COMM_ADDRESS, can this be changed on the inverter itself or something?
Do we need that as a configuration option, or is it always fixed if we know the model of the inverter?

[mletenay]

Only CONF_IP_ADDRESS and CONF_PORT are strictly necessary, all the rest is optional.
But I would prefer to keep some of the optionals, probably with this priority:
CONF_SCAN_INTERVAL is quite often specified to lower values, 30s is not enough for many installations.
CONF_COMM_ADDRESS is in theory configurable in the inverter, mostly when there are several inverters grouped together.
But I don't think it was used by anyone yet.
CONF_NETWORK_TIMEOUT, CONF_NETWORK_RETRIES these are under normal circumstances not necessary, the defaults are fine.
But there are quite a lot of people with wifi/udp reliability issues, for them this may be really helpful.
CONF_INVERTER_TYPE, well this was quite helpful at one point in time, now the auto mechanism is improved, so I will keep it at libary level, but it can be removed from HA plugin.
CONF_SENSOR_NAME_PREFIX, yes, this may be probably removed, but AFAIK was used by people with several inverters.
CONF_INCLUDE_UNKNOWN_SENSORS, it's mostly obsolete now, can be removed.

Edit: actually the port is always fixed to 8899, so it can/should be removed.

Edit2: I suggest to keep CONF_IP_ADDRESS, CONF_SCAN_INTERVAL, CONF_NETWORK_RETRIES

[starkillerOG]

Thanks for the info, I will remove the port then.

About the CONF_SCAN_INTERVAL, schould we then not just decrease the default to lets say 5 seconds?
Or some other value that you think is most sutable?

I have tried to add SCAN_INTERVAL to other integrations before, but the core team does not like it, they will just say chose a sutable default that works in the majority of installations or even make the default model specific if nessesary. If a user really want to change it there is the option to disable the scan interval in the system options and then they can make an autoamtion that calls the update service at the intervall they like.

[mletenay]

Hmm, I personally dislike this core team approach. It is really installation specific how often you want or can poll the device.
Edit: I have just asked people in our discord server what scan_interval they use to have better idea about real world.

[mletenay]

Regarding the CONF_NETWORK_RETRIES, that parameter makes less sense more frequent the polling is.
Since CONF_NETWORK_RETRIES * CONF_NETWORK_TIMEOUT has to be smaller that CONF_SCAN_INTERVAL.
And if we e.g. reduce the timeout default to 1s, scan_interval to 5s than the retries parameter is getting useless.

[mletenay]

Look here - https://discordapp.com/channels/758315145922740234/758315145922740237/884040048050315275.
3 users and the scan_interval values a 1, 10 and 20 ;-)

Edit: actually the argument of the user with 20s is valid one. In case of ET inverter, there are 142 ! sensors at this moment.
And for many people it is just spamming the recorder.

Edit2: Actually, some inverter types do not offer all the values and the energy has to be calculated via Riemann integration.
And less frequent polling means the integral value is more guess and less actual measurement.

Isn't there some negotiation/arguing with HA core team possible ?
Removing this parameter seems to have negative feedback from everyone so far ...

[starkillerOG]

The 142 sensors insn't quite an argument because you can just disable the enties you don't need.
Or exclude them from the recorder.
From the looks of it it seems to make sense to decrease the default scan intervall to something like 5 seconds.

I will add the option to change it (although this does require the addition of a options flow which is quite some work).
But mind my words: we will get questions about this once we make the PR for the official homeassistant integration.
Of course you can argui with the HA core team, and sometimes you can also win, but most of the times they are kind of right since they have to deal with 100+ integrations they need uniformity and need to deal with such things in one manner.

[starkillerOG]

@mletenay did you already see this PR: #59?

[mletenay]

Now I did and merged. Thanks !
I can understand the HA and complexity of their efforts, just in this case I think we are special :-)
The PV inverter is not as trivial as many other integrations and there may be reasonable need for small deviation of default HA policies.

[darek-margas]

I can understand the HA and complexity of their efforts, just in this case I think we are special :-)

Well, iotawatt just got merged with core and ruined in fact due to changes enforced by core.

[tinuva]

Regarding the home assistant core team not wanting too many advanced configs in the config flow.

I see that ZHA has limited settings on the gui settings, but it allows advanced configuration through yaml.
See advanced yaml config: https://www.home-assistant.io/integrations/zha/#configuration---yaml
Main gui config: https://www.home-assistant.io/integrations/zha/#configuration---gui

So can't we leave some settings for the more advanced users in the yaml config?

[starkillerOG]

@tinuva what other settings besides the scan_intervall would you like to have?

[starkillerOG]

@mletenay is there anything that needs adressing before we can make an PR in HomeAssistant to make it an official integration?

[mletenay]

I'm still struggling a bit with the available property and handling of those consecutive failures, but beside that I believe we're almost ready. I plan to finish this over the weekend, release the library, release the last HACS version and I hope next week we can make PR to HA.

[mletenay]

OK, I did all the code changes I had in mind, the code is ready to be offered to HA core.

During the conversion to UI flow configuration, the network_timeout, network_retries, inverter_type, sensor_name_prefix, include_unknown_sensors were lost and scan_interval became optional.
As for the network_timeout, inverter_type and include_unknown_sensors, it's really not a big deal, those params were rarely used and after recent progress in inverter protocol implementation they lost most of their original meaning.

The comm_address is now mandatory parameter (with default to none), but I'm starting to thing it's an overkill there.
In theory, one can have chain of coordinated inverters and in such case each has to be assigned unique value.
You also can see (and even change?) the value in the inverter mobile apps.
But looking closer at the code of those mobile apps, the protocol implementation is using hard-coded default addresses there.
So it really seems that comm_address is meant for big/industrial installations and those most likely will not be managed via HA.
For the vast majority of residential users, the default value is anyway the only acceptable as long as they use mobile app.
I'll definitely leave the option in library API code, but for HA I suggest to actually remove it.

Or maybe to replace it with the lost sensor_name_prefix. You obviously can rename devices/entities in HA, but I did not find a way ho to do it bulk for all sensors. So unless I missed something, the prefix may be welcome not only for people with 2 or more inverters, but also to generic user (like me), which may prefer no prefix at all (and not the default GoodWe).

As for the remaining network_retries which got lost, we can make it an optional param like scan_interval.
I created it as a workaround for people with unreliable networks/inverters, but to be honest, I have no idea if it was actually used by anyone.

@tinuva @fizcris @starkillerOG Your opinions ?

[tinuva]

Sounds good to me. I like that scan_interval is still available even though it is optional. Would be great to still be able to set it.

[fizcris]

I use the data from the integration as a "real-time" energy management by using a bunch of pyscript automations. So I need a quite high frequency scan interval (around 0.5 - 1Hz). So at those rates, a lot of errors occur because of radio and UDP unreliability. For my specific situation network_timeout, network_retries and scan_interval become mandatory, then eliminating these parameters would highly diminish the integration usability. Nonetheless, I am aware that my situation is a corner case and it is not the main purpose of this integration. For that, I don't understand how these configuration parameters could not be kept as optional through the yaml files as that approach would not interfere with the amateur user and would give more freedom to the more experienced developers.

[mletenay]

I have added network_timeout, network_retries to optional parameters section along existing scan_interval.
I am still not sure about fate of comm_address and sensor_name_prefix.

[mfaust78]

I think it would be good to have comm_address as an optional parameter. Having it mandatory does not make sense as for the majority it will be the default value.

[starkillerOG]

I think we can indeed remove the comm_address if it is also hard-coded in their official mobile application.
We can always add it back in if someone needs it.

About the sensor_name_prefix, I would not add that.
If you rename the device itself you get the option to rename all the entities with the same prefix as the devicename in bulk (I think).
Besides renaming the entities schould only be a one time thing when you install the integration for the first time.

About the network_timeout, network_retries I think it is fine that it is added as optional parameters.
Could be that the homeassistant core team is going to have an opinion about that, but we will see when we create the PR for the official integration.

Definitly do not add YAML configuration again, that just adds complexity and extra code paths.
If anything more is needed add them as optional settings in the config flow like it is done now.

[starkillerOG]

@mfaust78 comm_address can not be in the options flow since it is required for the connection to the device which is already checked during config flow. therefore comm_address will always need to be in the config flow.

One option I have used before is to only show the comm_address after a first connection error in the config_flow. In that way the majority of users does not see the option, but the ones that need it will get a first connection failure after which they will be able to fill in the correct comm_address.

[mletenay]

@starkillerOG Agreed, I'm going to remove the comm_addr and we can start PR activity.
I've seen somewhere a unit test is required by HA core team ?

[mfaust78]

@starkillerOG Thanks for your reply. Since it can not be optional, better remove it. If you find time later, would be cool to add the way you described to show it only after a first error.

[mletenay]

OK, progress started:
core mletenay/core@8855fad
brands https://github.com/mletenay/brands/commit/9c0ec16cef09a44fc011873d1450da33370254ed
docs mletenay/home-assistant.io@f4c3516

Config flow tests are missing, rest is from my point of view ready, any feedback/review welcome.

[mletenay]

@starkillerOG I'm looking at the config flow tests, trying to inspire from other integrations, but to be honest, I'm a bit lost. Any help is welcome.

[starkillerOG]

@mletenay no problem I can make the tests, I just need to find some time to do it, quite bussy at work at the moment.
So maybe this weekend.

[mletenay]

@starkillerOG I've again spent quite some effort trying to write proper tests for the config flow, but I failed again.
I'm afraid its still above my current python/HA skills :-(
Any help appreciated.

[starkillerOG]

@mletenay sorry for the late replay.
I was quite bussy the past weeks, but today I have some time to work on it.

[starkillerOG]

@mletenay I finished the tests, the PR is here: mletenay/core#1
I tested it locally and it passes with 100% test coverage.
You schould just be able to accept the PR and I think then we are good to go to open the HA PR right?

If you need anything else, please let me know.
This week I will have some time to work on it (specifically today).

[mletenay]

Perfect ! Thanks a lot.
I'll merge it tonight.
And then I think we have everything ready for PR to HA core.

[mletenay]

The PRs to HA were created:
home-assistant/core#58503
home-assistant/home-assistant.io#20000
home-assistant/core#58503

[mletenay]

@starkillerOG Would you be so kind and look at the failing tests ? I still have some troubles running them locally. Thanks

[starkillerOG]

@mletenay Yes, no problem. Will see if I have some time this weekend.

[starkillerOG]

@mletenay sorry for taking so long, been a bit bussy at work.
I tried the tests again and they still all pass, just need to wait for a core-team member to approve the workflow.

[mletenay]

@starkillerOG I have incorporated all the review feedback from MartinHjelmare with the only exception of that test of unique id.
I need your help there, so if you could find some time for that, it would be great.
Thanks

[mletenay]

The PRs to HA were accepted and merged, the goodwe integration will be native part of it since 2022.02