search

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

fix(datatypes): handle None if HP does not return a value #97

Closed wilriker closed 1 year ago

wilriker commented 1 year ago

In case the firmware does not support a given parameter listet in any of the know calculations (at least that's where the issue happened for me) it will return None and plugging that into any of the various mathematical calculations inside datatypes.py leads to exceptions. This PR handles them gracefully.

kbabioch commented 1 year ago

Sorry for ignoring this for so long. Let's get going here.

Changes to me look fine to me.

What I would like to see, though, are unit tests to make sure that this will not be forgotten for any new datatypes, etc.

@wilriker Would you be willing to work on this, or should I give it a go?

@Bouni Any feedback regarding this PR?

wilriker commented 1 year ago

Hi @kbabioch!

I just pushed a set of test cases checking for None. Is that sufficient?

Bouni commented 1 year ago

LGTM

kbabioch commented 1 year ago

Thank your for the test cases. Changes itself look fine, but I'm still trying to understand the root cause of the issue here. So in your case you were trying to access parameters that you're firmware doesn't know about.

What is returned as raw value in this case? None is a Python-specific value, but what is actually being returned by the Luxtronik controller?

gerw commented 1 year ago

Just a speculation: In https://github.com/Bouni/python-luxtronik/blob/16a5f6f91891a7ade1824e51f241c797aff21cef/luxtronik/datatypes.py#L16 the .raw is set to None. If the heat pump only returns 500 parameters (indicated by length), then the raw values of all the other parameters is still None.

kbabioch commented 1 year ago

the .raw is set to None. If the heat pump only returns 500 parameters (indicated by length), then the raw values of all the other parameters is still None.

That might be a good explanation. Is this something we should handle in the code, or if this is expected behaviour? Where have you seen / derived the 500 from?

wilriker commented 1 year ago

Since I have upgraded my heatpump firmware meanwhile I can no longer reproduce the initial problem. At that time I was running a 1.69 version (that still had the old Java Applet).

kbabioch commented 1 year ago

There are some Pythonic errors that need to be addressed:

tests/test_datatypes.py:149:51: E711 [] Comparison to None should be cond is None tests/test_datatypes.py:312:50: E711 [] Comparison to None should be cond is None tests/test_datatypes.py:337:52: E711 [] Comparison to None should be cond is None tests/test_datatypes.py:362:51: E711 [] Comparison to None should be cond is None tests/test_datatypes.py:432:50: E711 [] Comparison to None should be cond is None tests/test_datatypes.py:457:51: E711 [] Comparison to None should be cond is None tests/test_datatypes.py:482:49: E711 [] Comparison to None should be cond is None Found 7 errors. [] 7 potentially fixable with the --fix option. Error: Process completed with exit code 1.

At least I don't see any harm in merging this, so let's go forward with it.

wilriker commented 1 year ago

Could you also add this for the Hours2 datatype

Done

gerw commented 1 year ago

the .raw is set to None. If the heat pump only returns 500 parameters (indicated by length), then the raw values of all the other parameters is still None.

That might be a good explanation. Is this something we should handle in the code, or if this is expected behaviour? Where have you seen / derived the 500 from?

Here, 500 just was a number smaller than the number of current parameters.