Ensure backwards compatibility

[Guzz-T]

With the changes from these pull requests, the corresponding entries can be found under the old names again. This is possible because you can now assign more than one name to the entries.

The names are passed directly to the constructor. No additional files / additional constants are required. It is therefore immediately clear which names can be used to find the entry.

The current version can now be exchanged with version 0.3.14. Tested with BenPru/luxtronik/2024.10.5-beta on HA 2024.10.4 (with own bugfix for BenPru/luxtronik#283). A new release could then be created (0.4.0?)

Fixes #168.
Replaces #171.

[github-actions]

Coverage Report

File	Stmts	Miss	Cover	Missing
luxtronik
__init__.py	184	11	94%	42–43, 46–51, 257–258, 263
__main__.py	21	21	0%	3–49
calculations.py	23	1	96%	322
datatypes.py	290	1	99%	107
discover.py	40	34	15%	21–69
parameters.py	21	1	95%	1204
visibilities.py	12	1	92%	403
luxtronik/scripts
dump_changes.py	43	43	0%	7–85
dump_luxtronik.py	26	26	0%	6–52
TOTAL	713	139	81%

Tests	Skipped	Failures	Errors	Time
131	0 💤	0 ❌	0 🔥	7.371s ⏱️

[Bouni]

LGTM

[Bouni]

I'll wait for @gerw s review on this before merging it

[gerw]

I am a little bit biased, because I wrote the previous PR #171. I have had a careful look at both PRs and I must admit that I like my PR more. In my opinion,

• it is cleaner to have the compatibilities in an extra file,
• it is not so nice that Base._name has not a 'stable' type (str vs list of str)
• my test cases are more elaborate and make sure that no further incompatibilities arise in the future.

[Guzz-T]

• it is cleaner to have the compatibilities in an extra file,

Everyone has their own coding style :) In my opinion, an additional file is more difficult to maintain. Especially since sometimes 3 names are used (see HUP). If you want to assign a new name here, all you have to do is enter the name at the top of the list. Additionally, in this version, the check for obsolete names can also be done outside of the data vector.

• it is not so nice that Base._name has not a 'stable' type (str vs list of str)

To get around this, we could add the following:

if isinstance(name, list):
    self._name = name
else:
    self._name = [name]

Then at least self._name would be static.
Or we could always hand over a list. This would then usually only consist of one element.

• my test cases are more elaborate and make sure that no further incompatibilities arise in the future.

I think we could also use the test code for this.

BTW, your test doesn't reveal the bug in Visibilities with the index > 354. there is simply no such thing as 100% test coverage :)

[Guzz-T]

Updated source code to:

• convert self._names into a "static" list
• copied test_compatibilities.py from Add compatibility feature #171 (with minor adjustments: "obsolete" dict and check removed, visibilities bug added)

[gerw]

I still think it is more in the style of this library, if the compatibilities reside in their own file. Note that the constants have been moved to a separate file some time ago. In the same spirit, the compatibilities should be in a separate file and should not "pollute" other files. I do not see the benefit of this new PR over #171 What do the others say in this regard? @Bouni @kbabioch

[Guzz-T]

Please note that the current implementation in #171 cannot handle further name changes. As an example, if you change a name from a to b to c. Then you first had to add a: b. However, as soon as you switch to c (and add b: c), the look-up with a no longer works because b would then no longer be found either. Maybe you could:

• work with indices (only possible if you split up the obsolete dict into calcs,...)
• do the look-up within a loop as long no entry is found
• search and change all ...:bto ...:c (quite a lot of work for a small name change)

In addition, you may run into problems if entries from two different data vectors have the same name.

Of course it makes sense to put the constants in an extra file. However, the goal of that is to remove or reduce dependencies between the files.

I see the advantage of my implementation as:

• reduce complexity
• easy handling
• clearer structure
• name-check can be done without the data vector object

The concrete implementation of the data vectors is now nothing more than the assignment of names to the indices. I see no reason that outdated names need to be outsourced.

[Bouni]

