Re: [MM 0.6] Plumb up the SPN display-rule bits

[Aleksander Morgado <aleksander lanedo com>]

Hey,

> This is required for properly displaying
> local-operator-vs-home-provider information, per 3GPP
> (31.102 section 4.1.2 on EF_SPN and 22.101 Annex A).
> 

For anyone else checking, that's section 4.2.11 in release 11 of the
document.

> Suggestions welcome on the long and somewhat unwieldy property/function names.
> 
>     - Nathan
> 
> 
> 0001-Retrieve-and-plumb-up-the-SIM-SPN-bits-that-set-oper.patch
> 
> 
> From 96ce78202344bd928bf602874a087a0adb383cbe Mon Sep 17 00:00:00 2001
> From: Nathan Williams <njw chromium org>
> Date: Wed, 29 Feb 2012 16:25:03 -0500
> Subject: [PATCH] Retrieve and plumb up the SIM SPN bits that set operator
>  name display rules.
> 
> BUG=chromium-os:21522
> TEST=Insert SIMs from various operators (T-Mobile, MaxROAM), run mmcli -i 0
> Varying conditions include no SPN (AT&T), all-FF SPN data (3 Italy),
> SPN with neither display bit set (3 UK), SPN with a display bit set (MaxROAM)
> 
> Change-Id: Id83da0e094efe52922dcc21f1420d3f5db5883e3
> ---
>  cli/mmcli-sim.c                                    |   14 ++++--
>  .../org.freedesktop.ModemManager1.Sim.xml          |   17 +++++++
>  libmm-glib/mm-sim.c                                |   30 ++++++++++++
>  libmm-glib/mm-sim.h                                |    2 +
>  src/mm-sim.c                                       |   49 ++++++++++++++++++-
>  src/mm-sim.h                                       |    2 +
>  6 files changed, 106 insertions(+), 8 deletions(-)
> 
> diff --git a/cli/mmcli-sim.c b/cli/mmcli-sim.c
> index aa39a91..89ceeed 100644
> --- a/cli/mmcli-sim.c
> +++ b/cli/mmcli-sim.c
> @@ -161,14 +161,18 @@ print_sim_info (MMSim *sim)
>      g_print ("SIM '%s'\n",
>               mm_sim_get_path (sim));
>      g_print ("  -------------------------\n"
> -             "  Properties |          imsi : '%s'\n"
> -             "             |            id : '%s'\n"
> -             "             |   operator id : '%s'\n"
> -             "             | operator name : '%s'\n",
> +             "  Properties |                 imsi : '%s'\n"
> +             "             |                   id : '%s'\n"
> +             "             |          operator id : '%s'\n"
> +             "             |        operator name : '%s'\n"
> +             "             |    show PLMN at home : %s\n"
> +             "             | show op name roaming : %s\n",
>               VALIDATE (mm_sim_get_imsi (sim)),
>               VALIDATE (mm_sim_get_identifier (sim)),
>               VALIDATE (mm_sim_get_operator_identifier (sim)),
> -             VALIDATE (mm_sim_get_operator_name (sim)));
> +             VALIDATE (mm_sim_get_operator_name (sim)),
> +             mm_sim_get_display_registered_network_name_at_home (sim) ? "yes" : "no",
> +             mm_sim_get_display_operator_name_while_roaming (sim) ? "yes" : "no");
>  }
>  
>  static void
> diff --git a/introspection/org.freedesktop.ModemManager1.Sim.xml b/introspection/org.freedesktop.ModemManager1.Sim.xml
> index 14f9077..5833838 100644
> --- a/introspection/org.freedesktop.ModemManager1.Sim.xml
> +++ b/introspection/org.freedesktop.ModemManager1.Sim.xml
> @@ -99,5 +99,22 @@
>      -->
>      <property name="OperatorName" type="s" access="read" />
>  
> +    <!--
> +       DisplayRegisteredNetworkNameAtHome
> +
> +       If true, the user interface is required to display the name of
> +       the registered PLMN while registered on a home network (non-roaming).
> +    -->
> +    <property name="DisplayRegisteredNetworkNameAtHome" type="b" access="read" />


Quoting from the TS:
"b1=0: display of registered PLMN name not required when registered PLMN
is either HPLMN or a PLMN in the service provider PLMN list.
b1=1: display of registered PLMN name required when registered PLMN is
either HPLMN or a PLMN in the service provider PLMN list."

