[Home]

Summary:ASTERISK-14090: [patch] hints with 2+ devices that include ONHOLD are often set wrong
Reporter:Philippe Lindheimer (p_lindheimer)Labels:
Date Opened:2009-05-07 21:25:45Date Closed:2009-06-29 11:22:37
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/PBX
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) devicestate.c.trunk.patch
( 1) devicestate.patch
( 2) onhold_trunk.diff
( 3) pbx.c.1.4.patch
( 4) pbx.patch
Description:I brought this up in the dev mailing list, the crux of the issue is:

If you have a hint with two (or more) devices or a device and a custom devstate that includes ONHOLD, you get unexpected results. Two examples:

exten => 222,hint,SIP/222&SIP/211
exten => 333,hint,Custom:DND333&SIP/333

The following results are inconsistent as they should all return ONHOLD:

ONHOLD & IDLE = IDLE
ONHOLD & UNAVAILABLE => UNAVAILABLE
ONHOLD & UNKNOWN => IDLE
ONHOLD & INVALID => IDLE

Per Russell Bryant's response on the mailing list:

"I would agree that this is a bug.  I would expect that combination of  
states to result in an OnHold state."



****** ADDITIONAL INFORMATION ******

I have only tested this on 1.4 but believe it continued to be an issue in 1.6. Per Russell:

"The responsible code is in ast_extension_state2() of main/pbx.c in Asterisk 1.4.
In Asterisk trunk, the same logic was moved into some common code. Search for ast_devstate_aggregate in main/devicestate.c."
Comments:By: Philippe Lindheimer (p_lindheimer) 2009-05-07 21:31:04

I have attached a patch that makes all of the above examples return ONHOLD which is the expected behavior. This patch is against 1.4
By: Philippe Lindheimer (p_lindheimer) 2009-05-07 21:59:39

I attached devicestate.patch against trunk. I do not have a system to test this and have not even had a system setup to compile it but have applied the same logic as that applied to pbx.patch. Hopefully it is valid.
By: Leif Madsen (lmadsen) 2009-05-08 12:51:56

Assigned to dvossel to take a look at the issue. However, if you feel you can't move this forward, then I think re-assigning this to file would be appropriate. Thanks!
By: Philippe Lindheimer (p_lindheimer) 2009-05-21 08:27:26

any chance this can be looked at - at least a static review of the patches (particularly 1.6 which I could not test) would be useful. The 1.4 patch "worksforme" on my testing system.
By: Leif Madsen (lmadsen) 2009-05-21 10:24:53

dvossel has been out for a bit now, but he is back today. Perhaps he will be able to spend a bit of time on it in the near future. Thanks!
By: David Vossel (dvossel) 2009-05-26 12:40:15

I've reviewed devicestate.patch, onhold_trunk.diff fixes the few things I found wrong with it.
By: Philippe Lindheimer (p_lindheimer) 2009-05-27 10:58:44

There were some issues pointed out in the review which uncovered some more in further testing, so attaching two new patches that should now exhibit the following behavior:

BUSY & ONHOLD => BUSY
INUSE & ONHOLD => INUSE
ONHOLD & RINGING => RINGINUSE

this opens up a question, should the last return RINGINUSE or is a new state required to return RINGONHOLD? (and if we have ONHOLD&RINGING&INUSE do we return all three?)

My opinion would be RINGINUSE is appropriate...
By: Leif Madsen (lmadsen) 2009-05-27 11:58:24

I think RINGINUSE is appropriate as well.
By: David Vossel (dvossel) 2009-05-27 13:55:51

posted the new patch on reviewboard.  I uploaded a slightly modified version of devicestate.c.trunk.patch that fixes some compile time issues.  There were just some typos in some variable names.


By: Dmitry Andrianov (dimas) 2009-05-27 16:28:25

Isn't it just simpler (and better) to treat ONHOLD&RINGING as RINGING?
By: Philippe Lindheimer (p_lindheimer) 2009-05-27 17:10:23

If two devices, one RINGING and the other INUSE (and possibly being rung also if it supports multiple line appearances), is reported as RINGINUSE, not just RINGING. I therefore believe that the related situation of one RINGING and the other ONHOLD (and possibly also RINGING if multiple line appearances are supported), should be at least RINGINUSE unless we define a new RINGONHOLD. To report simply RINGING does not seem right, since it is in fact also INUSE (being the line is ONHOLD). As the hints are used to provide status information, often to BLF keys on other phones (like a receptionist console), you always want to be aware if an extension is 'INUSE' in any form, of which ONHOLD is.
By: Philippe Lindheimer (p_lindheimer) 2009-06-04 10:26:14

what's the status on this, looked like it got some testing (or at least feedback) from some comments in the code reviews?
By: David Vossel (dvossel) 2009-06-04 10:30:55

It looks good to go, its on my list to commit later today or tomorrow.
By: Digium Subversion (svnbot) 2009-06-05 16:19:57

Repository: asterisk
Revision: 199297

U   branches/1.4/main/pbx.c

------------------------------------------------------------------------
r199297 | dvossel | 2009-06-05 16:19:56 -0500 (Fri, 05 Jun 2009) | 14 lines

Fixes issue with hints giving unexpected results.

Hints with two or more devices that include ONHOLD gave unexpected results.

