[PATCH] Infineon: Add support for Infineon / Intel modems
Paul Bartell
paul.bartell at gmail.com
Thu Jun 1 00:54:00 UTC 2017
Hey Aleksander
I spent a little more time debugging the HL7588 today. This modem
seems to designed for phone-type SIM cards that have a context for MMS
in the PDP context #1 slot (my combination of SIM card and network has
the internet APN in this slot). It seems to lock configuration of this
PDP context unless I run +HBHV=3,1 (pdp_unlock_mode) and detach from
the network. Setting different +CGAUTO modes does not appear to affect
the outcome.
Regardless, once the modem is attached to a network, I am not able to
deactivate the PDP context without the modem detaching and reattaching
itself to the network (see the attached log mm_hl7588_cgdeact.log
).This causes ModemManager to cancel any ongoing connection attempts.
Sometimes a connection attempt succeeds without first detaching from
the network (I tried a build without the DIAL_3GPP_CONTEXT_STEP_DETACH
step), but often this is accompanied with no DNS addresses. The modem
returns +XDNS: 1, "0.0.0.0", "0.0.0.0". Querying the modem again after
30-60 seconds again returns no DNS servers. When this occurs,
sometimes traffic will pass when I bring up the NCM interface, other
times it will simply not pass any traffic.
As a better workaround, I made a build which does the following: (log
is mm_hl7588_cgact_cgatt.log).
1. Disables +CGREG message handlers
2. Sends a +CGACT=0,1 to "deactivate" the context instead of detaching
from the network. In the broken firmware implementation (like the
HL7588), this detaches, then immediately reattaches to the network. In
a well-behaved firmware, this would simply deactivate the PDP context.
3. Re-enables +CGREG handling once the bearer has been connected
successfully or upon cancellation.
Does this seem like an acceptable way to work around this issue? An
alternative would be to add an extra step that re-enables +CGREG
handling immediately after deactivating the context. Or is there a
better way to prevent ModemManager from canceling the connection
attempt?
Regarding +XDNS parsing: Ths is another broken part of the HL75xx
firmware. The return value from this command often contains duplicate
addresses (specifically when used with an IPV4V6 dual-stack context).
+XDNS: 3, "198.224.166.135", "198.224.167.135"
+XDNS: 3, "198.224.166.135", "198.224.166.135"
+XDNS: 3, "198.224.166.135", "198.224.167.135"
+XDNS: 3, "198.224.166.135", "2001:4888:53:FF00:524:D:0:0"
+XDNS: 3, "198.224.166.135", "2001:4888:52:FF00:528:D:0:0”
An IPv4-only or IPv6-only context tends to returns only one or two DNS
server addresses in a single line.
I will send an updated patch that takes into account your other
comments in the next few days.
Thanks again,
Paul
On Wed, May 31, 2017 at 1:10 PM, Aleksander Morgado
<aleksander at aleksander.es> wrote:
>
> Hey Paul,
>
> > From faacc4fb4775cf5f610b96f642160af9a0857ab7 Mon Sep 17 00:00:00 2001
> > From: Paul Bartell <p.bartell at temperednetworks.com>
> > Date: Tue, 9 Apr 2013 10:48:17 +0200
> > Subject: [PATCH] Add Infineon Modem and bearer plugin
> >
> > Signed-off-by: Paul Bartell <p.bartell at temperednetworks.com>
>
> Long patch, long review :)
>
> Thanks for taking time to implement this. Most of the comments I have are minor coding style issues that are easily fixable, don't be overwhelmed by the amount of comments :)
>
> I'm mostly interested in two things that should be improved or more detailed:
>
> * The whole AT+CGATT=0 thing *during* a connection attempt, even requiring to temporarily disable CREG/CGREG/CEREG, seems totally broken to me. You state it's due to a firmware bug, could you describe better what this is about? We need to see if we can handle this in some other way, because detaching from packet network during an actual connection attempt doesn't sound sane :)
>
> * The new response parsers (e.g. XDNS) really deserve their own unit tests. See other plugins, like e.g. ublox, to see how the unit tests are setup; you basically would need to setup new mm-modem-helpers-infineon.[c|h] source files and then add a test subdirectory with the corresponding tests for the helper methods.
>
> Other than these previous important things, I also added several other improvements that could be added.
>
> Let me know what you think.
>
>
> > ---
> > plugins/Makefile.am | 15 +
> > plugins/infineon/mm-broadband-bearer-infineon.c | 1051 +++++++++++++++++++++++
> > plugins/infineon/mm-broadband-bearer-infineon.h | 59 ++
> > plugins/infineon/mm-broadband-modem-infineon.c | 216 +++++
> > plugins/infineon/mm-broadband-modem-infineon.h | 47 +
> > plugins/infineon/mm-plugin-infineon.c | 88 ++
> > plugins/infineon/mm-plugin-infineon.h | 46 +
> > src/mm-modem-helpers.c | 40 +
> > src/mm-modem-helpers.h | 6 +
> > 9 files changed, 1568 insertions(+)
> > mode change 100644 => 100755 plugins/Makefile.am
> > create mode 100755 plugins/infineon/mm-broadband-bearer-infineon.c
> > create mode 100644 plugins/infineon/mm-broadband-bearer-infineon.h
> > create mode 100755 plugins/infineon/mm-broadband-modem-infineon.c
> > create mode 100644 plugins/infineon/mm-broadband-modem-infineon.h
> > create mode 100755 plugins/infineon/mm-plugin-infineon.c
> > create mode 100644 plugins/infineon/mm-plugin-infineon.h
> >
> > diff --git a/plugins/Makefile.am b/plugins/Makefile.am
> > old mode 100644
> > new mode 100755
> > index 2a4ff2b..eef0490
> > --- a/plugins/Makefile.am
> > +++ b/plugins/Makefile.am
> > @@ -815,6 +815,21 @@ libmm_plugin_via_la_CPPFLAGS = $(PLUGIN_COMMON_COMPILER_FLAGS)
> > libmm_plugin_via_la_LDFLAGS = $(PLUGIN_COMMON_LINKER_FLAGS)
> >
> > ################################################################################
> > +# plugin: Infineon
> > +################################################################################
> > +pkglib_LTLIBRARIES += libmm-plugin-infineon.la
> > +libmm_plugin_infineon_la_SOURCES = \
> > + infineon/mm-plugin-infineon.c \
> > + infineon/mm-plugin-infineon.h \
> > + infineon/mm-broadband-modem-infineon.c \
> > + infineon/mm-broadband-modem-infineon.h \
> > + infineon/mm-broadband-bearer-infineon.c \
> > + infineon/mm-broadband-bearer-infineon.h \
> > + $(NULL)
> > +libmm_plugin_infineon_la_CPPFLAGS = $(PLUGIN_COMMON_COMPILER_FLAGS)
> > +libmm_plugin_infineon_la_LDFLAGS = $(PLUGIN_COMMON_LINKER_FLAGS)
> > +
> > +################################################################################
> > # plugin: telit
> > ################################################################################
> >
> > diff --git a/plugins/infineon/mm-broadband-bearer-infineon.c b/plugins/infineon/mm-broadband-bearer-infineon.c
> > new file mode 100755
> > index 0000000..9bc57bc
> > --- /dev/null
> > +++ b/plugins/infineon/mm-broadband-bearer-infineon.c
> > @@ -0,0 +1,1051 @@
> > +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> > +/*
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details:
> > + *
> > + * Copyright (C) 2013 Aleksander Morgado <aleksander at gnu.org>
> > + * Copyright (C) 2017 Tempered Networks Inc.
> > + */
> > +
> > +#include <config.h>
> > +
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +#include <string.h>
> > +#include <ctype.h>
> > +#include <arpa/inet.h>
> > +
> > +#include <ModemManager.h>
> > +#define _LIBMM_INSIDE_MM
> > +#include <libmm-glib.h>
> > +
> > +#include "mm-base-modem-at.h"
> > +#include "mm-broadband-bearer-infineon.h"
> > +#include "mm-log.h"
> > +#include "mm-modem-helpers.h"
> > +
> > +G_DEFINE_TYPE (MMBroadbandBearerInfineon, mm_broadband_bearer_infineon, MM_TYPE_BROADBAND_BEARER)
> > +
> > +/*****************************************************************************/
> > +/* WWAN interface mapping */
> > +
> > +/* Map CDC-NCM port index to USB interface number */
> > +static const guint usb_ncm_configs[] = { 0x06, 0x08, 0x0a, 0x0c };
> > +
> > +/* Map CDC-ACM port index to USB interface number */
> > +static const guint usb_acm_configs[] = { 0x00, 0xFF, 0x04 };
> > +
>
> I wonder if we will need to maintain separate tables for different devices.
> Maybe this could have been managed easier just with udev rules that are applied to the port directly, e.g.:
>
> SUBSYSTEMS=="usb", ATTRS{bInterfaceNumber}=="?*", ENV{.MM_USBIFNUM}="$attr{bInterfaceNumber}"
>
> ATTRS{idVendor}=="1519", ATTRS{idProduct}=="0443", ENV{.MM_USBIFNUM}=="06", ENV{ID_MM_INFINEON_NCM_ID}="0"
> ATTRS{idVendor}=="1519", ATTRS{idProduct}=="0443", ENV{.MM_USBIFNUM}=="08", ENV{ID_MM_INFINEON_NCM_ID}="1"
> ATTRS{idVendor}=="1519", ATTRS{idProduct}=="0443", ENV{.MM_USBIFNUM}=="0a", ENV{ID_MM_INFINEON_NCM_ID}="2"
> ATTRS{idVendor}=="1519", ATTRS{idProduct}=="0443", ENV{.MM_USBIFNUM}=="0c", ENV{ID_MM_INFINEON_NCM_ID}="3"
>
> ATTRS{idVendor}=="1519", ATTRS{idProduct}=="0443", ENV{.MM_USBIFNUM}=="00", ENV{ID_MM_INFINEON_ACM_ID}="0"
> ATTRS{idVendor}=="1519", ATTRS{idProduct}=="0443", ENV{.MM_USBIFNUM}=="ff", ENV{ID_MM_INFINEON_ACM_ID}="1"
> ATTRS{idVendor}=="1519", ATTRS{idProduct}=="0443", ENV{.MM_USBIFNUM}=="04", ENV{ID_MM_INFINEON_ACM_ID}="2"
>
> And then we could just call mm_kernel_device_get_property_as_int_hex(ID_MM_INFINEON_{NCM|ACM}_ID)
>
> > +static gint
> > +get_usb_ncm_config_index (MMPort *data,
> > + GError **error)
> > +{
> > + guint usb_iface_num;
> > + guint i;
> > +
> > + usb_iface_num = mm_kernel_device_get_property_as_int_hex (mm_port_peek_kernel_device (data), "ID_USB_INTERFACE_NUM");
> > +
> > + for (i = 0; i < G_N_ELEMENTS (usb_ncm_configs); i++) {
> > + if (usb_ncm_configs[i] == usb_iface_num)
> > + return (gint) i;
> > + }
> > +
> > + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
> > + "Unsupported WWAN interface: unexpected interface number: 0x%02x", usb_iface_num);
> > + return -1;
> > +}
> > +
> > +static gint
> > +get_usb_acm_config_index (MMPortSerialAt *port,
> > + GError **error)
> > +{
> > + guint usb_iface_num;
> > + guint i;
> > +
> > + usb_iface_num = mm_kernel_device_get_property_as_int_hex (mm_port_peek_kernel_device (&port->parent.parent), "ID_USB_INTERFACE_NUM");
> > + for (i = 0; i < G_N_ELEMENTS (usb_acm_configs); i++) {
> > + if (usb_acm_configs[i] == usb_iface_num)
> > + return (gint) i;
> > + }
> > +
> > + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
> > + "Unsupported ACM interface: unexpected interface number: 0x%02x", usb_iface_num);
> > + return -1;
> > +}
> > +
> > +/*****************************************************************************/
> > +/* 3GPP Dialing (sub-step of the 3GPP Connection sequence) */
> > +
> > +typedef enum {
>
> We usually provide a _FIRST step which we use to initialize the step variable in the Context; this is so that there is no longer a need to update the initialization code if the actual first step changes. In the switch(step) FIRST case later on, we would fall down to the next step right away.
>
> > + DIAL_3GPP_CONTEXT_STEP_CHECK_CGACT = 0,
> > + DIAL_3GPP_CONTEXT_STEP_DETACH,
> > + DIAL_3GPP_CONTEXT_STEP_SET_XGAUTH,
> > + DIAL_3GPP_CONTEXT_STEP_SET_XDNS,
> > + DIAL_3GPP_CONTEXT_STEP_SET_CGACT,
> > + DIAL_3GPP_CONTEXT_STEP_SET_DATA_CHAN,
> > + DIAL_3GPP_CONTEXT_STEP_START_DATA_CHAN,
> > + DIAL_3GPP_CONTEXT_STEP_LAST
> > +} Dial3gppContextStep;
> > +
> > +typedef struct {
> > + MMBroadbandBearerInfineon *self;
>
> The self pointer can be retrieved from the GTask easily (transfer none) with g_task_get_source_object(), so no need to have it in the Context.
>
> > + MMBaseModem *modem;
> > + MMPortSerialAt *primary;
> > + MMPortSerialAt *secondary;
> > + guint cid;
> > + MMBearerIpFamily ip_family;
> > + MMPort *data;
> > + gint usb_interface_config_index;
> > + gint usb_acm_config_index;
> > + GPtrArray * creg_regexes;
> > + Dial3gppContextStep step;
> > +} Dial3gppContext;
> > +
> > +
>
> One single white line is enough.
>
> > +
> > +static void
> > +dial_3gpp_context_free (Dial3gppContext *ctx)
> > +{
> > + g_object_unref (ctx->self);
> > + g_object_unref (ctx->modem);
> > + g_object_unref (ctx->primary);
> > + g_object_unref (ctx->secondary);
>
> This would issue a critical if there is no secondary port. You can switch all these calls to g_clear_object() calls, which do check if the pointer object is NULL before unref-ing.
>
> > + g_object_unref (ctx->data);
> > + g_slice_free (Dial3gppContext, ctx);
> > +}
> > +
> > +/* Forward Function declarations */
> > +static void dial_3gpp_context_step (GTask *task);
> > +
> > +static MMPort *
> > +dial_3gpp_finish (MMBroadbandBearer *self,
> > + GAsyncResult *res,
> > + GError **error)
> > +{
> > + return MM_PORT (g_task_propagate_pointer (G_TASK (res), error));
> > +}
> > +
> > +static void
> > +dial_3gpp_cgact_check_ready (MMBaseModem *modem,
> > + GAsyncResult *res,
> > + GTask *task)
> > +{
> > + const gchar *response;
> > + Dial3gppContext *ctx;
> > + GError *error = NULL;
> > + GList *cgact_list = NULL;
> > + GList *l = NULL;
> > + MM3gppPdpContextActive *pdp_ctx_active = NULL;
> > +
> > + ctx = (Dial3gppContext *) g_task_get_task_data (task);
> > +
> > + response = mm_base_modem_at_command_finish (modem, res, &error);
> > + if (!response) {
> > + g_task_return_error (task, error);
> > + g_object_unref (task);
> > + return;
> > + }
> > +
> > + /* Parse response */
> > + cgact_list = mm_3gpp_parse_cgact_read_response (response, &error);
> > + g_assert(cgact_list != NULL);
> > +
> > + if (!cgact_list) {
> > + g_task_return_error (task, error);
> > + g_object_unref (task);
> > + return;
> > + }
> > +
> > + /* Iterate over list and check for active context with same CID */
> > + for (l = cgact_list; l != NULL; l = l ->next) {
>
> Just for consistency:
> for (l = cgact_list; l; l = g_list_next (l)) {
>
> > + pdp_ctx_active = l->data;
> > + if (pdp_ctx_active->cid == ctx->cid) {
> > + break;
> > + }
> > + else
> > + pdp_ctx_active = NULL;
> > + }
> > + g_assert(pdp_ctx_active != NULL);
>
> Not sure I understand the g_assert() here. Does this mean it will assert unless it finds a matching active PDP context?
>
> > + /* If context is already active. Detach from network before attempting to connect
> > + in order to work around a Sierra HL7588 firmware bug */
>
> Could you please explain with more detail what the firmware issue is? Is this related in any way to the "default LTE bearer" connection being active as soon as registered in the LTE network, and not being able to change that connection details otherwise?
>
> > + if (pdp_ctx_active != NULL && pdp_ctx_active->active){
> > + mm_dbg("Selected CID is currently active.");
> > + ctx->step++;
>
> So, the "cid" value we get in the "3GPP dial" step as input is the CID of the context that was either found to match the APN settings and IP type we requested, or otherwise one that was just created for this purpose. If we end up arriving here and AT+CGACT? tells us that the context is already active, this is very likely due to it being the default LTE bearer. What was happening at this point that made you do the explicit AT+CGACTT=0? Did the XGAUTH fail? If the context was already active, did you try to just run +XDATACHANNEL instead right away?
>
> > + }
> > + /* Otherwise, skip to auth step */
> > + else
> > + ctx->step = DIAL_3GPP_CONTEXT_STEP_SET_XGAUTH;
> > +
> > + mm_3gpp_pdp_context_active_list_free (cgact_list);
> > + dial_3gpp_context_step (task);
> > +}
> > +
> > +typedef enum {
> > + BEARER_INFINEON_AUTH_NONE = 0,
> > + BEARER_INFINEON_AUTH_PAP = 1,
> > + BEARER_INFINEON_AUTH_CHAP = 2
> > +} BearerInfineonAuthType;
> > +
> > +/* Generate XGAUTH string */
> > +static gchar *
> > +build_auth_string (Dial3gppContext *ctx)
> > +{
> > + gchar *command;
> > + const gchar *user;
> > + const gchar *password;
> > + MMBearerAllowedAuth auth_type;
> > +
> > + user = mm_bearer_properties_get_user (mm_base_bearer_peek_config (MM_BASE_BEARER (ctx->self)));
> > + password = mm_bearer_properties_get_password (mm_base_bearer_peek_config (MM_BASE_BEARER (ctx->self)));
> > + auth_type = mm_bearer_properties_get_allowed_auth (mm_base_bearer_peek_config (MM_BASE_BEARER (ctx->self)));
> > +
> > + /* Both user and password are required; otherwise firmware returns an error */
> > + if (user || password) {
> > + gchar *encoded_user;
> > + gchar *encoded_password;
> > + BearerInfineonAuthType auth_type_inf = BEARER_INFINEON_AUTH_NONE;
> > +
> > + /* Prefer CHAP over PAP */
> > + if (auth_type & MM_BEARER_ALLOWED_AUTH_CHAP)
> > + auth_type_inf = BEARER_INFINEON_AUTH_CHAP;
> > + else if (auth_type & MM_BEARER_ALLOWED_AUTH_PAP)
> > + auth_type_inf = BEARER_INFINEON_AUTH_PAP;
> > + else if (auth_type == MM_BEARER_ALLOWED_AUTH_UNKNOWN)
> > + auth_type_inf = BEARER_INFINEON_AUTH_CHAP;
> > + else if (auth_type == MM_BEARER_ALLOWED_AUTH_NONE)
> > + auth_type_inf = BEARER_INFINEON_AUTH_NONE;
> > +
> > + encoded_user = mm_broadband_modem_take_and_convert_to_current_charset (MM_BROADBAND_MODEM (ctx->modem),
> > + g_strdup (user));
> > + encoded_password = mm_broadband_modem_take_and_convert_to_current_charset (MM_BROADBAND_MODEM (ctx->modem),
> > + g_strdup (password));
> > +
> > + command = g_strdup_printf ("+XGAUTH=%u,%u,\"%s\",\"%s\"",
> > + ctx->cid,
> > + auth_type_inf,
> > + encoded_user ? encoded_user : "",
> > + encoded_password ? encoded_password : "");
> > + g_free (encoded_user);
> > + g_free (encoded_password);
> > + } else {
> > + command = g_strdup_printf ("+XGAUTH=%u,0,\"\",\"\"", ctx->cid);
> > + }
> > + return command;
> > +}
> > +
> > +/* Generic AT command result ready callback */
> > +static void
> > +dial_3gpp_res_ready (MMBaseModem *modem,
> > + GAsyncResult *res,
> > + GTask *task)
> > +{
> > + Dial3gppContext *ctx;
> > + GError *error = NULL;
> > +
> > + ctx = (Dial3gppContext *) g_task_get_task_data (task);
> > +
> > + if (!mm_base_modem_at_command_finish (modem, res, &error)) {
> > +
>
> No whiteline here.
>
> > + /* Disregard NO CARRIER message from modem when detaching from network */
> > + if(ctx->step == DIAL_3GPP_CONTEXT_STEP_DETACH &&
> > + g_error_matches (error,
> > + MM_CONNECTION_ERROR,
> > + MM_CONNECTION_ERROR_NO_CARRIER))
> > + {
>
> Open braces in the same line as the last line of the if().
>
> > + g_error_free (error);
> > + } else {
> > + g_task_return_error (task, error);
> > + g_object_unref (task);
> > + return;
> > + }
> > + }
> > +
> > + /* Go to next step */
> > + ctx->step++;
> > + dial_3gpp_context_step (task);
> > +}
> > +
> > +static void set_reg_msg_handlers (Dial3gppContext *ctx, gboolean enable)
>
> The "static void" part should go in its own line when the method is defined. Also, each parameter in its own line.
>
> > +{
> > + guint i;
> > +
> > + if (!ctx->creg_regexes)
> > + {
>
> Open braces in the same line as the if().
>
> > + ctx->creg_regexes = mm_3gpp_creg_regex_get(FALSE);
> > + }
> > + for (i = 0; i < ctx->creg_regexes->len; i++)
> > + {
>
> Open braces in the same line as the for().
>
> > + mm_port_serial_at_enable_unsolicited_msg_handler (ctx->primary, (GRegex *)ctx->creg_regexes->pdata[i], enable);
> > + mm_port_serial_at_enable_unsolicited_msg_handler (ctx->secondary, (GRegex *)ctx->creg_regexes->pdata[i], enable);
>
> I see that if you don't add this the whole connection attempt will be cancelled if we detect that the modem got detached from PS or EPS network due to the CGATT=0. Still would like to have some more info on why that is needed, as it would seem a very very weird firmware bug! Needing to detach from packet network for a connection attempt makes really no sense...
>
> > + }
> > + if (enable)
> > + mm_dbg("Enabling unsolicited registration message handlers.");
> > + else
> > + mm_dbg("Disabling unsolicited registration message handlers.");
> > +}
> > +
> > +static void
> > +dial_3gpp_context_step (GTask *task)
> > +{
> > + Dial3gppContext *ctx;
> > +
> > + ctx = (Dial3gppContext *) g_task_get_task_data (task);
> > +
> > + mm_dbg("Dial3goo Step : %d", ctx->step);
> > +
>
> Dial3goo? :)
>
> Instead of just priting the step number, you could print e.g.:
> mm_dbg ("running infineon dialing step %u/%u...", ctx->step, DIAL_3GPP_CONTEXT_STEP_LAST);
>
>
> > + /* Check for cancellation */
> > + if (g_task_return_error_if_cancelled (task)) {
> > + set_reg_msg_handlers(ctx, TRUE);
> > + g_object_unref (task);
> > + return;
> > + }
> > +
> > + switch (ctx->step) {
>
> i.e.
> case DIAL_3GPP_CONTEXT_STEP_FIRST:
> ctx->step++;
> /* fall down to next step */
>
> > + case DIAL_3GPP_CONTEXT_STEP_CHECK_CGACT: {
>
> There's no need group this block within {} as there are no variables defined inside.
>
> > + mm_base_modem_at_command (ctx->modem,
> > + "+CGACT?",
> > + 3,
> > + FALSE,
> > + (GAsyncReadyCallback)dial_3gpp_cgact_check_ready,
> > + task);
> > + return;
> > + }
> > +
> > + case DIAL_3GPP_CONTEXT_STEP_DETACH: {
>
> There's no need group this block within {} as there are no variables defined inside.
>
> > + /* Detach from network if context is already activated to bypass a firmware bug in sierra HL75xx modules */
> > +
> > + /* Disable registration message handlers so our connect attempt doesn't get cancelled */
> > + set_reg_msg_handlers(ctx, FALSE);
> > +
> > + mm_base_modem_at_command (ctx->modem,
> > + "+CGATT=0",
> > + 30,
> > + FALSE,
> > + (GAsyncReadyCallback) dial_3gpp_res_ready,
> > + task);
> > + return;
> > + }
> > + case DIAL_3GPP_CONTEXT_STEP_SET_XGAUTH: {
> > + gchar *command = build_auth_string (ctx);
>
> Always separate the line declaring the variable with one whiteline and then the first method call, e.g.:
>
> gchar *command;
>
> command = build_auth_string (ctx);
>
> > + mm_base_modem_at_command (ctx->modem,
> > + command,
> > + 3,
> > + FALSE,
> > + (GAsyncReadyCallback)dial_3gpp_res_ready,
> > + task);
> > + g_free (command);
> > + return;
> > + }
> > + case DIAL_3GPP_CONTEXT_STEP_SET_XDNS: {
> > + gchar *command = NULL;
>
> No need to initialize to NULL here.
>
> > +
> > + /* Select IP family with XDNS command */
> > + switch (mm_bearer_properties_get_ip_type (mm_base_bearer_peek_config (MM_BASE_BEARER (ctx->self)))) {
> > + case MM_BEARER_IP_FAMILY_IPV4:
> > + command = g_strdup_printf ("+XDNS=%u,1", ctx->cid);
> > + break;
> > + case MM_BEARER_IP_FAMILY_IPV6:
> > + command = g_strdup_printf ("+XDNS=%u,2", ctx->cid);
> > + break;
> > + case MM_BEARER_IP_FAMILY_IPV4V6: /* Default to IPV4V6 dynamic DNS request */
> > + default:
> > + command = g_strdup_printf ("+XDNS=%u,3", ctx->cid);
> > + break;
> > + }
> > + mm_base_modem_at_command (ctx->modem,
> > + command,
> > + 3,
> > + FALSE,
> > + (GAsyncReadyCallback)dial_3gpp_res_ready,
> > + task);
> > + g_free (command);
> > + return;
> > +
>
> This whiteline not needed.
>
> > + }
> > + case DIAL_3GPP_CONTEXT_STEP_SET_CGACT: {
> > + gchar *command;
>
> Whiteline here missing between variable declarations and the first method call.
>
> > + command = g_strdup_printf ("+CGACT=1,%u", ctx->cid);
> > + mm_base_modem_at_command (ctx->modem,
> > + command,
> > + 30,
> > + FALSE,
> > + (GAsyncReadyCallback)dial_3gpp_res_ready,
> > + task);
> > + g_free (command);
> > + return;
> > +
> > + }
> > + case DIAL_3GPP_CONTEXT_STEP_SET_DATA_CHAN: {
> > + gchar *command;
>
> Whiteline here missing between variable declarations and the first method call.
>
> > + /* Setup data channel mapping */
> > + /* See Intel XMM7160 AT Functional Spec for more info on this command */
> > + command = g_strdup_printf ("+XDATACHANNEL=1,1,\"/USBCDC/%u\",\"/USBHS/NCM/%u\",2,%u",
> > + ctx->usb_acm_config_index, ctx->usb_interface_config_index, ctx->cid);
> > + mm_base_modem_at_command (ctx->modem,
> > + command,
>
> The usb_acm_config_index value is associated to the primary port. Does this mean that this command MUST be sent via that same port? If so, you'd need to call mm_base_modem_at_command_full() instead, which is the method that accepts giving an explicit AT port.
>
> > + 3,
> > + FALSE,
> > + (GAsyncReadyCallback)dial_3gpp_res_ready,
> > + task);
> > + g_free (command);
> > + return;
> > + }
> > + case DIAL_3GPP_CONTEXT_STEP_START_DATA_CHAN: {
> > + gchar *command;
>
> Whiteline here missing between variable declarations and the first method call.
>
> > + command = g_strdup_printf ("+CGDATA=\"M-RAW_IP\",%u", ctx->cid);
> > + mm_base_modem_at_command (ctx->modem,
> > + command,
> > + 10,
> > + FALSE,
> > + (GAsyncReadyCallback)dial_3gpp_res_ready,
> > + task);
> > + g_free (command);
> > + return;
> > + }
> > + case DIAL_3GPP_CONTEXT_STEP_LAST: {
> > + /* Restore message handlers */
> > + set_reg_msg_handlers(ctx, TRUE);
>
> Whitespace needed before the '(' always.
>
> > +
> > + g_task_return_pointer (task, g_object_ref (ctx->data), g_object_unref);
> > + g_object_unref (task);
> > + return;
> > + }
> > + }
>
> In order to avoid having the ugly double line with a "}", you can just skip grouping with {} this LAST step as there is no variable being defined inside the block.
>
> > +
>
> Empty whiteline not needed.
>
> > +}
> > +
> > +static void
> > +dial_3gpp (MMBroadbandBearer *self,
> > + MMBaseModem *modem,
> > + MMPortSerialAt *primary,
> > + guint cid,
> > + GCancellable *cancellable,
> > + GAsyncReadyCallback callback,
> > + gpointer user_data)
> > +{
> > + GTask *task;
> > + Dial3gppContext *ctx;
> > + GError *error = NULL;
> > +
> > + g_assert (primary != NULL);
> > +
> > + /* Setup task */
> > + task = g_task_new (self, cancellable, callback, user_data);
> > + ctx = g_slice_new0 (Dial3gppContext);
> > + g_task_set_task_data (task, ctx, (GDestroyNotify) dial_3gpp_context_free);
> > +
> > + /* Setup Context */
> > + ctx->self = g_object_ref (self);
>
> ctx->self wouldn't be needed here.
>
> > + ctx->modem = g_object_ref (modem);
> > + ctx->primary = g_object_ref (primary);
> > + ctx->cid = cid;
> > + ctx->step = DIAL_3GPP_CONTEXT_STEP_CHECK_CGACT;
>
> E.g. as said before, you would do ctx->step = DIAL_3GPP_CONTEXT_STEP_FIRST; here always.
>
> > + ctx->creg_regexes = NULL;
> > +
> > + /* Get a net port for the connection */
> > + ctx->data = mm_base_modem_peek_best_data_port (MM_BASE_MODEM (modem), MM_PORT_TYPE_NET);
> > + if (!ctx->data) {
> > + g_task_return_new_error (task, MM_CORE_ERROR, MM_CORE_ERROR_NOT_FOUND,
> > + "No valid data port found to launch connection");
>
> Looks like this line previous isn't properly aligned?
>
> > + g_object_unref (task);
> > + return;
> > + }
> > + g_object_ref (ctx->data);
> > +
> > + ctx->secondary = mm_base_modem_get_port_secondary (MM_BASE_MODEM (modem));
>
> Don't rely on always having a secondary port; instead you can do:
>
> ctx->secondary = mm_base_modem_peek_port_secondary (MM_BASE_MODEM (modem));
> if (ctx->secondary)
> g_object_ref (ctx->secondary);
>
> > +
> > + /* Validate the USB configuration */
> > + ctx->usb_interface_config_index = get_usb_ncm_config_index(ctx->data, &error);
> > + ctx->usb_acm_config_index = get_usb_acm_config_index(ctx->primary, &error);
> > + if (ctx->usb_interface_config_index < 0 || ctx->usb_acm_config_index < 0)
> > + {
>
> The open braces should go in the same line as the if().
>
> > + g_task_return_error (task, error);
> > + g_object_unref (task);
> > + return;
> > + }
> > +
> > + /* Run! */
> > + dial_3gpp_context_step (task);
> > +}
> > +
> > +/*****************************************************************************/
> > +/* 3GPP IP config retrieval (sub-step of the 3GPP Connection sequence) */
> > +
> > +typedef enum {
>
> _FIRST step here as well.
>
> > + GET_IP_CONFIG_3GPP_CONTEXT_STEP_CGPADDR,
> > + GET_IP_CONFIG_3GPP_CONTEXT_STEP_XDNS,
> > + GET_IP_CONFIG_3GPP_CONTEXT_STEP_LAST
> > +} GetIPConfig3gppContextStep;
> > +
> > +typedef struct {
> > + MMBroadbandBearerInfineon *self;
>
> And no self needed as the owner object goes in the GTask as source_object.
>
> > + MMBaseModem *modem;
> > + MMPortSerialAt *primary;
> > + guint cid;
> > + MMBearerIpFamily ip_family;
> > + MMPort *data;
> > + GetIPConfig3gppContextStep step;
> > + MMBearerIpConfig *ipv4_config;
> > + MMBearerIpConfig *ipv6_config;
> > +} GetIPConfig3gppContext;
> > +
> > +static void
> > +get_ip_config_3gpp_context_free (GetIPConfig3gppContext *ctx)
> > +{
> > + g_object_unref (ctx->modem);
> > + g_object_unref (ctx->primary);
> > + g_object_unref (ctx->data);
> > + if (ctx->ipv4_config)
> > + g_object_unref (ctx->ipv4_config);
> > + if (ctx->ipv6_config)
> > + g_object_unref (ctx->ipv6_config);
>
> Just g_clear_object() instead of the if+unref; we're trying to switch to that new style.
>
> > +}
> > +
> > +/* forward declaration */
> > +static void get_ip_config_3gpp_context_step (GTask *task);
> > +
> > +static gboolean
> > +get_ip_config_3gpp_finish (MMBroadbandBearer *self,
> > + GAsyncResult *res,
> > + MMBearerIpConfig **ipv4_config,
> > + MMBearerIpConfig **ipv6_config,
> > + GError **error)
> > +{
> > + MMBearerConnectResult *configs;
> > + MMBearerIpConfig *ipv4;
> > + MMBearerIpConfig *ipv6;
> > +
> > + configs = g_task_propagate_pointer (G_TASK (res), error);
> > + if (!configs)
> > + return FALSE;
> > +
> > + g_assert(configs != NULL);
>
> No need for the previous assert really, it's implicit as part of the propagate_pointer() result. But if you want to keep it, add a whitespace before the '(' :)
>
> > +
> > + /* IPv4 config */
> > + ipv4 = mm_bearer_connect_result_peek_ipv4_config (configs);
> > + if (ipv4)
> > + *ipv4_config = g_object_ref (ipv4);
> > +
> > + /* IPv6 config */
> > + ipv6 = mm_bearer_connect_result_peek_ipv6_config (configs);
> > +
> > + if (ipv6)
> > + *ipv6_config = g_object_ref (ipv6);
> > +
> > + mm_bearer_connect_result_unref (configs);
> > + return TRUE;
> > +}
> > +
> > +static void
> > +ip_info_ready (MMBaseModem *modem,
> > + GAsyncResult *res,
> > + GTask *task)
> > +{
> > + const gchar *response;
> > + GError *error = NULL;
> > + guint cid;
> > + gchar *ipv4addr = NULL;
> > + gchar *ipv6addr = NULL;
> > + GetIPConfig3gppContext *ctx;
> > +
> > + ctx = (GetIPConfig3gppContext *) g_task_get_task_data (task);
> > +
> > + response = mm_base_modem_at_command_finish (modem, res, &error);
> > + if (!response) {
> > + g_task_return_error (task, error);
> > + g_object_unref (task);
> > + return;
> > + }
> > +
> > + /* Parse response */
> > + if (!mm_3gpp_parse_cgpaddr_write_response (response, &cid, &ipv4addr, &ipv6addr)) {
>
> This method should have set a GError output if FALSE being returned.
>
> > + g_task_return_new_error (task,
> > + MM_CORE_ERROR,
> > + MM_CORE_ERROR_FAILED,
> > + "Error parsing +CGPADDR response: '%s'",
> > + response);
> > + g_object_unref (task);
> > + return;
> > + }
> > +
> > + g_warn_if_fail (cid == ctx->cid);
> > + /* Create IPv4 config */
> > + if (ipv4addr)
> > + {
>
> The open braces in the same line as the if()
>
> > + ctx->ipv4_config = mm_bearer_ip_config_new ();
> > + mm_bearer_ip_config_set_method (ctx->ipv4_config, MM_BEARER_IP_METHOD_STATIC);
> > + mm_bearer_ip_config_set_address (ctx->ipv4_config, ipv4addr);
> > + mm_bearer_ip_config_set_prefix (ctx->ipv4_config, 0);
> > + g_free (ipv4addr);
> > + }
> > + /* IPv6 config */
> > + if (ipv6addr)
> > + {
>
> The open braces in the same line as the if()
>
> > + ctx->ipv6_config = mm_bearer_ip_config_new ();
> > + mm_bearer_ip_config_set_method (ctx->ipv6_config, MM_BEARER_IP_METHOD_STATIC);
> > + mm_bearer_ip_config_set_address (ctx->ipv6_config, ipv6addr);
> > + mm_bearer_ip_config_set_prefix (ctx->ipv6_config, 0);
> > + g_free (ipv6addr);
> > + }
> > +
> > + ctx->step++;
> > + get_ip_config_3gpp_context_step (task);
> > +}
> > +
> > +static GList
> > +*deduplicate_string_list (GList *list)
>
> The * goes in the same line as the "static GList".
>
> > +{
> > + GList *a, *b, *dup, *ret_list;
>
> Whiteline here.
>
> > + ret_list = list;
> > + a = list;
> > + for (a = ret_list; a != NULL; a = a->next) {
>
> for (a = ret_list; a; a = g_list_next (a)) {
>
> > + b = a->next;
> > + while (NULL != b) {
> > +
> > + if(0 == g_strcmp0 (a->data, b->data)) {
>
> if (!g_strcmp0 (a->data, b->data)) {
>
> > + dup = b;
> > + b = b->next;
> > + g_free (dup->data);
> > + ret_list = g_list_remove (ret_list, dup->data);
> > + } else {
> > + b = b->next;
> > + }
> > + }
> > + }
> > + return ret_list;
>
> Can this really happen? Can we get multiple XDNS lines with the same DNS servers being given? We should have unit tests for those cases.
>
> > +}
> > +
> > +static gboolean
> > +parse_xdns_query_response (const gchar *reply,
> > + guint cid,
> > + guint max_dns_servers,
> > + gchar **dns_v4,
> > + gchar **dns_v6)
>
> Instead of filling in the arrays given as input, the method should better just return newly allocated NULL-terminated arrays, see below.
>
> Also, it should set an output GError if FALSE is being returned, so that the caller can use it to finish the task.
>
> And actually, this parser deserves a whole new unit test setup with several test cases covering both IPv4 and IPv6 addresses. Could you also add that?
>
> > +{
> > + GRegex *r;
> > + GMatchInfo *match_info;
> > + guint this_cid = 0;
> > + guint num_v4 = 0;
> > + guint num_v6 = 0;
> > + GList *dns_list, *a;
> > + gchar *dns1, *dns2;
> > +
> > + dns1 = NULL;
> > + dns2 = NULL;
> > + dns_list = NULL;
> > + a = NULL;
>
> You can initialize the variables directly when declaring them above.
>
> > +
> > + r = g_regex_new ("\\+XDNS:\\s*(\\d+)\\s*,\\s*\"(.*)\"\\s*,\\s*\"(.*)\"",
> > + G_REGEX_OPTIMIZE | G_REGEX_RAW | G_REGEX_MULTILINE | G_REGEX_MATCH_NEWLINE_ANY,
> > + 0, NULL);
> > + g_assert (r != NULL);
> > +
> > + g_regex_match (r, reply, 0, &match_info);
> > + while (g_match_info_matches (match_info)) {
> > + if (mm_get_uint_from_match_info (match_info, 1, &this_cid) &&
> > + (this_cid == cid) &&
> > + (dns1 = mm_get_string_unquoted_from_match_info (match_info, 2)) != NULL &&
> > + (dns2 = mm_get_string_unquoted_from_match_info (match_info, 3)) != NULL)
> > + {
> > + dns_list = g_list_prepend(dns_list, dns1);
> > + dns_list = g_list_prepend(dns_list, dns2);
> > + }
> > + g_match_info_next (match_info, NULL);
> > + }
> > + g_match_info_free (match_info);
> > + g_regex_unref (r);
> > +
> > + dns_list = deduplicate_string_list(dns_list);
>
> missing whitespace before (
>
> > +
> > + for (a = dns_list; a != NULL; a = a->next) {
> > + /* Check if IPv6 address */
> > + if (g_strrstr (a->data, ":")) {
> > + if (num_v6 < max_dns_servers) {
> > + dns_v6[num_v6] = a->data;
> > + num_v6++;
> > + }
> > + else {
> > + g_free (a->data);
> > + }
> > + /* Process "empty" IPv4 address */
> > + } else if (0 == g_strcmp0("0.0.0.0", a->data)) {
> > + g_free (a->data);
> > + }
> > + /* Process IPv4 address */
> > + else if (num_v4 < max_dns_servers) {
> > + dns_v4[num_v4] = a->data;
> > + num_v4++;
> > + }
> > + else {
> > + g_free (a->data);
> > + }
> > + }
>
> If you use a GPtrArray to start adding the elements you find while parsing for each IPv4 IPv6 type, you can then add a last NULL item and get a GStrv (gchar **) out of it with, e.g.:
>
> arr = g_ptr_array_new ();
> g_ptr_array_add (arr, "1.2.3.4");
> g_ptr_array_add (arr, "8.8.8.8");
> g_ptr_array_add (arr, NULL);
> *dns_v4 = (GStrv) g_ptr_array_free (arr, FALSE);
>
> The caller would be responsible for g_strfreev()-ing the returned GStrv when no longer needed.
>
>
> > +
> > + g_list_free(dns_list);
> > + return num_v4 > 0 || num_v6 > 0;
> > +}
> > +
> > +static void
> > +dns_info_ready (MMBaseModem *modem,
> > + GAsyncResult *res,
> > + GTask *task)
> > +{
> > + static const gint MAX_DNS_SERVERS = 2;
> > + const gchar *response;
> > + GError *error = NULL;
> > + gchar *dns_v4[MAX_DNS_SERVERS + 1];
> > + gchar *dns_v6[MAX_DNS_SERVERS + 1];
> > + GetIPConfig3gppContext *ctx;
> > +
> > + memset(dns_v4, 0, sizeof( gchar *) * MAX_DNS_SERVERS + 1);
> > + memset(dns_v6, 0, sizeof( gchar *) * MAX_DNS_SERVERS + 1);
> > +
> > + ctx = (GetIPConfig3gppContext *) g_task_get_task_data (task);
> > +
> > + response = mm_base_modem_at_command_finish (modem, res, &error);
> > + if (!response) {
> > + g_task_return_error (task, error);
> > + g_object_unref (task);
> > + return;
> > + }
> > +
> > + /* Parse XDNS results and return two vectors containing IPV4 and IPV6 addresses */
> > + if (!parse_xdns_query_response (response, ctx->cid, MAX_DNS_SERVERS, dns_v4, dns_v6)) {
>
> This method could handle output GStrv (gchar **) for both dns_v4 and dns_v6. i.e. have the method do the parsing internally and return a NULL-terminated array of strings for each member. This would allow us to get rid of all MAX_DNS_SERVERS constant, and the static arrays. Then we can just g_strfreev() these returned arrays once they have been set in the MMBearerIpConfig below.
>
> > + g_task_return_new_error (task, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
> > + "Error parsing +XDNS response: '%s'",
> > + response);
>
> Alignment wrong in previous lines.
>
> > + g_object_unref (task);
> > + return;
> > + }
> > + if (ctx->ipv4_config)
> > + mm_bearer_ip_config_set_dns (ctx->ipv4_config, (const gchar **)dns_v4);
> > +
> > + if (ctx->ipv6_config)
> > + mm_bearer_ip_config_set_dns (ctx->ipv6_config, (const gchar **)dns_v6);
> > +
> > + ctx->step++;
> > + get_ip_config_3gpp_context_step (task);
> > +}
> > +
> > +static void
> > +get_ip_config_3gpp_context_step (GTask *task)
> > +{
> > + GetIPConfig3gppContext *ctx;
> > +
> > + ctx = (GetIPConfig3gppContext *) g_task_get_task_data (task);
> > +
> > + mm_dbg("GetIPConfig Step : %d", ctx->step);
>
> Same as above, better a more descriptive message for the user. Also, missing whitespace before '('.
>
> > +
> > + switch (ctx->step) {
> > + case GET_IP_CONFIG_3GPP_CONTEXT_STEP_CGPADDR: {
> > + gchar *command = g_strdup_printf ("+CGPADDR=%u", ctx->cid);
>
> Same as above, missing whiteline between the variable declaration and the first method call.
>
> > + mm_base_modem_at_command (ctx->modem,
> > + command,
> > + 3,
> > + FALSE,
> > + (GAsyncReadyCallback)ip_info_ready,
> > + task);
> > + g_free (command);
> > + return;
> > + }
> > + case GET_IP_CONFIG_3GPP_CONTEXT_STEP_XDNS: {
> > + /* query DNS addresses */
> > + mm_base_modem_at_command (ctx->modem,
> > + "+XDNS?",
> > + 3,
> > + FALSE,
> > + (GAsyncReadyCallback)dns_info_ready,
> > + task);
> > + return;
> > + }
>
> No need to group the case logic with {} if not defining variables inside.
>
> > + case GET_IP_CONFIG_3GPP_CONTEXT_STEP_LAST: {
> > +
> > + g_task_return_pointer (task,
> > + mm_bearer_connect_result_new (ctx->data, ctx->ipv4_config, ctx->ipv6_config),
> > + (GDestroyNotify) mm_bearer_connect_result_unref);
> > +
> > + g_object_unref (task);
> > + return;
> > + }
>
> No need to group the case logic with {} if not defining variables inside.
>
> > + }
> > +}
> > +
> > +static void
> > +get_ip_config_3gpp (MMBroadbandBearer *self,
> > + MMBroadbandModem *modem,
> > + MMPortSerialAt *primary,
> > + MMPortSerialAt *secondary,
> > + MMPort *data,
> > + guint cid,
> > + MMBearerIpFamily ip_family,
> > + GAsyncReadyCallback callback,
> > + gpointer user_data)
> > +{
> > + GTask *task;
> > + GetIPConfig3gppContext *ctx;
> > +
> > + g_assert (primary != NULL);
> > + g_assert (data != NULL);
> > + g_assert (modem != NULL);
> > +
> > + /* Setup task and create disconnect context */
> > + task = g_task_new (self, NULL, callback, user_data);
> > + ctx = g_slice_new0 (GetIPConfig3gppContext);
> > + g_task_set_task_data (task, ctx, (GDestroyNotify) get_ip_config_3gpp_context_free);
> > +
> > + /* Setup context */
> > + ctx->self = g_object_ref (self);
> > + ctx->modem = g_object_ref (modem);
> > + ctx->primary = g_object_ref (primary);
> > + ctx->cid = cid;
> > + ctx->ip_family = ip_family;
> > + ctx->data = g_object_ref (data);
> > + ctx->step = GET_IP_CONFIG_3GPP_CONTEXT_STEP_CGPADDR;
>
> step = FIRST;
>
> > + ctx->ipv4_config = NULL;
> > + ctx->ipv6_config = NULL;
>
> No need to explicitly initialize members to NULL when using g_slice_new0().
>
> > +
> > + /* Start */
> > + get_ip_config_3gpp_context_step (task);
> > +}
> > +
>
> Single whiteline here.
>
> > +
> > +/*****************************************************************************/
> > +/* Disconnect 3GPP */
> > +
> > +typedef enum {
>
> _FIRST enum as above.
>
> > + DISCONNECT_3GPP_CONTEXT_STEP_CGACT,
> > + DISCONNECT_3GPP_CONTEXT_STEP_DYN_DNS,
> > + DISCONNECT_3GPP_CONTEXT_STEP_LAST
> > +} Disconnect3gppContextStep;
> > +
> > +typedef struct {
> > + MMBroadbandBearerInfineon *self;
>
> No need to keep self in the context, as above.
>
> > + MMBaseModem *modem;
> > + MMPortSerialAt *primary;
> > + MMPort *data;
> > + guint cid;
> > + Disconnect3gppContextStep step;
> > +} Disconnect3gppContext;
> > +
> > +static void
> > +disconnect_3gpp_context_free (Disconnect3gppContext *ctx)
> > +{
> > + g_object_unref (ctx->self);
> > + g_object_unref (ctx->modem);
> > + g_object_unref (ctx->primary);
> > + g_object_unref (ctx->data);
> > + g_slice_free (Disconnect3gppContext, ctx);
> > +}
> > +
> > +static gboolean
> > +disconnect_3gpp_finish (MMBroadbandBearer *self,
> > + GAsyncResult *res,
> > + GError **error)
> > +{
> > + return g_task_propagate_boolean (G_TASK (res), error);
> > +}
> > +
> > +/* forward declaration */
> > +static void
> > +disconnect_3gpp_context_step (GTask *task);
> > +
> > +static void
> > +cgact_disconnect_ready (MMBaseModem *modem,
> > + GAsyncResult *res,
> > + GTask *task)
> > +{
> > + Disconnect3gppContext *ctx;
> > + GError *error = NULL;
> > +
> > + mm_base_modem_at_command_finish (modem, res, &error);
> > +
> > + if (error) {
> > + if(!g_error_matches (error,
>
> Missing whitespace before the '('.
>
> > + MM_CONNECTION_ERROR,
> > + MM_CONNECTION_ERROR_NO_CARRIER))
>
> Alignment wrong in previous lines.
>
> > + {
> > + mm_dbg ("PDP context deactivation failed (not fatal): %s", error->message);
>
> And indentation wrong in this previous one.
>
> > + }
> > + g_error_free (error);
> > + }
> > +
> > + ctx = (Disconnect3gppContext *) g_task_get_task_data (task);
> > +
> > + /* Go on to next step */
> > + ctx->step++;
> > + disconnect_3gpp_context_step (task);
> > +}
> > +
> > +static void
> > +xdns_disconnect_ready (MMBaseModem *modem,
> > + GAsyncResult *res,
> > + GTask *task)
> > +{
> > + Disconnect3gppContext *ctx;
> > + GError *error = NULL;
> > +
> > + mm_base_modem_at_command_finish (modem, res, &error);
> > +
> > + if (error) {
> > + mm_dbg ("Dynamic DNS disable request failed: %s", error->message);
> > + g_error_free (error);
> > + }
> > +
> > + ctx = (Disconnect3gppContext *) g_task_get_task_data (task);
> > +
> > + /* Go on to next step */
> > + ctx->step++;
> > + disconnect_3gpp_context_step (task);
> > +}
> > +
> > +static void
> > +disconnect_3gpp_context_step (GTask *task)
> > +{
> > + Disconnect3gppContext *ctx;
> > +
> > + ctx = (Disconnect3gppContext *) g_task_get_task_data (task);
> > +
> > + mm_dbg("Disconnect3gpp Step : %d", ctx->step);
>
> Same as above, maybe add some more descriptive message instead of the Disconnect3gpp thing. I always try to think that this may be read by a user, not by a developer.
>
> > +
> > + switch (ctx->step) {
> > + case DISCONNECT_3GPP_CONTEXT_STEP_CGACT: {
> > + gchar *command;
> > +
> > + command = g_strdup_printf ("+CGACT=0,%u",
> > + ctx->cid );
> > + mm_base_modem_at_command (ctx->modem,
> > + command,
> > + 30,
> > + FALSE,
> > + (GAsyncReadyCallback) cgact_disconnect_ready,
> > + task);
> > + g_free (command);
> > + return;
> > + }
> > +
> > + case DISCONNECT_3GPP_CONTEXT_STEP_DYN_DNS: {
> > + gchar *command;
> > +
> > + command = g_strdup_printf ("+XDNS=%u,0",
> > + ctx->cid);
> > + mm_base_modem_at_command (ctx->modem,
> > + command,
> > + 10,
> > + FALSE,
> > + (GAsyncReadyCallback) xdns_disconnect_ready,
> > + task);
> > + g_free (command);
> > + return;
> > + }
> > +
> > + case DISCONNECT_3GPP_CONTEXT_STEP_LAST:{
> > + g_task_return_boolean (task, TRUE);
> > + g_object_unref (task);
> > + return;
> > + }
>
> No need to group the case logic within {} as no variables are being defined within the block.
>
> > + }
> > +}
> > +
> > +static void
> > +disconnect_3gpp (MMBroadbandBearer *self,
> > + MMBroadbandModem *modem,
> > + MMPortSerialAt *primary,
> > + MMPortSerialAt *secondary,
> > + MMPort *data,
> > + guint cid,
> > + GAsyncReadyCallback callback,
> > + gpointer user_data)
> > +{
> > + GTask *task;
> > + Disconnect3gppContext *ctx;
> > +
> > + g_assert (primary != NULL);
> > + g_assert (data != NULL);
> > + g_assert (modem != NULL);
> > +
> > + /* Setup task and create disconnect context */
> > + task = g_task_new (self, NULL, callback, user_data);
> > + ctx = g_slice_new0 (Disconnect3gppContext);
> > + g_task_set_task_data (task, ctx, (GDestroyNotify) disconnect_3gpp_context_free);
> > +
> > + /* Setup context */
> > + ctx->self = g_object_ref (self);
> > + ctx->modem = g_object_ref (modem);
> > + ctx->primary = g_object_ref (primary);
> > + ctx->data = g_object_ref (data);
> > + ctx->cid = cid;
> > + ctx->step = DISCONNECT_3GPP_CONTEXT_STEP_CGACT;
> > +
> > + /* Start */
> > + disconnect_3gpp_context_step (task);
> > +}
> > +
> > +/*****************************************************************************/
> > +
> > +MMBaseBearer *
> > +mm_broadband_bearer_infineon_new_finish (GAsyncResult *res,
> > + GError **error)
> > +{
> > + GObject *bearer;
> > + GObject *source;
> > +
> > + source = g_async_result_get_source_object (res);
> > + bearer = g_async_initable_new_finish (G_ASYNC_INITABLE (source), res, error);
> > + g_object_unref (source);
> > +
> > + if (!bearer)
> > + return NULL;
> > +
> > + /* Only export valid bearers */
> > + mm_base_bearer_export (MM_BASE_BEARER (bearer));
> > +
> > + return MM_BASE_BEARER (bearer);
> > +}
> > +
> > +void
> > +mm_broadband_bearer_infineon_new (MMBroadbandModemInfineon *modem,
> > + MMBearerProperties *config,
> > + GCancellable *cancellable,
> > + GAsyncReadyCallback callback,
> > + gpointer user_data)
> > +{
> > + g_async_initable_new_async (
> > + MM_TYPE_BROADBAND_BEARER_INFINEON,
> > + G_PRIORITY_DEFAULT,
> > + cancellable,
> > + callback,
> > + user_data,
> > + MM_BASE_BEARER_MODEM, modem,
> > + MM_BASE_BEARER_CONFIG, config,
> > + NULL);
> > +}
> > +
> > +static void
> > +mm_broadband_bearer_infineon_init (MMBroadbandBearerInfineon *self)
> > +{
> > +
> > + return;
>
> This return is not needed.
>
> > +}
> > +
> > +static void
> > +mm_broadband_bearer_infineon_class_init (MMBroadbandBearerInfineonClass *klass)
> > +{
> > + MMBroadbandBearerClass *broadband_bearer_class = MM_BROADBAND_BEARER_CLASS (klass);
> > +
> > + broadband_bearer_class->dial_3gpp = dial_3gpp;
> > + broadband_bearer_class->dial_3gpp_finish = dial_3gpp_finish;
> > +
> > + broadband_bearer_class->get_ip_config_3gpp = get_ip_config_3gpp;
> > + broadband_bearer_class->get_ip_config_3gpp_finish = get_ip_config_3gpp_finish;
> > +
>
> Single whiteline here.
>
> > +
> > + broadband_bearer_class->disconnect_3gpp = disconnect_3gpp;
> > + broadband_bearer_class->disconnect_3gpp_finish = disconnect_3gpp_finish;
> > +}
> > diff --git a/plugins/infineon/mm-broadband-bearer-infineon.h b/plugins/infineon/mm-broadband-bearer-infineon.h
> > new file mode 100644
> > index 0000000..cfc6910
> > --- /dev/null
> > +++ b/plugins/infineon/mm-broadband-bearer-infineon.h
> > @@ -0,0 +1,59 @@
> > +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> > +/*
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details:
> > + *
> > + * Copyright (C) 2013 Aleksander Morgado <aleksander at gnu.org>
> > + */
> > +
> > +#ifndef MM_BROADBAND_BEARER_INFINEON_H
> > +#define MM_BROADBAND_BEARER_INFINEON_H
> > +
> > +#include <glib.h>
> > +#include <glib-object.h>
> > +
> > +#define _LIBMM_INSIDE_MM
> > +#include <libmm-glib.h>
> > +
> > +#include "mm-broadband-bearer.h"
> > +#include "mm-broadband-modem-infineon.h"
> > +
> > +#define MM_TYPE_BROADBAND_BEARER_INFINEON (mm_broadband_bearer_infineon_get_type ())
> > +#define MM_BROADBAND_BEARER_INFINEON(obj) (G_TYPE_CHECK_INSTANCE_CAST ((obj), MM_TYPE_BROADBAND_BEARER_INFINEON, MMBroadbandBearerInfineon))
> > +#define MM_BROADBAND_BEARER_INFINEON_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST ((klass), MM_TYPE_BROADBAND_BEARER_INFINEON, MMBroadbandBearerInfineonClass))
> > +#define MM_IS_BROADBAND_BEARER_INFINEON(obj) (G_TYPE_CHECK_INSTANCE_TYPE ((obj), MM_TYPE_BROADBAND_BEARER_INFINEON))
> > +#define MM_IS_BROADBAND_BEARER_INFINEON_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), MM_TYPE_BROADBAND_BEARER_INFINEON))
> > +#define MM_BROADBAND_BEARER_INFINEON_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS ((obj), MM_TYPE_BROADBAND_BEARER_INFINEON, MMBroadbandBearerInfineonClass))
> > +
> > +typedef struct _MMBroadbandBearerInfineon MMBroadbandBearerInfineon;
> > +typedef struct _MMBroadbandBearerInfineonClass MMBroadbandBearerInfineonClass;
> > +typedef struct _MMBroadbandBearerInfineonPrivate MMBroadbandBearerInfineonPrivate;
> > +
> > +struct _MMBroadbandBearerInfineon {
> > + MMBroadbandBearer parent;
> > + MMBroadbandBearerInfineonPrivate *priv;
> > +};
> > +
> > +struct _MMBroadbandBearerInfineonClass {
> > + MMBroadbandBearerClass parent;
> > +};
> > +
> > +GType mm_broadband_bearer_infineon_get_type (void);
> > +
> > +/* Default 3GPP bearer creation implementation */
> > +void mm_broadband_bearer_infineon_new (MMBroadbandModemInfineon *modem,
> > + MMBearerProperties *config,
> > + GCancellable *cancellable,
> > + GAsyncReadyCallback callback,
> > + gpointer user_data);
> > +MMBaseBearer *mm_broadband_bearer_infineon_new_finish (GAsyncResult *res,
> > + GError **error);
>
> Alignment wrong in previous line.
>
> > +
> > +#endif /* MM_BROADBAND_BEARER_INFINEON_H */
> > diff --git a/plugins/infineon/mm-broadband-modem-infineon.c b/plugins/infineon/mm-broadband-modem-infineon.c
> > new file mode 100755
> > index 0000000..f20804f
> > --- /dev/null
> > +++ b/plugins/infineon/mm-broadband-modem-infineon.c
> > @@ -0,0 +1,216 @@
> > +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> > +/*
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details:
> > + *
> > + * Copyright (C) 2013 Aleksander Morgado <aleksander at gnu.org>
> > + * Copyright (C) 2017 Tempered Networks Inc.
> > + */
> > +
> > +#include <config.h>
> > +
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <unistd.h>
> > +#include <ctype.h>
> > +
> > +#include "ModemManager.h"
> > +#include "mm-base-modem-at.h"
> > +#include "mm-serial-parsers.h"
> > +#include "mm-log.h"
> > +#include "mm-errors-types.h"
> > +#include "mm-iface-modem.h"
> > +#include "mm-iface-modem-3gpp.h"
> > +#include "mm-broadband-bearer-infineon.h"
> > +#include "mm-broadband-modem-infineon.h"
> > +
> > +static void iface_modem_init (MMIfaceModem *iface);
> > +
> > +
> > +G_DEFINE_TYPE_EXTENDED (MMBroadbandModemInfineon, mm_broadband_modem_infineon, MM_TYPE_BROADBAND_MODEM, 0,
> > + G_IMPLEMENT_INTERFACE (MM_TYPE_IFACE_MODEM, iface_modem_init))
> > +
> > +/*****************************************************************************/
> > +/* Create Bearer (Modem interface) */
> > +
> > +static MMBaseBearer *
> > +modem_create_bearer_finish (MMIfaceModem *self,
> > + GAsyncResult *res,
> > + GError **error)
> > +{
> > + MMBaseBearer *bearer;
> > +
> > + bearer = g_simple_async_result_get_op_res_gpointer (G_SIMPLE_ASYNC_RESULT (res));
> > + mm_dbg ("New Infineon bearer created at DBus path '%s'", mm_base_bearer_get_path (bearer));
> > +
> > + return g_object_ref (bearer);
> > +}
> > +
> > +static void
> > +broadband_bearer_infineon_new_ready (GObject *source,
> > + GAsyncResult *res,
> > + GSimpleAsyncResult *simple)
> > +{
> > + MMBaseBearer *bearer = NULL;
> > + GError *error = NULL;
> > +
> > + bearer = mm_broadband_bearer_infineon_new_finish (res, &error);
> > + if (!bearer)
> > + g_simple_async_result_take_error (simple, error);
> > + else
> > + g_simple_async_result_set_op_res_gpointer (simple,
> > + bearer,
> > + (GDestroyNotify)g_object_unref);
> > + g_simple_async_result_complete (simple);
> > + g_object_unref (simple);
> > +}
> > +
> > +static void
> > +modem_create_bearer (MMIfaceModem *self,
> > + MMBearerProperties *properties,
> > + GAsyncReadyCallback callback,
> > + gpointer user_data)
> > +{
> > + GSimpleAsyncResult *result;
>
> GTask?
>
> > +
> > + result = g_simple_async_result_new (G_OBJECT (self),
> > + callback,
> > + user_data,
> > + modem_create_bearer);
>
>
> If for any reason the modem ends up without a data port grabbed, it should create a generic bearer, e.g.:
>
> if (!mm_base_modem_peek_best_data_port (MM_BASE_MODEM (self), MM_PORT_TYPE_NET) {
> mm_broadband_bearer_new (MM_BROADBAND_MODEM_INFINEON (self),
> properties,
> NULL, /* cancellable */
> (GAsyncReadyCallback)broadband_bearer_new_ready,
> result);
> return;
> }
>
> > +
> > + mm_broadband_bearer_infineon_new (MM_BROADBAND_MODEM_INFINEON (self),
> > + properties,
> > + NULL, /* cancellable */
> > + (GAsyncReadyCallback)broadband_bearer_infineon_new_ready,
> > + result);
> > +}
> > +
> > +/*****************************************************************************/
> > +/* Reset (Modem interface) */
> > +
> > +static gboolean
> > +modem_reset_finish (MMIfaceModem *self,
> > + GAsyncResult *res,
> > + GError **error)
> > +{
> > + return !!mm_base_modem_at_command_finish (MM_BASE_MODEM (self), res, error);
> > +}
> > +
> > +static void
> > +modem_reset (MMIfaceModem *self,
> > + GAsyncReadyCallback callback,
> > + gpointer user_data)
> > +{
> > + mm_base_modem_at_command (MM_BASE_MODEM (self),
> > + "AT+CFUN=1,1",
> > + 3,
> > + FALSE,
> > + callback,
> > + user_data);
> > +}
> > +
> > +/*****************************************************************************/
> > +/* MODEM POWER DOWN */
>
> Un-capitalize the previous comment
>
> > +
> > +static gboolean
> > +modem_power_down_finish (MMIfaceModem *self,
> > + GAsyncResult *res,
> > + GError **error)
> > +{
> > + return !!mm_base_modem_at_command_finish (MM_BASE_MODEM (self), res, error);
> > +}
> > +
> > +static void
> > +modem_power_down (MMIfaceModem *self,
> > + GAsyncReadyCallback callback,
> > + gpointer user_data)
> > +{
> > + /* Use AT+CFUN=4 for power down. It will stop the RF (IMSI detach), and
> > + * keeps access to the SIM */
> > + mm_base_modem_at_command (MM_BASE_MODEM (self),
> > + "+CFUN=4",
> > + /* The modem usually completes +CFUN=4 within 1-2 seconds,
> > + * but sometimes takes a ridiculously long time (~30-35 seconds).
> > + * It's better to have a long timeout here than to have the
> > + * modem not responding to subsequent AT commands until +CFUN=4
> > + * completes. */
> > + 40,
> > + FALSE,
> > + callback,
> > + user_data);
> > +}
> > +
> > +
> > +/*****************************************************************************/
> > +/* Modem power up (Modem interface) */
> > +
> > +static gboolean
> > +modem_power_up_finish (MMIfaceModem *self,
> > + GAsyncResult *res,
> > + GError **error)
> > +{
> > + return !g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT (res), error);
>
> Actually this is wrong, if the async method is calling mm_base_modem_at_command() directly the finish method should call mm_base_modem_at_command_finish().
>
> > +}
> > +
> > +static void
> > +modem_power_up (MMIfaceModem *self,
> > + GAsyncReadyCallback callback,
> > + gpointer user_data)
> > +{
> > + mm_base_modem_at_command (MM_BASE_MODEM (self),
> > + "+CFUN=1",
> > + 10,
> > + FALSE,
> > + callback,
> > + user_data);
> > +}
>
> Is this custom power up method even needed? The generic implementation should already do this.
>
> > +
> > +/*****************************************************************************/
> > +
> > +MMBroadbandModemInfineon *
> > +mm_broadband_modem_infineon_new (const gchar *device,
> > + const gchar **drivers,
> > + const gchar *plugin,
> > + guint16 vendor_id,
> > + guint16 product_id)
> > +{
> > + return g_object_new (MM_TYPE_BROADBAND_MODEM_INFINEON,
> > + MM_BASE_MODEM_DEVICE, device,
> > + MM_BASE_MODEM_DRIVERS, drivers,
> > + MM_BASE_MODEM_PLUGIN, plugin,
> > + MM_BASE_MODEM_VENDOR_ID, vendor_id,
> > + MM_BASE_MODEM_PRODUCT_ID, product_id,
> > + NULL);
> > +}
> > +
> > +static void
> > +mm_broadband_modem_infineon_init (MMBroadbandModemInfineon *self)
> > +{
> > +}
> > +
> > +static void
> > +iface_modem_init (MMIfaceModem *iface)
> > +{
> > + iface->create_bearer = modem_create_bearer;
> > + iface->create_bearer_finish = modem_create_bearer_finish;
> > + iface->reset = modem_reset;
> > + iface->reset_finish = modem_reset_finish;
> > + iface->modem_power_down = modem_power_down;
> > + iface->modem_power_down_finish = modem_power_down_finish;
> > + iface->modem_power_up = modem_power_up;
> > + iface->modem_power_up_finish = modem_power_up_finish;
> > +
> > +}
> > +
> > +static void
> > +mm_broadband_modem_infineon_class_init (MMBroadbandModemInfineonClass *klass)
> > +{
> > +}
> > diff --git a/plugins/infineon/mm-broadband-modem-infineon.h b/plugins/infineon/mm-broadband-modem-infineon.h
> > new file mode 100644
> > index 0000000..1b4ee5a
> > --- /dev/null
> > +++ b/plugins/infineon/mm-broadband-modem-infineon.h
> > @@ -0,0 +1,47 @@
> > +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> > +/*
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details:
> > + *
> > + * Copyright (C) 2013 Aleksander Morgado <aleksander at gnu.org>
> > + */
> > +
> > +#ifndef MM_BROADBAND_MODEM_INFINEON_H
> > +#define MM_BROADBAND_MODEM_INFINEON_H
> > +
> > +#include "mm-broadband-modem.h"
> > +
> > +#define MM_TYPE_BROADBAND_MODEM_INFINEON (mm_broadband_modem_infineon_get_type ())
> > +#define MM_BROADBAND_MODEM_INFINEON(obj) (G_TYPE_CHECK_INSTANCE_CAST ((obj), MM_TYPE_BROADBAND_MODEM_INFINEON, MMBroadbandModemInfineon))
> > +#define MM_BROADBAND_MODEM_INFINEON_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST ((klass), MM_TYPE_BROADBAND_MODEM_INFINEON, MMBroadbandModemInfineonClass))
> > +#define MM_IS_BROADBAND_MODEM_INFINEON(obj) (G_TYPE_CHECK_INSTANCE_TYPE ((obj), MM_TYPE_BROADBAND_MODEM_INFINEON))
> > +#define MM_IS_BROADBAND_MODEM_INFINEON_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), MM_TYPE_BROADBAND_MODEM_INFINEON))
> > +#define MM_BROADBAND_MODEM_INFINEON_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS ((obj), MM_TYPE_BROADBAND_MODEM_INFINEON, MMBroadbandModemInfineonClass))
> > +
> > +typedef struct _MMBroadbandModemInfineon MMBroadbandModemInfineon;
> > +typedef struct _MMBroadbandModemInfineonClass MMBroadbandModemInfineonClass;
> > +
> > +struct _MMBroadbandModemInfineon {
> > + MMBroadbandModem parent;
> > +};
> > +
> > +struct _MMBroadbandModemInfineonClass{
> > + MMBroadbandModemClass parent;
> > +};
> > +
> > +GType mm_broadband_modem_infineon_get_type (void);
> > +
> > +MMBroadbandModemInfineon *mm_broadband_modem_infineon_new (const gchar *device,
> > + const gchar **drivers,
> > + const gchar *plugin,
> > + guint16 vendor_id,
> > + guint16 product_id);
> > +
> > +#endif /* MM_BROADBAND_MODEM_INFINEON_H */
> > diff --git a/plugins/infineon/mm-plugin-infineon.c b/plugins/infineon/mm-plugin-infineon.c
> > new file mode 100755
> > index 0000000..11ab596
> > --- /dev/null
> > +++ b/plugins/infineon/mm-plugin-infineon.c
> > @@ -0,0 +1,88 @@
> > +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> > +
> > +/*
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of the
> > + * License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public
> > + * License along with this program; if not, write to the
> > + * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
> > + * Boston, MA 02111-1307, USA.
> > + *
> > + * Copyright (C) 2013 Aleksander Morgado <aleksander at gnu.org>
> > + * Copyright (C) 2017 Tempered Networks Inc.
> > + */
> > +
> > +#include <string.h>
> > +#include <gmodule.h>
> > +
> > +#define _LIBMM_INSIDE_MM
> > +#include <libmm-glib.h>
> > +
> > +#include "mm-broadband-modem-infineon.h"
> > +#include "mm-plugin-infineon.h"
> > +#include "mm-log.h"
> > +
> > +G_DEFINE_TYPE (MMPluginInfineon, mm_plugin_infineon, MM_TYPE_PLUGIN)
> > +
> > +int mm_plugin_major_version = MM_PLUGIN_MAJOR_VERSION;
> > +int mm_plugin_minor_version = MM_PLUGIN_MINOR_VERSION;
> > +
> > +static MMBaseModem *
> > +create_modem (MMPlugin *self,
> > + const gchar *sysfs_path,
> > + const gchar **drivers,
> > + guint16 vendor,
> > + guint16 product,
> > + GList *probes,
> > + GError **error)
> > +{
> > + return MM_BASE_MODEM (mm_broadband_modem_infineon_new (sysfs_path,
> > + drivers,
> > + mm_plugin_get_name (self),
> > + vendor,
> > + product));
> > +}
> > +
> > +/*****************************************************************************/
> > +
> > +G_MODULE_EXPORT MMPlugin *
> > +mm_plugin_create (void)
> > +{
> > + static const gchar *subsystems[] = { "tty", "net", "usb", NULL };
> > + static const guint16 vendor_ids[] = { 0x1519, 0 };
> > + // We only want to support the NCM based USB composition.
> > + static const guint16 product_ids_allow[] = { 0x0443, 0};
> > + // Otherwise fall back on the Generic plugin.
>
> Comments always /* like this */ even if single-line ones.
>
> > + static const guint16 product_ids_forbid[] = { 0x0020, 0};
>
> You can remove the MM_PLUGIN_FORBIDDEN_PRODUCT_IDS if you're already giving a list of ALLOWED.
>
> > +
> > + return MM_PLUGIN (
> > + g_object_new (MM_TYPE_PLUGIN_INFINEON,
> > + MM_PLUGIN_NAME, "Infineon",
> > + MM_PLUGIN_ALLOWED_SUBSYSTEMS, subsystems,
> > + MM_PLUGIN_ALLOWED_VENDOR_IDS, vendor_ids,
> > + MM_PLUGIN_ALLOWED_PRODUCT_IDS, product_ids_allow,
> > + MM_PLUGIN_FORBIDDEN_PRODUCT_IDS, product_ids_forbid,
> > + MM_PLUGIN_ALLOWED_AT, TRUE,
>
> Align all values in the same column better.
>
> > + NULL));
> > +}
> > +
> > +static void
> > +mm_plugin_infineon_init (MMPluginInfineon *self)
> > +{
> > +}
> > +
> > +static void
> > +mm_plugin_infineon_class_init (MMPluginInfineonClass *klass)
> > +{
> > + MMPluginClass *plugin_class = MM_PLUGIN_CLASS (klass);
> > +
> > + plugin_class->create_modem = create_modem;
> > +}
> > diff --git a/plugins/infineon/mm-plugin-infineon.h b/plugins/infineon/mm-plugin-infineon.h
> > new file mode 100644
> > index 0000000..d0f8e47
> > --- /dev/null
> > +++ b/plugins/infineon/mm-plugin-infineon.h
> > @@ -0,0 +1,46 @@
> > +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> > +
> > +/*
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of the
> > + * License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public
> > + * License along with this program; if not, write to the
> > + * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
> > + * Boston, MA 02111-1307, USA.
> > + *
> > + * Copyright (C) 2013 Aleksander Morgado <aleksander at gnu.org>
> > + */
> > +
> > +#ifndef MM_PLUGIN_INFINEON_H
> > +#define MM_PLUGIN_INFINEON_H
> > +
> > +#include "mm-plugin.h"
> > +
> > +#define MM_TYPE_PLUGIN_INFINEON (mm_plugin_infineon_get_type ())
> > +#define MM_PLUGIN_INFINEON(obj) (G_TYPE_CHECK_INSTANCE_CAST ((obj), MM_TYPE_PLUGIN_INFINEON, MMPluginInfineon))
> > +#define MM_PLUGIN_INFINEON_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST ((klass), MM_TYPE_PLUGIN_INFINEON, MMPluginInfineonClass))
> > +#define MM_IS_PLUGIN_INFINEON(obj) (G_TYPE_CHECK_INSTANCE_TYPE ((obj), MM_TYPE_PLUGIN_INFINEON))
> > +#define MM_IS_PLUGIN_INFINEON_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), MM_TYPE_PLUGIN_INFINEON))
> > +#define MM_PLUGIN_INFINEON_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS ((obj), MM_TYPE_PLUGIN_INFINEON, MMPluginInfineonClass))
> > +
> > +typedef struct {
> > + MMPlugin parent;
> > +} MMPluginInfineon;
> > +
> > +typedef struct {
> > + MMPluginClass parent;
> > +} MMPluginInfineonClass;
> > +
> > +GType mm_plugin_infineon_get_type (void);
> > +
> > +G_MODULE_EXPORT MMPlugin *mm_plugin_create (void);
> > +
> > +#endif /* MM_PLUGIN_INFINEON_H */
> > diff --git a/src/mm-modem-helpers.c b/src/mm-modem-helpers.c
> > index 679f925..04984d1 100644
> > --- a/src/mm-modem-helpers.c
> > +++ b/src/mm-modem-helpers.c
> > @@ -2724,6 +2724,46 @@ mm_3gpp_parse_cscs_test_response (const gchar *reply,
> > /*************************************************************************/
> >
> > gboolean
> > +mm_3gpp_parse_cgpaddr_write_response (const gchar *reply,
> > + guint *cid,
> > + gchar **ipv4addr,
> > + gchar **ipv6addr)
>
> This should return a GError by reference if FALSE returned, as all the other response parsers.
>
> > +{
> > + GRegex *r;
> > + GMatchInfo *match_info;
> > + gboolean success = FALSE;
> > +
> > + g_return_val_if_fail (reply != NULL, FALSE);
> > + g_return_val_if_fail (cid != NULL, FALSE);
> > + g_return_val_if_fail (ipv4addr != NULL, FALSE);
> > + g_return_val_if_fail (ipv6addr != NULL, FALSE);
>
> This is not a library API, so if you want to make sure all parameters given are != NULL, g_assert() them directly :) E.g.
>
> g_assert (reply);
> g_assert (cid);
> g_assert (ipv4addr);
> g_assert (ipv6addr);
>
> > +
> > +
> > + r = g_regex_new ("\\+CGPADDR:\\s*(\\d+)\\s*,(\"([^\"]*)\"),*(\"([^\"]*)\")*",
> > + G_REGEX_OPTIMIZE | G_REGEX_RAW,
> > + 0, NULL);
> > + g_assert (r != NULL);
> > +
> > + if (g_regex_match (r, reply, 0, &match_info)) {
> > + if (mm_get_uint_from_match_info (match_info, 1, cid) &&
> > + (*ipv4addr = mm_get_string_unquoted_from_match_info (match_info, 3)) != NULL)
> > + success = TRUE;
> > + if (mm_get_uint_from_match_info (match_info, 1, cid) &&
> > + (*ipv6addr = mm_get_string_unquoted_from_match_info (match_info, 5)) != NULL)
> > + success = TRUE;
>
> Looks like the CID is being read twice? I guess you want to return TRUE if either of ipv4 or ipv6 address are parsed correctly; why not:
>
> success = FALSE;
>
> if (g_regex_match ()) {
> if (!mm_get_uint_from_match_info (match_info, 1, cid) {
> g_set_error (error, .... "couldn't parse cid");
> goto out;
> }
>
> *ipv4addr = mm_get_string_unquoted_from_match_info (match_info, 3);
> *ipv6addr = mm_get_string_unquoted_from_match_info (match_info, 5);
>
> if (!(*ipv4addr) && (!*ipv6addr)) {
> g_set_error (error, .... "couldn't parse cid");
> goto out;
> }
>
> success = TRUE;
> }
>
> out:
> g_match_info_free (match_info);
> g_regex_unref (r);
> return success;
>
>
> Also alignment seems broken in the second if().
>
> > + mm_dbg("Parsed ipv4 addr: %s", *ipv4addr);
> > + mm_dbg("Parsed ipv6 addr: %s", *ipv6addr);
>
> You would be trying to print a NULL string if any of the previous ifs() failed; better:
>
> mm_dbg("Parsed ipv4 addr: %s", *ipv4addr ? *ipv4addr : "none");
> mm_dbg("Parsed ipv4 addr: %s", *ipv6addr ? *ipv6addr : "none");
>
> > +
> > + }
> > + g_match_info_free (match_info);
> > + g_regex_unref (r);
> > +
> > + return success;
> > +}
> > +
> > +/*************************************************************************/
> > +
> > +gboolean
> > mm_3gpp_parse_clck_test_response (const gchar *reply,
> > MMModem3gppFacility *out_facilities)
> > {
> > diff --git a/src/mm-modem-helpers.h b/src/mm-modem-helpers.h
> > index 297e15d..1296d47 100644
> > --- a/src/mm-modem-helpers.h
> > +++ b/src/mm-modem-helpers.h
> > @@ -216,6 +216,12 @@ gboolean mm_3gpp_get_cpms_storage_match (GMatchInfo *match_info,
> > gboolean mm_3gpp_parse_cscs_test_response (const gchar *reply,
> > MMModemCharset *out_charsets);
> >
> > +/* AT+CGPADDR=X (IP address) response parser */
> > +gboolean mm_3gpp_parse_cgpaddr_write_response (const gchar *reply,
> > + guint *cid,
> > + gchar **ipv4addr,
> > + gchar **ipv6addr);
> > +
> > /* AT+CLCK=? (Supported locks) response parser */
> > gboolean mm_3gpp_parse_clck_test_response (const gchar *reply,
> > MMModem3gppFacility *out_facilities);
> > --
> > 2.7.4
>
>
> --
> Aleksander
> https://aleksander.es
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mm_hl7588_cgdeact.log
Type: application/octet-stream
Size: 187871 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20170531/1974f0e3/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mm_hl7588_cgact_cgatt.log
Type: application/octet-stream
Size: 210784 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20170531/1974f0e3/attachment-0003.obj>
More information about the ModemManager-devel
mailing list