Re: [PATCH] Enhancement to Icera-based ZTE MF192 modem support

[Aleksander Morgado <aleksander lanedo com>]

Hey,

> - Disabling the modem for power savings (using +CFUN=0). Also,
>    the command "Z E0 V1 X4&C1 +CMEE=1;+CFUN=1;" was not properly parsed
>    (the modem just ignores +CMEE=1 and +CFUN=1), so I broke it for two
>    commands. While enabling the modem, it gives error for +CPMS command
>    for the first time:
>
>    (ttyACM0): -->  'AT+CPMS?<CR>'
>    (ttyACM0):<-- '<CR><LF>+CMS ERROR: 517<CR><LF>'
>
>    With +CMEE=2 it produces
>
>    (ttyACM0):<-- '<CR><LF>+CMS ERROR: SM BL not ready<CR><LF>'
>
>    message. The second try for this command is OK. So I added it to
>    cpms_try_done(...) function. I should add a constant for this error,
>    I think.
>
> - USSD requests was not working:
>
>    (ttyACM0): -->  'AT+CUSD=1,"*100#",15<CR>'
>    (ttyACM0):<-- '<CR><LF>ERROR<CR><LF>'
>
>    That is because ModemManager sets UCS2 codepage for modem, so the
>    request should be encoded properly. Here character set should be
>    recognised automatically, but I do not know how to make it. Maybe
>    someone helps me?

Too many different issues in the same email :-) It is much better to 
provide separate patches/emails for different issues, so that they can 
be discussed in different threads and followed separately.

See my replies inline below.

> diff -u a/plugins/mm-modem-zte.c b/plugins/mm-modem-zte.c
> --- a/plugins/mm-modem-zte.c 2011-07-22 00:18:33.000000000 +0400
> +++ b/plugins/mm-modem-zte.c 2012-04-08 14:44:45.253923586 +0400
> @@ -27,6 +27,7 @@
>   #include "mm-modem-simple.h"
>   #include "mm-modem-icera.h"
>   #include "mm-modem-gsm-ussd.h"
> +#include "mm-utils.h"
>
>   static void modem_init (MMModem *modem_class);
>   static void modem_icera_init (MMModemIcera *icera_class);
> @@ -375,7 +376,8 @@
>
>       priv = MM_MODEM_ZTE_GET_PRIVATE (info->modem);
>
> -    if (error&&  g_error_matches (error, MM_MOBILE_ERROR,
> MM_MOBILE_ERROR_SIM_BUSY)) {
> +    if (error&&  (g_error_matches (error, MM_MOBILE_ERROR,
> MM_MOBILE_ERROR_SIM_BUSY) ||
> +    	g_error_matches (error, MM_MOBILE_ERROR, 517))) {

I don't think 517 means the same for every modem out there, so it 
probably isn't worth to define a new symbol for it. A good text comment 
next to it could help, though.

>           if (priv->cpms_tries++<  4) {
>               if (priv->cpms_timeout)
>                   g_source_remove (priv->cpms_timeout);
> @@ -471,7 +473,8 @@
>       } else {
>           /* Finish the initialization */
>           mm_modem_icera_is_icera (MM_MODEM_ICERA (self), icera_check_cb,
> self);
> -        mm_at_serial_port_queue_command (port, "Z E0 V1 X4&C1 +CMEE=1;
> +CFUN=1;", 10, init_modem_done, info);

^Unlucky place for the + of the +CFUN in the diff... :-)

You actually removed the ATZ call altogether; was that on purpose?


> +        mm_at_serial_port_queue_command (port, "+CMEE=1", 2, NULL,
> NULL);
> +        mm_at_serial_port_queue_command (port, "+CFUN=1", 10,
> init_modem_done, info);

It's probably worth to use the default POWER_UP command property for the 
CFUN=1. Although maybe there was a reason to have the CFUN=1 in the 
modem initialization sequence... Dan?

>       }
>   }
>
> @@ -483,7 +486,7 @@
>       if (error)
>           mm_generic_gsm_enable_complete (MM_GENERIC_GSM (info->modem),
> error, info);
>       else
> -        mm_at_serial_port_queue_command (MM_AT_SERIAL_PORT (port), "E0
> V1", 3, pre_init_done, user_data);
> +        mm_at_serial_port_queue_command (MM_AT_SERIAL_PORT (port), "E0
> V1 X4&C1", 3, pre_init_done, user_data);

Could you dig in the git logs to check why the pre-init sequence was 
needed in the ZTE modem? Maybe Dan remembers why. Anyway, you also moved 
"X4&C1" here, was that on purpose?

>   }
>
>   static void
> @@ -562,7 +565,7 @@
>       }
>
>       /* Random command to ensure unsolicited message disable completes
> */
> -    mm_at_serial_port_queue_command (primary, "E0", 5,
> disable_unsolicited_done, info);
> +    mm_at_serial_port_queue_command (primary, "+CFUN=0", 10,
> disable_unsolicited_done, info);

We do have a POWER_DOWN property for that. Are we sure that the 
power-down command will not harm other ZTE modems around?

>   }
>
>   /*****************************************************************************/
> @@ -690,10 +693,15 @@
>   static char*
>   ussd_encode (MMModemGsmUssd *self, const char* command, guint *scheme)
>   {
> +	gsize written = 0;
> +	gchar *ucs2;
>       char *cmd;
> -
> +
>       *scheme = MM_MODEM_GSM_USSD_SCHEME_7BIT;
> -    cmd = g_strdup (command);
> +
> +    ucs2 = g_convert (command, -1, "UCS-2BE", "UTF8", NULL,&written,
> NULL);
> +    cmd = utils_bin2hexstr ((const guint8 *) ucs2, written);
> +    g_free (ucs2);
>
>       return cmd;
>   }

As you say before, the encoded command needs to be in the same charset 
as defined by the modem, so you shouldn't assume it's UCS2 always. This 
would do it probably:

    if (mm_generic_gsm_get_charset (MM_GENERIC_GSM (self)) ==
        MM_MODEM_CHARSET_UCS2)
        cmd = mm_modem_charset_utf8_to_hex (command,
                                            MM_MODEM_CHARSET_UCS2);
    else
        cmd = g_strdup (command);

In git master, though, that would be just calling:

  cmd = mm_broadband_modem_take_and_convert_to_current_charset
            (MM_BROADBAND_MODEM (self),
             g_strdup (command));

Cheers,

--
Aleksander