search

ol-iver / denonavr

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

Add Audyssey settings #158

Closed MarBra closed 3 years ago

JPHutchins commented 4 years ago

Awesome, nice work! I will clone and test. If we're going to add this kind of functionality I might like to add a few more entries as well.

Can we find someone to test this with a "pre X" series receiver?

I like the way that you set this as a class where the class name is the XML name. So if I were to add the following I would do something like this. The XML for these is at the bottom.

class SurroundParameter:
    """Surround Sound Settings."""
    ...
    def set_dyncomp():
        ...
    def set_lfe():
        ...
    ...

Which makes me realize that it would be nice to have a base class AppCommand0300 or XmlAppCommand where subclass defines "name". So where you do ET.SubElement(cmd, "name").text = "SetAudyssey" it would just be self.name of the subclass Audyssey. What do you think about generalizing it this way? And I 100% support new files, maybe even folders, denonavr.py is huge! Then again maybe we leave everything in denonavr to scare Java people...

Dolby alternative to dynamic vol

DCO setting can take 0: off, 1: low, 2:med, 3: high
This setting will only work for Dolby tracks that contain
the dynamic range metadata but the effect is much better 
than traditional dynamic range compression.
0300
<?xml version="1.0" encoding="utf-8"?>
<tx>
  <cmd id="3">
    <name>SetSurroundParameter</name>
    <list>
      <param name="dyncomp">3</param>
    </list>
  </cmd>
</tx>

Set LFE

Low Frequency Effect takes settings from 0 (normal) down 
to -10 (-10dB).  This is a useful addition for night mode
macros.
0300
<?xml version="1.0" encoding="utf-8"?>
<tx>
  <cmd id="3">
    <name>SetSurroundParameter</name>
    <list>
      <param name="lfe">-10</param>
    </list>
  </cmd>
</tx>
cajuncoding commented 4 years ago

FYI, just chiming in support of this functionality! You guys are rock stars!

It will be great to automate these things between my normal movie mode and night movie mode, so I don't keep forgetting that LC is cranked up an hour into a great movie after wondering why the sound kinda sucks :-)

But my wife is muuuuuch happier when it's on during my normal nightly watching time because she is already sleeping!

MarBra commented 4 years ago

Can we find someone to test this with a "pre X" series receiver?

That would be great. I've got a AVR-X1600H.

Which makes me realize that it would be nice to have a base class AppCommand0300 or XmlAppCommand where subclass defines "name". So where you do ET.SubElement(cmd, "name").text = "SetAudyssey" it would just be self.name of the subclass Audyssey. What do you think about generalizing it this way? And I 100% support new files, maybe even folders, denonavr.py is huge! Then again maybe we leave everything in denonavr to scare Java people...

The idea of generalization sounds good to me. Maybe we can add this later, when more components are coming in. :)

JPHutchins commented 4 years ago

Great! I have AVR-X1500H so not much difference. I am working on generalizing it now so if it's not too much hassle I'd favor getting the groundwork agreed upon ahead of time.

First question: I see a total of 9 setters now. This seems inconsistent with much of the library. Should we consider converting everything to setters for consistency? It could be confusing to have setters as well as "set functions" for third-party integrations like Home Assistant.

Update: I see that setters were used as far back as 4 years ago but since then a lot of methods added without. So I'd agree with the setters for this PR and then maybe someday I can make the whole library consistent consistent while maintaining backwards compatibility.

JPHutchins commented 4 years ago

Fun problem: changes to state do not hold if the receiver is powered off even though it replies with "OK". To reproduce:

Works with power on:

import denonavr
d = denonavr.DenonAVR("IP ADDR")
d.power_on()
d.dynamic_volume = "Medium"
d.update()
d.dynamic_volume
d.dynamic_volume = "Off"
d.update()
d.dynamic_volume

But not with power off:

d.power_off()
d.dynamic_volume = "Medium"
d.dynamic_volume # Since "OK" was received this gets set to "Medium" - it assumes it worked!
d.update() # Update calls "GetAudyssey"
d.dynamic_volume # It is "Off"
d.power_on()
d.update()
d.dynamic_volume # Confirm that it didn't "queue" the change awaiting power

Trivial Solution

Don't allow changes to these states when power is off.

Automation

I think that many people interested in this, myself included, would like to automate the dynamic volume etc as a "night mode" feature that activates at 9PM for example and turns off again in the morning. This is simple enough in Home Assistant (right now I do it using Telnet commands) yet we would have to document a need to check for power at 9PM, turn on if not powered, change the setting, turn off again.

Nice To Have

What may be better is to implement a simple task queue for this library. Settings that are made when the unit is powered off are added to the queue and applied when the unit starts next. This is potentially error prone and outside the scope of this library.

JPHutchins commented 4 years ago

Generalization

For anyone that's interested, here is a draft PR with ideas for how to implement this while making it reasonably intuitive for folks to add their own XML style commands. It does include working Dolby Dynamic Compression and LFE setting 0dB -> -10dB.

https://github.com/MarBra/denonavr/pull/1/commits/c4d6c0e94331d063306b21ce425ee44467237fb9

To do

MarBra commented 4 years ago

First question: I see a total of 9 setters now. This seems inconsistent with much of the library.

Yes I agree. It would be good to have a consistent interface. Maybe we can have some thoughts from @scarface-4711

Fun problem: changes to state do not hold if the receiver is powered off even though it replies with "OK".

Yes this is indeed an issue. :D Do we handle this in general in the library?

@JPHutchins Thanks for all the input :)

@scarface-4711 Shall we put the contributions in one PR or shall we split them up?

ol-iver commented 4 years ago

Hey @MarBra, @JPHutchins, it took a while for me to check this. Home office is screwing up my work life balance 😅

First question: I see a total of 9 setters now. This seems inconsistent with much of the library.

Yes I agree. It would be good to have a consistent interface. Maybe we can have some thoughts from @scarface-4711

I started with the setters years ago, probably just to try how they are working. However, in my opinion they make sense in some cases to "protect" the attribute. Setting a input_func for example is more than just changing a attribute value, it means calling the receivers endpoint too.

Fun problem: changes to state do not hold if the receiver is powered off even though it replies with "OK".

Yes this is indeed an issue. :D Do we handle this in general in the library?

It seems to be an occasional problem. If I remember correctly some endpoints like setting an input function are throwing an error in case you provide an invalid function, others don't. But most of the endpoints return a HTTP 200 even in the error case. That's what complicates working with Denon/Marrantz receivers a lot...

@scarface-4711 Shall we put the contributions in one PR or shall we split them up?

Work is calling 😅 I think about this in the evening.

Btw. I own a Denon AVR-X 4100W which is pre-2016 and supports Audyssey and I'll test it in the evening.

ol-iver commented 3 years ago

This is running on my receiver too 😄 I'll merge it and add the missing setter later. I think there is really just volume missing. The other attributes you cannot really set from outside the class-

JPHutchins commented 3 years ago

Nice, thank you! I will make separate PR with the generalizations for further schema discussion.

On Tue, Nov 17, 2020, 12:03 AM Oliver notifications@github.com wrote:

Merged #158 https://github.com/scarface-4711/denonavr/pull/158 into master.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/scarface-4711/denonavr/pull/158#event-4004085237, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIESQLW5T3HLBEENJ6YUTWTSQIU3PANCNFSM4SYX6DZQ .