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

[Matthew Fredrickson]

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: Code-Review-1

> > > 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.

-- 
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