Parameters (and friends) should be members of Luxtronik

[gerw]

One of the changes of pull request #86 was to remove parameters, calculations and visibilities as members of Luxtronik. This change was not needed to arrive at the goal of #85 to support multiple instances of Luxtronik (this was solved by making the parameters instance members of Parameters).

In order to ease the implementation of a high-level interface for reading (see #119), I suggest to add back parameters as an instance member of Luxtronik. Otherwise, I do not see an easy way for providing functions like get_firmware_version to Luxtronik.

[kbabioch]

Are only parameters needed/wanted as part of Luxtronik, or also the other references to calculations and visibilities?

Also: Should we work on this right away for milestone, or only "later" when working on the high-level interface (https://github.com/Bouni/python-luxtronik/milestone/2)?

[gerw]

Definitely, we also need calculations. Concerning visibilities, I do not really know what these tell me.

In my opinion, it fits either milestone, but (if we do it), it should be done before the next release (since the current behavior breaks the API and this issue proposes to revert this).

[kbabioch]

Definitely, we also need calculations. Concerning visibilities, I do not really know what these tell me.

I would then argue that for the sake of consistency we should treat them equal, until we have a good reason to treat them differently. So let's keep a reference to all of those "data vectors".

In my opinion, it fits either milestone, but (if we do it), it should be done before the next release (since the current behavior breaks the API and this issue proposes to revert this).

Currently the milestones are not linked to releases, but that should be the goal. So let's add it to milestone 1, which is concerned about refactoring, etc.

[Guzz-T]

I'm still of the opinion that an (outdated) image of the data has nothing to do with the interface.

if you want to save the (actually negligible) time for creating the parameters objects, we could also offer an update() function, in which the read data is written into the passed object.

def __init__():
    self.parameters = Parameters()
    self.luxtronik = Luxtronik("192.168.1.1")

def read ():
    self.luxtronik.update(self.parameters)

...

A "higher-level-function" could looks like:

def get_hot_water_temperature(temperature):
    param_queue = ParamQueue()
    param_queue.set(ID_HOT_WATER_TEMPERATURE, temperature)
    data = self.write(param_queue)
    read_temperature = data.get(ID_HOT_WATER_TEMPERATURE)
    return read_temperature

Has anyone ever tried to read a single parameter instead of all together? This could be useful for "higher-level-functions".

[gerw]

I'm still of the opinion that an (outdated) image of the data has nothing to do with the interface.

In my opinion, the Luxtronik class is more than a read-write interface. Of course, the data might be outdated (if not re-read frequently), but this is the case for (almost) all the data on your computer ;)

A "higher-level-function" could looks like:

def get_hot_water_temperature(temperature):
    param_queue = ParamQueue()
    param_queue.set(ID_HOT_WATER_TEMPERATURE, temperature)
    data = self.write(param_queue)
    read_temperature = data.get(ID_HOT_WATER_TEMPERATURE)
    return read_temperature

But with such an approach, you have to read the parameters from the heat pump for each call of a "higher-level function"...

Has anyone ever tried to read a single parameter instead of all together? This could be useful for "higher-level-functions".

This is not possible (i.e., not supported by the heat pump interface). You can only read all parameters at once.

[Guzz-T]

But with such an approach, you have to read the parameters from the heat pump for each call of a "higher-level function"...

This would be the same if you refresh the cached data. Just returning the cached data instead of re-reading it again would be the same as:

params = luxtronik.get_parameters()
luxtronik.high_lvl_func_without_reading()
foo = params.get(ID)

No need to cache this inside the interface.

This is not possible (i.e., not supported by the heat pump interface). You can only read all parameters at once.

Not possible due to the Python interface or the Luxtronik control itself?

If data actually needs to be cached, I would recommend implementing another class like LuxtronikSequences that uses the Luxtronik interface.

[gerw]

But with such an approach, you have to read the parameters from the heat pump for each call of a "higher-level function"...

This would be the same if you refresh the cached data. Just returning the cached data instead of re-reading it again would be the same as:

params = luxtronik.get_parameters()
luxtronik.high_lvl_func_without_reading()
foo = params.get(ID)

What is achieved by the second line here? How should high_lvl_func_without_reading execute, when it does not have any data available.

This is not possible (i.e., not supported by the heat pump interface). You can only read all parameters at once.

Not possible due to the Python interface or the Luxtronik control itself?

This is not possible by the firmware of the heat pump.

[Guzz-T]

params = luxtronik.get_parameters()
luxtronik.high_lvl_func_without_reading()
foo = params.get(ID)

What is achieved by the second line here? How should high_lvl_func_without_reading execute, when it does not have any data available.

This is just to clarify that nothing is read back there. In principle, high-level functions with (SetHotWaterTemp(50)) and without parameters (ActivateHolidayMode()) would be possible

[gerw]

Just a quick reminder: If revert the API change, we should also call read() in __init__ again to load the current state of the heat pump.

[Guzz-T]

I still vote not to add the data to the interface.

We could implement something like this:

class LuxtronikInterface:
  def read(self): -> LuxtronikData
  def write(self, ...): -> LuxtronikData

@dataclass
class LuxtronikData:
  def __init__(self, params, visis, calcs):
    self.parameters = params
    self.visibilies = visis
    self.calculations = calcs
  
  def some_high_level_func(self):
    ...

class Luxtronik:
  def __init__(self, interface):
    self.Interface = interface
    self.Data = self.Interface.read()

  def __init__ (self, ip, port):
    self.Interface = LuxtronikInterface(ip, port)
    self.Data = self.Interface.read()

  def some_further_high_level_function(self):
    ...
    

(Did not checked the syntax)

Optional:

class Luxtronik(LuxtronikInterface): 

To provide the same interface as in the previous releases.

[Guzz-T]

https://github.com/Guzz-T/python-luxtronik/tree/issue/132/revert-api
Guzz-T#8

Here is a suggestion. Due to the base classes there is a little more code, but a cleaner separation of the interface from the data.
In principle I think that for high level functions it would be necessary to restore the old interface.

[Bouni]

@Guzz-T I just had a quick look at your "revert-api" proposal and so far this looks good to me.
Not breaking the existing API is a big bonus in my eyes, which your proposal fulfils, right?

I have not yet test your branch due to lack of time and I can't before wednesday.

@gerw and @kbabioch any thoughts on this?