Closed gerw closed 5 months ago
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)?
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).
Definitely, we also need
calculations
. Concerningvisibilities
, 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.
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".
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.
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.
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.
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
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.
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.
https://github.com/Guzz-T/python-luxtronik/tree/issue/132/revert-api https://github.com/Guzz-T/python-luxtronik/pull/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.
@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?
Do we really need this level of abstraction? Yes, it seems to be nice from a design point of view. But it blows up the code and I do not think that it improves the maintainability. I also think that (besides the socket connection) there will be no other way of obtaining data from the heat pump in the future.
I like the simplicity and the API of 0.3.14
. From my point of view, the only circumstance where the current API is better, is if you want to keep a history of the parameters. With the new API, one can use
l = []
while True:
p = lux.read_parameters()
l.push_back(p)
whereas the old API needs
l = []
while True:
lux.read_parameters()
l.push_back(copy(lux.parameters))
@gerw you are right. It's quite a lot of code for little functionality. I have updated the branch with a new proposal.
That would be a compromise of compatibility, simplicity and a clear separation of the interface from the data.
class LuxtronikData:
...
class LuxtronikInterface:
...
class Luxtronik(LuxtronikData, LuxtronikInterface)
@Bouni: I haven't completely checked it yet, but in theory it should be fully backwards compatible.
Yes, this looks nice. I have a few comments:
Should Luxtronik
really be inherited from LuxtronikInterface
? From my perspective, it would be more logical, if it is a member, i.e., self.interface = LuxtronikInterface(...)
, or rather self._interface
?
That Luxtronik
is inherited from LuxtronikData
seems to be ok, since we can call luxtronik.some_high_level_function()
without additional gluing code.
In the old API,
lux = Luxtronik(ip, port)
p = lux.parameters
...
lux.read()
# Use "p" to access the parameters, or just:
assert p == lux.parameters
would work, but it fails with your code. How about something like
class LuxtronikInterface:
def read(self, parameters = Parameters()):
[...]
return parameters
class Luxtronik:
def read(self):
return self.interface.read(self.parameters)
?
Although I suggested otherwise, I am now wondering if LuxtronikInterfaceBase
would be a good idea. We could implement some dummy interface(s) which would just present some data to add more test routines (and get more code coverage). In LuxtronikInterfaceBase
we could add the methods _with_lock_and_connect
, read
, read_*
, write
, _read
, _read_*
, _write
, since these are pretty generic. Child classes have to implement _connect
, _disconnect
, _send_ints
, _read_int
and _read_char
.
Typing seems to be nice, but it should not be introduced while working on these changes. This should go to a different PR.
Opened pull-request #163
@gerw:
LuxtronikInterface
is now a member of Luxtronik
p == lux.parameters
? To compare the data __eq__()
would be required. Do you want to save the reference to the object that should be automatically updated with the latest data? With the current implementation lux.read()
updates lux.parameters
but not p
.LuxtronikInterfaceBase
is not needed for this issue but could be added with another pull-request.Concerning p == lux.parameters
: One use case is that we save the reference to lux.parameters
at some place in our code. Then, we could call lux.read()
somewhere else (e.g. in a separate thread) every minute to update the parameters, which can still be accessed by p
.
[In my opinion, it is also not necessary to create a new Parameter
object for each reading, but the old one can be recycled.]
Now you answered too quickly. I would have revised the comment again.
Please have a look again. Alternatively DataVector
itself could provide a update(self, other_data_vector)
.
With
p1 = lux.read_parameters()
p2 = lux.parameters
lux.read()
read()
would update p2
with the new values but p1
are still the old ones.
Let's continue the discussion in the PR.
Please note that even after #163, this code is not a 1-to-1 replacement for the previous version 0.3.14. I still get a lot of errors when starting homeassistant.
Examples:
2024-02-20 21:38:38.291 WARNING (MainThread) [Luxtronik.Calculations] entry 'ID_WEB_SoftStand' not found
2024-02-20 21:38:38.304 WARNING (MainThread) [Luxtronik.Calculations] entry 'Unknown_Calculation_239' not found
AttributeError: 'PoolMode' object has no attribute 'measurement_type'
File "/config/custom_components/luxtronik/coordinator.py", line 322, in firmware_version_minor
return int(ver.split(".")[1])
IndexError: list index out of range
ID_WEB_SoftStand
is no longer there, should be replaced by a high-level-function; this also causes the IndexErrorVBO_Temp_Spread_Soll
measurement_type
is now datatype_unit
?Changes seem logical. However, they must be implemented by the user (BenPru/luxtronik). This means that no direct exchange is possible.
One of the changes of pull request #86 was to remove
parameters
,calculations
andvisibilities
as members ofLuxtronik
. This change was not needed to arrive at the goal of #85 to support multiple instances ofLuxtronik
(this was solved by making the parameters instance members ofParameters
).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 ofLuxtronik
. Otherwise, I do not see an easy way for providing functions likeget_firmware_version
toLuxtronik
.