I don't think that "DisplayRegisteredNetworkNameAtHome" covers
everything the property is meant to say. Actually, it hides the main
purpose of the property, which is the fact that the registered PLMN name
should be displayed even if the network is not the home network, but a
network allowed in the PLMN list. E.g. I have a card from the 'Yoigo'
operator, and when using 2G my phone connects to the 'Movistar'network:
but my phone still needs to display 'Yoigo' instead of 'Movistar'. The
fact that the device should display the name of the registered PLMN when
in the same home PLMN is quite obvious already.

But, I can't think of a better or shorter name...
"DisplayRegisteredNetworkNameAtHomeOrAllowed" is even longer... :-)


> +
> +    <!--
> +       DisplayOperatorNameWhileRoaming
> +
> +       If true, the user interface is required to display the operator
> +       name from the SIM (the OperatorName) when registered on a
> +       non-home network (roaming).
> +    -->
> +    <property name="DisplayOperatorNameWhileRoaming" type="b" access="read" />
> +

Quoting from the TS:
"b2=0: display of the service provider name is required when registered
PLMN is neither HPLMN nor a PLMN in the service provider PLMN list.
b2=1: display of the service provider name is not required when
registered PLMN is neither HPLMN nor a PLMN in the service provider PLMN
list."

The name "DisplayOperatorNameWhileRoaming" is also not fully clear.
'Roaming' means for the modem/phone 'not in the home network', and that
covers both being in a network allowed in the list stored in the SIM
card, or really roaming in another not-listed network. Following my
example before, when I connect to the Movistar network with a Yoigo SIM
card, I will be reported as being roaming, even if the network is one of
the allowed networks in the list stored in the SIM card.

But again, I cannot thing of a better name without the WhileRoaming
part... "DisplaySpnWhenNotAtHomeOrAllowed" is even longer.

