-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
add Audio API support for axis network speakers #521
base: master
Are you sure you want to change the base?
Conversation
This is great! I can't promise i can give you feedback within the next.two weeks (busy IRL) but I eagerly want to see this in the integration. Good job! |
I may fling some more code over the wall for the Audio Device Control API (for volume control), but no rush on any review! |
Added support for the Audio Device Control API. This can control the gain and mute status for output speakers. https://www.axis.com/vapix-library/subjects/t10100065/section/t10169359/display The API is a bit kludgy. The GetDevicesSettings returns a huge deeply-nested JSON structure. SetDevicesSetting expects that same structure, even if just tweaking the gain or mute parameters. One approach would be to Get the big structure, modify it, and then Set it, every time you want to change one of those parameters. Another approach, which I took, was to find the minimal JSON fields required for the request to be accepted and Set that. I opened a case with Axis support to see if they have additional guidance. |
…mitted audio is done playing. that means that any audio longer than the http timeout will raise a timeout exception. we can catch and dampen this timeout for the transmit.cgi call. however, we still do want to catch legitimate http timeouts, so we use httpx's finer grained readtimeout to abort waiting from the response and handle the timeout exception gracefully. this requires some plumbing of api-specific timeout definitions.
…eaking was causing a clipping noise at the beginning of the playback since the speaker was interpreting the wav header as audio data.
Hey! I should be able to start review this on wednesday |
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.
This is not a complete review, but as it takes a few days between possible times for me right now its better to get it published
) | ||
|
||
async def request( | ||
self, | ||
method: str, | ||
path: str, | ||
timeout: int | httpx.Timeout, # noqa: ASYNC109 |
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.
timeout: int | httpx.Timeout, # noqa: ASYNC109 | |
timeout: int | type(httpx.Timeout), |
Let's avoid noqa as much as possible. You can't use type?
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.
ASYNC109 is ruff thinking we're doing timeouts in async code:
https://docs.astral.sh/ruff/rules/async-function-with-timeout/
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.
Would it be enough with the int? Im planning on adding support for aiohttp so avoiding one more httpx reference would be appreciated :)
Co-authored-by: Robert Svensson <[email protected]>
stdout=asyncio.subprocess.PIPE, | ||
stderr=asyncio.subprocess.PIPE, | ||
) | ||
stdout, stderr = await process.communicate() |
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.
Should stderr be evaluated?
) | ||
|
||
async def request( | ||
self, | ||
method: str, | ||
path: str, | ||
timeout: int | httpx.Timeout, # noqa: ASYNC109 |
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.
Would it be enough with the int? Im planning on adding support for aiohttp so avoiding one more httpx reference would be appreciated :)
content_type = "application/json" | ||
error_codes = general_error_codes | ||
|
||
devices: Any |
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.
This should be properly typed
Once coming closer to the end remember test coverage is important as well |
@Kane610 Sorry for the delay. Unfortunately I think I have to abandon this PR. It turns out the Axis speakers are lacking in capabilities needed for our project. If it's at all helpful, I can ship you one, if you're interested in trying it for testing/maintenance purposes. |
What was it you wanted to achieve that the speaker couldnt manage? |
Limited decoding codec support and audio quality. Also, the Audio Edge Manager API is not sufficient in it's current state (GETs/read-only). Lastly, the behavior of the transmit.cgi endpoint is pretty stateless so makes the HA media player integration not as pleasant (eg. play state, pause, stop, etc). |
Ill keep it open i might try to complete this |
Nice. If you email me your address, I'll ship you a C1410 Mk II. firstname.lastname @ gmail |
Ok, I stepped out of HA-land [1] for a bit to add the Audio API support natively to the axis library.
This PR:
[1] home-assistant/core#130163