[PATCH] core: prefer g_task_return_error_if_cancelled() than custom errors

[Aleksander Morgado]

When the GCancellable is added to the GTask, we can use a single
method call to check for the task being cancelled, and complete it
right away if so.

This patch also clears up the logic in the Novatel plugin, where the
code was trying to return "TRUE" when the task was cancelled, but
wouldn't work as the check-cancellable flag in the GTask is TRUE by
default (i.e. when completing the GTask, if it was cancelled, a
G_IO_ERROR_CANCELLED would be returned by default, regardless of any
other return value set).

This patch also introduces a small variation of the logic in the
Cinterion plugin: instead of running SWWAN=0 before completing the
async action, the command is now sent just after completion of the
async action. This shouldn't be an issue, as the SWWAN result itself
is ignored.
---

Hey Ben,

What do you think of these changes? I believe we should start using this where 
applicable, instead of the custom checks and errors.

---
 plugins/cinterion/mm-broadband-bearer-cinterion.c | 7 ++-
 plugins/novatel/mm-common-novatel.c   | 3 +--
 src/mm-auth-provider-polkit.c | 6 +-
 src/mm-iface-modem-signal.c   | 6 +-
 4 files changed, 5 insertions(+), 17 deletions(-)

diff --git a/plugins/cinterion/mm-broadband-bearer-cinterion.c 
b/plugins/cinterion/mm-broadband-bearer-cinterion.c
index 741d2935..a1c699e9 100644
--- a/plugins/cinterion/mm-broadband-bearer-cinterion.c
+++ b/plugins/cinterion/mm-broadband-bearer-cinterion.c
@@ -343,10 +343,6 @@ handle_cancel_dial (GTask *task)
NULL,
NULL);
 g_free (command);
-
-g_task_return_new_error (task, MM_CORE_ERROR, MM_CORE_ERROR_CANCELLED,
- "Connection operation has been cancelled");
-g_object_unref (task);
 }

 static void
@@ -357,8 +353,9 @@ dial_3gpp_context_step (GTask *task)
 ctx = (Dial3gppContext *) g_task_get_task_data (task);

 /* Check for cancellation */
