Re: ModemManager: Allow plugin to specify custom replies (was: ModemManager: Handle partial replies in AT+CPIN? (READY without OK))

[Dan Williams <dcbw redhat com>]

On Wed, 2011-03-30 at 20:12 +0200, Aleksander Morgado wrote:
> Hi all,
> 
> > So we have this "AirLink Fastrack Xtend" Sierra Wireless modem [1],
> > ModemManager uses the generic GSM plugin to handle it.
> > 
> > On PIN check the following flow happens:
> >   --> 'AT+CPIN?'
> >   <-- '<CR><LF>+CPIN: READY<CR><LF>'
> > (see [2] for the logs reported by Thomas Bechtold a while ago).
> > 
> > Once that is received, response is parsed but no successful response is
> > found, as there is no "<CR><LF>OK<CR><LF>" after the READY [3]. Thus,
> > ModemManager will wait for more data to come, and ends up timing out
> > ("Serial command timed out").
> > 
> > Once timed out, pin_check_done() gets called in mm-generic-gsm.c, where
> > both the response GString (with the partial reply) and the GError are
> > set. If a GError is set in pin_check_done(), the contents of the GString
> > are fully discarded, and therefore, the pin check is retried up to 3
> > times and never succeeds.
> > 
> > Now, what would be the proper way to handle this situation? One option
> > is to make the OK on the reply to AT+CPIN? optional, by parsing partial
> > replies. See attached patch.
> > 
> > Another option, I guess, is to write a new plugin to handle this
> > specific case. Maybe there is some other plugin handling it?
> > 
> 
> So, in order to handle this specific case of AT+CPIN? not replying OK,
> instead of waiting for the timeout and processing partial replies, which
> is quite a dirty hack, it's definitely better to instead allow specific
> plugins to set their own expected responses.
> 
> The plugin for the modem with this AT+CPIN? reply issue could then
> enable a regex like this as valid expected response:
>         "\\r\\n\\+CPIN: .*\\r\\n"
> 
> The attached patch implements a new method for this purpose:
> 
>         void mm_serial_parser_v1_set_custom_regex (gpointer data,
>                                                    GRegex *successful,
>                                                    GRegex *error);
> 
> This method allows setting custom regex expressions for successful and
> error expected responses.
> 
> The plugin then would need to implement grab_port() and as soon as the
> port is created, set a new specific response parser with an extended v1
> parser, something like this:
> 
>     port = mm_generic_gsm_grab_port (gsm, subsys, name, ptype, error);
>     if (port && MM_IS_AT_SERIAL_PORT (port)) {
>         gpointer parser;
>         GRegex *regex;
> 
>         parser = mm_serial_parser_v1_new ();
> 
>         /* AT+CPIN? replies will never have an OK appended */
>         regex = g_regex_new ("\\r\\n\\+CPIN: .*\\r\\n",
>                              G_REGEX_RAW | G_REGEX_OPTIMIZE,
>                              0, NULL);
>         mm_serial_parser_v1_set_custom_regex (parser, regex, NULL);
>         g_regex_unref (regex);
> 
>         mm_at_serial_port_set_response_parser (
>             MM_AT_SERIAL_PORT (port),
>             mm_serial_parser_v1_parse,
>             parser,
>             mm_serial_parser_v1_destroy);
>     }
> 
> 
> Comments?

Looks fine to me, pushed, thanks!

Dan