Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide this integration in HACS #165

Open
BenPru opened this issue Oct 21, 2023 · 17 comments
Open

Provide this integration in HACS #165

BenPru opened this issue Oct 21, 2023 · 17 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@BenPru
Copy link
Owner

BenPru commented Oct 21, 2023

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.

@BenPru BenPru added enhancement New feature or request help wanted Extra attention is needed labels Oct 21, 2023
@rhammen
Copy link
Contributor

rhammen commented Oct 21, 2023

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
Copy link
Contributor

rhammen commented Oct 21, 2023

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
Copy link
Contributor

rhammen commented Oct 21, 2023

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
Copy link
Owner Author

BenPru commented Oct 22, 2023

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
Copy link
Contributor

rhammen commented Oct 22, 2023

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
Copy link
Owner Author

BenPru commented Oct 22, 2023

There are several problems with the translation keys

Where can I see the log for the problems?

@rhammen
Copy link
Contributor

rhammen commented Oct 22, 2023

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
Copy link
Contributor

rhammen commented Oct 22, 2023

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
Copy link
Owner Author

BenPru commented Oct 22, 2023

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
Copy link
Contributor

rhammen commented Oct 25, 2023

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
Copy link
Contributor

rhammen commented Oct 25, 2023

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
Copy link
Owner Author

BenPru commented Oct 25, 2023

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
Copy link
Owner Author

BenPru commented Oct 25, 2023

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

@rhammen
Copy link
Contributor

rhammen commented Oct 25, 2023

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
Copy link
Contributor

rhammen commented Oct 25, 2023

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
Copy link
Owner Author

BenPru commented Oct 26, 2023

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
Copy link
Owner Author

BenPru commented Nov 7, 2023

@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.

@BenPru BenPru pinned this issue Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

2 participants