[MM][PATCH] altair-lte: add support for 3GPP Vendor PCO info
Aleksander Morgado
aleksander at lanedo.com
Mon Jul 1 23:52:16 PDT 2013
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.
> + mm_get_int_from_match_info (match_info, 1, &cid);
Just in case, place this mm_get_int_from_match_info() within an if().
> +
> + 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.
> +
> + 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?
> +}
> +
> +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.
> +{
> + 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().
> + 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.
> + 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.
> + }
> +
> + 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?
> + }
> + }
> +
> + 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.
> +
> + 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().
> }
>
> 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
More information about the ModemManager-devel
mailing list