ol-iver / denonavr

Automation Library for Denon AVR receivers.
MIT License
176 stars 67 forks source link

Going async #184

Closed ol-iver closed 3 years ago

ol-iver commented 3 years ago

@JPHutchins, @starkillerOG , @MarBra: Happy new year! I hope you are good.

I spend some time over christmas to create an async version of denonavr. In the end it turns out that it resembles like a redesign. Maybe you would like to have a look at it πŸ˜„

The logic of the functions (sound mode, tone control, inputs, volume) is more or less the same. I just changed some parts where it really looked too complicated. But they are restructured now to break the huge DenonAVR class into pieces.

All functions even the DenonAVR class itself is now a subclass of denonavr.DenonAVRFoundation. This class offers some base functions for AppCommand and the StatusXml interfaces, which are very common amongst all functions. AppCommand queries are defined in a structured way in appcomand.AppCommands class. For StatusXml you can define own XPath queries, to get values from XML.

Information which are relevant for all functions are shared in a common instance of DenonAVRDeviceInfo which is part of DenonAVRFoundation. This class offers a API instance with caching functions too. When the global async_update method of DenonAVR class is called. All queries to the receiver run with one cache_id. In case an endpoint is called multiple times with the same parameters it is taken from cache. The good thing here is, that the functions do not need to know which endpoints already have been called. This is all handled by the API class. For AppCommand.xml and AppCommand0300.xml calls there is a shared tag stock where all functions are able to add tag relevant for the global update, that we can get all data with one single query. Some queries like power and mute status get receive responses with the same attribute names. That's why the API class adds attributes to the <cmd> tag of the response which are evaluated by the general query function I mentioned before.

I used httpx for the async HTTP queries, because I like its interface. All major classes now use attrs to improve type safety. Additionally its converters and custom setter functions are quite useful to implement shared query functions, but still support type conversions we need them quite a few times.

Initially I started to build a library with native sync and async support. But it turned out quite early, that this would mean lots of boilerplate code. That's why the library is now async only. In order to not break home-assistant when someone manually uses the future async version in a current sync HA installation, there is a little decorator function which creates a sync function out of an async function. Basically, it creates a new async event loop on each call and closes it in the end. That's not very efficient, but probably better than breaking stuff. Denonavr now needs python 3.6 minimum. I kept compatibility to python 3.6 even though it complicated some things because it is the default python version of Ubuntu 18.04 LTS which is still in use by many people. For sake of better readability all interfaces and attrs data types are annotated.

Your feedback is highly appreciated πŸ˜ƒ

ol-iver commented 3 years ago

Two more things.

Audyssey is still not included to the global update async_update of the receiver class. On my receiver the query to AppCommand0300.xml takes more than 10 seconds! when Spotify is running. Maybe you can test it on your receiver async_update_audyssey

Extensions for other AppCommand related functions should be done by following the examples of soundmode, tone control and audyssey implementations and adding additional queries to appcommand.Appcommands class

MarBra commented 3 years ago

πŸ‘πŸ˜€ I will give it a try this weekend. @scarface-4711 Is the whole async_update_audyssey taking 10 seconds or do you see any certain request which is causing the issue?

MarBra commented 3 years ago

πŸ‘πŸ˜€ I will give it a try this weekend. @scarface-4711 Is the whole async_update_audyssey taking 10 seconds or do you see any certain request which is causing the issue?

Works for me :) I also couldn't find an issue with update audyssey method.

ol-iver commented 3 years ago

πŸ‘πŸ˜€ I will give it a try this weekend. @scarface-4711 Is the whole async_update_audyssey taking 10 seconds or do you see any certain request which is causing the issue?

It is the GetAudysseyInfo query which takes that long. I tested it with Postman. Thatβ€˜s probably an issue of my receiver and maybe others too. How long does that query take at your receiver?

MarBra commented 3 years ago

πŸ‘πŸ˜€ I will give it a try this weekend. @scarface-4711 Is the whole async_update_audyssey taking 10 seconds or do you see any certain request which is causing the issue?

It is the GetAudysseyInfo query which takes that long. I tested it with Postman. Thatβ€˜s probably an issue of my receiver and maybe others too. How long does that query take at your receiver?

Just checked with my receiver and it takes 76ms. I just found one issue while activating DEBUG logging:

>>> import logging
>>> logging.basicConfig(level=logging.DEBUG)
>>> receiver.update_audyssey()
DEBUG:asyncio:Using selector: KqueueSelector
DEBUG:denonavr.api:Content for /goform/AppCommand0300.xml endpoint: b'<?xml version=\'1.0\' encoding=\'utf-8\'?>\n<tx><cmd id="3"><name>GetAudyssey</name><list><param name="dynamiceq" /><param name="reflevoffset" /><param name="dynamicvol" /><param name="multeq" /></list></cmd></tx>'
DEBUG:httpx._client:HTTP Request: POST http://192.168.1.6:8080/goform/AppCommand0300.xml "HTTP/1.0 200 OK"
DEBUG:denonavr.foundation:Failed updating attribute _multeq for zone Main
Traceback (most recent call last):
  File "/Users/m/repos/denonavr.org/denonavr/foundation.py", line 491, in async_update_attrs_appcommand
    set_value = xml.find(search_strings[i]).text
