[asterisk-dev] [Code Review] ETSI Message Waiting Indication (MWI) (Asterisk component)

[rmudgett at digium.com]

> On 2010-04-01 07:04:42, Russell Bryant wrote:
> > /team/group/CCSS/channels/chan_dahdi.c, lines 17121-17125
> > <https://reviewboard.asterisk.org/r/599/diff/1/?file=9058#file9058line17121>
> >
> >     Why does this need a new option instead of the existing "mailbox" option?

The existing "mailbox" option is per channel.  For ISDN this is per B channel which is not useful as all B channels on a span should be configured identically.  The new "mwi_mailbox" option is per span and is a list of mailboxes that are reported to that span.


> On 2010-04-01 07:04:42, Russell Bryant wrote:
> > /team/group/CCSS/channels/sig_pri.h, lines 245-246
> > <https://reviewboard.asterisk.org/r/599/diff/1/?file=9059#file9059line245>
> >
> >     Why 8?  Why any limit at all?

The limit of 8 is the limit typically placed on BRI lines.  The BRI line typically can have a maximum of 8 MSNs or devices connected to it.  It was also easier to keep and use the results of parsing the mwi_mailbox as an array.


> On 2010-04-01 07:04:42, Russell Bryant wrote:
> > /team/group/CCSS/channels/sig_pri.h, line 393
> > <https://reviewboard.asterisk.org/r/599/diff/1/?file=9059#file9059line393>
> >
> >     docs for the new sig_pri API call?

Docs are where the function is defined.


> On 2010-04-01 07:04:42, Russell Bryant wrote:
> > /team/group/CCSS/channels/sig_pri.c, lines 4790-4791
> > <https://reviewboard.asterisk.org/r/599/diff/1/?file=9060#file9060line4790>
> >
> >     Use ast_debug()

Done.


> On 2010-04-01 07:04:42, Russell Bryant wrote:
> > /team/group/CCSS/channels/sig_pri.c, line 4852
> > <https://reviewboard.asterisk.org/r/599/diff/1/?file=9060#file9060line4852>
> >
> >     I'd rather see ARRAY_LEN() on pri->mbox than the #define used

Done.


> On 2010-04-01 07:04:42, Russell Bryant wrote:
> > /team/group/CCSS/channels/sig_pri.c, lines 4925-4928
> > <https://reviewboard.asterisk.org/r/599/diff/1/?file=9060#file9060line4925>
> >
> >     Could this be reworked to use strsep()?  Asterisk code pretty much uses strsep() everywhere instead of strtok_r()

Done.  It also simplified the code somewhat.


> On 2010-04-01 07:04:42, Russell Bryant wrote:
> > /team/group/CCSS/channels/sig_pri.h, line 298
> > <https://reviewboard.asterisk.org/r/599/diff/1/?file=9059#file9059line298>
> >
> >     The buffer size here is a bit of a magic number.  If you're going to use AST_MAX_EXTENSION, it seems that multiplying that by the number of mailboxes possible would be more appropriate.  That's a lot of wasted space, though.  Perhaps this object needs to be updated to use the stringfields API?

Using stringfields is a possibility.  However, it would currently be the only string.

I created a new define to document the size and handle better the possibility of all 8 mailboxes using typical max lengths of mailbox number and contexts.


- rmudgett


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


On 2010-03-31 11:41:00, rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/599/
> -----------------------------------------------------------
> 
> (Updated 2010-03-31 11:41:00)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Add the ability to report waiting messages to ISDN endpoints (phones).
> 
> Relevant specification: EN 300 650 and EN 300 745
> 
> This is based off of the team/group/CCSS branch to get the
> sig_pri_load()/sig_pri_unload() improvements which turned out to not be
> necessary.
> 
> The libpri portion of this review is locatged in review:
> https://reviewboard.asterisk.org/r/600/
> 
> 
> Diffs
> -----
> 
>   /team/group/CCSS/CHANGES 255495 
>   /team/group/CCSS/channels/chan_dahdi.c 255495 
>   /team/group/CCSS/channels/sig_pri.h 255495 
>   /team/group/CCSS/channels/sig_pri.c 255495 
>   /team/group/CCSS/configs/chan_dahdi.conf.sample 255495 
>   /team/group/CCSS/configure.ac 255495 
> 
> Diff: https://reviewboard.asterisk.org/r/599/diff
> 
> 
> Testing
> -------
> 
> 1) Setup app_voicemail
> 2) Left messages in various voice mail contexts
> 3) Retrieved messages
> 
> Verified that the MWIIndication message reported the expected number of waiting messages.
> Also verified that the MWIIndication message is reported when Asterisk is started.
> 
> 
> Thanks,
> 
> rmudgett
> 
>