-
Notifications
You must be signed in to change notification settings - Fork 2
Denon AVR get_audio_format from webAPI #196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for retrieving the current audio format from Denon AVR receivers via their web API. The implementation adds a new get_audio_format() method across the abstraction layers.
Key changes:
- Added
get_audio_format()abstract method to the base AudioAmplifier class - Implemented the method in DenonAVRController using direct HTTPS requests to the Denon web API
- Added test coverage for the new functionality
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| framework/core/audioAmplifier/base.py | Added abstract method definition for get_audio_format() |
| framework/core/audioAmplifier/denon_controller.py | Implemented get_audio_format() using web API requests and XML parsing |
| framework/core/audioAmplifierController.py | Added wrapper method for get_audio_format() |
| tests/audioAmp_test.py | Added test case for audio format retrieval |
Comments suppressed due to low confidence (1)
tests/audioAmp_test.py:92
- Except block directly handles BaseException.
except:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| def __init__(self, host: str): | ||
| self.receiver = DenonAVR(host) | ||
| self.url = f"https://{host}:10443/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't hardcode the port. People could use some strange setup with port forwarding that would change the port.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have updated to pass as parameter
| } | ||
|
|
||
| def get_audio_format(self): | ||
| response = requests.get(f'{self.url}ajax/general/get_config?type=12', verify=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the only way to get this information? Can't it be done using the denonavr library we're using for everything else in the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried all the available APIs. Could not find the current input format after multiple tries with the denonavr package. It was coming in web browser though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
framework/core/audioAmplifier/denon_controller.py:84
- Mixing implicit and explicit returns may indicate an error, as implicit returns always return None.
def get_audio_format(self):
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def get_audio_format(self): | ||
| try: | ||
| # The 'type=12' query parameter requests the configuration from the Denon AVR. | ||
| response = requests.get(f'{self.url}ajax/general/get_config?type=12', verify=False, timeout=5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we please have a docstring/comment explaining why we can't use the denonavr lib. that's used for everything else in the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added.
| def get_audio_format(self): | ||
| try: | ||
| # The 'type=12' query parameter requests the configuration from the Denon AVR. | ||
| response = requests.get(f'{self.url}ajax/general/get_config?type=12', verify=False, timeout=5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is verify=false necessary? if so, then can we please explain why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
|
|
||
| def get_audio_format(self): | ||
| try: | ||
| # The 'type=12' query parameter requests the configuration from the Denon AVR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is type=12 and what does it correspond to in the denon api?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could not find the corresponding in Denon lib. type=12 in URL returns an xml of configurations from AVR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I have read the CLA Document and I hereby sign the CLA |
Added support for getting audio format from webAPI.