BenPru / luxtronik

Luxtronik integration for Home Assistant
MIT License
74 stars 28 forks source link

Provide this integration in HACS #165

Open BenPru opened 11 months ago

BenPru commented 11 months ago

At the moment this integration is only available if you add it a custom repo in HACS. It would be nice if HACS know it without custom repo.

rhammen commented 11 months ago

Documentation how to add to HACS is here: https://hacs.xyz/docs/publisher/ I created a PR to add the 2 needed workflow actions. The workflow action still shows some things that must be resolved.

rhammen commented 11 months ago

HACS validation passes. With hassfest, biggest problem seems to be "Error: R] [MANIFEST] Domain does not match dir name" This is about integration name "luxtronik" versus dirname "luxronik2" ...

rhammen commented 11 months ago

There are several problems with the translation keys, e.g. : Invalid translation key 'heatpump running', need to be [a-z0-9-_]+ and cannot start or end with a hyphen or underscore. for dictionary value @ data['entity']['sensor']['status_line_1']['state']. Got {'heatpump running': 'The heatpump runs', 'heatpump idle': 'The heatpump is idle', 'heatpump coming': 'The heatpump is coming', 'heatpump shutdown': 'The heatpump is shuting down', 'errorcode slot': 'Heatpump error code in slot 0', 'pump forerun': 'Pump forerun', 'defrost': 'Defrost', 'writing on LIN connection': 'Writing on LIN connection', 'compressor heating up': 'The Compressor is heating up', 'compressor heater': 'Compressor heater'}

So spaces are not allowed. Anything against changing all spaces in the keys (e.g. in class LuxOperationMode(StrEnum) ) to underscores?

BenPru commented 11 months ago

LuxOperationMode

I think it have to match with the return values from Bouni/python-luxtronik. We can try to get the raw id value from the module or copy the connection code and use our own... For the copy we have to ask Bouni.

But wait... Bounis Luxtronik is available in HACS. How does he fullfil the requirements?

rhammen commented 11 months ago

But wait... Bounis Luxtronik is available in HACS. How does he fullfil the requirements?

He does not use translations, so no issue with the translation keys.

BenPru commented 11 months ago

There are several problems with the translation keys

Where can I see the log for the problems?

rhammen commented 11 months ago

There are several problems with the translation keys

Where can I see the log for the problems?

Just go to the 'Actions' tab in the repository. (It's to the right of the 'Pull requests' tab).

rhammen commented 11 months ago

Reading the translations documentation, isn't it as simple as adding a "translation_key" properties to the entity definitions? I see several enityties with "a translation_key_name" property instead of a "translation_key" property. And several entities completely lack a translation_key property.

I'll have a look if I can fix the translation errors in this way.

BenPru commented 11 months ago

This message is wrong:

extra keys not allowed @ data['entity']['sensor']['error_reason']['cause']. Got {'name': 'Cause'}

Does the script not allow the new translation keys for attributes?

rhammen commented 11 months ago

With PR #171 the translation key errors are fixed.

The following 2 hassfest errors still need a fix: Error: R] [MANIFEST] Invalid manifest: extra keys not allowed @ data['homeassistant']. Got '2023.1.0' Error: R] [MANIFEST] Domain does not match dir name

rhammen commented 11 months ago

I also fixed the

Error: R] [MANIFEST] Invalid manifest: extra keys not allowed @ data['homeassistant']. Got '2023.1.0' error in #171 .

Only the

Error: R] [MANIFEST] Domain does not match dir name remains.

I suspect this one may have a relation with the existing (Bouni) luxtronik integration in HACS? @BenPru : What is the plan/approach to fix this one?

BenPru commented 11 months ago

Error: R] [MANIFEST] Domain does not match dir name remains. I suspect this one may have a relation with the existing (Bouni) luxtronik integration in HACS? @BenPru : What is the plan/approach to fix this one?

Yes. This is a problem. To become a core integration we can not use "luxtronik2". We have to use "luxtronik". So I think we have to switch. But then you can not use Bounis and this one at the same time. And the current version removes the yaml config and points to Bounis... I don't know how many users are using Bounis and this one at once. Or need more than the default sensors...

