ASTERISK-14178: [patch] Add ability to use extension state as well as device state when adding queue memebers

[Home]

Summary: ASTERISK-14178: [patch] Add ability to use extension state as well as device state when adding queue memebers

Reporter: Philippe Lindheimer (p_lindheimer) Labels:

Date Opened: 2009-05-20 15:43:39 Date Closed: 2009-11-03 15:21:39.000-0600

Priority: Major Regression? No

Status: Closed/Complete Components: Applications/app_queue

Versions: Frequency of
Occurrence

Related
Issues:

Environment: Attachments: ( 0) queue_extenstate_1.4.svn.patch
( 1) queue_extenstate2_1.4.svn.patch
( 2) queue_extenstate3_1.4.svn.patch
( 3) queue_extenstate4_1.4.svn..2.patch
( 4) queue_extenstate5_1.4.svn.patch

Description: The new ability to add a device state associated with a Queue member when configuring a queue or adding a member with AddQueueMembers solves a lot of problems. However, it would be EXTREMELY valuable to use extension state information as well (e.g. hints).

hints are extensively used in may dialplans to track extension state of a system. This ability becomes even more important when you have a pbx that supports 'hot-desking' where a user can log into and out of one or multiple devices.

It would be extremely valuable, and from what I understand, relatively straight forward, to add this ability such that conceptually you could an entry such as:

member => Local/3000@default,0,John Smith,HINT:3000@default

and the queue would parse that this is a hint instead of a device, and use ast_extension_state() instead of ast_device_state()

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

discussed with jsmith and putnopvut on IRC

Comments: By: Philippe Lindheimer (p_lindheimer) 2009-05-24 12:20:36

I've played with this a bit, may have some code to tack on here shortly. It does turn out that there is a 'small' complication.

There is a function: handle_statechange(struct statechange *sc) called by device_state_thread(void *data) that tracks device state changes. It or something similar will have to track extension state changes, or when device states change, check if there are any members looking at hints that currently contain those devices. As hints have callbacks, is it possible to have queues subscribe to those callbacks if there are hints?
By: Philippe Lindheimer (p_lindheimer) 2009-05-25 13:50:38

oops - forgot to check the box, it is a code submission (but I've signed the waiver)

The following patch, queue_extenstate_1.4.svn.patch provides my first pass at modifying the code so that it can included hints as well as devices. It is applied against r196621 of the 1.4 branch.

It expects HINT:nnn@context format if you want to use a HINT instead of a device. This patch also includes a fix in the handle_statechange() function that monitors state change. That function currently ignores state changes for Custom DEVSTATESs. Note that the bug fix is not complete, since it will also ignore state changes for MeetMe:nnn devstates and probably others. But that is a separate bug that should be filed separately. (note - it may be fixed in trunk, but given that it was chosen to backport this new feature to 1.4 and given the widespread use of func_devstate.c backport to 1.4, it would appear a little questionable not to address this shortcoming in 1.4).
By: Philippe Lindheimer (p_lindheimer) 2009-05-27 11:45:49

modified patch to handle hints entered as HINT:nnn (where @context is left off). Was crashing, now defaults to "@default" as does EXTENSION_STATE() function.

The other two patch files should be removed but I don't think I have permission to do so (or am just not seeing where).
By: Mark Michelson (mmichelson) 2009-05-29 15:40:35

Hi Philippe. I took some time to look at your patch and I have some suggestions.

1. First off, for us to consider including this into Asterisk, you need to create the patch against trunk. The main difference between trunk and 1.4 is the way the device state and extension state callbacks work. In trunk, there is an event API that is used to subscribe to the changes. Otherwise, it's mostly the same.

2. The TODO comments regarding parsing out the "HINT:" are really good ideas. I think this should be done before committing the change.

3. There are many coding guidelines violations here. Most of them deal with using spaces instead of tabs to indent lines, use of C++ style comments, and lack of spaces around punctuation (e.g. spaces after commas, and spaces before and after binary operators). Check out doc/CODING-GUIDELINES for all the details.

4. You've created a lot of work for yourself by not registering an extension state callback. You can use ast_extension_state_add in order to subscribe to extension state changes. This way, you won't have to try to parse devices out of the hints manually. You can leave that job to the pbx core instead.

Those were the big things I noticed. Other than that, great work!
By: Philippe Lindheimer (p_lindheimer) 2009-05-29 16:09:17

mmichelson,
thanks for the feedback. I figured there were probably a fair share of coding guidelines (though I did use tabs, not spaces :) )

Here is the issue with the extension state callback, if I understand it properly. You could have the following time sequence of events:

1. Add a queue member to a queue, with no current hint created
2. callback tries to register and fails because no hint exists
3. a hint is subsequently created dynamically or upon a reload
4. the queue is not keeping track of it because the callback failed

In the current scenario, each time there is a device change we get called back so once the hint gets created in step (3), the next time a device changes that is part of that hint, it will be detected.

from what you say in 1.6, the ast_event API might provide for an alternate solution.
By: Philippe Lindheimer (p_lindheimer) 2009-08-26 19:07:38

For FreePBX users, the Queues Module has been updated on FreePBX 2.6 so that the inclusion of the following flag in amportal.conf:

USEQUEUESTATE=yes

will generate configuration compatible with this patch. When we know what version of Asterisk this "HINT:" syntax will be standard in we will make it automatically generate the required syntax regardless of the patch if that version is running.
By: Bryan Walters (gamegamer43) 2009-09-30 21:12:41

Please note that this patch does not work against Asterisk 1.4.26.1 rev 211596. This issue is that the following has been defined elsewhere and is no longer needed in pbx.h:

struct ast_exten *ast_hint_extension(struct ast_channel *c, const char *context, const char *exten);

As a result the only thing that needs to be patched still is app_queue.c
By: Bryan Walters (gamegamer43) 2009-10-02 12:09:14

Ok after playing with and test some things, it turns out that the pbx.h definition wasn't the issue. The issue was that the original patch forgot to make a change to pbx.c. While I read through the previous comment and I know my patch isn't made against trunk (I created the patch against 211596 which was the release of 1.4.26.1) I'm uploading it, as I hope it will still be useful.
By: Digium Subversion (svnbot) 2009-11-03 15:21:37.000-0600

Repository: asterisk
Revision: 227424

U trunk/apps/app_queue.c
U trunk/configs/queues.conf.sample

------------------------------------------------------------------------
r227424 | file | 2009-11-03 15:21:37 -0600 (Tue, 03 Nov 2009) | 7 lines

Add support for using a hint when configuring a state interface using the format hint:<extension>@<context>.

(closes issue ASTERISK-14178)
Reported by: p_lindheimer
Patches:
queue_extenstate5_1.4.svn.patch uploaded by GameGamer43 (license 894)

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

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