[prev in list] [next in list] [prev in thread] [next in thread]
List: asterisk-dev
Subject: Re: [asterisk-dev] [Code Review] SIP NOTIFY dialog event infos: Number & Name Presentation for BLF s
From: "Mark Michelson" <reviewboard () asterisk ! org>
Date: 2012-07-31 16:07:09
Message-ID: 20120731160709.1432.36367 () hotblack ! digium ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2048/#review6842
-----------------------------------------------------------
From what I can tell, all the changes here are good. The problem is that th=
e chan_sip.c portion of the diff has a lot of unrelated changes. I looked t=
hrough and found the changes you made to extensionstate_update(). Did you m=
ake any other changes in the latest version of the diff?
- Mark
On July 30, 2012, 4:43 a.m., Guenther Kelleter wrote:
> =
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2048/
> -----------------------------------------------------------
> =
> (Updated July 30, 2012, 4:43 a.m.)
> =
> =
> Review request for Asterisk Developers, Mark Michelson, Matt Jordan, and =
Thomas Arimont.
> =
> =
> Summary
> -------
> =
> (For Digium: this is the implementation of the feature described in chapt=
er 2 of the document SIP_NOTIFY_dialog_event_infos_DATUS_v1_4.odt)
> =
> This patch extends the extension state callbacks so that monitoring chann=
els (as chan_sip) get more information of the devices which are responsible=
for an extension state change. The additional information is needed by cha=
n_sip to present names/numbers of the caller and callee in an early-state S=
IP notification. Users of extenstion state callback not interested in the a=
dditional information are not affected by the changes.
> =
> Motivation: to present the involved party's name/number in an early-state=
nofification (used by the notified device as a pickup offer) one after ano=
ther so that a user can see which call he will pick up in an undirected pic=
kup.
> Such a pickup offer to a user shall indicate the same call (number/name-A=
calls number/name-B) as the call which would be picked up when an undirect=
ed pickup is executed.
> =
> =
> Users interested in additional state info must use the new functions ast_=
extension_state_add_extended() resp. ast_extension_state_add_destroy_extend=
ed() to register an extended state callback. When the callback is registere=
d this way, an extra member device_state_info of struct ast_state_cb_info i=
s passed to the callback in addition to the aggregated extension state. Thi=
s container holds an object for every device of the monitored extension hin=
t consisting of the device name, the device state and a channel reference t=
o the channel which (presumably) caused the device state.
> =
> The information is used by chan_sip for early-state notifications. When t=
he state of a device changes and the new state contains AST_EVENT_RINGING, =
an early-state notification is sent to the subscribed devices with the call=
er/callee names/numbers of the oldest ringing channel of the monitored exte=
nsion.
> The notified user may then invoke a direct pickup, which will pickup exac=
tly this channel.
> =
> Users of the old non-extended callbacks will only be called when the aggr=
egated state did change (same behavior as before). Users of the extended ca=
llback will also be called when the state is unchanged but does contain AST=
_EVENT_RINGING. That could be the case if two channels are ringing at one d=
evice and one of them hangs up, so the aggregated state does not change. Th=
is way the monitoring channel can create a new early-state notification wit=
h the now ringing party-ids.
> =
> This patch requires the channel creation time member of ast_channel and t=
he changes for the direct pickup to pick up the oldest ringing channel, whi=
ch are both introduced in review https://reviewboard.asterisk.org/r/2043/.
> =
> =
> Diffs
> -----
> =
> /trunk/channels/chan_sip.c 370418 =
> /trunk/channels/sip/include/sip.h 370418 =
> /trunk/include/asterisk/pbx.h 370418 =
> /trunk/main/pbx.c 370418 =
> =
> Diff: https://reviewboard.asterisk.org/r/2048/diff
> =
> =
> Testing
> -------
> =
> Checked the name/number of caller/callee displayed on the notified phone.
> Checked that a second call at an already ringing device does not invoke a=
notification as long as the first call is ringing, instead it is sent when=
the first ringing channel is picked up by some other party or hung up.
> Checked that the picked up caller is the same as advertized in the early-=
state notification when more than one calls are ringing on the subscribed e=
xtension.
> Ref counting for the containers and channels checked.
> Checked that a notification is sent to newly subscribed watchers.
> =
> =
> Thanks,
> =
> Guenther
> =
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/2048/">https://reviewboard.asterisk.org/r/2048/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">From what I can tell, \
all the changes here are good. The problem is that the chan_sip.c portion of the diff \
has a lot of unrelated changes. I looked through and found the changes you made to \
extensionstate_update(). Did you make any other changes in the latest version of the \
diff?</pre> <br />
<p>- Mark</p>
<br />
<p>On July 30th, 2012, 4:43 a.m., Guenther Kelleter wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for Asterisk Developers, Mark Michelson, Matt Jordan, and Thomas \
Arimont.</div> <div>By Guenther Kelleter.</div>
<p style="color: grey;"><i>Updated July 30, 2012, 4:43 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">(For Digium: this is the implementation of the feature described in \
chapter 2 of the document SIP_NOTIFY_dialog_event_infos_DATUS_v1_4.odt)
This patch extends the extension state callbacks so that monitoring channels (as \
chan_sip) get more information of the devices which are responsible for an extension \
state change. The additional information is needed by chan_sip to present \
names/numbers of the caller and callee in an early-state SIP notification. Users of \
extenstion state callback not interested in the additional information are not \
affected by the changes.
Motivation: to present the involved party's name/number in an early-state \
nofification (used by the notified device as a pickup offer) one after another so \
that a user can see which call he will pick up in an undirected pickup. Such a pickup \
offer to a user shall indicate the same call (number/name-A calls number/name-B) as \
the call which would be picked up when an undirected pickup is executed.
Users interested in additional state info must use the new functions \
ast_extension_state_add_extended() resp. ast_extension_state_add_destroy_extended() \
to register an extended state callback. When the callback is registered this way, an \
extra member device_state_info of struct ast_state_cb_info is passed to the callback \
in addition to the aggregated extension state. This container holds an object for \
every device of the monitored extension hint consisting of the device name, the \
device state and a channel reference to the channel which (presumably) caused the \
device state.
The information is used by chan_sip for early-state notifications. When the state of \
a device changes and the new state contains AST_EVENT_RINGING, an early-state \
notification is sent to the subscribed devices with the caller/callee names/numbers \
of the oldest ringing channel of the monitored extension. The notified user may then \
invoke a direct pickup, which will pickup exactly this channel.
Users of the old non-extended callbacks will only be called when the aggregated state \
did change (same behavior as before). Users of the extended callback will also be \
called when the state is unchanged but does contain AST_EVENT_RINGING. That could be \
the case if two channels are ringing at one device and one of them hangs up, so the \
aggregated state does not change. This way the monitoring channel can create a new \
early-state notification with the now ringing party-ids.
This patch requires the channel creation time member of ast_channel and the changes \
for the direct pickup to pick up the oldest ringing channel, which are both \
introduced in review https://reviewboard.asterisk.org/r/2043/. </pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Checked the name/number of caller/callee displayed on the notified \
phone. Checked that a second call at an already ringing device does not invoke a \
notification as long as the first call is ringing, instead it is sent when the first \
ringing channel is picked up by some other party or hung up. Checked that the picked \
up caller is the same as advertized in the early-state notification when more than \
one calls are ringing on the subscribed extension. Ref counting for the containers \
and channels checked. Checked that a notification is sent to newly subscribed \
watchers.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/channels/chan_sip.c <span style="color: grey">(370418)</span></li>
<li>/trunk/channels/sip/include/sip.h <span style="color: grey">(370418)</span></li>
<li>/trunk/include/asterisk/pbx.h <span style="color: grey">(370418)</span></li>
<li>/trunk/main/pbx.c <span style="color: grey">(370418)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2048/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--
_____________________________________________________________________
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic