Re: [MM master] Re: [MM06] [PATCH] sierra: use +CFUN=4 for powering down

[Aleksander Morgado <aleksander lanedo com>]

>> See patch attached.
>>
>> sierra: use +CFUN=4 for powering down
>>
>>  plugins/mm-modem-sierra-gsm.c |   32 ++++++++++++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
> 
> This also needs to be intregrated in the master branch version. Attached
> is my attempt to do it, but it seems not to work. I see no effect of
> disabling the modem with "mmcli -m X -d".

See review below.

> 
> Also, it seems like the power down is not done before the modem enters
> disabled state at startup of MM or after modem is inserted, so I filed a
> bug about that:
> 
> https://bugzilla.gnome.org/show_bug.cgi?id=683681
> 

Yes, that's a known issue, see TODO in the source tree.

Dan, what do you think of this, should we run the whole disabling
sequence, including the power-down command, just after the modem gets
initialized? I would do it, but now sure if that would have any
unexpected complication.

The only problem I could think of is when you want to connect your modem
as soon as it appears in DBus, running the Simple.Connect() command. In
this case you don't really want to go to the low-power mode as you're
going to connect it right away.


> -- 
> Marius
> 
> 
> 0001-sierra-use-CFUN-4-for-powering-down.patch
> 
> 
>>From ac69eb9a08f821efb674f54292ee90707ca81e29 Mon Sep 17 00:00:00 2001
> From: "Marius B. Kotsbak" <marius kotsbak com>
> Date: Sun, 9 Sep 2012 18:06:30 +0200
> Subject: [PATCH] sierra: use +CFUN=4 for powering down
> 
> ---
>  plugins/sierra/mm-broadband-modem-sierra.c |   11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/plugins/sierra/mm-broadband-modem-sierra.c b/plugins/sierra/mm-broadband-modem-sierra.c
> index 49c2e22..c8e8f3e 100644
> --- a/plugins/sierra/mm-broadband-modem-sierra.c
> +++ b/plugins/sierra/mm-broadband-modem-sierra.c
> @@ -527,11 +527,14 @@ modem_power_down (MMIfaceModem *self,
>          return;
>      }
>  
> -    /* For 3GPP modems we should call parent's power down, but there is no
> -     * such power down command in MMBroadbandModem, so just finish here. */
> -    g_simple_async_result_set_op_res_gboolean (result, TRUE);
> -    g_simple_async_result_complete_in_idle (result);
>      g_object_unref (result);
> +
> +    mm_base_modem_at_command (MM_BASE_MODEM (self),
> +                              "+CFUN=4",
> +                              3,
> +                              FALSE,
> +                              callback,
> +                              user_data);
>  }
>  

There are 2 main ways to handle the async command execution when an AT
command is issued:
 1) If only the AT command can be issued, just run
base_modem_at_command() passing callback and user_data. In that case you
don't need the _ready() method and you can just run
base_modem_at_command_finish() in the async method finish().
 2) If the logic of the async command may end up not issuing an AT
command, you need to create a GSimpleAsyncResult, and use a specific
ready() method passed as GAsyncReadyCallback in base_modem_at_command().
In this case, your new ready() method is responsible for calling
base_modem_at_command_finish() and setting the simple async result in
the GSimpleAsyncResult. Your async method finish would then just get the
result or the error from the GSimpleAsyncResult.

Now, before your patch, this async method was using approach 2, as the
async would end up issuing an AT command if it was a CDMA modem, but
just returning in idle if it was a 3GPP modem. After the patch, you're
merging both approaches, which is wrong.

Given that now the async command will always issue an AT command, you
can use the approach to pass the callback/user_data directly in *both*
the at_command() methods instead of providing new custom ready()
methods, modifying the async method finish() as well so that it calls
_at_command_finish() itself.

-- 
Aleksander