>    </interface>
>  </node>
> diff --git a/libmm-glib/mm-sim.c b/libmm-glib/mm-sim.c
> index 40bb93b..0645ef4 100644
> --- a/libmm-glib/mm-sim.c
> +++ b/libmm-glib/mm-sim.c
> @@ -207,6 +207,36 @@ mm_sim_dup_operator_name (MMSim *self)
>  }
>  
>  /**
> + * mm_sim_get_display_registered_network_name_at_home:
> + * @self: A #MMSim.
> + *
> + * Returns: %TRUE if the name of the registered network must be
> + * displayed while registered on a home network, %FALSE otherwide.
> + */
> +gboolean
> +mm_sim_get_display_registered_network_name_at_home (MMSim *self)
> +{
> +    g_return_val_if_fail (MM_GDBUS_IS_SIM (self), FALSE);
> +
> +    return mm_gdbus_sim_get_display_registered_network_name_at_home (self);
> +}
> +
> +/**
> + * mm_sim_get_display_operator_name_while_roaming:
> + * @self: A #MMSim.
> + *
> + * Returns: %TRUE if the operator name from the SIM must be displayed
> + * while registered on a non-home network, %FALSE otherwide.
> + */
> +gboolean
> +mm_sim_get_display_operator_name_while_roaming (MMSim *self)
> +{
> +    g_return_val_if_fail (MM_GDBUS_IS_SIM (self), FALSE);
> +
> +    return mm_gdbus_sim_get_display_operator_name_while_roaming (self);
> +}
> +
> +/**
>   * mm_sim_send_pin:
>   * @self: A #MMSim.
>   * @pin: The PIN code.
> diff --git a/libmm-glib/mm-sim.h b/libmm-glib/mm-sim.h
> index 7301434..4205b9b 100644
> --- a/libmm-glib/mm-sim.h
> +++ b/libmm-glib/mm-sim.h
> @@ -38,6 +38,8 @@ const gchar *mm_sim_get_identifier          (MMSim *self);
>  const gchar *mm_sim_get_imsi                (MMSim *self);
>  const gchar *mm_sim_get_operator_identifier (MMSim *self);
>  const gchar *mm_sim_get_operator_name       (MMSim *self);
> +gboolean mm_sim_get_display_registered_network_name_at_home (MMSim *self);
> +gboolean mm_sim_get_display_operator_name_while_roaming     (MMSim *self);
>  
>  gchar *mm_sim_dup_path                (MMSim *self);
>  gchar *mm_sim_dup_identifier          (MMSim *self);
> diff --git a/src/mm-sim.c b/src/mm-sim.c
> index 129584e..e468be7 100644
> --- a/src/mm-sim.c
> +++ b/src/mm-sim.c
> @@ -1212,6 +1212,8 @@ load_operator_identifier (MMSim *self,
>  
>  static gchar *
>  parse_spn (const gchar *response,
> +           gboolean *display_reg_home,
> +           gboolean *display_op_roaming,
>             GError **error)
>  {
>      gint sw1;
> @@ -1268,7 +1270,16 @@ parse_spn (const gchar *response,
>          while (buflen > 1 && bin[buflen - 1] == (char)0xff)
>              buflen--;
>  
> -        /* First byte is metadata; remainder is GSM-7 unpacked into octets; convert to UTF8 */
> +        /* Bits in the first byte specify rules for name display */
> +        if (bin[0] == '\xFF' && bin[1] == '\xFF') {
> +            /* If we got all-FF, don't enable display */
> +            *display_reg_home = FALSE;
> +            *display_op_roaming = FALSE;
> +        } else {
> +            *display_reg_home = ((bin[0] & 1) == 1);
> +            *display_op_roaming = ((bin[0] & 2) == 2);

You sure this is not the inverse? The documentation says that "b2=0"
means "required" and "b1=0" means "not required".


> +        }
> +        /* Remainder is GSM-7 unpacked into octets; convert to UTF8 */
>          utf8 = (gchar *)mm_charset_gsm_unpacked_to_utf8 ((guint8 *)bin + 1, buflen - 1);
>          g_free (bin);
>          return utf8;
> @@ -1285,6 +1296,8 @@ parse_spn (const gchar *response,
>  static gchar *
>  load_operator_name_finish (MMSim *self,
>                             GAsyncResult *res,
> +                           gboolean *display_reg_home,
> +                           gboolean *display_op_roaming,
>                             GError **error)
>  {
>      const gchar *result;
> @@ -1293,7 +1306,7 @@ load_operator_name_finish (MMSim *self,
>          return NULL;
>      result = g_simple_async_result_get_op_res_gpointer (G_SIMPLE_ASYNC_RESULT (res));
>  
> -    return parse_spn (result, error);
> +    return parse_spn (result, display_reg_home, display_op_roaming, error);
>  }
>  
>  STR_REPLY_READY_FN (load_operator_name)
> @@ -1418,6 +1431,37 @@ load_sim_identifier_ready (MMSim *self,
>      interface_initialization_step (ctx);
>  }
>  
> +static void
> +load_operator_name_ready (MMSim *self,
> +                          GAsyncResult *res,
> +                          InitAsyncContext *ctx)
> +{
> +    GError *error = NULL;
> +    gchar *val;
> +    gboolean home, roaming;
> +
> +    val = MM_SIM_GET_CLASS (ctx->self)->load_operator_name_finish (self,
> +                                                                   res,
> +                                                                   &home,
> +                                                                   &roaming,
> +                                                                   &error);
> +    mm_gdbus_sim_set_operator_name (MM_GDBUS_SIM (self), val);
> +    mm_gdbus_sim_set_display_registered_network_name_at_home (MM_GDBUS_SIM (self),
> +                                                              home);
> +    mm_gdbus_sim_set_display_operator_name_while_roaming (MM_GDBUS_SIM (self),
> +                                                          roaming);
> +    g_free (val);
> +
> +    if (error) {
> +        mm_warn ("couldn't load operator name: '%s'", error->message);
> +        g_error_free (error);
> +    }
> +
> +    /* Go on to next step */
> +    ctx->step++;
> +    interface_initialization_step (ctx);
> +}
> +
>  #undef STR_REPLY_READY_FN
>  #define STR_REPLY_READY_FN(NAME,DISPLAY)                                \
>      static void                                                         \
> @@ -1444,7 +1488,6 @@ load_sim_identifier_ready (MMSim *self,
>  
>  STR_REPLY_READY_FN (imsi, "IMSI")
>  STR_REPLY_READY_FN (operator_identifier, "Operator identifier")
> -STR_REPLY_READY_FN (operator_name, "Operator name")
>  
>  static void
>  interface_initialization_step (InitAsyncContext *ctx)
> diff --git a/src/mm-sim.h b/src/mm-sim.h
> index cba15c0..09c470d 100644
> --- a/src/mm-sim.h
> +++ b/src/mm-sim.h
> @@ -77,6 +77,8 @@ struct _MMSimClass {
>                                   gpointer user_data);
>      gchar * (* load_operator_name_finish) (MMSim *self,
>                                             GAsyncResult *res,
> +                                           gboolean *home_display,
> +                                           gboolean *roam_display,
>                                             GError **error);
>  
>      /* Change PIN (async) */


-- 
Aleksander