[MM][PATCH] altair-lte: add support for 3GPP Vendor PCO info
ori inbar
ori.inbar.altair.semi.com at gmail.com
Wed Jul 10 19:18:30 PDT 2013
hello Aleksander.
see below.
On Tue, Jul 2, 2013 at 1:52 AM, Aleksander Morgado <aleksander at lanedo.com>wrote:
> Hey hey,
>
> > plugins/altair/mm-broadband-modem-altair-lte.c | 179
> +++++++++++++++++++++++--
> > 1 file changed, 169 insertions(+), 10 deletions(-)
> > mode change 100644 => 100755
> plugins/altair/mm-broadband-modem-altair-lte.c
> >
> > diff --git a/plugins/altair/mm-broadband-modem-altair-lte.c
> b/plugins/altair/mm-broadband-modem-altair-lte.c
> > old mode 100644
> > new mode 100755
> > index a2ae2ba..c1b16f3
> > --- a/plugins/altair/mm-broadband-modem-altair-lte.c
> > +++ b/plugins/altair/mm-broadband-modem-altair-lte.c
> > @@ -51,6 +51,7 @@ G_DEFINE_TYPE_EXTENDED (MMBroadbandModemAltairLte,
> mm_broadband_modem_altair_lte
> > struct _MMBroadbandModemAltairLtePrivate {
> > /* Regex for bearer related notifications */
> > GRegex *statcm_regex;
> > + GRegex *pcoinfo_regex;
> > };
> >
> > static MMIfaceModem3gpp *iface_modem_3gpp_parent;
> > @@ -553,6 +554,19 @@ altair_statcm_changed (MMAtSerialPort *port,
> > }
> >
> > static void
> > +altair_pcoinfo_changed (MMAtSerialPort *port,
> > + GMatchInfo *match_info,
> > + MMBroadbandModemAltairLte *self)
> > +{
> > + gint cid = 0;
>
> Leave a full whiteline between variable declarations and first method call.
>
done
>
> > + mm_get_int_from_match_info (match_info, 1, &cid);
>
> Just in case, place this mm_get_int_from_match_info() within an if().
>
done
>
> > +
> > + mm_dbg ("altair_pcoinfo_changed for cid %d", cid);
>
> Log messages need to be readable; and the method name will already be
>
> available in the log. So don't re-add the method name in the actual
>
> message. Instead, just say "PCO info changed for cid %d" or something
>
> like that.
>
>
done
> +
> > + mm_iface_modem_3gpp_reload_current_vendor_pco_info
> (MM_IFACE_MODEM_3GPP (self), NULL, NULL);
>
> So... we only get notification that PCO info changed, not the change
> itself?
>
we get the change as well - but since we get it for only one PDN we need to
re-evaluate the system wide PCO info we will
notify up - so we call for a full reload.
>
> > +}
> > +
> > +static void
> > set_3gpp_unsolicited_events_handlers (MMBroadbandModemAltairLte *self,
> > gboolean enable)
> > {
> > @@ -574,6 +588,14 @@ set_3gpp_unsolicited_events_handlers
> (MMBroadbandModemAltairLte *self,
> > enable ? (MMAtSerialUnsolicitedMsgFn)altair_statcm_changed
> : NULL,
> > enable ? self : NULL,
> > NULL);
> > +
> > + /* Vendor PCO info change */
> > + mm_at_serial_port_add_unsolicited_msg_handler (
> > + ports[i],
> > + self->priv->pcoinfo_regex,
> > + enable ? (MMAtSerialUnsolicitedMsgFn)altair_pcoinfo_changed
> : NULL,
> > + enable ? self : NULL,
> > + NULL);
> > }
> > }
> >
> > @@ -687,6 +709,12 @@ own_enable_unsolicited_events_ready (MMBaseModem
> *self,
> > g_object_unref (simple);
> > }
> >
> > +static const MMBaseModemAtCommand unsolicited_enable_sequence[] = {
> > + { "%STATCM=1", 5, FALSE, NULL },
> > + { "%PCOINFO=1", 3, FALSE, NULL },
> > + { NULL }
> > +};
> > +
> > static void
> > parent_enable_unsolicited_events_ready (MMIfaceModem3gpp *self,
> > GAsyncResult *res,
> > @@ -702,13 +730,12 @@ parent_enable_unsolicited_events_ready
> (MMIfaceModem3gpp *self,
> > }
> >
> > /* Our own enable now */
> > - mm_base_modem_at_command_full (
> > + mm_base_modem_at_sequence_full (
> > MM_BASE_MODEM (self),
> > mm_base_modem_peek_port_primary (MM_BASE_MODEM (self)),
> > - "%STATCM=1",
> > - 6,
> > - FALSE, /* allow_cached */
> > - FALSE, /* raw */
> > + unsolicited_enable_sequence,
> > + NULL, /* response_processor_context */
> > + NULL, /* response_processor_context_free */
> > NULL, /* cancellable */
> > (GAsyncReadyCallback)own_enable_unsolicited_events_ready,
> > simple);
> > @@ -781,6 +808,12 @@ own_disable_unsolicited_events_ready (MMBaseModem
> *self,
> > simple);
> > }
> >
> > +static const MMBaseModemAtCommand unsolicited_disable_sequence[] = {
> > + { "%STATCM=0", 5, FALSE, NULL },
> > + { "%PCOINFO=0", 5, FALSE, NULL },
> > + { NULL }
> > +};
> > +
> > static void
> > modem_3gpp_disable_unsolicited_events (MMIfaceModem3gpp *self,
> > GAsyncReadyCallback callback,
> > @@ -794,13 +827,12 @@ modem_3gpp_disable_unsolicited_events
> (MMIfaceModem3gpp *self,
> >
> modem_3gpp_disable_unsolicited_events);
> >
> > /* Our own disable first */
> > - mm_base_modem_at_command_full (
> > + mm_base_modem_at_sequence_full (
> > MM_BASE_MODEM (self),
> > mm_base_modem_peek_port_primary (MM_BASE_MODEM (self)),
> > - "%STATCM=0",
> > - 5,
> > - FALSE, /* allow_cached */
> > - FALSE, /* raw */
> > + unsolicited_disable_sequence,
> > + NULL, /* response_processor_context */
> > + NULL, /* response_processor_context_free */
> > NULL, /* cancellable */
> > (GAsyncReadyCallback)own_disable_unsolicited_events_ready,
> > result);
> > @@ -893,6 +925,128 @@ modem_3gpp_load_operator_name (MMIfaceModem3gpp
> *self,
> > callback,
> > user_data);
> > }
> >
> +/*****************************************************************************/
> > +/* Vendor PCO Info loading (3GPP interface) */
> > +
> > +static gchar *
> > +mm_broadband_modem_altair_lte_parse_vendor_pco_info (const gchar *reply)
>
>
> We should have unit tests for all these parsers, so let's add them. See
>
> for example this one:
>
>>
>
> http://cgit.freedesktop.org/ModemManager/ModemManager/commit/?h=aleksander/huawei-ndisstatqry&id=af7564cf82b380c814ce14ca80faacd62e20585a
>
>>
> That's a parser plus unit tests for a Huawei-specific thing.
>
>>
> Please do the same here; move this method to a pair of new
>
> "mm-modem-helpers-altair-lte.[ch]" files, rename it as
>
> "mm_altair_lte_parse_vendor_pco_info", and then add unit tests for it
>
> with different example replies that we get for the PCO info. You'll
>
> probably need to copy some bits from the Makefile.am in that patch, as
> that is not yet pushed to git master.
>
i will look into it - i hope for now u can accept it without this addition.
>
>
> > +{
> > + GError *inner_error = NULL;
> > + GRegex *r;
> > + GMatchInfo *match_info;
> > + guint cid;
> > + gchar *result = NULL;
> > +
> > + if (!reply[0])
> > + /* No APNs configured, all done */
> > + return NULL;
> > +
> > + r = g_regex_new
> ("\\%PCOINFO:\\s*(\\d+)\\s*,(\\d+)\\s*(,([^,\\)]*),([0-9A-Fa-f]*))?",
> > + G_REGEX_DOLLAR_ENDONLY | G_REGEX_RAW,
> > + 0, &inner_error);
> > + if (r) {
>
> Just g_assert (r != NULL) here instead. If you wrote the regex
>
> expression well, it should never fail. Also, don't pass &inner_error,
> not needed in g_regex_new().
>
done.
>
>
> > + g_regex_match_full (r, reply, strlen (reply), 0, 0,
> &match_info, &inner_error);
> > +
> > + while (!inner_error && g_match_info_matches (match_info)) {
> > + gchar *pco_id;
> > +
> > + if (!mm_get_uint_from_match_info (match_info, 2, &cid)) {
> > + inner_error = g_error_new (MM_CORE_ERROR,
> > + MM_CORE_ERROR_FAILED,
> > + "Couldn't parse CID from
> reply: '%s'",
> > + reply);
> > + break;
> > + }
> > +
> > + pco_id = mm_get_string_unquoted_from_match_info
> (match_info, 4);
> > +
> > + /* If this is the first PCO info we're getting, use it.
> Otherwise only use CID 3
> > + PCO info, since this is the internet bearer. */
>
> Missing asterisk in the comment; all comment lines should start with an
> asterisk.
>
done.
>
> > + if (pco_id && (!result || cid == 3)) {
> > + gchar *pco_payload;
> > +
> > + pco_payload = mm_get_string_unquoted_from_match_info
> (match_info, 5);
> > + if (!pco_payload) {
> > + inner_error = g_error_new (MM_CORE_ERROR,
> > + MM_CORE_ERROR_FAILED,
> > + "Couldn't parse PCO
> payload from reply: '%s'",
> > + reply);
> > + g_free (pco_id);
> > + break;
> > + }
> > +
> > + if (result) {
> > + g_free (result);
> > + result = NULL;
>
> No need to result = NULL if you're going to change it right away. You
>
> can remove the if() all together and just place g_free (result); as
> result will start being NULL and g_free(NULL) is perfectly fine.
>
done
>
> > + }
> > +
> > + result = g_strdup_printf ("%s%s", pco_id, pco_payload);
> > +
> > + mm_dbg ("Vendor PCO info for CID %d: '%s'", cid,
> result);
> > +
> > + g_free (pco_id);
> > + g_free (pco_payload);
> > + } else {
> > + if (pco_id)
> > + mm_dbg ("Vendor PCO info for CID %d is disregarded
> since we already have one", cid);
> > + else
> > + mm_dbg ("Vendor PCO info for CID %d is disregarded
> since it has no PCO data", cid);
> > + }
> > +
> > + g_match_info_next (match_info, &inner_error);
> > + }
> > +
> > + g_match_info_free (match_info);
> > + g_regex_unref (r);
> > + }
> > +
> > + if (inner_error) {
> > + mm_dbg ("Error parsing %%PCOINFO response: '%s'",
> inner_error->message);
> > + g_error_free (inner_error);
> > + if (result) {
> > + g_free (result);
> > + result = NULL;
>
> Instead of removing the error; why not propagate it to the caller?
>
done
>
> > + }
> > + }
> > +
> > + return result;
> > +}
> > +
> > +static gchar *
> > +modem_3gpp_load_vendor_pco_info_finish (MMIfaceModem3gpp *self,
> > + GAsyncResult *res,
> > + GError **error)
> > +{
> > + const gchar *result;
> > + gchar *vendor_pco_info;
> > +
> > + result = mm_base_modem_at_command_finish (MM_BASE_MODEM (self),
> res, error);
> > + if (!result)
> > + return NULL;
> > +
> > + vendor_pco_info =
> mm_broadband_modem_altair_lte_parse_vendor_pco_info (result);
> > + if (vendor_pco_info)
> > + mm_dbg ("loaded Vendor PCO Info: %s", vendor_pco_info);
> > + else
> > + mm_dbg ("loaded empty Vendor PCO Info");
>
> In case of error, you're returning NULL but the GError not set. If we
>
> return NULL, you should really set the GError. I would just propagate
> the GError from the parse_vendor_pco_info() message.
>
done
> > +
> > + return vendor_pco_info;
> > +}
> > +
> > +static void
> > +modem_3gpp_load_vendor_pco_info (MMIfaceModem3gpp *self,
> > + GAsyncReadyCallback callback,
> > + gpointer user_data)
> > +{
> > + mm_dbg ("Loading Vendor PCO Info...");
> > +
> > + mm_base_modem_at_command (MM_BASE_MODEM (self),
> > + "%PCOINFO?",
> > + 6,
> > + FALSE,
> > + callback,
> > + user_data);
> > +}
> >
> >
> /*****************************************************************************/
> > /* Generic ports open/close context */
> > @@ -955,6 +1109,9 @@ mm_broadband_modem_altair_lte_init
> (MMBroadbandModemAltairLte *self)
> >
> > self->priv->statcm_regex = g_regex_new
> ("\\r\\n\\%STATCM:\\s*(\\d*),?(\\d*)\\r+\\n",
> > G_REGEX_RAW |
> G_REGEX_OPTIMIZE, 0, NULL);
> > +
> > + self->priv->pcoinfo_regex = g_regex_new
> ("\\r\\n\\%PCOINFO:\\s*(\\d*),([^,\\s]*),([^,\\s]*)\\r+\\n",
> > + G_REGEX_RAW |
> G_REGEX_OPTIMIZE, 0, NULL);
>
>
> You're missing a g_regex_unref() for this GRegex; possibly in dispose()
> or finalize().
>
done
> > }
> >
> > static void
> > @@ -1028,6 +1185,8 @@ iface_modem_3gpp_init (MMIfaceModem3gpp *iface)
> > iface->load_operator_code_finish =
> modem_3gpp_load_operator_code_finish;
> > iface->load_operator_name = modem_3gpp_load_operator_name;
> > iface->load_operator_name_finish =
> modem_3gpp_load_operator_name_finish;
> > + iface->load_vendor_pco_info = modem_3gpp_load_vendor_pco_info;
> > + iface->load_vendor_pco_info_finish =
> modem_3gpp_load_vendor_pco_info_finish;
> >
> > }
> >
> >
>
>
> --
> Aleksander
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/modemmanager-devel/attachments/20130710/c3984caa/attachment-0001.html>
More information about the ModemManager-devel
mailing list