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

[Zach R]

Zach R 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?

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