-if (g_cancellable_is_cancelled (g_task_get_cancellable (task))) {
+if (g_task_return_error_if_cancelled (task)) {
 handle_cancel_dial (task);
+g_object_unref (task);
 return;
 }

diff --git a/plugins/novatel/mm-common-novatel.c 
b/plugins/novatel/mm-common-novatel.c
index 96d845f1..f8c78f51 100644
--- a/plugins/novatel/mm-common-novatel.c
+++ b/plugins/novatel/mm-common-novatel.c
@@ -87,10 +87,9 @@ custom_init_step (GTask *task)
 ctx = g_task_get_task_data (task);

 /* If cancelled, end */
-if (g_cancellable_is_cancelled (g_task_get_cancellable (task))) {
+if (g_task_return_error_if_cancelled (task)) {
 mm_dbg ("(Novatel) no need to keep on running custom init in (%s)",
 mm_port_get_device (MM_PORT (ctx->port)));
-g_task_return_boolean (task, TRUE);
 g_object_unref (task);
 return;
 }
diff --git a/src/mm-auth-provider-polkit.c b/src/mm-auth-provider-polkit.c
index 3b9b4250..7e88d67c 100644
--- a/src/mm-auth-provider-polkit.c
+++ b/src/mm-auth-provider-polkit.c
@@ -72,11 +72,7 @@ check_authorization_ready (PolkitAuthority *authority,
 GError *error = NULL;
 AuthorizeContext *ctx;

-if (g_cancellable_is_cancelled (g_task_get_cancellable (task))) {
-g_task_return_new_error (task,
- MM_CORE_ERROR,
- MM_CORE_ERROR_CANCELLED,
- "PolicyKit authorization attempt cancelled");
+if (g_task_return_error_if_cancelled (task)) {
 g_object_unref (task);
 return;
 }
diff --git a/src/mm-iface-modem-signal.c b/src/mm-iface-modem-signal.c
index 9751b5d6..99096a95 100644
--- a/src/mm-iface-modem-signal.c
+++ b/src/mm-iface-modem-signal.c
@@ -418,11 +418,7 @@ interface_initialization_step (GTask *task)
 InitializationContext *ctx;

 /* Don't run new steps if we're cancelled */
-if (g_cancellable_is_cancelled (g_task_get_cancellable (task))) {
-g_task_return_new_error (task,
- MM_CORE_ERROR,
- MM_CORE_ERROR_CANCELLED,
- "Interface initialization cancelled");
+if (g_task_return_error_if_cancelled (task)) {
 g_object_unref (task);
 return;
 }
--
2.12.2
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: [PATCH 2/3] iface-modem-signal: port mm_iface_modem_signal_enable to use GTask

[Aleksander Morgado]

On 13/04/17 03:52, Ben Chan wrote:
> ---
>  src/mm-iface-modem-signal.c | 16 ++--
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 

Pushed to git master, thanks.

> diff --git a/src/mm-iface-modem-signal.c b/src/mm-iface-modem-signal.c
> index fdfdc7e0..f8348831 100644
> --- a/src/mm-iface-modem-signal.c
> +++ b/src/mm-iface-modem-signal.c
> @@ -330,7 +330,7 @@ mm_iface_modem_signal_enable_finish (MMIfaceModemSignal 
> *self,
>   GAsyncResult *res,
>   GError **error)
>  {
> -return !g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT 
> (res), error);
> +return g_task_propagate_boolean (G_TASK (res), error);
>  }
>  
>  void
> @@ -339,21 +339,17 @@ mm_iface_modem_signal_enable (MMIfaceModemSignal *self,
>GAsyncReadyCallback callback,
>gpointer user_data)
>  {
> -GSimpleAsyncResult *result;
> +GTask *task;
>  GError *error = NULL;
>  
> -result = g_simple_async_result_new (G_OBJECT (self),
> -callback,
> -user_data,
> -mm_iface_modem_signal_enable);
> +task = g_task_new (self, cancellable, callback, user_data);
>  
>  if (!setup_refresh_context (self, FALSE, 0, ))
> -g_simple_async_result_take_error (result, error);
> +g_task_return_error (task, error);
>  else
> -g_simple_async_result_set_op_res_gboolean (result, TRUE);
> +g_task_return_boolean (task, TRUE);
>  
> -g_simple_async_result_complete_in_idle (result);
> -g_object_unref (result);
> +g_object_unref (task);
>  }
>  
>  
> /*/
> 


-- 
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: [PATCH 1/3] iface-modem-signal: port mm_modem_iface_signal_disable to use GTask

[Aleksander Morgado]

On 13/04/17 03:52, Ben Chan wrote:
> ---
>  src/mm-iface-modem-signal.c | 15 +--
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 

Pushed to git master, thanks.

> diff --git a/src/mm-iface-modem-signal.c b/src/mm-iface-modem-signal.c
> index dbf11515..fdfdc7e0 100644
> --- a/src/mm-iface-modem-signal.c
> +++ b/src/mm-iface-modem-signal.c
> @@ -306,7 +306,7 @@ mm_iface_modem_signal_disable_finish (MMIfaceModemSignal 
> *self,
>GAsyncResult *res,
>GError **error)
>  {
> -return !g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT 
> (res), error);
> +return g_task_propagate_boolean (G_TASK (res), error);
>  }
>  
>  void
> @@ -314,18 +314,13 @@ mm_iface_modem_signal_disable (MMIfaceModemSignal *self,
> GAsyncReadyCallback callback,
> gpointer user_data)
>  {
> -GSimpleAsyncResult *result;
> -
> -result = g_simple_async_result_new (G_OBJECT (self),
> -callback,
> -user_data,
> -mm_iface_modem_signal_disable);
> +GTask *task;
>  
>  teardown_refresh_context (self);
>  
> -g_simple_async_result_set_op_res_gboolean (result, TRUE);
> -g_simple_async_result_complete_in_idle (result);
> -g_object_unref (result);
> +task = g_task_new (self, NULL, callback, user_data);
> +g_task_return_boolean (task, TRUE);
> +g_object_unref (task);
>  }
>  
>  
> /*/
> 


-- 
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: [PATCH v3] telit: unsupported CSIM lock should not skip loading unlock retries

[Aleksander Morgado]

On 13/04/17 11:53, Carlo Lobrano wrote:
> Some modems do not support CSIM lock/unlock, but they do support
> querying SIM unlock retries through +CSIM command.
> 
> If CSIM lock returns with "unsupported command" do not propagate
> the error and continue with the other CSIM queries instead, moreover the
> CSIM lock feature is signed as FEATURE_UNSUPPORTED in Telit modem
> private structure to prevent being sent again (e.g. calling CSIM
> unlock AT command).
> ---
> 
> Fixed csim_lock_support never set to FEATURE_SUPPORTED
> 
> ---

Pushed to git master, thanks!

>  plugins/telit/mm-broadband-modem-telit.c | 94 
> ++--
>  plugins/telit/mm-broadband-modem-telit.h |  2 +
>  2 files changed, 80 insertions(+), 16 deletions(-)
> 
> diff --git a/plugins/telit/mm-broadband-modem-telit.c 
> b/plugins/telit/mm-broadband-modem-telit.c
> index cce0229..0d8a34b 100644
> --- a/plugins/telit/mm-broadband-modem-telit.c
> +++ b/plugins/telit/mm-broadband-modem-telit.c
> @@ -42,6 +42,15 @@ G_DEFINE_TYPE_EXTENDED (MMBroadbandModemTelit, 
> mm_broadband_modem_telit, MM_TYPE
>  G_IMPLEMENT_INTERFACE (MM_TYPE_IFACE_MODEM, 
> iface_modem_init)
>  G_IMPLEMENT_INTERFACE (MM_TYPE_IFACE_MODEM_3GPP, 
> iface_modem_3gpp_init));
>  
> +typedef enum {
> +FEATURE_SUPPORT_UNKNOWN,
> +FEATURE_NOT_SUPPORTED,
> +FEATURE_SUPPORTED
> +} FeatureSupport;
> +
> +struct _MMBroadbandModemTelitPrivate {
> +FeatureSupport csim_lock_support;
> +};
>  
>  
> /*/
>  /* After Sim Unlock (Modem interface) */
> @@ -521,10 +530,19 @@ csim_unlock_ready (MMBaseModem  *self,
>  /* Ignore errors */
>  response = mm_base_modem_at_command_finish (self, res, );
>  if (!response) {
> +if (g_error_matches (error,
> + MM_MOBILE_EQUIPMENT_ERROR,
> + MM_MOBILE_EQUIPMENT_ERROR_NOT_SUPPORTED)) {
> +ctx->self->priv->csim_lock_support = FEATURE_NOT_SUPPORTED;
> +}
>  mm_warn ("Couldn't unlock SIM card: %s", error->message);
>  g_error_free (error);
>  }
>  
> +if (ctx->self->priv->csim_lock_support != FEATURE_NOT_SUPPORTED) {
> +ctx->self->priv->csim_lock_support = FEATURE_SUPPORTED;
> +}
> +
>  ctx->step++;
>  load_unlock_retries_step (ctx);
>  }
> @@ -591,10 +609,22 @@ csim_lock_ready (MMBaseModem  *self,
>  
>  response = mm_base_modem_at_command_finish (self, res, );
>  if (!response) {
> -g_prefix_error (, "Couldn't lock SIM card: ");
> -g_simple_async_result_take_error (ctx->result, error);
> -load_unlock_retries_context_complete_and_free (ctx);
> -return;
> +if (g_error_matches (error,
> + MM_MOBILE_EQUIPMENT_ERROR,
> + MM_MOBILE_EQUIPMENT_ERROR_NOT_SUPPORTED)) {
> +ctx->self->priv->csim_lock_support = FEATURE_NOT_SUPPORTED;
> +mm_warn ("Couldn't lock SIM card: %s. Continuing without CSIM 
> lock.", error->message);
> +g_error_free (error);
> +} else {
> +g_prefix_error (, "Couldn't lock SIM card: ");
> +g_simple_async_result_take_error (ctx->result, error);
> +load_unlock_retries_context_complete_and_free (ctx);
> +return;
> +}
> +}
> +
> +if (ctx->self->priv->csim_lock_support != FEATURE_NOT_SUPPORTED) {
> +ctx->self->priv->csim_lock_support = FEATURE_SUPPORTED;
>  }
>  
>  ctx->step++;
> @@ -602,6 +632,40 @@ csim_lock_ready (MMBaseModem  *self,
>  }
>  
>  static void
> +handle_csim_locking (LoadUnlockRetriesContext *ctx, gboolean is_lock)
> +{
> +switch (ctx->self->priv->csim_lock_support) {
> +case FEATURE_SUPPORT_UNKNOWN:
> +case FEATURE_SUPPORTED:
> +if (is_lock) {
> +mm_base_modem_at_command (MM_BASE_MODEM (ctx->self),
> +  CSIM_LOCK_STR,
> +  CSIM_QUERY_TIMEOUT,
> +  FALSE,
> +  (GAsyncReadyCallback) 
> csim_lock_ready,
> +  ctx);
> +} else {
> +mm_base_modem_at_command (MM_BASE_MODEM (ctx->self),
> +  CSIM_UNLOCK_STR,
> +  CSIM_QUERY_TIMEOUT,
> +  FALSE,
> +  (GAsyncReadyCallback) 
> csim_unlock_ready,
> +  ctx);
> +}
> +break;
> +case FEATURE_NOT_SUPPORTED:
> +mm_dbg ("CSIM lock not supported by this modem. Skipping %s 
> command",
> +is_lock ? "lock" : "unlock");

Re: [review] dcbw/qcdm-cleanups

[Aleksander Morgado]

Hey Dan,

On 10/04/17 17:50, Dan Williams wrote:
> Port some stuff over to GTask, clean up some QCDM stuff, and also make
> sure QCDM code opens the port when it needs to instead of assuming
> there's a port already open.  That's necessary for Huawei DDSETEX voice
> support.
> 

I've reviewed these patches:

3297fd9b broadband-modem: open QCDM port for CDMA Serving System
checking when needed
5cfa8ccc broadband-modem: open QCDM port for Call Manager state checking
when needed
2168fb1d broadband-modem: open QCDM port for HDR state checking when needed
9b71cdd7 broadband-modem: open QCDM port for access tech checking when
needed
cb578c72 broadband-modem: open QCDM port for signal checking when needed
764cb5f9 broadband-modem-novatel: $NWRSSI can report lower values than -115
7c64a894 broadband-modem-novatel: clean up detailed registration state
handling
a8b846df broadband-modem-novatel: clean up access technology reporting

They all look good to me, feel free to merge them.

Some comments:

 * Why don't we setup a pair of "mm_base_modem_qcdm_command()" and
""mm_base_modem_qcdm_command_full()" methods? The former would
automatically peek a QCDM port, open it, and run the QCDM command on it.
The _full() version would get as input a MMPortSerialQcdm directly. This
would be similar to what's done for AT commands, and could probably
simplify the logic as there wouldn't be any need to explicitly
open/close the port at each QCDM command implemented.

 * I was thinking that it could make sense to have a common
"task_serial_port_cleanup" method instead of the three
hdr_state_cleanup_port(), cm_state_cleanup_port() and
cdma1x_serving_system_state_cleanup_port(), but it probably isn't worth
it, unless there are many more cases where it would apply.


-- 
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: MC7455 location support

[Karoly Pados]

Hi!

Sorry for the late reply, we have a 4-day holiday here and I spend less time at 
the computer.

>> But it does appear to support the LOC service (location) which is a
>> newer service that ModemManager (and libqmi) doesn't yet support. So I
>> think that's the current problem.
> 
> Ouch :/ Patches welcome!! :)

I can look into it, given there is some description or specification of the loc 
service protocol I can use. Do you have any pointers? Unfortunately I don't 
think I can allow myself the time to do full reverse engineering of the modem 
communication, so some doc about the QMI messages for the loc service would be 
nice (I'm not even familiar with QMI itself). 

LG,
Károly Pados

tec4data
finding of facts!
thoerl 35; 8621 thoerl; austria
mob +43 680 5577 310
uid atu44131505
mail k...@tec4data.at
http://www.tec4data.at

This email and any associated attachment are confidential.
Unless you belong to the specified recipients, immediately notify the sender.
The content may not be copied and given to third parties.
___

April 15, 2017 12:11 AM, "Aleksander Morgado"  wrote:

> On Thu, Apr 13, 2017 at 8:30 PM, Dan Williams  wrote:
> 
 also, if you have ModemManager debug logs, that would be great to
 see
 too so we can make sure things are being set up correctly.
>>> 
>>> Attached. Also output of mmcli -m 0 in separate file.
>> 
>> Thanks. This shows your device is in fact in QMI mode, and that the
>> firmware doesn't implement the "PDS" (Position Determination Service)
>> that is normally used for this.
>> 
>> But it does appear to support the LOC service (location) which is a
>> newer service that ModemManager (and libqmi) doesn't yet support. So I
>> think that's the current problem.
> 
> Ouch :/ Patches welcome!! :)
> 
> --
> Aleksander
> https://aleksander.es
> ___
> ModemManager-devel mailing list
> ModemManager-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: [PATCH 1/2] modem-helpers: if operator not in UCS2, see if already valid UTF-8

[Colin Helliwell]

> On 14 April 2017 at 23:15 Aleksander Morgado  wrote:
> 
> On Fri, Apr 14, 2017 at 4:57 PM, Colin Helliwell
> 
>  wrote:
> 
> > Might have hung this off the wrong thread, but it's along the same lines.
> > I've just noticed that whilst the mmcli status now decodes the operator, 
> > the --sim status doesn't:
> > 
> > # mmcli -m 0
> > ...
> >  3GPP | imei: '358606050452806'
> >  | enabled locks: 'none'
> >  | operator id: '23415'
> >  | operator name: 'vodafone UK'
> >  | subscription: 'unknown'
> > 
> > | registration: 'home'
> > 
> > -
> > ...
> > 
> > # mmcli -m 0 --sim=0
> > 
> > SIM '/org/freedesktop/ModemManager1/SIM/0'
> > 
> > -
> >  Properties | imsi : '234159108784146'
> >  | id : '89441000301658713096'
> >  | operator id : '23415'
> >  | operator name : 'unknown'
> 
> Could you gather debug logs so that we can see whether this is due to
> the string being wrongly decoded or just not provided by the SIM?
> 

Yep, it was something I spotted late on yesterday and fired in in case it was a 
quicky. 
At first I thought they both would have been coming from the same source 
command/response, though when I looked at the code quickly it was seeming 
perhaps not (which your comment also suggests).
 - I'll try to dig into it further today.
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel