[MM][PATCH] modem: add support for 3GPP Vendor PCO info
Aleksander Morgado
aleksander at lanedo.com
Mon Jul 1 23:33:45 PDT 2013
Hey Ori,
See comments below.
On 02/07/13 07:27, ori inbar wrote:
> From: ori inbar <ori.inbar at altair-semi.com>
>
> Change-Id: I139802238868e9743390906c1cfc77fef9179599
> ---
> ...g.freedesktop.ModemManager1.Modem.Modem3gpp.xml | 17 +++
> libmm-glib/mm-simple-status.c | 26 +++-
> libmm-glib/mm-simple-status.h | 1 +
> src/mm-broadband-bearer.c | 10 ++
> src/mm-iface-modem-3gpp.c | 145 +++++++++++++++++++--
> src/mm-iface-modem-3gpp.h | 18 +++
> 6 files changed, 206 insertions(+), 11 deletions(-)
>
> diff --git a/introspection/org.freedesktop.ModemManager1.Modem.Modem3gpp.xml b/introspection/org.freedesktop.ModemManager1.Modem.Modem3gpp.xml
> index 83e1499..14b5e02 100644
> --- a/introspection/org.freedesktop.ModemManager1.Modem.Modem3gpp.xml
> +++ b/introspection/org.freedesktop.ModemManager1.Modem.Modem3gpp.xml
> @@ -136,5 +136,22 @@
> -->
> <property name="EnabledFacilityLocks" type="u" access="read" />
>
> + <!--
> + VendorPcoInfo
> +
> + Vendor-specific protocol configuration option (PCO) info received from the LTE
> + mobile network to which the mobile is currently registered (see 3GPP TS 24.008
> + Section 10.5.6.3).
> +
> + Returned in the format <literal>"PCOIdPCOPayload"</literal>, where
> + <literal>PCOId</literal> is the 4-digit hex-encoded string of PCO ID and
> + <literal>PCOPayload</literal> is the hex-encoded string of the PCO payload data,
> + e.g. <literal>"FF003114800"</literal>.
> +
> + If the <literal>PCOId</literal> and <literal>PCOPayload</literal> are not known
> + or the mobile is not registered to a mobile network, this property will
> + be a zero-length (blank) string.
> + -->
> + <property name="VendorPcoInfo" type="s" access="read" />
> </interface>
> </node>
What is the point of adding this property in the Simple Status info?
Simple Status should have the most commonly used values (e.g. operator
name, code, signal quality...). This PCO info doesn't seem very commonly
used to me; so I would not add it to Simple Status.
> diff --git a/libmm-glib/mm-simple-status.c b/libmm-glib/mm-simple-status.c
> index 10dfee3..0560a34 100644
> --- a/libmm-glib/mm-simple-status.c
> +++ b/libmm-glib/mm-simple-status.c
> @@ -44,6 +44,7 @@ enum {
> PROP_3GPP_REGISTRATION_STATE,
> PROP_3GPP_OPERATOR_CODE,
> PROP_3GPP_OPERATOR_NAME,
> + PROP_3GPP_VENDOR_PCO_INFO,
> PROP_CDMA_CDMA1X_REGISTRATION_STATE,
> PROP_CDMA_EVDO_REGISTRATION_STATE,
> PROP_CDMA_SID,
> @@ -72,6 +73,8 @@ struct _MMSimpleStatusPrivate {
> gchar *modem_3gpp_operator_code;
> /* 3GPP operator name, given only when registered, signature 's' */
> gchar *modem_3gpp_operator_name;
> + /* 3GPP LTE vendor PCO info, given only when registered, signature 's' */
> + gchar *modem_3gpp_vendor_pco_info;
>
> /* <--- From the Modem CDMA interface ---> */
> /* CDMA/CDMA1x registration state, signature 'u' */
> @@ -350,6 +353,11 @@ mm_simple_status_get_dictionary (MMSimpleStatus *self)
> "{sv}",
> MM_SIMPLE_PROPERTY_3GPP_OPERATOR_NAME,
> g_variant_new_string (self->priv->modem_3gpp_operator_name));
> + if (self->priv->modem_3gpp_vendor_pco_info)
> + g_variant_builder_add (&builder,
> + "{sv}",
> + MM_SIMPLE_PROPERTY_3GPP_VENDOR_PCO_INFO,
> + g_variant_new_string (self->priv->modem_3gpp_vendor_pco_info));
>
> if (self->priv->modem_cdma_cdma1x_registration_state != MM_MODEM_CDMA_REGISTRATION_STATE_UNKNOWN) {
> g_variant_builder_add (&builder,
> @@ -423,7 +431,8 @@ mm_simple_status_new_from_dictionary (GVariant *dictionary,
> key, g_variant_get_uint32 (value),
> NULL);
> } else if (g_str_equal (key, MM_SIMPLE_PROPERTY_3GPP_OPERATOR_CODE) ||
> - g_str_equal (key, MM_SIMPLE_PROPERTY_3GPP_OPERATOR_NAME)) {
> + g_str_equal (key, MM_SIMPLE_PROPERTY_3GPP_OPERATOR_NAME) ||
> + g_str_equal (key, MM_SIMPLE_PROPERTY_3GPP_VENDOR_PCO_INFO)) {
> /* string properties */
> g_object_set (properties,
> key, g_variant_get_string (value, NULL),
> @@ -501,6 +510,10 @@ set_property (GObject *object,
> g_free (self->priv->modem_3gpp_operator_code);
> self->priv->modem_3gpp_operator_code = g_value_dup_string (value);
> break;
> + case PROP_3GPP_VENDOR_PCO_INFO:
> + g_free (self->priv->modem_3gpp_vendor_pco_info);
> + self->priv->modem_3gpp_vendor_pco_info = g_value_dup_string (value);
> + break;
> case PROP_3GPP_OPERATOR_NAME:
> g_free (self->priv->modem_3gpp_operator_name);
> self->priv->modem_3gpp_operator_name = g_value_dup_string (value);
> @@ -553,6 +566,9 @@ get_property (GObject *object,
> case PROP_3GPP_OPERATOR_NAME:
> g_value_set_string (value, self->priv->modem_3gpp_operator_name);
> break;
> + case PROP_3GPP_VENDOR_PCO_INFO:
> + g_value_set_string (value, self->priv->modem_3gpp_vendor_pco_info);
> + break;
> case PROP_CDMA_CDMA1X_REGISTRATION_STATE:
> g_value_set_enum (value, self->priv->modem_cdma_cdma1x_registration_state);
> break;
> @@ -678,6 +694,14 @@ mm_simple_status_class_init (MMSimpleStatusClass *klass)
> G_PARAM_READWRITE);
> g_object_class_install_property (object_class, PROP_3GPP_OPERATOR_NAME, properties[PROP_3GPP_OPERATOR_NAME]);
>
> + properties[PROP_3GPP_VENDOR_PCO_INFO] =
> + g_param_spec_string (MM_SIMPLE_PROPERTY_3GPP_VENDOR_PCO_INFO,
> + "3GPP LTE vendor PCO info",
> + "Hex-encoded string of the current vendor PCO info",
> + NULL,
> + G_PARAM_READWRITE);
> + g_object_class_install_property (object_class, PROP_3GPP_VENDOR_PCO_INFO, properties[PROP_3GPP_VENDOR_PCO_INFO]);
> +
> properties[PROP_CDMA_CDMA1X_REGISTRATION_STATE] =
> g_param_spec_enum (MM_SIMPLE_PROPERTY_CDMA_CDMA1X_REGISTRATION_STATE,
> "CDMA1x registration state",
> diff --git a/libmm-glib/mm-simple-status.h b/libmm-glib/mm-simple-status.h
> index 8292fdb..be4f7f6 100644
> --- a/libmm-glib/mm-simple-status.h
> +++ b/libmm-glib/mm-simple-status.h
> @@ -88,6 +88,7 @@ guint mm_simple_status_get_cdma_nid
> #define MM_SIMPLE_PROPERTY_3GPP_REGISTRATION_STATE "m3gpp-registration-state"
> #define MM_SIMPLE_PROPERTY_3GPP_OPERATOR_CODE "m3gpp-operator-code"
> #define MM_SIMPLE_PROPERTY_3GPP_OPERATOR_NAME "m3gpp-operator-name"
> +#define MM_SIMPLE_PROPERTY_3GPP_VENDOR_PCO_INFO "m3gpp-vendor-pco-info"
>
> #define MM_SIMPLE_PROPERTY_CDMA_CDMA1X_REGISTRATION_STATE "cdma-cdma1x-registration-state"
> #define MM_SIMPLE_PROPERTY_CDMA_EVDO_REGISTRATION_STATE "cdma-evdo-registration-state"
> diff --git a/src/mm-broadband-bearer.c b/src/mm-broadband-bearer.c
> index f8f449d..90fd33f 100644
> --- a/src/mm-broadband-bearer.c
> +++ b/src/mm-broadband-bearer.c
> @@ -1100,6 +1100,7 @@ connect_3gpp_ready (MMBroadbandBearer *self,
> ConnectContext *ctx)
> {
> MMBearerConnectResult *result;
> + MMBaseModem *modem = NULL;
> GError *error = NULL;
>
> result = MM_BROADBAND_BEARER_GET_CLASS (self)->connect_3gpp_finish (self, res, &error);
> @@ -1109,6 +1110,15 @@ connect_3gpp_ready (MMBroadbandBearer *self,
> return;
> }
>
> + /* Get the owner modem object */
> + g_object_get (self,
> + MM_BEARER_MODEM, &modem,
> + NULL);
> + g_assert (modem != NULL);
> +
> + /* In case of 3GPP LTE, there is a need to refresh the vendor PCO info at this stage. */
> + mm_iface_modem_3gpp_reload_current_vendor_pco_info (MM_IFACE_MODEM_3GPP (modem), NULL, NULL);
> +
You need to g_object_unref (modem) here, to balance out with the one you
got in g_object_get (). A useful hint to avoid these issues is to always
make sure that when MM exits and the modem is still plugged in you see
the "Modem (whatever)... completely disposed" log message. If you don't
see that message, there's a reference to balance out somewhere.
But anyway, could you explain a bit why this update is needed? Why do
you say this is needed only in 3GPP-LTE? And if it is 3GPP-LTE only, why
isn't it within a if (mm_iface_modem_is_3gpp_lte())?
> /* take result */
> connect_succeeded (ctx, CONNECTION_TYPE_3GPP, result);
> }
> diff --git a/src/mm-iface-modem-3gpp.c b/src/mm-iface-modem-3gpp.c
> index 53399d5..7fdce03 100644
> --- a/src/mm-iface-modem-3gpp.c
> +++ b/src/mm-iface-modem-3gpp.c
> @@ -64,6 +64,10 @@ mm_iface_modem_3gpp_bind_simple_status (MMIfaceModem3gpp *self,
> status, MM_SIMPLE_PROPERTY_3GPP_OPERATOR_NAME,
> G_BINDING_DEFAULT | G_BINDING_SYNC_CREATE);
>
> + g_object_bind_property (skeleton, "vendor-pco-info",
> + status, MM_SIMPLE_PROPERTY_3GPP_VENDOR_PCO_INFO,
> + G_BINDING_DEFAULT | G_BINDING_SYNC_CREATE);
> +
As said, no need this in Simple Status.
> g_object_unref (skeleton);
> }
>
> @@ -75,7 +79,7 @@ typedef struct {
> MMModem3gppRegistrationState eps;
> gboolean manual_registration;
> GCancellable *pending_registration_cancellable;
> - gboolean reloading_operator;
> + gboolean reloading_registration_info;
> } RegistrationStateContext;
>
> static void
> @@ -951,6 +955,114 @@ mm_iface_modem_3gpp_clear_current_operator (MMIfaceModem3gpp *self)
>
> /*****************************************************************************/
>
> +typedef struct {
> + MMIfaceModem3gpp *self;
> + MmGdbusModem3gpp *skeleton;
> + GSimpleAsyncResult *result;
> +} ReloadCurrentVendorPcoInfoContext;
> +
> +static void
> +reload_current_vendor_pco_info_context_complete_and_free (ReloadCurrentVendorPcoInfoContext *ctx)
> +{
> + g_simple_async_result_complete_in_idle (ctx->result);
> + g_object_unref (ctx->result);
> + if (ctx->skeleton)
> + g_object_unref (ctx->skeleton);
> + g_object_unref (ctx->self);
> + g_slice_free (ReloadCurrentVendorPcoInfoContext, ctx);
> +}
> +
> +gboolean
> +mm_iface_modem_3gpp_reload_current_vendor_pco_info_finish (MMIfaceModem3gpp *self,
> + GAsyncResult *res,
> + GError **error)
> +{
> + return !g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT (res), error);
> +}
> +
> +static void
> +load_vendor_pco_info_ready (MMIfaceModem3gpp *self,
> + GAsyncResult *res,
> + ReloadCurrentVendorPcoInfoContext *ctx)
> +{
> + GError *error = NULL;
> + gchar *str;
> +
> + str = MM_IFACE_MODEM_3GPP_GET_INTERFACE (self)->load_vendor_pco_info_finish (self, res, &error);
> + if (error) {
> + mm_warn ("Couldn't load Vendor PCO Info: '%s'", error->message);
> + g_error_free (error);
> + }
> +
> + if (ctx->skeleton)
> + mm_gdbus_modem3gpp_set_vendor_pco_info (ctx->skeleton, str);
On error, the PCO info is not reseted to an empty string, it will leave
the previous PCO info. Is this intended?
> +
> + g_free (str);
> +
> + g_simple_async_result_set_op_res_gboolean (ctx->result, TRUE);
> + reload_current_vendor_pco_info_context_complete_and_free (ctx);
> +}
> +
> +void
> +mm_iface_modem_3gpp_reload_current_vendor_pco_info (MMIfaceModem3gpp *self,
> + GAsyncReadyCallback callback,
> + gpointer user_data)
> +{
> + ReloadCurrentVendorPcoInfoContext *ctx;
> +
> + ctx = g_slice_new0 (ReloadCurrentVendorPcoInfoContext);
> + ctx->self = g_object_ref (self);
> + ctx->result = g_simple_async_result_new (G_OBJECT (self),
> + callback,
> + user_data,
> + mm_iface_modem_3gpp_reload_current_vendor_pco_info);
> +
> + g_object_get (self,
> + MM_IFACE_MODEM_3GPP_DBUS_SKELETON, &ctx->skeleton,
> + NULL);
> +
> + if (!ctx->skeleton) {
> + g_simple_async_result_set_error (ctx->result,
> + MM_CORE_ERROR,
> + MM_CORE_ERROR_FAILED,
> + "Couldn't get interface skeleton");
> + reload_current_vendor_pco_info_context_complete_and_free (ctx);
> + return;
> + }
> +
> + if (!MM_IFACE_MODEM_3GPP_GET_INTERFACE (self)->load_vendor_pco_info ||
> + !MM_IFACE_MODEM_3GPP_GET_INTERFACE (self)->load_vendor_pco_info_finish) {
> +
> + mm_gdbus_modem3gpp_set_vendor_pco_info (ctx->skeleton, NULL);
Didn't you say in the interface description that if no value given, it
would be an empty string? (i.e. "")
> +
> + g_simple_async_result_set_op_res_gboolean(ctx->result, TRUE);
> + reload_current_vendor_pco_info_context_complete_and_free(ctx);
> + return;
> + }
> +
> + /* Launch vendor PCO info update */
> + MM_IFACE_MODEM_3GPP_GET_INTERFACE (ctx->self)->load_vendor_pco_info (
> + ctx->self,
> + (GAsyncReadyCallback)load_vendor_pco_info_ready,
> + ctx);
> +}
> +
> +void
> +mm_iface_modem_3gpp_clear_current_vendor_pco_info (MMIfaceModem3gpp *self)
> +{
> + MmGdbusModem3gpp *skeleton = NULL;
> +
> + g_object_get (self,
> + MM_IFACE_MODEM_3GPP_DBUS_SKELETON, &skeleton,
> + NULL);
> + if (!skeleton)
> + return;
> +
> + mm_gdbus_modem3gpp_set_vendor_pco_info (skeleton, NULL);
Same here, I was expecting an empty string "".
> +}
> +/*****************************************************************************/
> +
> +
> void
> mm_iface_modem_3gpp_update_access_technologies (MMIfaceModem3gpp *self,
> MMModemAccessTechnology access_tech)
> @@ -969,7 +1081,7 @@ mm_iface_modem_3gpp_update_access_technologies (MMIfaceModem3gpp *self,
> * but only if something valid to report */
> if (state == MM_MODEM_3GPP_REGISTRATION_STATE_HOME ||
> state == MM_MODEM_3GPP_REGISTRATION_STATE_ROAMING ||
> - ctx->reloading_operator) {
> + ctx->reloading_registration_info) {
> if (access_tech != MM_MODEM_ACCESS_TECHNOLOGY_UNKNOWN)
> mm_iface_modem_update_access_technologies (MM_IFACE_MODEM (self),
> access_tech,
> @@ -1034,7 +1146,19 @@ update_registration_reload_current_operator_ready (MMIfaceModem3gpp *self,
> MM_MODEM_STATE_CHANGE_REASON_UNKNOWN);
>
> ctx = get_registration_state_context (self);
> - ctx->reloading_operator = FALSE;
> + ctx->reloading_registration_info = FALSE;
> +}
> +
> +static void
> +update_registration_reload_current_vendor_pco_info_ready (MMIfaceModem3gpp *self,
> + GAsyncResult *res,
> + gpointer user_data)
> +{
> + mm_iface_modem_3gpp_reload_current_operator (
> + self,
> + (GAsyncReadyCallback)update_registration_reload_current_operator_ready,
> + user_data);
> +
> }
>
> static void
> @@ -1044,6 +1168,7 @@ update_non_registered_state (MMIfaceModem3gpp *self,
> {
> /* Not registered neither in home nor roaming network */
> mm_iface_modem_3gpp_clear_current_operator (self);
> + mm_iface_modem_3gpp_clear_current_vendor_pco_info(self);
Whitespace before parenthesis needed.
>
> /* The property in the interface is bound to the property
> * in the skeleton, so just updating here is enough */
> @@ -1081,20 +1206,20 @@ update_registration_state (MMIfaceModem3gpp *self,
>
> if (new_state == MM_MODEM_3GPP_REGISTRATION_STATE_HOME ||
> new_state == MM_MODEM_3GPP_REGISTRATION_STATE_ROAMING) {
> - /* If already reloading operator, skip it */
> - if (ctx->reloading_operator)
> + /* If already reloading registration info, skip it */
> + if (ctx->reloading_registration_info)
> return;
>
> mm_info ("Modem %s: 3GPP Registration state changed (%s -> registering)",
> g_dbus_object_get_object_path (G_DBUS_OBJECT (self)),
> mm_modem_3gpp_registration_state_get_string (old_state));
>
> - /* Reload current operator. ONLY update the state to REGISTERED after
> - * having loaded operator code/name */
> - ctx->reloading_operator = TRUE;
> - mm_iface_modem_3gpp_reload_current_operator (
> + /* Reload current registration info - vendor PCO info and operator.
> + * ONLY update the state to REGISTERED after having loaded operator code/name */
> + ctx->reloading_registration_info = TRUE;
> + mm_iface_modem_3gpp_reload_current_vendor_pco_info (
> self,
> - (GAsyncReadyCallback)update_registration_reload_current_operator_ready,
> + (GAsyncReadyCallback)update_registration_reload_current_vendor_pco_info_ready,
> GUINT_TO_POINTER (new_state));
Instead of chaining the PCO update and the current operator info update
with separate calls; just include within reload_current_operator() the
logic to update the PCO info in addition to operator code and such. If
the PCO should be updated whenever operator changes, you'll better
handle that directly inside reload_current_öperator().
> return;
> }
> diff --git a/src/mm-iface-modem-3gpp.h b/src/mm-iface-modem-3gpp.h
> index f4014c4..79342e6 100644
> --- a/src/mm-iface-modem-3gpp.h
> +++ b/src/mm-iface-modem-3gpp.h
> @@ -34,6 +34,7 @@
> #define MM_IFACE_MODEM_3GPP_PS_NETWORK_SUPPORTED "iface-modem-3gpp-ps-network-supported"
> #define MM_IFACE_MODEM_3GPP_EPS_NETWORK_SUPPORTED "iface-modem-3gpp-eps-network-supported"
> #define MM_IFACE_MODEM_3GPP_IGNORED_FACILITY_LOCKS "iface-modem-3gpp-ignored-facility-locks"
> +#define MM_IFACE_MODEM_3GPP_VENDOR_PCO_INFO "iface-modem-3gpp-vendor-pco-info"
>
> #define MM_IFACE_MODEM_3GPP_ALL_ACCESS_TECHNOLOGIES_MASK \
> (MM_MODEM_ACCESS_TECHNOLOGY_GSM | \
> @@ -176,6 +177,14 @@ struct _MMIfaceModem3gpp {
> GAsyncResult *res,
> GError **error);
>
> + /* Loading of the Vendor PCO Info property */
> + void (*load_vendor_pco_info) (MMIfaceModem3gpp *self,
> + GAsyncReadyCallback callback,
> + gpointer user_data);
> + gchar * (*load_vendor_pco_info_finish) (MMIfaceModem3gpp *self,
> + GAsyncResult *res,
> + GError **error);
> +
> /* Scan current networks, expect a GList of MMModem3gppNetworkInfo */
> void (* scan_networks) (MMIfaceModem3gpp *self,
> GAsyncReadyCallback callback,
> @@ -249,6 +258,15 @@ gboolean mm_iface_modem_3gpp_reload_current_operator_finish (MMIfaceModem3gpp *s
> GError **error);
> void mm_iface_modem_3gpp_clear_current_operator (MMIfaceModem3gpp *self);
>
> +/* Request to reload vendor PCO info */
> +void mm_iface_modem_3gpp_reload_current_vendor_pco_info (MMIfaceModem3gpp *self,
> + GAsyncReadyCallback callback,
> + gpointer user_data);
> +gboolean mm_iface_modem_3gpp_reload_current_vendor_pco_info_finish (MMIfaceModem3gpp *self,
> + GAsyncResult *res,
> + GError **error);
> +void mm_iface_modem_3gpp_clear_current_vendor_pco_info (MMIfaceModem3gpp *self);
> +
When is 'clear_current_vendor_pco_info()' used? Didn't see any use of it.
> /* Allow registering in the network */
> gboolean mm_iface_modem_3gpp_register_in_network_finish (MMIfaceModem3gpp *self,
> GAsyncResult *res,
>
--
Aleksander
More information about the ModemManager-devel
mailing list