(closes issue ASTERISK-14090)
Reported by: p_lindheimer
Patches:
     onhold_trunk.diff uploaded by dvossel (license 671)
     pbx.c.1.4.patch uploaded by p (license 558)
     devicestate.c.trunk.patch uploaded by p (license 671)
Tested by: p_lindheimer, dvossel

Review: https://reviewboard.asterisk.org/r/254/

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=199297
By: Digium Subversion (svnbot) 2009-06-05 16:21:23

Repository: asterisk
Revision: 199298

_U  trunk/
U   trunk/include/asterisk/devicestate.h
U   trunk/main/devicestate.c

------------------------------------------------------------------------
r199298 | dvossel | 2009-06-05 16:21:22 -0500 (Fri, 05 Jun 2009) | 21 lines

Merged revisions 199297 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
 r199297 | dvossel | 2009-06-05 16:19:56 -0500 (Fri, 05 Jun 2009) | 14 lines
 
 Fixes issue with hints giving unexpected results.
 
 Hints with two or more devices that include ONHOLD gave unexpected results.
 
 (closes issue ASTERISK-14090)
 Reported by: p_lindheimer
 Patches:
       onhold_trunk.diff uploaded by dvossel (license 671)
       pbx.c.1.4.patch uploaded by p (license 558)
       devicestate.c.trunk.patch uploaded by p (license 671)
 Tested by: p_lindheimer, dvossel
 
 Review: https://reviewboard.asterisk.org/r/254/
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=199298
By: Digium Subversion (svnbot) 2009-06-05 16:25:50

Repository: asterisk
Revision: 199299

_U  branches/1.6.2/
U   branches/1.6.2/include/asterisk/devicestate.h
U   branches/1.6.2/main/devicestate.c

------------------------------------------------------------------------
r199299 | dvossel | 2009-06-05 16:25:50 -0500 (Fri, 05 Jun 2009) | 28 lines

Merged revisions 199298 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r199298 | dvossel | 2009-06-05 16:21:22 -0500 (Fri, 05 Jun 2009) | 21 lines
 
 Merged revisions 199297 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r199297 | dvossel | 2009-06-05 16:19:56 -0500 (Fri, 05 Jun 2009) | 14 lines
   
   Fixes issue with hints giving unexpected results.
   
   Hints with two or more devices that include ONHOLD gave unexpected results.
   
   (closes issue ASTERISK-14090)
   Reported by: p_lindheimer
   Patches:
         onhold_trunk.diff uploaded by dvossel (license 671)
         pbx.c.1.4.patch uploaded by p (license 558)
         devicestate.c.trunk.patch uploaded by p (license 671)
   Tested by: p_lindheimer, dvossel
   
   Review: https://reviewboard.asterisk.org/r/254/
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=199299
By: Digium Subversion (svnbot) 2009-06-05 16:32:17

Repository: asterisk
Revision: 199300

_U  branches/1.6.1/
U   branches/1.6.1/include/asterisk/devicestate.h
U   branches/1.6.1/main/devicestate.c

------------------------------------------------------------------------
r199300 | dvossel | 2009-06-05 16:32:17 -0500 (Fri, 05 Jun 2009) | 28 lines

Merged revisions 199298 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r199298 | dvossel | 2009-06-05 16:21:22 -0500 (Fri, 05 Jun 2009) | 21 lines
 
 Merged revisions 199297 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r199297 | dvossel | 2009-06-05 16:19:56 -0500 (Fri, 05 Jun 2009) | 14 lines
   
   Fixes issue with hints giving unexpected results.
   
   Hints with two or more devices that include ONHOLD gave unexpected results.
   
   (closes issue ASTERISK-14090)
   Reported by: p_lindheimer
   Patches:
         onhold_trunk.diff uploaded by dvossel (license 671)
         pbx.c.1.4.patch uploaded by p (license 558)
         devicestate.c.trunk.patch uploaded by p (license 671)
   Tested by: p_lindheimer, dvossel
   
   Review: https://reviewboard.asterisk.org/r/254/
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=199300
By: Digium Subversion (svnbot) 2009-06-05 16:37:02

Repository: asterisk
Revision: 199301

_U  branches/1.6.0/
U   branches/1.6.0/main/pbx.c

------------------------------------------------------------------------
r199301 | dvossel | 2009-06-05 16:37:02 -0500 (Fri, 05 Jun 2009) | 28 lines

Merged revisions 199298 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r199298 | dvossel | 2009-06-05 16:21:22 -0500 (Fri, 05 Jun 2009) | 21 lines
 
 Merged revisions 199297 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r199297 | dvossel | 2009-06-05 16:19:56 -0500 (Fri, 05 Jun 2009) | 14 lines
   
   Fixes issue with hints giving unexpected results.
   
   Hints with two or more devices that include ONHOLD gave unexpected results.
   
   (closes issue ASTERISK-14090)
   Reported by: p_lindheimer
   Patches:
         onhold_trunk.diff uploaded by dvossel (license 671)
         pbx.c.1.4.patch uploaded by p (license 558)
         devicestate.c.trunk.patch uploaded by p (license 671)
   Tested by: p_lindheimer, dvossel
   
   Review: https://reviewboard.asterisk.org/r/254/
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=199301