kbr / fritzconnection

Python-Tool to communicate with the AVM Fritz!Box by the TR-064 protocol and the AHA-HTTP-Interface
MIT License
303 stars 59 forks source link

Error when calling get_basic_device_stats for newly installed thermostat #215

Open jmozmoz opened 3 months ago

jmozmoz commented 3 months ago

When calling get_basic_device_stats() for a newly installed thermostat which does not have temperature data recorded for 24 h, the method throws an error:

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[10], line 3
      1 d = fh.get_homeautomation_devices()[0]
      2 for d in fh.get_homeautomation_devices():
----> 3     data = d.get_basic_device_stats()
      4     print(data['temperature']['datatime'], data['temperature']['count'])
      5     date = data['temperature']['datatime'] - timedelta(seconds=data['temperature']['grid'])*np.arange(data['temperature']['count']) #95, -1, -1)

File ~/heizung/lib/python3.9/site-packages/fritzconnection/lib/fritzhomeauto.py:374, in HomeAutomationDevice.get_basic_device_stats(self)
    348 """
    349 Returns a dictionary of device statistics. The content depends on
    350 the actors supported by a device. The keys can be:
   (...)
    371 properties are much(!) faster.
    372 """
    373 response = self.call_http("getbasicdevicestats")
--> 374 return self.extract_basicdevicestats_response(response)

File ~/heizung/lib/python3.9/site-packages/fritzconnection/lib/fritzhomeauto.py:398, in HomeAutomationDevice.extract_basicdevicestats_response(response)
    396             value = datetime.datetime.fromtimestamp(value)  # type: ignore
    397         content[key] = value
--> 398     content["data"] = list(map(int, stats.text.split(",")))  # type: ignore
    399     elements[element.tag] = content
    400 return elements

ValueError: invalid literal for int() with base 10: '-'

The reason is, that the returned XML does not contain numbers but "-":

d.call_http('getbasicdevicestats')
{'content-type': 'text/xml',
 'encoding': 'utf-8',
 'content': '<devicestats><temperature><stats count="96" grid="900" datatime="1711822481">190,195,195,190,185,190,195,195,195,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-,-</stats></temperature></devicestats>\n'}
kbr commented 3 months ago

Thank you for reporting the issue and the according pull request.

Beside this I'm not sure whether the change will be physically correct. To understand this it is important to know how the sensor works. Is it okay to change the count-value according to the changed number of values which will invalidate the grid-value? I would assume that the grid-value is constant and also the number of counts do not change, but a dash in the data represents a missing datapoint. If this is the case the missing datapoint can not be ignored. Using the original dash (or None) would result in a list of data with mixed types (delegating further exception handling elsewhere in the application based on the library). To keep same types an integer representing missing (not unknown!) data could be taken like -999, which is historical used for missing temperature records. Again it would be up to applications to handle this.

Also these changes must go to the test-code and -data and also to the documentation.

(It could also be an idea to get in contact with the thermostat-vendor to get first hand information about the protocol for missing data. So far I'm just guessing about the interpretation of missing data.)

jmozmoz commented 3 months ago

The current pull request is just a first attempt to get this to work. I think, an exception in fritzconnection itself should not happen. So I think it wont make sense to try to convert everything to integers and throw a ValueError or something else, if this does not work. Also, different types in the returned list probably are hard to handle. Therefore, I reduced the list so only integers are returned.

So I guess, if there is an "official" integer value for an invalid value, this should be used for the '-'.

kbr commented 3 months ago

I agree, the ValueError should get catched and the dash should get converted to something else. In my original test data from a sensor I have had just zeros and no dashes. A possible integer value for missing data could be -999 as by historical DWD weather data. However I would prefer None even if this results in a list of mixed types. This list can assumed as "raw data".

On top of this the library may get an additional method to provide a list based on the "raw data" with tuples of timestamps and values (which are integers and not None). This would be a datastructure that can get easily adapted for plotting or i.e. as input for pandas series. Optional it could also provide the missing data as None or another user defined default value.