VoIP: Add support for announce #135291
VoIP: Add support for announce #135291
Conversation
|
Hey there @balloob, @synesthesiam, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
We will need to add some way for the HassVoipDatagramProtocol to pick an appropriate protocol factory for the different use cases: HA receiving a call, HA calling out for an announcement, and HA calling out as part of the start_conversation service. I imagine at some point we will want to account for what happens when one of those services, announce or start_conversation, is called while there is an active call with the target VOIP device. Currently the code assumes there is not an active call and will always start by calling the phone. |
| ), | ||
| ): str, | ||
| vol.Required( | ||
| CONF_SIP_HOST, |
There was a problem hiding this comment.
Why do we want to ask the user these values?
There was a problem hiding this comment.
Is this to get the IP address of the HA box? We should instead import async_get_source_ip from homeassistant.components.network
There was a problem hiding this comment.
Is that guaranteed to be the external IP for the HA box? I think there would still be value in allowing the host sent in the outgoing invite message to be configurable, but using async_get_source_ip as a default value would make sense. Depending on SIP server configurations a specific user might be required in the INVITE From header, so even though that configuration isn't officially supported, it is helpful for me. If we make those optional with sane defaults is that still a workable solution?
There was a problem hiding this comment.
We don't want them in the main configuration flow, as users will be confused (we want to ask as little information as possible when setting up an integration).
Can you try if it works for you with async_get_source_ip ?
There was a problem hiding this comment.
I moved user and host to an "advanced options" page and defaulted host to the value of async_get_source_ip, I hope that is an acceptable solution?
There was a problem hiding this comment.
No, let's stick with just calling async_get_source_ip when we need to do an announcement. That method exists to get the right IP address for exactly this reason:
@bind_hass
async def async_get_source_ip(
hass: HomeAssistant, target_ip: str | UndefinedType = UNDEFINED
) -> str:
"""Get the source ip for a target ip."""Adding this option puts an extra burden on the user having a bunch of IP addresses configured hidden inside config entry configurations.
| voip_id = ( | ||
| call_info.caller_endpoint.uri | ||
| if call_info.caller_endpoint | ||
| else call_info.caller_ip |
There was a problem hiding this comment.
Can we do an auto-migration here? So every incoming device, we check if it exists under the old ID, and if so, we migrate it to the new endpoint uri.
There was a problem hiding this comment.
I tried to update it to do an auto-migration, but it would be difficult for me to test that since I already have new devices for all of my phones.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
We should also make sure that we start with speaking the announcement message. Once we did that, Home Assistant should hang up. Users should use |
jaminh
force-pushed
the
voip-assist-sat-announce
branch
from
430dda1 to
a4b7063
Compare
|
jaminh
force-pushed
the
voip-assist-sat-announce
branch
from
293d6b6 to
8216fd1
Compare
This PR is already pretty unwieldy as it is, I figure we can get this stable and add start_conversation support as a separate PR. |
It should be implemented, line 134 homeassistant/components/voip/voip.py |
|
It's not explicitely mentioned, but this PR requires home-assistant-libs/voip-utils#25 to be merged and released right? |
Yes, is there a way to specify an unreleased version in the manifest? Edit: I think I found it, https://developers.home-assistant.io/docs/creating_integration_manifest/#custom-requirements-during-development--testing |
Currently when making an announcement the phone will be called and will go into the regular assist pipeline when the call is answered.
The create rtp server method is in voip_utils
Made a queue of pipeline operations that can be added to in case the announcement service is called during an ongoing call.
Improve task queue handling by using put_nowait for adding tasks to avoid the need for creating a task for adding items. The task queue should also be cleared when the call ends. If the call needs to end, either because it is the end of an announcement or there was an unrecoverable error, HA should initiate a hang up in an attempt to cleanly end the call.
jaminh
force-pushed
the
voip-assist-sat-announce
branch
from
6796078 to
4897501
Compare
|
The pre-commit checks complain if I try to set the voip_utils requirement to point to my branch, so I left that as is. I am still having an issue where when I hang up after doing the regular voice command pipeline stuff the TimeoutError isn't being caught in the _run_pipeline method which means the call isn't hung up. This means if I try to then play an announcement after that it thinks the previous call is ongoing and tries to play the announcement on that non-existent call. I still haven't figured out a way to get that to work such that it hangs up after I hang up my phone without it randomly hanging up in the middle of processing commands. |
|
Yeah we need to do a VoIP utils release for it to be mergeable. |
|
Do you have a stacktrace of where timeout is raised ? |
Sorry I have been trying a lot of things to debug this issue and I'm having trouble reproducing it now and lost the stacktrace from my logs. From what I remember it seemed like the timeout was coming from the with timeout block in the stt_stream method/coroutine. There seem to be some kind of race conditions going on, I seem to recall there being instances of CancelledError being in the stack trace as well. It seems like the way it is now if I hang up the phone depending on when the TimeoutError is thrown from the stt_stream function it doesn't get caught in the _run_pipeline method in the voip assist_satellite class, which means the call doesn't get hung up on the HA side. I have a suspicion that there is something that is sometimes cancelling the parent task of the running stt_stream instance, and if that happens while that is within the timeout block it is translating that into a TimeoutError. If that is the case that would explain why I would sometimes have the call hangup on me in the middle of processing. In that case I think we could solve the issue by finding a way to differentiate between the CancelledError and an actual TimeoutError, and only hangup in the case of an actual Timeout. Also if the stt_stream isn't running for the entire duration of the call then if the phone hangs up outside of the time stt_stream is running there isn't a way for HA to detect that the call was ended, which is another situation I think I have experienced. Long story short it seems like there are still issues around detecting when a phone hangs up and I'm still trying to wrap my head around asyncio. |
|
Investigating the issue more, I added an except clause in the _run_pipeline method and while trying to send a command I heard the tone after my command that would indicate that it heard the command successfully, but it then hung up immediately after. It logged out the following exception from the except clause I added: So it seems like something is cancelling the task, but I haven't been able to see what might be doing it and why. |
|
I was able to increase the reliability of the pipeline by retrying the event read operation if the task is cancelled in the wyoming stt class. I still occasionally get CancelledErrors in other locations though, for example: I initially thought it had to do with the stt_stream timeout but I think that is likely a red herring based on putting try/excepts in that function for CancelledError. Seems like there is some kind of race condition going on and when I was changing the stt timeout value it may have changed at what point in the process the cancel was happening which made it appeared to fix the issue but it wasn't really having the direct impact I thought it was. Maybe we just need to make pretty much the whole assist pipeline more tolerant of task cancellation? |
|
Yeah, we should always be able to expect task cancellation. It's how asyncio works: any task can be cancelled by the code that spawned it. We are doing this actively inside |
|
When either happens, the exception is raised wherevever the task is currently awaiting. Fun fact: you can catch |
If CancelledError is caught, wait a little bit longer to see if the pipeline task finishes, then restart the pipeline.
|
OK, I updated it to catch CancelledError, wait a little bit longer to finish the accept pipeline task, and then restart the pipeline. After making that change I haven't actually been able to reproduce the CancelledError, but it is running more reliably so in that sense mission accomplished I guess? I'll continue testing on my end, it would be nice to get some feedback from other people's testing to make sure I didn't break things for everyone else though. |
|
Please rebase after this is merged: #136648 |
|
@jaminh I'll take the PR from here over the finish line. Thanks for all of your efforts 🙂 |
Breaking change
Proposed change
This adds the "Announce" service to the VOIP Assist Satellite component. Currently when announce is called HA will call the phone and go into the standard assist pipeline when the call is answered.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: