[asterisk-dev] [Code Review] Add CCBS/CCNR and enhanced COLP features to chan_misdn.

[Kevin Fleming]

> On 2009-04-10 15:21:27, Russell Bryant wrote:
> > /trunk/channels/chan_misdn.c, lines 231-242
> > <http://reviewboard.digium.com/r/218/diff/1/?file=4123#file4123line231>
> >
> >     It looks like you have some fields that could be made into bit fields.
> 
>  wrote:
>     decline.  Bit-fields are notoriously non-portable and can be code intensive for the compiler to implement.  The resulting code bloat can make the reduction in the structure size not worth it.

We do not have portability concerns with this code, as all Asterisk code is intended to be compiled by GCC (or GCC-compatible) compilers, and uses many GCC extensions that would make it difficult (or impossible) compile with other compilers. Since GCC implements bitfields properly on every reasonably modern platform, there is no portability concern using them. Since we use bitfields (or bit flags in integer fields ala 'struct ast_flags') all over the place in Asterisk already, I'd like to see some evidence of this 'code intensiveness' using a current version of GCC before we change course and start using 4-byte fields for flags.


- Kevin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/218/#review680
-----------------------------------------------------------


On 2009-04-10 19:51:02, rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/218/
> -----------------------------------------------------------
> 
> (Updated 2009-04-10 19:51:02)
> 
> 
> Review request for Asterisk Developers, Russell Bryant and Mark Michelson.
> 
> 
> Summary
> -------
> 
> This diff adds the following features to chan_misdn:
> * CCBS/CCNR Party A support for PTMP and PTP modes.
> * Enhances COLP support for call diversion and explicit call transfer.
> 
> These enhanced features require a modified version of mISDN.
> 
> The latest modified mISDN v1.1.x based version is available at:
> http://svn.digium.com/svn/thirdparty/mISDN/trunk
> http://svn.digium.com/svn/thirdparty/mISDNuser/trunk
> 
> Taged versions of the modified mISDN code are available under:
> http://svn.digium.com/svn/thirdparty/mISDN/tags
> http://svn.digium.com/svn/thirdparty/mISDNuser/tags
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 188005 
>   /trunk/channels/chan_misdn.c 188005 
>   /trunk/channels/misdn/chan_misdn_config.h 188005 
>   /trunk/channels/misdn/ie.c 188005 
>   /trunk/channels/misdn/isdn_lib.h 188005 
>   /trunk/channels/misdn/isdn_lib.c 188005 
>   /trunk/channels/misdn/isdn_lib_intern.h 188005 
>   /trunk/channels/misdn/isdn_msg_parser.c 188005 
>   /trunk/channels/misdn_config.c 188005 
>   /trunk/configs/misdn.conf.sample 188005 
> 
> Diff: http://reviewboard.digium.com/r/218/diff
> 
> 
> Testing
> -------
> 
> Digium's Product Quality department has tested a similar version of these enhancements. In addition, we know of a customer who has been testing these changes.
> 
> There is a test cli command that can be conditionally compiled into the code by defining CCBS_TEST_MESSAGES in chan_misdn.c.  The test command allows you to send canned facility messages that I used to test the encoding and decoding routines in mISDN.
> 
> misdn send facility test (<port> [<msg#>]) | (<channel-name> <msg#>)
> 
> 
> Thanks,
> 
> rmudgett
> 
>