AttributeError: 'NoneType' object has no attribute 'text'
DEBUG:denonavr.foundation:Failed updating attribute _multeq_control for zone Main
Traceback (most recent call last):
  File "/Users/m/repos/denonavr.org/denonavr/foundation.py", line 489, in async_update_attrs_appcommand
    search_strings[i]).get(pattern.get_xml_attribute)
AttributeError: 'NoneType' object has no attribute 'get'
DEBUG:denonavr.foundation:Changing variable _dynamiceq to value 1
DEBUG:denonavr.foundation:Changing variable _dynamiceq_control to value 2
DEBUG:denonavr.foundation:Changing variable _reflevoffset to value 0
DEBUG:denonavr.foundation:Changing variable _reflevoffset_control to value 2
DEBUG:denonavr.foundation:Changing variable _dynamicvol to value 0
DEBUG:denonavr.foundation:Changing variable _dynamicvol_control to value 2
ol-iver commented 3 years ago

πŸ‘πŸ˜€ I will give it a try this weekend. @scarface-4711 Is the whole async_update_audyssey taking 10 seconds or do you see any certain request which is causing the issue?

It is the GetAudysseyInfo query which takes that long. I tested it with Postman. Thatβ€˜s probably an issue of my receiver and maybe others too. How long does that query take at your receiver?

Just checked with my receiver and it takes 76ms. I just found one issue while activating DEBUG logging:

>>> import logging
>>> logging.basicConfig(level=logging.DEBUG)
>>> receiver.update_audyssey()
DEBUG:asyncio:Using selector: KqueueSelector
DEBUG:denonavr.api:Content for /goform/AppCommand0300.xml endpoint: b'<?xml version=\'1.0\' encoding=\'utf-8\'?>\n<tx><cmd id="3"><name>GetAudyssey</name><list><param name="dynamiceq" /><param name="reflevoffset" /><param name="dynamicvol" /><param name="multeq" /></list></cmd></tx>'
DEBUG:httpx._client:HTTP Request: POST http://192.168.1.6:8080/goform/AppCommand0300.xml "HTTP/1.0 200 OK"
DEBUG:denonavr.foundation:Failed updating attribute _multeq for zone Main
Traceback (most recent call last):
  File "/Users/m/repos/denonavr.org/denonavr/foundation.py", line 491, in async_update_attrs_appcommand
    set_value = xml.find(search_strings[i]).text
AttributeError: 'NoneType' object has no attribute 'text'
DEBUG:denonavr.foundation:Failed updating attribute _multeq_control for zone Main
Traceback (most recent call last):
  File "/Users/m/repos/denonavr.org/denonavr/foundation.py", line 489, in async_update_attrs_appcommand
    search_strings[i]).get(pattern.get_xml_attribute)
AttributeError: 'NoneType' object has no attribute 'get'
DEBUG:denonavr.foundation:Changing variable _dynamiceq to value 1
DEBUG:denonavr.foundation:Changing variable _dynamiceq_control to value 2
DEBUG:denonavr.foundation:Changing variable _reflevoffset to value 0
DEBUG:denonavr.foundation:Changing variable _reflevoffset_control to value 2
DEBUG:denonavr.foundation:Changing variable _dynamicvol to value 0
DEBUG:denonavr.foundation:Changing variable _dynamicvol_control to value 2

Good catch, thx πŸ˜„ there was a typo in multeq search pattern. Wow, your receiver is way faster than mine...just tested again 7.54 seconds...

ol-iver commented 3 years ago

There are two more changes I'd like to mention

  1. Update and setter methods now return None when running successfull. In case of errors an exception from here will be raised. True and False are not too useful in case you want to know the reason of a failed request
  2. There are much less error messages to avoid log spam. Instead the exceptions mentioned before are raised. Now its up to users like home assistant how much they would like to log.

Thanks for your feedback, I will give it a try and merge it into the main branch.

ol-iver commented 3 years ago

Oh yes another thing I like to mention.

The library is not updating attributes internally when sending a command. You have to run the async_update method again. The reason for this was that the receivers nearly always return HTTP 200 even if the command failed and I wanted to avoid the attributes switching back and forth in that case.

I should write all that stuff into a documentation file πŸ˜…

sbraz commented 3 years ago

It would be nice to update the README and replace this: d.input_func = "Tuner" with something like d.set_input_func("Tuner"). I would also have appreciated it if the release page mentioned the need to run update() after each command (and maybe other changes I'm not aware of because I don't use a lot of features).

filmcriticbeta commented 3 years ago

I have to concur with @sbraz regarding the need to update the README in this regard. If not for finding his comment above I would have submitted a new issue to report the input switching was currently non functional.