Issue-85: support multiple object instances

[Guzz-T]

fixes #85

Branch tested with home-assistant 2022.9.7 and a master/slave AIT-heatpump setup. Therefore i added a second renamed "luxtronik" integration (see https://github.com/Guzz-T/luxtronik/tree/try/second-integration-for-slave-heatpump).

Pull-Request should not contain any breaking changes. The diff in calculations (and ...) seems pretty hard, but I just had to change the indentation (= use 'hide withspaces').

[Bouni]

@Guzz-T Looks good on the first glance. I need to test this very well as you probably understand as this is quite a big change. So give me and the others some time to do that.

[Bouni]

@Guzz-T I finally found the time to change my setup so that it uses your PR. I'll let this run for a week or so in order to see if I run into problems. So far it looks good, thanks for your efforts!

[Bouni]

@Guzz-T Please rebase your PR, the checks should succeed after that!

[Guzz-T]

Done. (Github added a merge instead of a rebase :( ).

[Bouni]

Done. (Github added a merge instead of a rebase :( ).

I'm no expert in that matter either, so don't worry about it 😅

[Guzz-T]

Now the linter should finally be satisfied

[Bouni]

@kbabioch @BenPru This works for me quite well. Any opinions about it?

[kbabioch]

Seems like a massive change / break to the API. If it doesn't break anything and was tested by you, it's fine with me, although I don't fully understand why the change to the read functions is needed.

[Bouni]

@Guzz-T Can you give a clarification for @kbabioch question?

... although I don't fully understand why the change to the read functions is needed.

[Guzz-T]

Sure. Let me digress a little.

The issue can be divided into two aspects:

"The luxtronik read/write interface owns (second aspect) the read data as class variables (first aspect)"

First aspect:

Problem: Due to the use of class variables it is not possible to use two instances of the interface at the same time. When instance A is updated, the data for instance B is also overwritten.

Solution: The easiest way would be to use only normal variables instead of class variables. No big deal.

The implementation I prefer and in my opinion better would be that the interface only returns the read data instead of storing it. And so we come to the ...

Second aspect:

There is probably more room for discussion about this part. In terms of object-oriented programming, the read data does not belong in a read/write interface. For this, the (class) variables have to be removed and the read/write functions have to be adjusted accordingly (return value for read, parameter for write). But to maintain this change completely would break the API. So i prepared everything so far for an intermediate version, so that users still have time to switch to the new implementation while the old one is still supported.

Currently the interface:

• stores and returns the read data
• offers some read properties for the read data
• got an optional parameter for the write function for the data to be written

To come back to your question: The changes in the read functions are some preparations for some API-breaking-changes where only the read data are returned.

To-Do's after the next release:

• Remove the (class) variables
• Remove the related properties
• Make the paramter for the write function mandatory
• (Split the data-dictionary from the write-queue)

[kbabioch]

Okay, thank you for the detailed explanation. Makes sense to me.

On the other hand I'm wondering whether we shouldn't just introduce those other breaking changes as well with the next release instead of releasing "intermediate" versions that break some aspects, but not all, with more yet to come.

[Guzz-T]

That would be my individual release strategy.

If you don't mind that this pull request already breaks the API, I can adapt the code accordingly.

[Bouni]

@kbabioch @Guzz-T In my opinion we should go for the breaking changes directly. We will not release a new version before all (most) of the open issues are done anyway. An then we'll release a major version so that everybody should be aware that that release contains breaking changes.

[kbabioch]

Seems like we all agree then. Then let's break some more of the API. @Guzz-T will you create follow-up commits here in this pull request, or will there be another one?

[Guzz-T]

I will add them here.

[kbabioch]

@Guzz-T Thank you for all of your effort, very good code with code quality. Is this pull request considered to be done, or should we continue with reviews?

[Guzz-T]

All changes have been made. The rest is up to you. Maybe we should wait for #94 and then rebase this branch.

Update: As I just noticed, the README.md is now outdated. I will fix it later.

[Bouni]

@Guzz-T Just merged #94, so you can rebase. After tat I'll merge this one!

[Guzz-T]

1. Rebased to squash some commits. No code changes
2. Rebased branch to the top of 'main'. No code changes
3. Edit some commit messages. Again no code changes :) -> branch is ready to merge

[gerw]

I am a bit late to the party, but I don't like one of the changes introduced in the PR. It is good, that parameters is no longer a class variable of Parameters, but a (private) member variable. However, now a new class instance is created for each reading of the heat pump, and this involves copying a large dictionary with 1000+ members.

For me, an instance of Luxtronik is not only a "read/write interface" (as mentioned by Guzz-T), but a "virtual copy" of my real heat pump. In particular, each instance can connect only to one heat pump (since _host is a member variable). Thus, it is reasonable, that the instances caches the last reading of the heat pump. (Maybe it could/should also save the last read time?) Is there any use case, where this (old) behaviour would have any bad impact?

[Guzz-T]

I don't think there are advantages to store the read data inside the class instance:

• the "virtual copy" is always outdated
• It is not possible to save several data sets

Adding a timestamp shouldn't be a problem.

The time required to instantiate a class should be negligible compared to the reading time.

[kbabioch]

Since this is an already merged PR: Could we discuss any potential changes (or thing we don't like) in separate issues? For instance, if you want to have the feature "timestamp for last reading" let's create an issue for that and discuss it over there. This PR has been merged 2-3 months ago, and for now it's set.