[asterisk-dev] [Code Review] enable SMS polling in chan_mobile

[Matthew Nicholson]

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



/trunk/addons/chan_mobile.c
<https://reviewboard.asterisk.org/r/908/#comment5962>

    Why was the "No Service" state removed here?



/trunk/addons/chan_mobile.c
<https://reviewboard.asterisk.org/r/908/#comment5963>

    This breaks debugging output.  The rfcomm_read_and_expect_char() should be used instead.



/trunk/addons/chan_mobile.c
<https://reviewboard.asterisk.org/r/908/#comment5964>

    This is how the following "\r\nOK\r\n" is read, but both of these reads should be done using a more general function that extends rfcomm_read_and_expect_char to read and expect an entire string.  Then you could do something like rfcomm_read_and_expect_string(rsock, &result, result_size, expected).



/trunk/addons/chan_mobile.c
<https://reviewboard.asterisk.org/r/908/#comment5965>

    None of the other parsing functions seem to have these debug level 1 messages.



/trunk/addons/chan_mobile.c
<https://reviewboard.asterisk.org/r/908/#comment5966>

    Same as above.



/trunk/addons/chan_mobile.c
<https://reviewboard.asterisk.org/r/908/#comment5972>

    There should be no space between '*' and 'charset'.



/trunk/addons/chan_mobile.c
<https://reviewboard.asterisk.org/r/908/#comment5974>

    Missing spaces here  Also you need to make sure msg exists as well.  Should be:
    
    } else if (msg && msg->expected == AT_CMGL_TEST) {



/trunk/addons/chan_mobile.c
<https://reviewboard.asterisk.org/r/908/#comment5975>

    These should be debug messages.



/trunk/addons/chan_mobile.c
<https://reviewboard.asterisk.org/r/908/#comment5976>

    Spaces should be between function arguments.



/trunk/addons/chan_mobile.c
<https://reviewboard.asterisk.org/r/908/#comment5977>

    There are better ways to wait for initialization.  You could simply start the thread once initialization has completed.



/trunk/addons/chan_mobile.c
<https://reviewboard.asterisk.org/r/908/#comment5978>

    This should be a debug message.



/trunk/addons/chan_mobile.c
<https://reviewboard.asterisk.org/r/908/#comment5979>

    Don't use c++ style comments.



/trunk/addons/chan_mobile.c
<https://reviewboard.asterisk.org/r/908/#comment5980>

    Spaces should be between function arguments.


- Matthew


On 2010-09-04 05:08:20, krafte wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/908/
> -----------------------------------------------------------
> 
> (Updated 2010-09-04 05:08:20)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch enables polling on phones that do not support SMS indication (AT+CNMI) but support listing of SMS (AT+CMGL). So a lot more mobile phones get SMS support. I also implemented a config variable to set the charset. Polling is done ansyncronously but indication is also ansyncronous so this is no problem.
> 
> 
> Diffs
> -----
> 
>   /trunk/addons/chan_mobile.c 284880 
> 
> Diff: https://reviewboard.asterisk.org/r/908/diff
> 
> 
> Testing
> -------
> 
> tested on trunk and on 1.6.2.11
> tested with Nokia E75 (no polling support), Sony Ericsson K800i (no polling support) and Motorola L6 (polling support)
> 
> 
> Thanks,
> 
> krafte
> 
>