-
Notifications
You must be signed in to change notification settings - Fork 6
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
VOIP Assist Satellite Announce support #25
VOIP Assist Satellite Announce support #25
Conversation
Updates to make assist satellite announcements work with the VOIP integration. Allow using the existing SIP transport for Home Assistant for both incoming and outgoing calls.
The main changes in this PR are moving the INVITE response and BYE message handling from the separate DatagramProtocol class to the original SipDatagramProtocol class. Also added a method for sending an outgoing call to the same SipDatagramProtocol class. Making outgoing calls requires keeping track of the local RTP port that will be used for each call. |
voip_utils/sip.py
Outdated
f"To: {to_header}", | ||
f"Call-ID: {call_id}", | ||
"CSeq: 50 ACK", | ||
"User-Agent: test-agent 1.0", |
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 we allow the consumer of this library to pass in a user agent ? Or should we put in voip-utils
?
voip_utils/sip.py
Outdated
f"To: {to_header}", | ||
f"Call-ID: {callid_header}", | ||
f"CSeq: {cseq_header}", | ||
"User-Agent: test-agent 1.0", |
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.
same as above: let's set it / allow setting it .
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.
Looks good! Left some comments. Mark it as ready when addressed 👍
local_rtp_port=local_rtp_port, | ||
) | ||
) | ||
elif method == "bye": |
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 this cleanup the call / call an abstract method to indicate the call is over? We would have to update the state in HA
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.
It was already calling a method to clean up this class' call dictionary, but I added an abstract on_hangup method as well.
The on_hangup method will be called when a BYE message is received to allow any state about the call to be cleaned up after the call has ended.
I'm able to confirm that the announce action now calls the phone, however it doesn't go into a pipeline for me. |
When the phone answers and sends the invite response message SipDatagramProtocol should be calling on_call just like when a phone calls in, and that is where _create_rtp_server is called. |
|
||
@abstractmethod | ||
def on_call(self, call_info: CallInfo): | ||
"""Handle incoming calls.""" | ||
|
||
def on_hangup(self, call_info: CallInfo): |
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 don't think this method needs to be required to be implemented in subclasses. That way if we want to release and use the updated voip_utils without updating HA yet things should still work.
Thanks @jaminh! |
Updates to make assist satellite announcements work with the VOIP integration. Allow using the existing SIP transport for Home Assistant for both incoming and outgoing calls.