Bouni / python-luxtronik

python-luxtronik is a library that allow you to interact with a Luxtronik heatpump controller.
MIT License
37 stars 20 forks source link

issue-91: Return the raw value instead of "None" in the __str__ method of the data fields #92

Closed Guzz-T closed 1 year ago

Guzz-T commented 1 year ago

If the selection value is unknown, output the raw value instead of None to simplify the "reverse engineering".

fixes #91

Update: Summary of the changes:

kbabioch commented 1 year ago

Is this only relevant for the from_heatpump function, or should it also be applied to to_heatpump?

Guzz-T commented 1 year ago

In to heatpump value is a string, but an integer should be written to the heatpump. If we return the string, the interface may write the ASCII code of the first char? This could set the heatpump into an unexpected state. So we must not return the value there.

kbabioch commented 1 year ago

In to heatpump value is a string, but an integer should be written to the heatpump. If we return the string, the interface may write the ASCII code of the first char? This could set the heatpump into an unexpected state. So we must not return the value there.

Ok, that makes sense.

However, I'm wondering: How is a normal API consumer (e.g. the Home Assistant integration) supposed to deal with from_heatpump with this change in place? Right now you will receive None when there is no mapping available. So you could easily do something like:

if from_heatpump(...):

With your change, you now have to check if a value from the options has been returned or some other integer, etc. This makes it less usable for an API consumer.

If I understand you correctly the actual go is to improve debugging / reverse engineering. Shouldn't we use some logging in that case? Unfortunately it seems like no logger is available, but this shouldn't be too hard.

Guzz-T commented 1 year ago

I would wonder if homeassistant does anything other than print the raw value. To use the value in your example you have to write

If to_heatpump(...):
    Value = to_heatpump(...)

Better would be

Value = to_heatpump(...)
If Value:

But then you could use

If isinstance(Value, str):

to ensure that value is of type string

Another solution could be to implement a TryGet function, which outputs also a success flag.

Or

return str(value)

then the return value would always be of type string.

kbabioch commented 1 year ago

Then let's go with the proposed str(...) solution. Personally I'm still not convinced that this is the right way (i.e. changing the API for the sake of debugging), but I can understand why this might be needed.

In any case, probably this will break some test cases?

Guzz-T commented 1 year ago

I see you are not happy with these changes. I will try to solve this differently.

Guzz-T commented 1 year ago

Please checkout the new solution: The raw data (_raw) is now stored instead of the already converted value (value). The conversions are now processed as needed. This should increase the performance, as usually only a small part of the >1000 data fields is used. In addition, we can also output the raw value afterwards (property raw). One use case is the new string representation _str_ which returns the raw value when the converted value is None

Bouni commented 1 year ago

@Guzz-T Can you rebase this please, I've just merged your multiple heatpumps PR #86

Guzz-T commented 1 year ago

@Guzz-T Can you rebase this please, I've just merged your multiple heatpumps PR #86

Done