[Asterisk-code-review] res pjsip: Match Asterisk 1.8 BLF behavior of chan sip (asterisk[13])

Wed May 17 09:28:36 CDT 2017

Matthew Fredrickson has posted comments on this change. ( https://gerrit.asterisk.org/5484 )

Change subject: res_pjsip: Match Asterisk 1.8 BLF behavior of chan_sip
......................................................................

Patch Set 3:

> > > > > Could this change break other endpoints that rely on the
 > > > > > statestring being "confirmed" vs "early" for this case?
 > > > > >
 > > > > > In chan_sip there was an option, notifyringing, that
 > > depending
 > > > > how
 > > > > > set is used to switch between the two. Is a similar option
 > > > needed
 > > > > > here?
 > > > >
 > > > > Bump.
 > > > >
 > > > > Zach, were you planning on answering Kevin's question?
 > > >
 > > > > > Could this change break other endpoints that rely on the
 > > > > > statestring being "confirmed" vs "early" for this case?
 > > > > >
 > > > > > In chan_sip there was an option, notifyringing, that
 > > depending
 > > > > how
 > > > > > set is used to switch between the two. Is a similar option
 > > > needed
 > > > > > here?
 > > > >
 > > > > Bump.
 > > > >
 > > > > Zach, were you planning on answering Kevin's question?
 > > >
 > > > Sorry, I did not see the question as I have been a bit busy at
 > > work
 > > > with things that had came up.
 > > >
 > > > I don't believe PJSIP currently has any sort of yes or no flag
 > > that
 > > > could be used to simulate the notifyringing option of chan_sip.
 > I
 > > > read up on that option since it wasn't too familiar to me so
 > > > correct me if I am wrong on my understanding of the option.
 > > Instead
 > > > of it controlling if the phone is sent a notify on ringing, it
 > > > instead controls what the XML dialog has in the state
 > > information,
 > > > early or confirmed.
 > > >
 > > > So it is quite possible that if a specific phone does something
 > > on
 > > > the confirmed vs early, such as how the Cisco SPA phones
 > > >
 > > > > > Could this change break other endpoints that rely on the
 > > > > > statestring being "confirmed" vs "early" for this case?
 > > > > >
 > > > > > In chan_sip there was an option, notifyringing, that
 > > depending
 > > > > how
 > > > > > set is used to switch between the two. Is a similar option
 > > > needed
 > > > > > here?
 > > > >
 > > > > Bump.
 > > > >
 > > > > Zach, were you planning on answering Kevin's question?
 > > >
 > > > > > Could this change break other endpoints that rely on the
 > > > > > statestring being "confirmed" vs "early" for this case?
 > > > > >
 > > > > > In chan_sip there was an option, notifyringing, that
 > > depending
 > > > > how
 > > > > > set is used to switch between the two. Is a similar option
 > > > needed
 > > > > > here?
 > > > >
 > > > > Bump.
 > > > >
 > > > > Zach, were you planning on answering Kevin's question?
 > > >
 > > > Sorry, I had not seen the question between the last time I had
 > > > checked this for any new posts.
 > > >
 > > >
 > > >
 > > > > > Could this change break other endpoints that rely on the
 > > > > > statestring being "confirmed" vs "early" for this case?
 > > > > >
 > > > > > In chan_sip there was an option, notifyringing, that
 > > depending
 > > > > how
 > > > > > set is used to switch between the two. Is a similar option
 > > > needed
 > > > > > here?
 > > > >
 > > > > Bump.
 > > > >
 > > > > Zach, were you planning on answering Kevin's question?
 > > >
 > > > I read up on the notifyringing option as I wasn't fully sure I
 > > > understood it, and from what I read it basically controlled
 > > whether
 > > > a notify included confirmed or early as the state tag rather
 > than
 > > > controlling if it sent something on the ringing state. The
 > thing
 > > I
 > > > am not sure about related to this option, if the phone was idle
 > > and
 > > > had this option set to no would it send confirmed or early for
 > > the
 > > > first.
 > > >
 > > > Currently I don't believe PJSIP has an option that is similar
 > to
 > > > the one from chan_sip. Such a new option would be necessary if
 > > you
 > > > wanted a flag to switch the behavior on an endpoint between the
 > > two
 > > > like chan_sip.
 > > >
 > > > Though I don't believe my patch will break any endpoints, since
 > > the
 > > > default behavior in chan_sip was set to yes, so it would send
 > > early
 > > > on ringing.
 > > >
 > > > The changes I proposed flip this from it currently replicating
 > > > notifyringing=no to the default of chan_sip notifyringing=yes.
 > > >
 > > > Though it may be worth having the switch setting in pjsip
 > > endpoints
 > > > in the off chance there is a phone or sip object out there that
 > > > relies on confirmed vs early in the ringing && in use
 > situation.
 > > As
 > > > well it would be useful such that PJSIP would replicate how
 > > > chan_sip truly behaved instead of just one or the other option.
 > >
 > > Ok, just to set expectations properly, I think in order for this
 > to
 > > go into a release branch (13 or 14), it will need to be optional
 > > behavior, with the behavior hidden behind an option flag.  Also,
 > > tests will need to be written which exercise the behavior.  One
 > > thing we try to avoid is change default behavior within the
 > context
 > > of a branch.
 > >
 > > Sounds like you're working on getting the testsuite setup, which
 > is
 > > excellent.  Just need to an option that makes behavior default to
 > > the way it worked prior to your patch.
 > 
 > 
 > 
 > I understand why you say it's default should match what it was
 > prior to my patch, as people may be running Asterisk and expecting
 > it's default behavior to be as it has been doing this since January
 > in ASTERISK-25712 was closed.
 > 
 > Though I was thinking wouldn't it be better this new option on the
 > endpoint to replicate the default from chan_sip. Right now to my
 > understanding it is replicating notifyringing=no from chan_sip, but
 > the default for notifyringing was yes.
 > 
 > Which of the two options would be more reasonable for me to add to
 > the patch, the default set to 'no' as the code basically is doing
 > now, or 'yes' to replicate chan_sip?

I can see your position, but IMHO we should still replicate the existing branch behavior.  I'd rather not see unintended consequences of changing expected behavior in those already using chan_pjsip play out with this change.

-- 
To view, visit https://gerrit.asterisk.org/5484
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4326228f83a327a7510fecae0fec43c2945a3f25
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Zach R <zrothy at monmouth.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Matthew Fredrickson <creslin at digium.com>
Gerrit-Reviewer: Zach R <zrothy at monmouth.com>
Gerrit-HasComments: No