[asterisk-dev] [Code Review] 2657: Replace chan_agent with app_agent_pool.

Tue Jul 9 12:23:49 CDT 2013

> On July 8, 2013, 11:44 p.m., Mark Michelson wrote:
> > /trunk/apps/app_agent_pool.c, lines 203-205
> > <https://reviewboard.asterisk.org/r/2657/diff/2/?file=41156#file41156line203>
> >
> >     Possibly another instance where an entire channel snapshot could be useful instead of just a single piece of information.

A snapshot is not possible.  BRIDGEPEER can be a comma separated list of peer channels.

> On July 8, 2013, 11:44 p.m., Mark Michelson wrote:
> > /trunk/apps/app_agent_pool.c, lines 250-252
> > <https://reviewboard.asterisk.org/r/2657/diff/2/?file=41156#file41156line250>
> >
> >     I would suggest a rename of this from "global" to "general" since the actual name of the reserved configuration section in agents.conf is "general". For someone getting CLI help, this will make things more clear.

I could do this.  I was just following other modules.  The features_config.c and conf_config_parser.c uses globals/global respectively.

> On July 8, 2013, 11:44 p.m., Mark Michelson wrote:
> > /trunk/apps/app_agent_pool.c, line 258
> > <https://reviewboard.asterisk.org/r/2657/diff/2/?file=41156#file41156line258>
> >
> >     Consider adding default values to configOptions.

The defaults are obtained from the config framework by the online documentation.

> On July 8, 2013, 11:44 p.m., Mark Michelson wrote:
> > /trunk/apps/app_agent_pool.c, lines 599-612
> > <https://reviewboard.asterisk.org/r/2657/diff/2/?file=41156#file41156line599>
> >
> >     For alignment purposes, these single-bit fields should be at the end of the struct.

These options do not need to be at the end of the struct for alignment purposes.  They are packed into an unsigned int and thus have unsigned int alignment.  However, they need to be grouped together to be packed into the same unsigned int.

> On July 8, 2013, 11:44 p.m., Mark Michelson wrote:
> > /trunk/apps/app_agent_pool.c, lines 1190-1211
> > <https://reviewboard.asterisk.org/r/2657/diff/2/?file=41156#file41156line1190>
> >
> >     It seems wasteful to continue any further if you already know that you are going to return in error. Why try to setup the interval hook if setting up the DTMF hook already failed?

Doing this just reduces the number of exit points and thus repeated calls to ast_channel_remove_bridge_role().  Speed is not really a concern in this part of the code.

- rmudgett

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2657/#review9094
-----------------------------------------------------------

On July 5, 2013, 9:12 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2657/
> -----------------------------------------------------------
> 
> (Updated July 5, 2013, 9:12 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-21554
>     https://issues.asterisk.org/jira/browse/ASTERISK-21554
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> The ill conceived chan_agent is no more.  It is now replaced by app_agent_pool.
> 
> Agents login using the AgentLogin() application as before.  The AgentLogin() application no longer does any authentication.  Authentication is now the responsibility of the dialplan.  (Besides, the authentication done by chan_agent did not match what the voice prompts asked for.)
> 
> Sample extensions.conf
> [login]
> ; Sample agent 1001 login
> ; Set COLP for in between calls so the agent does not see the last caller COLP.
> exten => 1001,1,Set(CONNECTEDLINE(all)="Agent Waiting" <1001>)
> ; Give the agent DTMF transfer and disconnect features when connected to a caller.
> same => n,Set(CHANNEL(dtmf-features)=TX)
> same => n,AgentLogin(1001)
> same => n,NoOp(AGENT_STATUS is ${AGENT_STATUS})
> same => n,Hangup()
> 
> [caller]
> ; Sample caller direct connect to agent 1001
> exten => 800,1,AgentRequest(1001)
> same => n,NoOp(AGENT_STATUS is ${AGENT_STATUS})
> same => n,Hangup()
> 
> ; Sample caller going through a Queue to agent 1001
> exten => 900,1,Queue(agent_q)
> same => n,Hangup()
> 
> Sample queues.conf
> [agent_q]
> member => Local/800 at caller,,SuperAgent,Agent:1001
> 
> Under the hood operation overview:
> 1) Logged in agents wait for callers in an agents holding bridge.
> 2) Caller requests an agent using AgentRequest()
> 3) A basic bridge is created, the agent is notified, and caller joins the basic bridge to wait for the agent.
> 4) The agent is either automatically connected to the caller or must ack the call to connect.
> 5) The agent is moved from the agents holding bridge to the basic bridge.
> 6) The agent and caller talk.
> 7) The connection is ended by either party.
> 8) The agent goes back to the agents holding bridge.
> 
> To avoid some locking issues with the agent holding bridge, I needed to make some changes to the after bridge callback support.  The after bridge callback is now a list of requested callbacks with the last to be added the only active callback.  The after bridge callback for failed callbacks will always happen in the channel thread when the channel leaves the bridging system or is destroyed.
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 393766 
>   /trunk/UPGRADE.txt 393766 
>   /trunk/apps/app_agent_pool.c PRE-CREATION 
>   /trunk/channels/chan_agent.c 393766 
>   /trunk/configs/agents.conf.sample 393766 
>   /trunk/configs/queues.conf.sample 393766 
>   /trunk/include/asterisk/bridging.h 393766 
>   /trunk/include/asterisk/config_options.h 393766 
>   /trunk/include/asterisk/stasis_channels.h 393766 
>   /trunk/main/bridging.c 393766 
>   /trunk/main/stasis_channels.c 393766 
> 
> Diff: https://reviewboard.asterisk.org/r/2657/diff/
> 
> 
> Testing
> -------
> 
> Tested the features of agents and they work as expected.
> 1) Agents login and get MOH and any COLP set by the dialplan before AgentLogin().
> 2) Agents logging in with a local channel chain wait for the local channels to optimize out.
> 3) Caller channels directly running AgentRequest() are able to connect to the agent.
> 4) Callers going through Queue() can connect to agents via local channels.  The local channels can optimize themselves out.
> 5) Tested recording agent calls.  Note: Agent calls cannot be recorded currently if the caller came in on an optimizing local channel because MixMonitor audio hooks are not being handled by the optimization.
> 6) Caller COLP is shown to the agent before the agent accepts the call.
> 7) The original COLP when the agent logged in is restored while the agent is between calls.
> 8) Tested agent wrapup time.
> 9) Tested agent not acknowledging the call before autologoff time.
> 
> Many more.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130709/5c75d797/attachment-0001.htm>