BenPru commented 11 months ago

With PR #171 the translation key errors are fixed.

The following 2 hassfest errors still need a fix: Error: R] [MANIFEST] Invalid manifest: extra keys not allowed @ data['homeassistant']. Got '2023.1.0' Error: R] [MANIFEST] Domain does not match dir name

Did you test it? There are many places with checks value == enum.value. With changing the enum to valid hass texts all those checks fails. I think we have to wait for the new pypi python-luxtronik version to get the raw value and change the enums to IDs. Alternative copy the python-luxtronik code after asking Bouni. See https://github.com/Bouni/python-luxtronik/blob/653f8a575ec57703155999d49007c4e506a27d33/luxtronik/datatypes.py#L50 https://github.com/Bouni/python-luxtronik/issues/136

rhammen commented 11 months ago

With PR #171 the translation key errors are fixed. The following 2 hassfest errors still need a fix: Error: R] [MANIFEST] Invalid manifest: extra keys not allowed @ data['homeassistant']. Got '2023.1.0' Error: R] [MANIFEST] Domain does not match dir name

Did you test it? There are many places with checks value == enum.value. With changing the enum to valid hass texts all those checks fails. I think we have to wait for the new pypi python-luxtronik version to get the raw value and change the enums to IDs. Alternative copy the python-luxtronik code after asking Bouni. See https://github.com/Bouni/python-luxtronik/blob/653f8a575ec57703155999d49007c4e506a27d33/luxtronik/datatypes.py#L50 Bouni/python-luxtronik#136

Yes, I have tested it (with english language). I did not test the german and polish languages, so there the risk of a problem/bug is bigger, The Heatpump Status, Status1 and Status3 get the correct text values. And when the Heatpump is on, the climate entity shows that it is Heating.

I achieved this by modifying the Status, Status1 and Status3 values when they are read. See the modified correct_key_value() function in common.py :

def correct_key_value(
    value: Any,
    sensors: LuxtronikCoordinatorData | None,
    sensor_id: str | LP | LC | LV,
) -> Any:
    """Handle special value corrections."""
    if (
        sensor_id in [
        LC.C0080_STATUS,
        LC.C0117_STATUS_LINE_1,
        LC.C0119_STATUS_LINE_3,
        ]
    ):
        value = value.replace(' ','_').replace('/','_').lower()  
.....

I think with this approach I addressed that the checks value == enum.value remain working correctly. So I believe these checks remain working as they should, and I do not recognize them all failing.

Using the raw values from luxtronik could be an alternative, but then we we need a new pypi release of bouni/Luxtronik. If I understood correctly there is currently no timeline for that, and - maybe reading a bit between the lines - it could take months for a new release to be available there... I'd rather not wait that long...

Of course it is always possible that I made a mistake somewhere. In case you or somebody else observe something is not working correctly, let me know and I will have a look.

rhammen commented 11 months ago

A more general question about this issue "Provide this integration in HACS": Does opening this mean that you have abandoned the approach to get this integration integrated in HA Core (for the near future) ?

BenPru commented 11 months ago

Does opening this mean that you have abandoned the approach to get this integration integrated in HA Core (for the near future) ?

No. It's still my plan. But... My time is short. And it doesn't work without a new python-luxtronik version or copying Bouni's source code and fixing #108. And unfortunately there was not enough support for the HA Core PR review. I don't know if it will be better next time. Furthermore, you have to submit multiple PRs because only a limited number of domains are allowed each time. This means that the project has to be divided up in a complex way and each part has to be tested! So unfortunately it’s a longer, more complex process. So I find HACS to be an interim solution and, as you can see, a good interim step to improve the code. Thank you for your great support.

BenPru commented 11 months ago

@rhammen Sorry for the delay. I have created a new branch luxtronik_by_ids. I change the key management to simplify the enums. No double string and int enum. Just 3 IntEnums for Params, Calcs and Visis. It is not running and I'm still working on it.