[MM][PATCH] modem: add support for 3GPP Vendor PCO info
Aleksander Morgado
aleksander at lanedo.com
Wed Jul 10 23:08:44 PDT 2013
>
> 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.
>
>
> we need it since we need to print the PCO in mmcli .
>
Not really. You can still print the PCO in mmcli without having it in
the *Simple Status*, so please remove it from there. We cannot add every
property in the Simple Status, doesn't make much sense.
In mmcli, when printing the modem status info (e.g. -m 0 without further
action) you can add the PCO info in the 3GPP section of the printed
output, next to operator code and name.
> >
> > + /* 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())?
>
>
> ok - i've put it under if (mm_iface_modem_is_3gpp_lte())
> Vendor PCO can come with any PDN (bearer) initiation - and in LTE
> network where there are more than one PDN
> you might get different Vendor PCO on each of the PDN - so when u raise
> the bearer here (which actually is the 3rd PDN in
> Verizon network) you need to check the vendor PCO value in the network.
> I'm an LTE guy - so and as far as i know only 3GPP-LTE has support for
> VENDOR PCO inside it's signaling.
>
Ok.
> > +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?
>
>
> i followed the same idea as load_operator_code_ready().
>
Then we should probably do the same (resetting the string to NULL) when
failing to load the operator code or name. Could you write a patch for that?
> > +
> > + 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. "")
>
>
> again i followed the same as operator name which states:
>
> If the <literal>MCC</literal> and <literal>MNC</literal> are not known
> or the mobile is not registered to a mobile network, this
> property will
> be a zero-length (blank) string.
>
> and uses the same technique to specify it
> mm_gdbus_modem3gpp_set_operator_code (ctx->skeleton, NULL);
> in mm_iface_modem_3gpp_reload_current_operator
>
Yeah, forget what I said, setting to NULL should be fine.
>
> >
> > /* 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().
>
>
> i wanted to seperate the two
> since mm_iface_modem_3gpp_reload_current_vendor_pco_info() is called
> from connect_3gpp_ready() and there
> is no need to update the operator at the stage.
>
Not very sure about this, but ok.
>
>
>
> > 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.
>
>
> in update_non_registered_state()
> see above.
>
Ah yes :)
--
Aleksander
More information about the ModemManager-devel
mailing list