I just looked at both, this PR and #171

I like the idea of @Guzz-T to have names as a list and have the first element to be the prefered one and all after that the obsolete names.

On the other hand, I agree with @gerw that we should have these in a seperate constants file.

How about we merge both approaches?

We have a constants file like this (with a tubple of tuples for each, parameters, calculations and visibilities):

LUXTRONIK_PARAMETER_COMPATIBILITIES = (
    ("New_fancy_name", "ID_Einst_SuSilence", "Unknown_Parameter_1092"), 
    ("ID_Einst_SilenceTimer_0", "Unknown_Parameter_1093"),  
    ("ID_Einst_SilenceTimer_1", "Unknown_Parameter_1094"),
    ("ID_Einst_SilenceTimer_2", "Unknown_Parameter_1095"),
    ...
)

Then we change the 3 categories (parameters for example) like so:

...
self._data = {
            0: Unknown(),
            1: Celsius(True),
            2: Celsius(True),
            3: HeatingMode(True),
            4: HotWaterMode(True),
...

We then lookup the possible names by the number of the parameter, calculation or visibility.

If the lookup is done via name, we have to iterate over the const until we find the right entry, but that has to be done in either of your approaches anyway if I'm right.

What do you think?

[Guzz-T]

With this proposal, it is even more difficult for the user to look up the assignment of the names to the data types.

Should the user work with the constants at all? If not, then I would not implement constants.

[gerw]

I would also prefer if parameters.py would contain the name, because this is where the user could look up the names. Moreover, it would be nice if this file stays as simple at possible. What about changing compatibilities.pyto use something like

LUXTRONIK_COMPATIBILITIES = {
    "ID_Einst_SuSilence" : ["Unknown_Parameter_1092", "bar", "foo"],
    ....
}

where ID_Einst_SuSilence is the current (preferred name) and the list contains the old / obsolete names? Then, there is no need to change parameters.py.

[Guzz-T]

I would also prefer if parameters.py would contain the name, because this is where the user could look up the names.

For the same reason I would also include the “alternative” names in parameter.py.

only if you insist on outsourcing the “alternative” names. In this case I would like your current suggestion better.

If so, how should we deal with the following:

• identical names in different data vectors (parameter and calculation for example)

• Should we support functions like in this pull-request Base.get_supported_names() and Base.check_name()

[gerw]

I would also prefer if parameters.py would contain the name, because this is where the user could look up the names.

For the same reason I would also include the “alternative” names in parameter.py.

But the user does not have to look up the old names?

* identical names in different data vectors (parameter and calculation for example)

Is this really an issue? Is there some example where we have a name clash? (I know that 0.3.14 uses Unknown_Parameters_# also for visibilities. This seems to be a weak argument, since I do not know any use for the visibilities.)

* Should we support functions like in this pull-request `Base.get_supported_names()` and `Base.check_name()`

I am not sure. In my opinion, it would be enough to access the current name of a property, but this should be possible by

parameters.get("outdated_name").name

[Guzz-T]

But the user does not have to look up the old names?

I mean, anyone who has worked with this repository is used to looking in the parameters.py (or the others) to see what fields and data types there are. The users who already use the old names will wonder why they are no longer there and their code still works (= very magic code).

Is this really an issue? Is there some example where we have a name clash? (I know that 0.3.14 uses Unknown_Parameters_# also for visibilities. This seems to be a weak argument, since I do not know any use for the visibilities.)

That's exactly what I thought of first. See BenPru's usage of visibilities.Unknown_Parameter_357 which is titled as V0357_SILENT_MODE_TIME_MENU. So we would need an entry "Unknown_Parameter_357": "Silent_mode_time_menu" for the visibilites and "Unknown_Parameter_357": "ID_Einst_SuMk225_zeit_0_2" for the parameters.

[Guzz-T]

[Update]:
This comment focuses more on the topic of "Direct Interchangeability of the Two Versions."

[Original]:
The longer I think about the topic, I think we also need to support earlier data types. Consider this example:

op_mode = lux.calculations.get("Unknown_Calculation_80")  # outdated name for "ID_WEB_WP_BZ_akt"
op_mode.raw = 1             # op_mode.value returns "1"
if op_mode.value == 1:     # evaluates to "true"
    ...

In this example, an object of type Unknown is returned. When switching to the latest version, you now get an object of type OperationMode. Then:

op_mode = lux.calculations.get("Unknown_Calculation_80") 
op_mode.raw = 1             # op_mode.value returns "hot water"
if op_mode.value == 1:     # evaluates to "false"
    ...

This construct would be a possible solution:

self._data = {
     ...
    80: [OperationMode("ID_WEB_WP_BZ_akt"), 
        Unknown("Unknown_Calculation_80")],
    ...

or

self._data = (
     ...
    ([38], OperationMode("ID_WEB_WP_BZ_akt")), 
    ([38], Unknown("Unknown_Calculation_80")),
    ([81..90], IPv4Address("ID_WEB_SoftStand")),
    ...

To be able to handle type changes without name changes, we could implement a factory pattern (something like this):

class CalculationsFactory:

    @classmethod
    def get_calculations_data(cls, interface_version):
        if interface_version == "v1":
            return {
                38: Unknown("ID_WEB_BZ_akt"),
                ...
                }
        if interface_version == "v2":
            return {
                38: OperationMode("ID_WEB_BZ_akt"),
                ...
                }

class Calculations(DataVector):

    def __init__(self, interface_version = "v2"):
        super().__init__()
        self._data = CalculationsFactory.get_calculations_data(interface_version)

[Guzz-T]

Some of the changes in this pull request go beyond just informing the user about the new names. My focus here was on the direct interchangeability of the versions. However, this is not strictly necessary. Do you see it differently?

As a result, I created pull request #191.

[Bouni]

Hi everyone, sorry for the long silence. Free time is a rarity in the last months ....

Regarding name overlaps

as mentioned by @Guzz-T s question in #187 (comment)

If so, how should we deal with the following:

* identical names in different data vectors (parameter and calculation for example)

I did a quick check and searched all names in all 3 groups for overlaps. There are None so we can rule that out.

Regarding the lookup issue for the user

As I proposed in #189 I did a first draft in #192 which autogenerates docs for the user to lookup the desired names / ids in a GH pages page: https://bouni.github.io/python-luxtronik/

This gives the user the ability to search through the 3 groups to find the wanted entry.

I could imagine to add a description attribute to all datatype classes, that way we could even show that for every entry.
We also could pass a description to all instances, that could also be picked up by the docs generator and show that.

In my opinion its a bad idea to let the user search through the code for the desired name / id.

Concider this a draft with much room for improvement.

Regarding outdated names

We should print a warning that an outdated name is used and directly show the new name in that message.
That would allow the user to change to the right name very quickly.

I'm also not sure if we should keep that burden of keeping old names at all. Maybe we shoud just raise an Exception if the name is not found.

For me the next release should also bump to the next minor version (0.4.0 I guess) to indicate that this might contain breaking changes.

On the other hand I like the idea of non breaking stuff, but it makes it harder to keep the library slim and easy to understand.

Isn't it enough to have a good changelog which indicates breaking changes!?
To me that feels better than haveing a fairly complicated magic solution under the hood 🤔

[gerw]

We should print a warning that an outdated name is used and directly show the new name in that message. That would allow the user to change to the right name very quickly.

I'm also not sure if we should keep that burden of keeping old names at all. Maybe we shoud just raise an Exception if the name is not found.

But if we drop the old names, how could the user be informed of the new name?

[Bouni]

Raise an exception maybe!?
That would than need to be caught in the HA integration, but not a big deal to me ...

[Bouni]

The more I think about it, maybe we should disencourage or drop the use of the name all together an only allow the ID 🤔
The ID will never changes and we can change the name if the function becomes clear in the future without breaking anything.

Combined with the auto generated docs its easy for a user to look up the desired ID.