<div dir="ltr">Back from the grave,<div> </div><div>Here is finally the update... to the above code reviewed patch. A big thank you Dan and Aleksander for the detail in the review it was really a great learning experience for me. Please don't hold back with this one either :). It's a pretty big rewrite so I'm sure there will still be some minor things to tidy up still but I think it should be more inline now. There is one thing in here for sure that needs fixing still but I wanted your options so please see the note at line 1192 and let me know what you think. I promise I will be much faster on getting the next tidy up done (>_<) and after this I still have a few more patches.</div><div> </div><div>Cheers,</div><div>Matt</div></div><div class="gmail_extra"> <div class="gmail_quote">On Thu, Sep 15, 2016 at 5:30 AM, Aleksander Morgado <<a href="mailto:aleksander@aleksander.es" target="_blank">aleksander@aleksander.es</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hey Matthew, I reviewed the first patch of the series only for now; let's try to get this right first and then we go on with the other ones. See comments inline below. Some of the comments are coding style only things, but others try to simplify or change the flow of the logic, let me know what you think. On 06/09/16 03:47, matthew stanger wrote: > Here is try two. First off if you are uncomfortable with the attachment let > me know and I'll send the patch in the body. It's just 4k lines and Chrome > almost crashes when I paste it in. I won't take offence if that is needed > though. I can also squash it down to a single commit if that makes viewing > it easier. > You're not supposed to "paste" the patch in the email body or "merge" the patches in a single file and attach them to the email :) The target is that we can take the patches and quickly apply them with "git am"; if you paste the patch in the body it's likely that your email application may decide to modify the body before sending it; if you merge patches together in a single file, "git am" may end up choking when processing it. Sending patches can be done in 3 different ways: a) Use git-send-email command line tool, this will generate emails that can then be saved and applied with "git am". b) Use git format-patch and generate 1 file per patch; then attach the patches to the email. Again, we can use "git am" to apply them. c) Prepare a custom branch somewhere; e.g. bitbucket or github, and email the link to the branch so that we can pull from it. For reviewing, I usually prefer a), and if too complex to setup, just do b). For large developments c) is preferred, though. > The attached patch consist of 5 commits since I branched from ' > 59f57befa4be81b562f9adfbac39fbe19d88c111'. The first commit being the > original I sent and the next 4 address suggestions made by Dan. I tried to > get the GPS separated but there is some hardship in doing that with our > commit history. Reviewing logic is very very hard with commits including different things :/ I was able to make all of the changes except, I don't think > I can move PDP_CID to modem initialization as it is tied to the net > interface being used by the bearer. Leading in to answer your last > question, it's not hard coded technically but our tec rep's at Cinterion > told me to map these contexts to these interface to avoid issues, though it > does seem to work with any context to any interface. I could not > get report_connection_status() to trigger off a manual connect cancellation > either so I left that logic in. As far as using the MM core for CGDCONT and > PDP ctx I thought that would only work if I was using > the MM_BROADBAND_MODEM class instead of the MM_BROADBAND_MODEM_CINTERION > class when creating a bearer. I may be super wrong on that though. I just > say that as I wasn't able to trace how it was called inside > of find_cid_ready in mm-bb-bearer.c. I though because of that class I had > to handle our own CGD & PDP ctx but if that's not the case then please let > me know how I can do that. I made all the other changes & to be honest it > was quite a mess so thanks for the feedback. > > I also see you are using an old AT command ref for this modem. Cinterions > newest is 3.017. I can't give it to you due to NDA but if you want it I'd > be happy to lobby for you to Cinterion so they will give you a new copy. I > have already been lobbying MM to them just FYI, hoping they might take some > of the programming burden but they seemed really only focused on windows > atm. I'm not sure if you guys have PLS8 modems either but if you want ones > that's another thing I'm sure I could help you get. Having you test > anything I push even for 5 mins is well worth it so if you need anything > like that please let me know. Cinterion did send me a test-only PLS8 to play with the QMI integration they had, but not sure if they're still targeting to use QMI or not. See comments for the first patch below. > From 3b45e2f8afdd9f3aa284f3938e618b4b5437c572 Mon Sep 17 00:00:00 2001 > From: Matthew Stanger <<a href="mailto:matthew_stanger@trimble.com">matthew_stanger@trimble.com</a>> > Date: Fri, 26 Aug 2016 16:35:49 -0600 > Subject: [PATCH 1/5] Features: Added SWWAN support in Cinterion plugin. > Updated GPS support for Cinterion modems that use new SGPSC commands. Added > LTE status for Cinterion plugin. Made general Cinterion plugin improvements > to better support PLS8-X & PLS8-E modems. > > Known issues: > SWWAN connection not tied into mmcli status updates. > Unknown logic flow for APN's which require User & Password. > No support for IP version. > Does not pull IP address from SWWAN into mmcli. > Does not auto preform DHCP for SWWAN connection. > Dual pdp context connections not yet supported. > Simple connect/disconnect does not clean up bearer. > --- > plugins/Makefile.am | 2 + > plugins/cinterion/77-mm-cinterion-port-types.rules | 2 +- > plugins/cinterion/mm-broadband-bearer-cinterion.c | 1126 ++++++++++++++++++++ > plugins/cinterion/mm-broadband-bearer-cinterion.h | 56 + > plugins/cinterion/mm-broadband-modem-cinterion.c | 279 ++++- > plugins/cinterion/mm-broadband-modem-cinterion.h | 3 + > plugins/cinterion/mm-common-cinterion.c | 402 ++++++- > plugins/cinterion/mm-modem-helpers-cinterion.c | 188 ++++ > plugins/cinterion/mm-modem-helpers-cinterion.h | 17 + > plugins/cinterion/mm-plugin-cinterion.c | 141 ++- > src/mm-base-modem.c | 20 + > src/mm-base-modem.h | 1 + > 12 files changed, 2182 insertions(+), 55 deletions(-) > create mode 100644 plugins/cinterion/mm-broadband-bearer-cinterion.c > create mode 100644 plugins/cinterion/mm-broadband-bearer-cinterion.h > > diff --git a/plugins/Makefile.am b/plugins/Makefile.am > index 018b696..744fd18 100644 > --- a/plugins/Makefile.am > +++ b/plugins/Makefile.am > @@ -565,6 +565,8 @@ libmm_plugin_cinterion_la_SOURCES = \ > cinterion/mm-common-cinterion.h \ > cinterion/mm-broadband-modem-cinterion.c \ > cinterion/mm-broadband-modem-cinterion.h \ > + cinterion/mm-broadband-bearer-cinterion.c \ > + cinterion/mm-broadband-bearer-cinterion.h \ > $(NULL) > if WITH_QMI > libmm_plugin_cinterion_la_SOURCES += \ > diff --git a/plugins/cinterion/77-mm-cinterion-port-types.rules b/plugins/cinterion/77-mm-cinterion-port-types.rules > index 09de742..ee386f0 100644 > --- a/plugins/cinterion/77-mm-cinterion-port-types.rules > +++ b/plugins/cinterion/77-mm-cinterion-port-types.rules > @@ -8,4 +8,4 @@ SUBSYSTEMS=="usb", ATTRS{bInterfaceNumber}=="?*", ENV{.MM_USBIFNUM}="$attr{bInte > > ATTRS{idVendor}=="1e2d", ATTRS{idProduct}=="0053", ENV{.MM_USBIFNUM}=="01", ENV{ID_MM_CINTERION_PORT_TYPE_GPS}="1" > > -LABEL="mm_cinterion_port_types_end" > +LABEL="mm_cinterion_port_types_end" > \ No newline at end of file This previous change may be skipped. > diff --git a/plugins/cinterion/mm-broadband-bearer-cinterion.c b/plugins/cinterion/mm-broadband-bearer-cinterion.c > new file mode 100644 > index 0000000..eb73cf5 > --- /dev/null > +++ b/plugins/cinterion/mm-broadband-bearer-cinterion.c > @@ -0,0 +1,1126 @@ > +/* -*- 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) 2016 Trimble Navigation Limited > + * Author: Matthew Stanger <<a href="mailto:matthew_stanger@trimble.com">matthew_stanger@trimble.com</a>> > + */ > + > +#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> > +#include "mm-base-modem-at.h" > +#include "mm-broadband-bearer-cinterion.h" > +#include "mm-log.h" > +#include "mm-modem-helpers.h" > +#include "mm-modem-helpers-cinterion.h" > +#include "mm-daemon-enums-types.h" > + > +G_DEFINE_TYPE (MMBroadbandBearerCinterion, mm_broadband_bearer_cinterion, MM_TYPE_BROADBAND_BEARER); No need for the last ";" > + > +/*****************************************************************************/ > +/* Common enums and structs */ > + > +typedef enum { > + MM_BEARER_CINTERION_AUTH_UNKNOWN = -1, > + MM_BEARER_CINTERION_AUTH_NONE = 0, > + MM_BEARER_CINTERION_AUTH_PAP = 1, > + MM_BEARER_CINTERION_AUTH_CHAP = 2, > + MM_BEARER_CINTERION_AUTH_MSCHAPV2 = 3, > +} MMBearerCinterionAuthPref; > + No need for the MM prefix in the enum if it's not public. > +typedef enum { > + CONNECTION_UNKNOWN = 0, > + CONNECTION_ACTIVE, > + CONNECTION_INACTIVE, > +} SWWAN_USB_CONNECTION_STATE; Enum types should be CamelCaseLikeThis. > + > +typedef enum { > + CONNECT_3GPP_CONTEXT_STEP_INIT = 0, > + CONNECT_3GPP_CONTEXT_STEP_VERIFY_SWWAN, > + CONNECT_3GPP_CONTEXT_STEP_BEARER_PROPERTIES, > + CONNECT_3GPP_CONTEXT_STEP_SET_SWWAN, > + CONNECT_3GPP_CONTEXT_STEP_CONNECTION_STATUS, > + CONNECT_3GPP_CONTEXT_STEP_DONE, > +} Connect3gppContextStep; > + > +typedef enum { > + DISCONNECT_3GPP_CONTEXT_STEP_INIT = 0, > + DISCONNECT_3GPP_CONTEXT_STEP_SWWAN_DETACH, > + DISCONNECT_3GPP_CONTEXT_STEP_CONNECTION_STATUS, > + DISCONNECT_3GPP_CONTEXT_STEP_DONE > +} Disconnect3gppContextStep; > + > +typedef struct { > + MMBroadbandBearerCinterion *self; > + MMBaseModem *modem; > + MMPortSerialAt *primary; > + MMPort *data; > + Connect3gppContextStep connect; > + Disconnect3gppContextStep disconnect; > + SWWAN_USB_CONNECTION_STATE usb0_state; > + SWWAN_USB_CONNECTION_STATE usb1_state; > + MMBearerIpConfig *ipv4_config; > + GCancellable *cancellable; > + GSimpleAsyncResult *result; > +} Control3gppContext; I think it should be better to have separate Connect/Disconnect contexts. > + > +struct _MMBroadbandBearerCinterionPrivate { > + gpointer connect_pending; > + gpointer disconnect_pending; > + guint network_disconnect_pending_id;/* Tag for the post task for network-initiated disconnect */ > + const gchar *bearer_interface; > + const gchar *pdp_cid; This should be a uint. > + guint retry_count; Retry count looks like part of the connection/disconnect logic; shouldn't it go in the connect/disconnect context? > + guint swwan_read_write; > +}; > + > +/*****************************************************************************/ > +/* Common 3GPP Function Declarations */ > +static void connect_3gpp_context_step (Control3gppContext *ctx); > +static void disconnect_3gpp_context_step (Control3gppContext *ctx); > +static void connect_3gpp_context_complete_and_free (Control3gppContext *ctx); > +static void disconnect_3gpp_context_complete_and_free (Control3gppContext *ctx); The connect/disconnect logic should have different contexts, and then the complete_and_free() names could match the context name. E.g. Connect3gppContext with connect_3gpp_context_step() and connect_3gpp_context_complete_and_free (). > + > +/*****************************************************************************/ > +/* Common 3GPP */ > + > +static MMPortSerialAt * > +get_dial_port (MMBroadbandModemCinterion *modem, > + MMPort *data, > + MMPortSerialAt *primary) > +{ > + //Use only the primary port for sending AT commands. Coding style thing; we don't use "//" ever, not even for single-comment lines. Use /* this instead */. Applies to multiple places in the code. > + return g_object_ref (primary); What is the purpose of this method if it's just ref-ing the passed primary port? Why not just "g_object_ref (primary)" in the caller? > +} > + > +static void > +cinterion_3gpp_state_machine_logic (MMBroadbandBearerCinterion *self) > +{ > + Control3gppContext *ctx; > + > + //For connecting flows. > + if (self->priv->connect_pending != NULL) > + { Coding style thing only; the open-braces at the end of the if() line; e.g. if (1) { /* something */ } > + ctx = self->priv->connect_pending; > + > + switch (ctx->connect) { > + > + //Always go from INIT to VERIFY_SWWAN. > + case CONNECT_3GPP_CONTEXT_STEP_INIT: { > + ctx->connect = CONNECT_3GPP_CONTEXT_STEP_VERIFY_SWWAN; > + break; This step just wants to get to the next step. So just do ctx->connect++; and fall down to the next case without a break; E.g. case CONNECT_3GPP_CONTEXT_STEP_INIT: ctx->connect++; /* fall down */ case CONNECT_3GPP_CONTEXT_STEP_VERIFY_SWWAN: .... > + } > + > + //Always go from VERIFY_SWWAN to BEARER_PROPERTIES. > + case CONNECT_3GPP_CONTEXT_STEP_VERIFY_SWWAN: { The {} for the case is only really needed if you're going to define variables within the case clause; I would remove those otherwise. > + //Only try 5 times to check the connection then give up. > + if (self->priv->retry_count > 5) > + { > + g_simple_async_result_set_error (ctx->result, > + MM_CORE_ERROR, > + MM_CORE_ERROR_TOO_MANY, > + "Unknown bearer state during connection attempt."); > + > + connect_3gpp_context_complete_and_free (ctx); > + return; > + } > + self->priv->retry_count++; > + ctx->connect = CONNECT_3GPP_CONTEXT_STEP_BEARER_PROPERTIES; > + break; Same here for the step and fall-down. > + } > + > + //When in this state we can be setting up a brand new connection or, > + //have just torn down a connection to setup a new one. > + case CONNECT_3GPP_CONTEXT_STEP_BEARER_PROPERTIES: { > + //We were connected (flags still set) & just issued SWWAN disconnect. > + //Now refresh our usb connection state & retry. > + //TODO: Rework for dual sim connections. > + if (ctx->usb0_state == CONNECTION_ACTIVE || ctx->usb1_state == CONNECTION_ACTIVE) > + { > + ctx->connect = CONNECT_3GPP_CONTEXT_STEP_VERIFY_SWWAN; Just set the new step to run and call connect_3gpp_context_step (ctx); directly. E.g.: if (...) { ctx->connect = NEXT_STEP; connect_3gpp_context_step (ctx); return; } if (some other thing) { ctx->connect = SOME_OTHER_NEXT_STEP; connect_3gpp_context_step (ctx); return; } ... > + } > + //There is no active connection and we set the context correctly so continue to active SWWAN. > + else if (ctx->usb0_state == CONNECTION_INACTIVE || ctx->usb1_state == CONNECTION_INACTIVE) > + ctx->connect = CONNECT_3GPP_CONTEXT_STEP_SET_SWWAN; > + else > + { > + //Error, should be impossible to get here. State machine broken. If this is impossible to get here, just g_assert_not_reached(); don't return an error to the caller if this is a programmer error. > + ctx->self->priv->connect_pending = NULL; > + g_simple_async_result_set_error (ctx->result, > + MM_CORE_ERROR, > + MM_CORE_ERROR_WRONG_STATE, > + "Unknown bearer state during connection attempt."); > + > + connect_3gpp_context_complete_and_free (ctx); > + return; > + } > + > + break; > + } > + > + //Always verify that the SWWAN connection is active after trying to set it up. > + //Same step as CONNECT_3GPP_CONTEXT_STEP_VERIFY_SWWAN but makes it easy to detect > + //if we just came from a new or old connection. > + case CONNECT_3GPP_CONTEXT_STEP_SET_SWWAN: { > + ctx->connect = CONNECT_3GPP_CONTEXT_STEP_CONNECTION_STATUS; > + break; Again, fall-down without break;. > + } > + > + //If SWWAN was just set, we expect it should be on. > + case CONNECT_3GPP_CONTEXT_STEP_CONNECTION_STATUS: { > + if ((g_ascii_strncasecmp(ctx->self->priv->bearer_interface, "usb0", 4)==0 && ctx->usb0_state == CONNECTION_ACTIVE) || > + (g_ascii_strncasecmp(ctx->self->priv->bearer_interface, "usb1", 4)==0 && ctx->usb1_state == CONNECTION_ACTIVE)) No, this is not right, usb0 and usb1 are network interface names that the kernel decided to use; what if you have 2 Cinterion modems at the same time? Instead of relying on the interface name, you may want to rely on the USB interface number; e.g. g_udev_device_get_property_as_int ( udev_device, "ID_USB_INTERFACE_NUM") NOTE: you'll need a GUdevDevice for the WWAN; which you could get using a new GUdevClient. This will be much easier to handle once the work I'm doing to allow skipping udev is integrated, as then you can just get a MMKernelDevice from the MMPort and query the property directly, like: mm_kernel_device_get_property_as_int ( mm_port_peek_kernel_device (port), "ID_USB_INTERFACE_NUM"); See e.g. this similar case in the Huawei plugin: <a href="https://lists.freedesktop.org/archives/modemmanager-devel/2016-August/003279.html" rel="noreferrer" target="_blank">https://lists.freedesktop.org/archives/modemmanager-devel/2016-August/003279.html</a> > + ctx->connect = CONNECT_3GPP_CONTEXT_STEP_DONE; > + else > + { > + g_simple_async_result_set_error (ctx->result, > + MM_CORE_ERROR, > + MM_CORE_ERROR_WRONG_STATE, > + "Unknown bearer state during connection attempt."); > + > + connect_3gpp_context_complete_and_free (ctx); > + return; > + } You could have a single if() to catch the error case, and then do the ctx->connect++; and /* fall-down */ thing to get to the next case. > + > + break; > + } > + > + case CONNECT_3GPP_CONTEXT_STEP_DONE: { > + //Place holder, nothing should call this. Fall into defualt error. > + } > + > + default: { I personally don't like a default case in a state machine where the step is a known enumeration. > + mm_err ("Unexpected SWWAN connect state. Unable to advance."); > + ctx->self->priv->connect_pending = NULL; > + g_simple_async_result_set_error (ctx->result, > + MM_CORE_ERROR, > + MM_CORE_ERROR_WRONG_STATE, > + "Unexpected SWWAN connect state. Unable to advance."); > + > + connect_3gpp_context_complete_and_free (ctx); > + return; > + } > + > + > + } > + > + connect_3gpp_context_step (ctx); This connect_3gpp_context_step() wouldn't be needed as each case above should call it itself when needed and directly return. > + } > + //Assume disconnecting flow, assert protects assumption. Separate logic methods for connection and disconnection, please. > + else > + { > + g_assert (self->priv->disconnect_pending != NULL); > + ctx = self->priv->disconnect_pending; > + > + switch (ctx->disconnect) { > + > + case DISCONNECT_3GPP_CONTEXT_STEP_INIT: { > + ctx->disconnect = DISCONNECT_3GPP_CONTEXT_STEP_SWWAN_DETACH; > + break; Same as above, when just going to the next case, ctx->disconnect++; and /* fall down */ > + } > + > + case DISCONNECT_3GPP_CONTEXT_STEP_SWWAN_DETACH: { > + ctx->disconnect = DISCONNECT_3GPP_CONTEXT_STEP_CONNECTION_STATUS; > + break; Another fall-down? Why do we need this step then? > + } > + > + case DISCONNECT_3GPP_CONTEXT_STEP_CONNECTION_STATUS: { > + //We expect the bearer to be disconnected now. If it's not try again. > + //TODO: Rework for dual sim connections. > + if ((g_ascii_strncasecmp(ctx->self->priv->bearer_interface, "usb0", 4)==0 && ctx->usb0_state == CONNECTION_INACTIVE) || > + (g_ascii_strncasecmp(ctx->self->priv->bearer_interface, "usb1", 4)==0 && ctx->usb1_state == CONNECTION_INACTIVE)) As above, never rely on interface names. > + ctx->disconnect = DISCONNECT_3GPP_CONTEXT_STEP_DONE; > + else > + { > + ctx->disconnect = DISCONNECT_3GPP_CONTEXT_STEP_SWWAN_DETACH; > + sleep (1); //Wait a second on disconnect retry's. Never never ever just sleep() :) If you need to reschedule the same step in 1s, you should: g_timeout_add_seconds ( 1, (GSourceFunc) cinterion_3gpp_state_machine_logic_reschedule_cb, ctx); return; And then implement the callback: static gboolean cinterion_3gpp_state_machine_logic_cb (Control3gppContext *ctx) { cinterion_3gpp_state_machine_logic (ctx); return G_SOURCE_REMOVE; } > + } > + break; > + } > + > + case DISCONNECT_3GPP_CONTEXT_STEP_DONE: { > + //Place holder, nothing should call this. Fall into defualt error. Nothing should call this, yet there is an explicit step selection for this one some lines above... :) ctx->disconnect = DISCONNECT_3GPP_CONTEXT_STEP_DONE; > + } > + > + default: { > + mm_err ("Unexpected SWWAN disconnect state. Unable to advance."); > + ctx->self->priv->connect_pending = NULL; > + g_simple_async_result_set_error (ctx->result, > + MM_CORE_ERROR, > + MM_CORE_ERROR_WRONG_STATE, > + "Unexpected SWWAN disconnect state. Unable to advance."); > + > + connect_3gpp_context_complete_and_free (ctx); > + > + return; > + } > + > + } > + > + disconnect_3gpp_context_step (ctx); As for the connect logic, I think it's better to just call the context_step() method directly after setting the next step, not doing a break; and a single context_step() here. This is because each step will be auto-contained within the case(), I think it helps readability of the state machine. > + } > +} > + > +static gboolean > +proccess_swwan_response(MMBroadbandBearerCinterion *self, GList *result) > +{ > + Control3gppContext *ctx; > + > + if (self->priv->connect_pending != NULL) > + ctx = self->priv->connect_pending; > + else > + { > + g_assert (self->priv->disconnect_pending != NULL); > + ctx = self->priv->disconnect_pending; > + } > + I see this logic mixing connect/disconnect again; I think we shouldn't do this. If the SWWAN response is parsed in the same way for both, better write a parser method and let the different connect/disconnect flows call it. > + > + if (g_list_length(result) != 0) { > + int first_result = GPOINTER_TO_INT(result->data); > + > + //mm_serial_parser_v1_parse will catch CME Error. Parent function will then send > + //that error to dbus out. > + if(first_result == -1) > + return FALSE; > + //Recived an 'OK' response from a write command, place holder so we don't error below. *Received > + if (first_result == 0 && ctx->self->priv->swwan_read_write == 1) > + NULL; > + //Recived an 'OK'(0) response from an swwwan read command. *Received, *swwan > + else if (first_result == 0 && ctx->self->priv->swwan_read_write == 0) > + { > + ctx->usb0_state = CONNECTION_INACTIVE; > + ctx->usb1_state = CONNECTION_INACTIVE; > + } > + //1 || 3 result is the CID, given when that context is activated. > + //TODO: Rework for dual sim connections. > + else if (first_result == 1 && ctx->self->priv->swwan_read_write == 0) > + ctx->usb1_state = CONNECTION_ACTIVE; > + else if (first_result == 3 && ctx->self->priv->swwan_read_write == 0) > + ctx->usb0_state = CONNECTION_ACTIVE; > + else > + { > + for (; result; result = g_list_next (result)) > + mm_err ("Unknown SWWAN response data:%i", GPOINTER_TO_INT(result->data)); > + > + g_simple_async_result_set_error (ctx->result, > + MM_CORE_ERROR, > + MM_CORE_ERROR_FAILED, > + "Internal error while processing SWWAN response."); > + > + return FALSE; > + } > + } > + else { > + mm_err ("Unable to parse zero length SWWAN response."); > + return FALSE; > + } > + > + return TRUE; > +} > + > +static void > +get_swwan_response (MMBaseModem *modem, > + GAsyncResult *res, > + MMBroadbandBearerCinterion *self) > +{ > + Control3gppContext *ctx; > + const gchar *response; > + GError *error = NULL; > + GList *response_parsed = NULL; > + > + if (self->priv->connect_pending != NULL) > + ctx = self->priv->connect_pending; > + else > + { > + g_assert (self->priv->disconnect_pending != NULL); > + ctx = self->priv->disconnect_pending; > + } > + > + // Balance refcount > + g_object_unref (self); Too complex, too complex :). If you had one logic for connect and another logic for disconnect, you could pass around the actual Context struct in the async calls user_data, which holds already a reference to the bearer object. Instead, you're storing the contexts in private info and passing the bearer object. It is good to store the connect and disconnect contexts in the private info ONLY if there is no other place to manage them. If the logic relies on queries to the modem to check e.g. SWWAN status, you can pass around the contexts in the async calls and not ever store them in private info. One of the good reasons to store the context in private info is when you rely on unsolicited messages from the modem. For your use case, I think you could totally avoid storing the contexts in private info, and the logic would be much much simpler. What do you think? > + > + //Parse the swwan response. > + response = mm_base_modem_at_command_finish (modem, res, &error); If !response, just return the error right away. > + > + //1st elem of response_parsed will be: > + //1 or 3 for an active swwan, -1 for a valid error, 0 for assumed 'OK' (no conection) > + if (error == NULL && !mm_cinterion_parse_swwan_response (response, &response_parsed, &error)) > + NULL; Hum? > + > + > + if (error != NULL || !proccess_swwan_response(ctx->self, response_parsed)) { > + ctx->self->priv->connect_pending = NULL; > + ctx->self->priv->disconnect_pending = NULL; > + > + g_simple_async_result_take_error (ctx->result, error); But proccess_swwan_response() doesn't set a GError to return here if it fails. > + > + if (ctx->connect) > + connect_3gpp_context_complete_and_free (ctx); > + else > + disconnect_3gpp_context_complete_and_free (ctx); > + > + return; > + > + } > + > + mm_dbg ("usb0-state:%i usb1-state:%i", ctx->usb0_state, ctx->usb1_state); > + > + g_list_free(response_parsed); > + g_clear_error (&error); You would never need a g_clear_error() here if errors are propagated earlier whenever they happen. > + > + cinterion_3gpp_state_machine_logic(ctx->self); > + Will this run the same state machine step again? I think it is usually more clear to update the step to run, and then call the state machine logic, something like: ctx->step++; context_step (ctx); > + return; Just remove this return; :) > +} > + > +/*****************************************************************************/ > +/* Cinterion AT Command Wrappers */ > + > +static void > +send_swwan_read_command(Control3gppContext *ctx) > +{ > + ctx->self->priv->swwan_read_write = 0; > + I'm sure this swwan_read_write flag could be avoided if different GAsyncReadyCallback are used for read/write operations :) > + //Check for swwan connection. The next state will be preformed by the callback. *performed > + mm_base_modem_at_command_full (ctx->modem, > + ctx->primary, > + "^SWWAN?", > + 5, > + FALSE, > + FALSE, > + NULL, > + (GAsyncReadyCallback)get_swwan_response, > + g_object_ref (ctx->self)); > +} > + > +static void > +send_swwan_connect_command(Control3gppContext *ctx) > +{ > + //USB0(1st wwan adapt) -> 3rd context, USB1(2nd wwan adapt) -> 1st context > + gchar *command; Remove wrong alignment whitespaces. > + command = g_strdup_printf ("^SWWAN=%s,%s,%s", > + "1", > + ctx->self->priv->pdp_cid, //Expect 1 or 3 > + g_ascii_strncasecmp(ctx->self->priv->pdp_cid, "3", 1)==0 ? "1" : "2"); Could you comment here on why "1" or "2" are given here based on CID number? > + > + ctx->self->priv->swwan_read_write = 1; > + > + //Start the swwan connection. The next state will be preformed by the callback. *performed > + mm_base_modem_at_command_full (ctx->modem, > + ctx->primary, > + command, > + 10,/*Seen it take 5 seconds :0 */ > + FALSE, > + FALSE, > + NULL, > + (GAsyncReadyCallback)get_swwan_response, > + g_object_ref (ctx->self)); As said earlier, passing around the context is much simpler. > + > + g_free (command); > +} > + > +static void > +send_swwan_disconnect_command(Control3gppContext *ctx) > +{ > + gchar *command; Remove wrong alignment whitespaces. > + command = g_strdup_printf ("^SWWAN=%s,%s,%s", > + "0", > + ctx->self->priv->pdp_cid, > + g_ascii_strncasecmp(ctx->self->priv->pdp_cid, "3", 1)==0 ? "1" : "2"); > + > + ctx->self->priv->swwan_read_write = 1; > + > + //Check for swwan connection. The next state will be preformed by the callback. > + mm_base_modem_at_command_full (ctx->modem, > + ctx->primary, > + command, > + 10, > + FALSE, > + FALSE, > + NULL, > + (GAsyncReadyCallback)get_swwan_response, > + g_object_ref (ctx->self)); As said earlier, passing around the context is much simpler. > + > + g_free (command); > +} > + Too many whitelines here :) Easy change! > + > + > + > +/*****************************************************************************/ > +/* Connect 3GPP */ > + > +static gint > +cinterion_parse_auth_type (MMBearerAllowedAuth mm_auth) > +{ > + switch (mm_auth) { > + case MM_BEARER_ALLOWED_AUTH_NONE: > + return MM_BEARER_CINTERION_AUTH_NONE; > + case MM_BEARER_ALLOWED_AUTH_PAP: > + return MM_BEARER_CINTERION_AUTH_PAP; > + case MM_BEARER_ALLOWED_AUTH_CHAP: > + return MM_BEARER_CINTERION_AUTH_CHAP; > + case MM_BEARER_ALLOWED_AUTH_MSCHAPV2: > + return MM_BEARER_CINTERION_AUTH_MSCHAPV2; > + default: > + return MM_BEARER_CINTERION_AUTH_UNKNOWN; > + } > +} > + > +static void > +connect_3gpp_context_complete_and_free (Control3gppContext *ctx) > +{ > + g_simple_async_result_complete_in_idle (ctx->result); > + g_object_unref (ctx->cancellable); > + g_object_unref (ctx->result); > + g_object_unref (ctx->modem); > + g_object_unref (ctx->self); > + g_clear_object (&ctx->ipv4_config); > + g_clear_object (&ctx->data); > + g_clear_object (&ctx->primary); > + > + g_slice_free (Control3gppContext, ctx); > +} > + > +static MMBearerConnectResult * > +connect_3gpp_finish (MMBroadbandBearer *self, > + GAsyncResult *res, > + GError **error) > +{ > + if (g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT (res), error)) > + return NULL; > + > + return mm_bearer_connect_result_ref (g_simple_async_result_get_op_res_gpointer (G_SIMPLE_ASYNC_RESULT (res))); > +} > + > +static gchar * > +build_cinterion_pdp_context_string(Control3gppContext *ctx) > +{ > + const gchar *apn = NULL; > + const gchar *user = NULL; > + const gchar *passwd = NULL; > + MMBearerAllowedAuth auth; > + gint encoded_auth = MM_BEARER_CINTERION_AUTH_UNKNOWN; > + gchar *command; > + > + apn = mm_bearer_properties_get_apn (mm_base_bearer_peek_config (MM_BASE_BEARER (ctx->self))); > + user = mm_bearer_properties_get_user (mm_base_bearer_peek_config (MM_BASE_BEARER (ctx->self))); > + passwd = mm_bearer_properties_get_password (mm_base_bearer_peek_config (MM_BASE_BEARER (ctx->self))); > + auth = mm_bearer_properties_get_allowed_auth (mm_base_bearer_peek_config (MM_BASE_BEARER (ctx->self))); > + encoded_auth = cinterion_parse_auth_type (auth); > + > + /* Default to no authentication if not specified */ > + if (encoded_auth == MM_BEARER_CINTERION_AUTH_UNKNOWN) { > + encoded_auth = MM_BEARER_CINTERION_AUTH_NONE; > + mm_dbg ("Unable to detect authentication type. Defaulting to:%i", encoded_auth); > + } > + > + //TODO: Get IP type if specific protocol was specified. Hardcoded to IPV4 for now. > + > + if (!user && !passwd) > + command = g_strdup_printf ("+CGDCONT=%s,\"IP\",\"%s\",\"0.0.0.0\",0,0", > + ctx->self->priv->pdp_cid, > + apn == NULL ? "" : apn); > + > + /*TODO: Can't test this as we can't get a hold of a SIM w/ this feature atm. This is a place > + * holder. When we can test this then it should be refactored & split into another state so we > + * can tell independently if the SGAUTH fails. > + * Write Command > + * AT^SGAUTH=<cid>[, <auth_type>[, <passwd>, <user>]] > + * Response(s)  > + * OK > + * ERROR > + * +CME ERROR: <err> > + */ > + else > + command = g_strdup_printf ("^SGAUTH=%s,%i,%s,%s; +CGDCONT=%s,\"IP\",\"%s\",\"0.0.0.0\",0,0 ", > + ctx->self->priv->pdp_cid, > + encoded_auth, > + passwd == NULL ? "" : passwd, > + user == NULL ? "" : user, > + ctx->self->priv->pdp_cid, > + apn == NULL ? "" : apn); I wouldn't chain up these 2 AT commands. > + return command; > +} > + > +static gboolean > +set_pdp_cid(Control3gppContext *ctx) > +{ > + GUdevClient *client; > + GUdevDevice *data_device; > + > + //Need the data port to figure out what context to activate. > + client = g_udev_client_new (NULL); > + data_device = (g_udev_client_query_by_subsystem_and_name ( > + client, > + "net", > + mm_port_get_device (ctx->data))); > + ctx->self->priv->bearer_interface = g_udev_device_get_name(data_device); > + > + //Map PDP context from the current Bearer. USB0 -> 3rd context, USB1 -> 1st context > + if (g_ascii_strncasecmp(ctx->self->priv->bearer_interface, "usb0", 4) == 0) > + ctx->self->priv-> pdp_cid = "3"; > + else if (g_ascii_strncasecmp(ctx->self->priv->bearer_interface, "usb1", 4) == 0) > + ctx->self->priv->pdp_cid = "1"; > + else > + { > + mm_err ("Unable to map usb interface:(%s) to context", ctx->self->priv->bearer_interface); > + > + g_simple_async_result_set_error (ctx->result, > + MM_CORE_ERROR, > + MM_CORE_ERROR_FAILED, > + "Internal error while mapping USB network interface."); > + > + ctx->self->priv->connect_pending = NULL; > + ctx->self->priv->disconnect_pending = NULL; > + > + if (ctx->connect) > + connect_3gpp_context_complete_and_free (ctx); > + else > + disconnect_3gpp_context_complete_and_free (ctx); > + > + return FALSE; > + } > + > + return TRUE; > +} > + > +static void > +handle_cancel_connect(Control3gppContext *ctx) > +{ > + gchar *command; > + > + // Clear context > + ctx->self->priv->connect_pending = NULL; > + > + command = g_strdup_printf ("^SWWAN=%s,%s,%s", > + "0", > + ctx->self->priv->pdp_cid, //Expect 1 or 3, > + g_ascii_strncasecmp(ctx->self->priv->pdp_cid, "3", 1)==0 ? "1" : "2"); > + > + //Disconnect, may not succeed. Will not check response on cancel. > + mm_base_modem_at_command_full (ctx->modem, > + ctx->primary, > + command, > + 3, > + FALSE, > + FALSE, > + NULL, > + NULL, // Do not care the AT response > + NULL); > + > + g_simple_async_result_set_error (ctx->result, > + MM_CORE_ERROR, > + MM_CORE_ERROR_CANCELLED, > + "Cinterion connection operation has been cancelled"); > + connect_3gpp_context_complete_and_free (ctx); > + > +} > + > +static void > +context_ready (MMBaseModem *modem, > + GAsyncResult *res, > + MMBroadbandBearerCinterion *self) > +{ > + Control3gppContext *ctx; > + GError *error = NULL; > + const gchar *response; > + > + ctx = self->priv->connect_pending; > + g_assert (ctx != NULL); > + > + /* Balance refcount */ > + g_object_unref (self); > + > + //Get the cgdcont write response. We expect: 'OK' > + //Responses: OK, ERROR > + response = mm_base_modem_at_command_finish (modem, res, &error); > + > + if (!response || error != NULL) { > + mm_err ("CGDCONT read- Error:%d response:%s", error->code, response); > + > + self->priv->connect_pending = NULL; > + g_simple_async_result_take_error (ctx->result, error); > + connect_3gpp_context_complete_and_free (ctx); > + > + return; > + } > + > + /* Go to next step */ > + //ctx->connect = CONNECT_3GPP_CONTEXT_STEP_SET_SWWAN; > + cinterion_3gpp_state_machine_logic(ctx->self); > +} > + > +static void > +connect_3gpp_context_step (Control3gppContext *ctx) > +{ > + /* Check for cancellation */ > + if (g_cancellable_is_cancelled (ctx->cancellable)) { > + //Init the current context if not done already. > + if (g_ascii_strncasecmp(ctx->self->priv->pdp_cid, "0", 1)==0) > + if(set_pdp_cid(ctx)) //Don't care about failure case for cancel. > + > + handle_cancel_connect(ctx); > + return; > + } > + > + // Network-initiated disconnect should not be outstanding at this point, > + // because it interferes with the connect attempt. > + g_assert (ctx->self->priv->network_disconnect_pending_id == 0); > + g_assert (ctx->self->priv->disconnect_pending == NULL); > + > + > + switch (ctx->connect) { > + case CONNECT_3GPP_CONTEXT_STEP_INIT: { > + MMBearerIpFamily ip_family; > + > + //Initialize variables, reminder! > + ctx->usb0_state = CONNECTION_UNKNOWN; > + ctx->usb1_state = CONNECTION_UNKNOWN; You could do this just after allocating the context. > + ctx->self->priv->pdp_cid = "0"; //Cinterion doesn't have cid == 0 I think I said it earlier, please make pdp_cid a uint. > + ctx->self->priv->swwan_read_write = -1; > + ctx->self->priv->retry_count = 0; > + And you shouldn't really need those 2 things above in private info... > + if(!set_pdp_cid(ctx)) > + return; > + > + ip_family = mm_bearer_properties_get_ip_type (mm_base_bearer_peek_config (MM_BASE_BEARER (ctx->self))); > + > + //TODO: Fix so more than IPV4 can be used. > + if (ip_family == MM_BEARER_IP_FAMILY_NONE || > + ip_family == MM_BEARER_IP_FAMILY_ANY) { > + gchar *ip_family_str; > + > + ip_family = mm_base_bearer_get_default_ip_family (MM_BASE_BEARER (ctx->self)); > + ip_family_str = mm_bearer_ip_family_build_string_from_mask (ip_family); > + mm_dbg ("No specific IP family requested, defaulting to %s", > + ip_family_str); > + g_free (ip_family_str); > + } > + > + /* Default to automatic/DHCP addressing */ Oh, weird, you're creating the MMBearerIpConfig before even knowing that the modem is connected... ok... > + ctx->ipv4_config = mm_bearer_ip_config_new (); > + mm_bearer_ip_config_set_method (ctx->ipv4_config, MM_BEARER_IP_METHOD_DHCP); > + > + /* Store the context */ > + ctx->self->priv->connect_pending = ctx; > + > + cinterion_3gpp_state_machine_logic(ctx->self); > + return; > + } > + case CONNECT_3GPP_CONTEXT_STEP_VERIFY_SWWAN: { > + send_swwan_read_command(ctx); > + return; > + } > + > + case CONNECT_3GPP_CONTEXT_STEP_BEARER_PROPERTIES: { > + gchar *command = NULL; > + > + //TODO: Rework for dual sim connections, don't disconnect the wrong one. > + //If there is already an active SWWAN connection disconnect before trying to set it's context. > + if ((ctx->usb0_state == CONNECTION_ACTIVE && g_ascii_strncasecmp(ctx->self->priv->pdp_cid, "3", 1)) || ctx->usb0_state == CONNECTION_UNKNOWN || > + (ctx->usb1_state == CONNECTION_ACTIVE && g_ascii_strncasecmp(ctx->self->priv->pdp_cid, "1", 1)) || ctx->usb1_state == CONNECTION_UNKNOWN) > + { If "3" and "1" CIDs are special, we should have some enumeration or something flagging them with a name, now that I think of it. > + send_swwan_disconnect_command(ctx); > + g_free (command); > + return; > + } > + > + command = build_cinterion_pdp_context_string(ctx); > + > + //Set the PDP context with cgdcont. Isn't there already a generic way in MMBroadbandBearer doing this? > + mm_base_modem_at_command_full (ctx->modem, > + ctx->primary, > + command, > + 3, > + FALSE, > + FALSE, > + NULL, > + (GAsyncReadyCallback)context_ready, > + g_object_ref (ctx->self)); > + > + g_free (command); > + return; > + } > + case CONNECT_3GPP_CONTEXT_STEP_SET_SWWAN: { > + send_swwan_connect_command(ctx); > + return; > + } > + case CONNECT_3GPP_CONTEXT_STEP_CONNECTION_STATUS: { > + send_swwan_read_command(ctx); > + return; > + } > + > + case CONNECT_3GPP_CONTEXT_STEP_DONE: { > + /* Clear context */ > + ctx->self->priv->connect_pending = NULL; > + ctx->self->priv->bearer_interface = NULL; > + ctx->self->priv->pdp_cid = NULL; > + ctx->self->priv->swwan_read_write = -1; > + > + /* Setup result */ > + { > + if (ctx->ipv4_config) { > + g_simple_async_result_set_op_res_gpointer ( > + ctx->result, > + mm_bearer_connect_result_new (ctx->data, ctx->ipv4_config, NULL), > + (GDestroyNotify)mm_bearer_connect_result_unref); > + } > + else { > + g_simple_async_result_set_error (ctx->result, > + MM_CORE_ERROR, > + MM_CORE_ERROR_WRONG_STATE, > + "Cinterion connection failed to set IP protocol"); > + } > + } > + > + connect_3gpp_context_complete_and_free (ctx); > + return; > + } > + } > +} > + > +static void > +connect_3gpp (MMBroadbandBearer *self, > + MMBroadbandModem *modem, > + MMPortSerialAt *primary, > + MMPortSerialAt *secondary, > + GCancellable *cancellable, > + GAsyncReadyCallback callback, > + gpointer user_data) > +{ > + Control3gppContext *ctx; > + MMPort *port; > + > + g_assert (primary != NULL); > + > + // We need a net port > + port = mm_base_modem_peek_best_data_port (MM_BASE_MODEM (modem), MM_PORT_TYPE_NET); > + if (!port) { > + g_simple_async_report_error_in_idle (G_OBJECT (self), > + callback, > + user_data, > + MM_CORE_ERROR, > + MM_CORE_ERROR_NOT_FOUND, > + "No valid data port found to launch connection"); > + return; > + } > + > + > + /* Setup connection context */ > + ctx = g_slice_new0 (Control3gppContext); > + ctx->self = g_object_ref (self); > + ctx->modem = g_object_ref (modem); > + ctx->data = g_object_ref (port); > + ctx->result = g_simple_async_result_new (G_OBJECT (self), > + callback, > + user_data, > + connect_3gpp); > + ctx->cancellable = g_object_ref (cancellable); > + ctx->connect = CONNECT_3GPP_CONTEXT_STEP_INIT; > + > + g_assert (ctx->self->priv->connect_pending == NULL); > + g_assert (ctx->self->priv->disconnect_pending == NULL); > + > + ctx->primary = get_dial_port (MM_BROADBAND_MODEM_CINTERION (ctx->modem), ctx->data, primary); What if primary is connected? Shouldn't we be checking that? > + > + /* Run! */ > + connect_3gpp_context_step (ctx); > +} > + > +/*****************************************************************************/ > +/* Disconnect 3GPP */ > + > +static void > +disconnect_3gpp_context_complete_and_free (Control3gppContext *ctx) > +{ > + g_simple_async_result_complete_in_idle (ctx->result); > + g_object_unref (ctx->result); > + g_object_unref (ctx->primary); > + g_object_unref (ctx->self); > + g_object_unref (ctx->modem); > + g_slice_free (Control3gppContext, ctx); > +} Yeah, nasty nasty to have 2 different complete_and_free() for the same Context struct based on different logics; just use 2 different contexts :) > + > +static gboolean > +disconnect_3gpp_finish (MMBroadbandBearer *self, > + GAsyncResult *res, > + GError **error) > +{ > + return !g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT (res), error); > +} > + > +static void > +disconnect_3gpp_context_step (Control3gppContext *ctx) > +{ > + //There is no cancel handling b/c the only thing we could do would > + //be disconnect, which is already happening and bad things can happen > + //if we abandon the modem in an unknown state. > + > + //Don't allow disconnect while connect in progress. > + g_assert (ctx->self->priv->connect_pending == NULL); > + > + switch (ctx->disconnect) { > + case DISCONNECT_3GPP_CONTEXT_STEP_INIT: > + /* Store the context */ > + ctx->self->priv->disconnect_pending = ctx; > + > + //Initialize variables, reminder! > + ctx->self->priv->pdp_cid = "0"; > + ctx->self->priv->retry_count = 0; > + ctx->self->priv->swwan_read_write = -1; > + > + // We ignore any pending network-initiated disconnection in order to prevent it > + // from interfering with the client-initiated disconnection, as we would like to > + // proceed with the latter anyway. > + if (ctx->self->priv->network_disconnect_pending_id != 0) { > + g_source_remove (ctx->self->priv->network_disconnect_pending_id); > + ctx->self->priv->network_disconnect_pending_id = 0; > + } > + > + if(!set_pdp_cid(ctx)) > + return; > + > + cinterion_3gpp_state_machine_logic(ctx->self); > + return; > + > + case DISCONNECT_3GPP_CONTEXT_STEP_SWWAN_DETACH: > + // If too many retries (1s of wait between the retries), failed > + if ( ctx->self->priv->retry_count > 5) { > + // Clear context > + ctx->self->priv->disconnect_pending = NULL; > + g_simple_async_result_set_error (ctx->result, > + MM_CORE_ERROR, > + MM_CORE_ERROR_TOO_MANY, > + "Disconnection attempt timed out"); > + > + disconnect_3gpp_context_complete_and_free (ctx); > + return; > + } > + > + //Has call back to next state. > + send_swwan_disconnect_command(ctx); > + > + //Only try to detach so many times. > + ctx->self->priv->retry_count++; > + > + return; > + case DISCONNECT_3GPP_CONTEXT_STEP_CONNECTION_STATUS: > + //Has call back to next state. > + send_swwan_read_command(ctx); > + return; > + > + case DISCONNECT_3GPP_CONTEXT_STEP_DONE: > + // Clear context > + ctx->self->priv->disconnect_pending = NULL; > + ctx->self->priv->pdp_cid = NULL; > + ctx->self->priv->swwan_read_write = -1; > + // Set data port as result > + g_simple_async_result_set_op_res_gboolean (ctx->result, TRUE); > + disconnect_3gpp_context_complete_and_free (ctx); > + > + return; > + } > +} > + > +static void > +disconnect_3gpp (MMBroadbandBearer *self, > + MMBroadbandModem *modem, > + MMPortSerialAt *primary, > + MMPortSerialAt *secondary, > + MMPort *data, > + guint cid, > + GAsyncReadyCallback callback, > + gpointer user_data) > +{ > + Control3gppContext *ctx; > + MMPort *port; > + > + g_assert (primary != NULL); > + > + ctx = g_slice_new0 (Control3gppContext); > + ctx->self = g_object_ref (self); > + ctx->modem = MM_BASE_MODEM (g_object_ref (modem)); > + ctx->result = g_simple_async_result_new (G_OBJECT (self), > + callback, > + user_data, > + disconnect_3gpp); > + ctx->disconnect = DISCONNECT_3GPP_CONTEXT_STEP_INIT; > + > + g_assert (ctx->self->priv->connect_pending == NULL); > + g_assert (ctx->self->priv->disconnect_pending == NULL); > + > + //TODO: Not sure how else to get active data port? Can this be done without adding this > + //function to mm-base-modem.c? > + //TODO: Dual SIM how do we know which interface to grab/disconnect if two are active? > + port = mm_base_modem_peek_current_data_port (MM_BASE_MODEM (modem), MM_PORT_TYPE_NET); > + if (!port) { > + g_simple_async_report_error_in_idle (G_OBJECT (self), > + callback, > + user_data, > + MM_CORE_ERROR, > + MM_CORE_ERROR_NOT_FOUND, > + "No valid data port found to tear down."); > + return; > + } > + > + ctx->data = g_object_ref (port); > + > + ctx->primary = get_dial_port (MM_BROADBAND_MODEM_CINTERION (ctx->modem), data, primary); > + > + /* Start! */ > + disconnect_3gpp_context_step (ctx); > +} > + > +/*****************************************************************************/ > + > +static gboolean > +network_disconnect_3gpp_delayed (MMBroadbandBearerCinterion *self) > +{ > + mm_dbg ("Disconnect bearer '%s' on network request.", > + mm_base_bearer_get_path (MM_BASE_BEARER (self))); > + > + self->priv->network_disconnect_pending_id = 0; > + mm_base_bearer_report_connection_status (MM_BASE_BEARER (self), > + MM_BEARER_CONNECTION_STATUS_DISCONNECTED); > + return G_SOURCE_REMOVE; > +} > + > +//TODO: Test this. > +static void > +report_connection_status (MMBaseBearer *bearer, > + MMBearerConnectionStatus status) > +{ report_connection_status() will be called by the MODEM object when a specific unsolicited message is detected in the TTY port. If you don't setup an unsolicited message handler to catch disconnections, you don't need this. > + MMBroadbandBearerCinterion *self = MM_BROADBAND_BEARER_CINTERION (bearer); > + > + g_assert (status == MM_BEARER_CONNECTION_STATUS_CONNECTED || > + status == MM_BEARER_CONNECTION_STATUS_DISCONNECTING || > + status == MM_BEARER_CONNECTION_STATUS_DISCONNECTED); > + > + /* When a pending connection / disconnection attempt is in progress, we use > + * ^SWWAN? to check the connection status and thus temporarily ignore > + * unsolicited messages */ > + if (self->priv->connect_pending || self->priv->disconnect_pending) > + return; > + > + /* Ignore 'CONNECTED' */ > + if (status == MM_BEARER_CONNECTION_STATUS_CONNECTED) > + return; > + > + /* We already use ^SWWAN? to poll the connection status, so only > + * handle network-initiated disconnection here. */ > + if (status == MM_BEARER_CONNECTION_STATUS_DISCONNECTING) { > + /* MM_BEARER_CONNECTION_STATUS_DISCONNECTING is used to indicate that the > + * reporting of disconnection should be delayed. See MMBroadbandModemCinterion's > + * bearer_report_connection_status for details. */ > + if (mm_base_bearer_get_status (bearer) == MM_BEARER_STATUS_CONNECTED && > + self->priv->network_disconnect_pending_id == 0) { > + mm_dbg ("Delay network-initiated disconnection of bearer '%s'", > + mm_base_bearer_get_path (MM_BASE_BEARER (self))); > + self->priv->network_disconnect_pending_id = (g_timeout_add_seconds ( > + 4, > + (GSourceFunc) network_disconnect_3gpp_delayed, > + self)); > + } > + return; > + } > + > + /* Report disconnected right away */ > + MM_BASE_BEARER_CLASS (mm_broadband_bearer_cinterion_parent_class)->report_connection_status ( > + bearer, > + MM_BEARER_CONNECTION_STATUS_DISCONNECTED); > +} > + > +/*****************************************************************************/ > + > +MMBaseBearer * > +mm_broadband_bearer_cinterion_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); > +} > + > +static void > +dispose (GObject *object) > +{ > + MMBroadbandBearerCinterion *self = MM_BROADBAND_BEARER_CINTERION (object); > + > + if (self->priv->network_disconnect_pending_id != 0) { > + g_source_remove (self->priv->network_disconnect_pending_id); > + self->priv->network_disconnect_pending_id = 0; > + } > + > + G_OBJECT_CLASS (mm_broadband_bearer_cinterion_parent_class)->dispose (object); > +} > + > +void > +mm_broadband_bearer_cinterion_new (MMBroadbandModemCinterion *modem, > + MMBearerProperties *config, > + GCancellable *cancellable, > + GAsyncReadyCallback callback, > + gpointer user_data) > +{ > + g_async_initable_new_async ( > + MM_TYPE_BROADBAND_BEARER_CINTERION, > + G_PRIORITY_DEFAULT, > + cancellable, > + callback, > + user_data, > + MM_BASE_BEARER_MODEM, modem, > + MM_BASE_BEARER_CONFIG, config, > + NULL); > +} > + > +static void > +mm_broadband_bearer_cinterion_init (MMBroadbandBearerCinterion *self) > +{ > + // Initialize private data > + self->priv = G_TYPE_INSTANCE_GET_PRIVATE (self, > + MM_TYPE_BROADBAND_BEARER_CINTERION, > + MMBroadbandBearerCinterionPrivate); > +} > + > +static void > +mm_broadband_bearer_cinterion_class_init (MMBroadbandBearerCinterionClass *klass) > +{ > + GObjectClass *object_class = G_OBJECT_CLASS (klass); > + MMBaseBearerClass *base_bearer_class = MM_BASE_BEARER_CLASS (klass); > + MMBroadbandBearerClass *broadband_bearer_class = MM_BROADBAND_BEARER_CLASS (klass); > + > + g_type_class_add_private (object_class, sizeof (MMBroadbandBearerCinterionPrivate)); > + > + object_class->dispose = dispose; > + base_bearer_class->report_connection_status = report_connection_status; //TODO:What triggers this? > + > + broadband_bearer_class->connect_3gpp = connect_3gpp; > + broadband_bearer_class->connect_3gpp_finish = connect_3gpp_finish; > + broadband_bearer_class->disconnect_3gpp = disconnect_3gpp; > + broadband_bearer_class->disconnect_3gpp_finish = disconnect_3gpp_finish; > +} > diff --git a/plugins/cinterion/mm-broadband-bearer-cinterion.h b/plugins/cinterion/mm-broadband-bearer-cinterion.h > new file mode 100644 > index 0000000..8f315db > --- /dev/null > +++ b/plugins/cinterion/mm-broadband-bearer-cinterion.h > @@ -0,0 +1,56 @@ > +/* -*- 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) 2016 Trimble Navigation Limited > + * Author: Matthew Stanger <<a href="mailto:Matthew_Stanger@trimble.com">Matthew_Stanger@trimble.com</a>> > + */ > + > +#ifndef MM_BROADBAND_BEARER_CINTERION_H > +#define MM_BROADBAND_BEARER_CINTERION_H > + > +#include <glib.h> > +#include <glib-object.h> > + > +#include "mm-broadband-bearer.h" > +#include "mm-broadband-modem-cinterion.h" > + > +#define MM_TYPE_BROADBAND_BEARER_CINTERION (mm_broadband_bearer_cinterion_get_type ()) > +#define MM_BROADBAND_BEARER_CINTERION(obj) (G_TYPE_CHECK_INSTANCE_CAST ((obj), MM_TYPE_BROADBAND_BEARER_CINTERION, MMBroadbandBearerCinterion)) > +#define MM_BROADBAND_BEARER_CINTERION_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST ((klass), MM_TYPE_BROADBAND_BEARER_CINTERION, MMBroadbandBearerCinterionClass)) > +#define MM_IS_BROADBAND_BEARER_CINTERION(obj) (G_TYPE_CHECK_INSTANCE_TYPE ((obj), MM_TYPE_BROADBAND_BEARER_CINTERION)) > +#define MM_IS_BROADBAND_BEARER_CINTERION_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), MM_TYPE_BROADBAND_BEARER_CINTERION)) > +#define MM_BROADBAND_BEARER_CINTERION_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS ((obj), MM_TYPE_BROADBAND_BEARER_CINTERION, MMBroadbandBearerCinterionClass)) > + > +typedef struct _MMBroadbandBearerCinterion MMBroadbandBearerCinterion; > +typedef struct _MMBroadbandBearerCinterionClass MMBroadbandBearerCinterionClass; > +typedef struct _MMBroadbandBearerCinterionPrivate MMBroadbandBearerCinterionPrivate; > + > +struct _MMBroadbandBearerCinterion { > + MMBroadbandBearer parent; > + MMBroadbandBearerCinterionPrivate *priv; > +}; > + > +struct _MMBroadbandBearerCinterionClass { > + MMBroadbandBearerClass parent; > +}; > + > +GType mm_broadband_bearer_cinterion_get_type (void); > + > +void mm_broadband_bearer_cinterion_new (MMBroadbandModemCinterion *modem, > + MMBearerProperties *config, > + GCancellable *cancellable, > + GAsyncReadyCallback callback, > + gpointer user_data); > +MMBaseBearer *mm_broadband_bearer_cinterion_new_finish (GAsyncResult *res, > + GError **error); > + > +#endif /* MM_BROADBAND_BEARER_CINTERION_H */ > diff --git a/plugins/cinterion/mm-broadband-modem-cinterion.c b/plugins/cinterion/mm-broadband-modem-cinterion.c > index 4882a41..a7b8afc 100644 > --- a/plugins/cinterion/mm-broadband-modem-cinterion.c > +++ b/plugins/cinterion/mm-broadband-modem-cinterion.c > @@ -12,7 +12,9 @@ > * > * Copyright (C) 2011 Ammonit Measurement GmbH > * Copyright (C) 2011 Google Inc. > + * Copyright (C) 2016 Trimble Navigation Limited > * Author: Aleksander Morgado <<a href="mailto:aleksander@lanedo.com">aleksander@lanedo.com</a>> > + * Contributor: Matthew Stanger <<a href="mailto:matthew_stanger@trimble.com">matthew_stanger@trimble.com</a>> > */ > > #include <config.h> > @@ -22,6 +24,8 @@ > #include <string.h> > #include <unistd.h> > #include <ctype.h> > +#include <gudev/gudev.h> > + > > #include "ModemManager.h" > #include "mm-modem-helpers.h" > @@ -36,6 +40,7 @@ > #include "mm-broadband-modem-cinterion.h" > #include "mm-modem-helpers-cinterion.h" > #include "mm-common-cinterion.h" > +#include "mm-broadband-bearer-cinterion.h" > > static void iface_modem_init (MMIfaceModem *iface); > static void iface_modem_3gpp_init (MMIfaceModem3gpp *iface); > @@ -50,6 +55,12 @@ G_DEFINE_TYPE_EXTENDED (MMBroadbandModemCinterion, mm_broadband_modem_cinterion, > G_IMPLEMENT_INTERFACE (MM_TYPE_IFACE_MODEM_MESSAGING, iface_modem_messaging_init) > G_IMPLEMENT_INTERFACE (MM_TYPE_IFACE_MODEM_LOCATION, iface_modem_location_init)) > > +typedef enum { > + FEATURE_SUPPORT_UNKNOWN, > + FEATURE_NOT_SUPPORTED, > + FEATURE_SUPPORTED > +} FeatureSupport; > + > struct _MMBroadbandModemCinterionPrivate { > /* Flag to know if we should try AT^SIND or not to get psinfo */ > gboolean sind_psinfo; > @@ -69,6 +80,9 @@ struct _MMBroadbandModemCinterionPrivate { > GArray *cnmi_supported_bm; > GArray *cnmi_supported_ds; > GArray *cnmi_supported_bfr; > + > + FeatureSupport swwan_support; > + > }; > > /*****************************************************************************/ > @@ -117,7 +131,7 @@ cnmi_test_ready (MMBaseModem *self, > { > GError *error = NULL; > > - mm_base_modem_at_command_finish (MM_BASE_MODEM (self), res, &error); > + mm_base_modem_at_command_finish (MM_BASE_MODEM (self), res, &error); > if (error) > g_simple_async_result_take_error (simple, error); > else > @@ -727,10 +741,16 @@ get_access_technology_from_psinfo (const gchar *psinfo, > case 9: > case 10: > return (MM_MODEM_ACCESS_TECHNOLOGY_HSDPA | MM_MODEM_ACCESS_TECHNOLOGY_HSUPA); > + case 16: > + case 17: > + return MM_MODEM_ACCESS_TECHNOLOGY_LTE; > default: > + mm_dbg ("Unable to identify access technology in case:%i", psinfoval); > break; > } > } > + else > + mm_err ("FAILED get_access_technology_from_psinfo-int"); > > g_set_error (error, > MM_CORE_ERROR, > @@ -1648,6 +1668,259 @@ setup_ports (MMBroadbandModem *self) > } > > /*****************************************************************************/ > +/* Create Bearer (Modem interface) */ > + > +typedef struct { > + MMBroadbandModemCinterion *self; > + GSimpleAsyncResult *result; > + MMBearerProperties *properties; > +} CreateBearerContext; > + > +static void > +create_bearer_context_complete_and_free (CreateBearerContext *ctx) > +{ > + g_simple_async_result_complete (ctx->result); > + g_object_unref (ctx->result); > + g_object_unref (ctx->self); > + g_object_unref (ctx->properties); > + g_slice_free (CreateBearerContext, ctx); > +} > + > +static MMBaseBearer * > +cinterion_modem_create_bearer_finish (MMIfaceModem *self, > + GAsyncResult *res, > + GError **error) > +{ > + MMBaseBearer *bearer; > + > + if (g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT (res), error)) > + return NULL; > + > + bearer = g_simple_async_result_get_op_res_gpointer (G_SIMPLE_ASYNC_RESULT (res)); > + mm_dbg ("New cinterion bearer created at DBus path '%s'", mm_base_bearer_get_path (bearer)); > + return g_object_ref (bearer); > +} > + > +static void > +broadband_bearer_cinterion_new_ready (GObject *source, > + GAsyncResult *res, > + CreateBearerContext *ctx) > +{ > + MMBaseBearer *bearer; > + GError *error = NULL; > + > + bearer = mm_broadband_bearer_cinterion_new_finish (res, &error); > + if (!bearer) > + g_simple_async_result_take_error (ctx->result, error); > + else > + g_simple_async_result_set_op_res_gpointer (ctx->result, bearer, (GDestroyNotify)g_object_unref); > + create_bearer_context_complete_and_free (ctx); > +} > + > +static void > +broadband_bearer_new_ready (GObject *source, > + GAsyncResult *res, > + CreateBearerContext *ctx) > +{ > + MMBaseBearer *bearer; > + GError *error = NULL; > + > + bearer = mm_broadband_bearer_new_finish (res, &error); > + if (!bearer) > + g_simple_async_result_take_error (ctx->result, error); > + else > + g_simple_async_result_set_op_res_gpointer (ctx->result, bearer, (GDestroyNotify)g_object_unref); > + create_bearer_context_complete_and_free (ctx); > +} > + > +static void > +create_bearer_for_net_port (CreateBearerContext *ctx) > +{ > + switch (ctx->self->priv->swwan_support) { > + case FEATURE_SUPPORT_UNKNOWN: > + g_assert_not_reached (); > + case FEATURE_NOT_SUPPORTED: > + mm_dbg ("^SWWAN not supported, creating default bearer..."); > + mm_broadband_bearer_new (MM_BROADBAND_MODEM (ctx->self), > + ctx->properties, > + NULL, /* cancellable */ > + (GAsyncReadyCallback)broadband_bearer_new_ready, > + ctx); > + return; > + case FEATURE_SUPPORTED: > + mm_dbg ("^SWWAN supported, creating cinterion bearer..."); > + mm_broadband_bearer_cinterion_new (MM_BROADBAND_MODEM_CINTERION (ctx->self), > + ctx->properties, > + NULL, /* cancellable */ > + (GAsyncReadyCallback)broadband_bearer_cinterion_new_ready, > + ctx); > + return; > + } > +} > + > +static MMPortSerialAt * > +peek_port_at_for_data (MMBroadbandModemCinterion *self, > + MMPort *port) > +{ > + GList *cdc_wdm_at_ports, *l; cdc_wdm_at_ports doesn't look like a proper name, as you don't have any cdc-wdm port, just TTYs :) > + const gchar *net_port_parent_path; > + > + g_warn_if_fail (mm_port_get_subsys (port) == MM_PORT_SUBSYS_NET); > + net_port_parent_path = mm_port_get_parent_path (port); > + > + if (!net_port_parent_path) { > + g_warning ("(%s) no parent path for net port", mm_port_get_device (port)); > + return NULL; > + } > + > + //Find the port to send AT commands on... > + cdc_wdm_at_ports = mm_base_modem_find_ports (MM_BASE_MODEM (self), > + MM_PORT_SUBSYS_USB, > + MM_PORT_TYPE_NET, > + NULL); > + > + > + for (l = cdc_wdm_at_ports; l; l = l->next) { > + const gchar *wdm_port_parent_path; > + > + g_assert (MM_IS_PORT_SERIAL_AT (l->data)); > + wdm_port_parent_path = mm_port_get_parent_path (MM_PORT (l->data)); > + > + if (wdm_port_parent_path && g_str_equal (wdm_port_parent_path, net_port_parent_path)) > + return MM_PORT_SERIAL_AT (l->data); > + } > + > + return NULL; > +} > + > +MMPortSerialAt * > +mm_broadband_modem_cinterion_peek_port_at_for_data (MMBroadbandModemCinterion *self, > + MMPort *port) > +{ > + MMPortSerialAt *found; > + > + g_assert (self->priv->swwan_support == FEATURE_SUPPORTED); > + > + found = peek_port_at_for_data (self, port); > + if (!found) > + mm_warn ("Couldn't find associated cdc-wdm port for 'net/%s'", > + mm_port_get_device (port)); > + return found; > +} > + > +static void > +ensure_swwan_support_checked (MMBroadbandModemCinterion *self, > + MMPort *port) > +{ > + GUdevClient *client; > + GUdevDevice *data_device; > + > + /* Initialize the swwan feature */ > + if (self->priv->swwan_support != FEATURE_SUPPORT_UNKNOWN) > + return; > + > + /* First, check for devices which could support SWWAN. They > + * will be tagged by udev as net devices. */ > + client = g_udev_client_new (NULL); > + data_device = (g_udev_client_query_by_subsystem_and_name ( > + client, > + "net", > + mm_port_get_device (port))); > + > + /* udevadm info for Cinterion PLS8-X v3.017 Modem > + P: /devices/pci0000:00/0000:00:14.0/usb3/3-2/3-2:1.10/net/usb0 > + E: DEVPATH=/devices/pci0000:00/0000:00:14.0/usb3/3-2/3-2:1.10/net/usb0 > + E: ID_BUS=usb > + E: ID_MM_CANDIDATE=1 > + E: ID_MODEL=LTE_Modem > + E: ID_MODEL_ENC=LTE\x20Modem > + E: ID_MODEL_FROM_DATABASE=2.0 root hub > + E: ID_MODEL_ID=0061 > + E: ID_NET_NAME_MAC=enxdeadbeef0000 > + E: ID_NET_NAME_PATH=enp0s20u2i10 > + E: ID_REVISION=0232 > + E: ID_SERIAL=Cinterion_LTE_Modem > + E: ID_TYPE=generic > + E: ID_USB_DRIVER=cdc_ether > + E: ID_USB_INTERFACES=:020201:0a0000:020600: > + E: ID_USB_INTERFACE_NUM=0a > + E: ID_VENDOR=Cinterion > + E: ID_VENDOR_ENC=Cinterion > + E: ID_VENDOR_FROM_DATABASE=Linux Foundation > + E: ID_VENDOR_ID=1e2d > + E: IFINDEX=11 > + E: INTERFACE=usb0 > + E: SUBSYSTEM=net > + E: USEC_INITIALIZED=4142603761 > + > + P: /devices/pci0000:00/0000:00:14.0/usb3/3-2/3-2:1.11 > + E: DEVPATH=/devices/pci0000:00/0000:00:14.0/usb3/3-2/3-2:1.11 > + E: DEVTYPE=usb_interface > + E: DRIVER=cdc_ether > + E: ID_MODEL_FROM_DATABASE=2.0 root hub > + E: ID_VENDOR_FROM_DATABASE=Linux Foundation > + E: INTERFACE=10/0/0 > + E: MODALIAS=usb:v1E2Dp0061d0232dc00dsc00dp00ic0Aisc00ip00in0B > + E: PRODUCT=1e2d/61/232 > + E: SUBSYSTEM=usb > + E: TYPE=0/0/0 > + E: USEC_INITIALIZED=604142604196 > + */ > + > + //Assumption - Cinterion modems that use cdc_ether will support the swwan via usb ethernet. > + //Cinterion alluded to this idea when I spoke with their engineering support but stopped short > + //of being able to confirm if all future/current models will be this way. > + if (data_device && g_str_equal(g_udev_device_get_property(data_device, "ID_USB_DRIVER"),"cdc_ether")) { > + mm_dbg ("This device (%s) can support swwan feature", mm_port_get_device (port)); > + self->priv->swwan_support = FEATURE_SUPPORTED; > + } > + else > + { > + self->priv->swwan_support = FEATURE_NOT_SUPPORTED; > + mm_dbg ("This device (%s) can not support swwan feature", mm_port_get_device (port)); > + } > + > + /* Free the g_object*/ > + if (data_device) > + g_object_unref (data_device); > + if (client) > + g_object_unref (client); > +} > + > +static void > +cinterion_modem_create_bearer (MMIfaceModem *self, > + MMBearerProperties *properties, > + GAsyncReadyCallback callback, > + gpointer user_data) > +{ > + CreateBearerContext *ctx; > + MMPort *port; > + > + ctx = g_slice_new0 (CreateBearerContext); > + ctx->self = g_object_ref (self); > + ctx->properties = g_object_ref (properties); > + ctx->result = g_simple_async_result_new (G_OBJECT (self), > + callback, > + user_data, > + cinterion_modem_create_bearer); > + > + port = mm_base_modem_peek_best_data_port (MM_BASE_MODEM (self), MM_PORT_TYPE_NET); > + > + if (port) { > + ensure_swwan_support_checked (ctx->self, port); > + create_bearer_for_net_port (ctx); > + return; > + } > + > + mm_dbg ("Creating default bearer..."); > + mm_broadband_bearer_new (MM_BROADBAND_MODEM (self), > + properties, > + NULL, /* cancellable */ > + (GAsyncReadyCallback)broadband_bearer_new_ready, > + ctx); > +} > + > +/*****************************************************************************/ > > MMBroadbandModemCinterion * > mm_broadband_modem_cinterion_new (const gchar *device, > @@ -1675,6 +1948,7 @@ mm_broadband_modem_cinterion_init (MMBroadbandModemCinterion *self) > > /* Set defaults */ > self->priv->sind_psinfo = TRUE; /* Initially, always try to get psinfo */ > + self->priv->swwan_support = FEATURE_SUPPORT_UNKNOWN; > } > > static void > @@ -1704,6 +1978,8 @@ iface_modem_init (MMIfaceModem *iface) > { > iface_modem_parent = g_type_interface_peek_parent (iface); > > + iface->create_bearer = cinterion_modem_create_bearer; > + iface->create_bearer_finish = cinterion_modem_create_bearer_finish; > iface->load_supported_modes = load_supported_modes; > iface->load_supported_modes_finish = load_supported_modes_finish; > iface->set_current_modes = set_current_modes; > @@ -1764,7 +2040,6 @@ mm_broadband_modem_cinterion_class_init (MMBroadbandModemCinterionClass *klass) > { > GObjectClass *object_class = G_OBJECT_CLASS (klass); > MMBroadbandModemClass *broadband_modem_class = MM_BROADBAND_MODEM_CLASS (klass); > - > g_type_class_add_private (object_class, sizeof (MMBroadbandModemCinterionPrivate)); > > /* Virtual methods */ > diff --git a/plugins/cinterion/mm-broadband-modem-cinterion.h b/plugins/cinterion/mm-broadband-modem-cinterion.h > index 47f0dcb..b675f3d 100644 > --- a/plugins/cinterion/mm-broadband-modem-cinterion.h > +++ b/plugins/cinterion/mm-broadband-modem-cinterion.h > @@ -48,4 +48,7 @@ MMBroadbandModemCinterion *mm_broadband_modem_cinterion_new (const gchar *device > guint16 vendor_id, > guint16 product_id); > > +MMPortSerialAt *mm_broadband_modem_cinterion_peek_port_at_for_data (MMBroadbandModemCinterion *self, > + MMPort *port); > + > #endif /* MM_BROADBAND_MODEM_CINTERION_H */ > diff --git a/plugins/cinterion/mm-common-cinterion.c b/plugins/cinterion/mm-common-cinterion.c All the changes here look like GPS related, shouldn't go in this patch. > index 6e4da70..c9119ec 100644 > --- a/plugins/cinterion/mm-common-cinterion.c > +++ b/plugins/cinterion/mm-common-cinterion.c > @@ -11,13 +11,15 @@ > * GNU General Public License for more details: > * > * Copyright (C) 2014 Ammonit Measurement GmbH > + * Copyright (C) 2016 Trimble Navigation Limited > * Author: Aleksander Morgado <<a href="mailto:aleksander@aleksander.es">aleksander@aleksander.es</a>> > + * Contributor: Matthew Stanger <<a href="mailto:matthew_stanger@trimble.com">matthew_stanger@trimble.com</a>> > */ > > #include "mm-common-cinterion.h" > #include "mm-base-modem-at.h" > +#include "mm-log.h" > > -static MMIfaceModemLocation *iface_modem_location_parent; > > /*****************************************************************************/ > > @@ -25,11 +27,40 @@ static MMIfaceModemLocation *iface_modem_location_parent; > static GQuark cinterion_location_context_quark; > > /*****************************************************************************/ > +/* Def's and Enum's */ > + > +typedef enum { > + GPS_CONTEXT_STEP_SGPSS = 0, > + GPS_CONTEXT_STEP_SGPSC_ANTENNA, > + GPS_CONTEXT_STEP_SGPSC_ENGINE, > + GPS_CONTEXT_STEP_SGPSC_OUTPUT, > + GPS_CONTEXT_STEP_DONE, > +} ConnectGpsContextStep; > + > +typedef struct { > + MMBaseModem *self; > + GSimpleAsyncResult *result; > + MMModemLocationSource source; > + ConnectGpsContextStep enable_state; > + ConnectGpsContextStep disable_state; > + gpointer connect_pending; > + gpointer disconnect_pending; > + gint gps_retry; > +} LocationGatheringContext; > > typedef struct { > MMModemLocationSource enabled_sources; > } LocationContext; > > +static MMIfaceModemLocation *iface_modem_location_parent; > +static void try_gps_enable (MMIfaceModemLocation *self, > + LocationGatheringContext *ctx); > +static void try_gps_disable (MMIfaceModemLocation *self, > + LocationGatheringContext *ctx); > +static void send_gps_command (MMIfaceModemLocation *self, > + LocationGatheringContext *ctx, > + gchar **command); > + > static void > location_context_free (LocationContext *ctx) > { > @@ -127,13 +158,7 @@ mm_common_cinterion_location_load_capabilities (MMIfaceModemLocation *self, > } > > /*****************************************************************************/ > -/* Enable/Disable location gathering (Location interface) */ > - > -typedef struct { > - MMBaseModem *self; > - GSimpleAsyncResult *result; > - MMModemLocationSource source; > -} LocationGatheringContext; > +/* Common location gathering (Location interface) */ > > static void > location_gathering_context_complete_and_free (LocationGatheringContext *ctx) > @@ -144,6 +169,145 @@ location_gathering_context_complete_and_free (LocationGatheringContext *ctx) > g_slice_free (LocationGatheringContext, ctx); > } > > +static void > +location_gathering_context_complete_and_free_full (LocationGatheringContext *ctx) > +{ > + //It's importiant that this parent function remains protected by calls where > + //both context '*_pending' pointers could be active. > + if (ctx->connect_pending != NULL) > + ctx->connect_pending = NULL; > + else > + ctx->disconnect_pending = NULL; > + > + location_gathering_context_complete_and_free(ctx); > +} > + > +static void > +promote_gps_state_from_response (MMBaseModem *self, > + GAsyncResult *res, > + LocationGatheringContext *ctx) > +{ > + GError *error = NULL; > + ConnectGpsContextStep state; > + gchar *result; > + > + //Adjust for whether we're dis/en-ableing > + if (ctx->connect_pending != NULL) > + state = ctx->enable_state; > + else > + { > + g_assert(ctx->disconnect_pending != NULL); > + state = ctx->disable_state; > + } > + > + //We see that the 'Engine,1' command (PLS8) fails for no apparent reason > + //on ATMEL AT91SAM9263 ARM & not x86. See 'mm_common_cinterion_setup_gps_port' > + //comments. > + result = g_strdup (mm_base_modem_at_command_full_finish (self, res, &error)); > + > + //If we get an error don't progress at first, try the same state again. > + if ((!result && state != GPS_CONTEXT_STEP_SGPSS && ctx-> gps_retry < 3)) > + { > + state = GPS_CONTEXT_STEP_SGPSS; > + ctx->gps_retry++; > + } > + else { > + > + > + switch (state) { > + > + //First step try's the legacy SGPSS command. If it fails don't error out > + //continue on to send new Cinterion GPS commands and see if they work. > + case GPS_CONTEXT_STEP_SGPSS: { > + if (!result) { > + mm_info ("SGPSS command failed, will try another Cinterion GPS command."); > + state = GPS_CONTEXT_STEP_SGPSC_ANTENNA; > + } > + else > + state = GPS_CONTEXT_STEP_DONE; > + > + break; > + } > + //If the new GPS enable comand fails then exit we are out of things to try. > + //Only connect flow cares about errors. > + case GPS_CONTEXT_STEP_SGPSC_ANTENNA: { > + if (!result && ctx->disconnect_pending == NULL) { > + mm_info ("SGPSC command failed, we are out of things to try."); > + g_simple_async_result_take_error (ctx->result, error); > + location_gathering_context_complete_and_free_full (ctx); > + return; > + } > + else > + state = GPS_CONTEXT_STEP_SGPSC_ENGINE; > + > + break; > + } > + case GPS_CONTEXT_STEP_SGPSC_ENGINE: { > + if (!result && ctx->disconnect_pending == NULL) { > + g_simple_async_result_take_error (ctx->result, error); > + location_gathering_context_complete_and_free_full (ctx); > + return; > + } > + else > + state = GPS_CONTEXT_STEP_SGPSC_OUTPUT; > + > + break; > + } > + case GPS_CONTEXT_STEP_SGPSC_OUTPUT: { > + if (!result && ctx->disconnect_pending == NULL) { > + g_simple_async_result_take_error (ctx->result, error); > + location_gathering_context_complete_and_free_full (ctx); > + return; > + } > + else > + state = GPS_CONTEXT_STEP_DONE; > + > + break; > + } > + case GPS_CONTEXT_STEP_DONE: { > + //Shouldn't get here, fall into default error. > + } > + default: { > + g_simple_async_result_set_error (ctx->result, > + MM_CORE_ERROR, > + MM_CORE_ERROR_WRONG_STATE, > + "Unknown gps state during setup."); > + > + location_gathering_context_complete_and_free_full (ctx); > + g_free(result); > + return; > + } > + } > + } > + > + //Save state and go back to dis/en-ableing > + if (ctx->connect_pending != NULL) { > + ctx->enable_state = state; > + try_gps_enable(ctx->connect_pending, ctx); > + } > + else { > + ctx->disable_state = state; > + try_gps_disable(ctx->disconnect_pending, ctx); > + } > + g_free(result); > +} > + > +static void > +send_gps_command (MMIfaceModemLocation *self, > + LocationGatheringContext *ctx, > + gchar **command) > +{ > + mm_base_modem_at_command_full (MM_BASE_MODEM (self), > + mm_base_modem_peek_best_at_port (MM_BASE_MODEM (self), NULL), > + *command, > + 5, > + FALSE, > + FALSE, /* raw */ > + NULL, /* cancellable */ > + (GAsyncReadyCallback)promote_gps_state_from_response, > + ctx); > +} > + > /******************************/ > /* Disable location gathering */ > > @@ -157,15 +321,9 @@ mm_common_cinterion_disable_location_gathering_finish (MMIfaceModemLocation *sel > > static void > gps_disabled_ready (MMBaseModem *self, > - GAsyncResult *res, > LocationGatheringContext *ctx) > { > - GError *error = NULL; > - > - if (!mm_base_modem_at_command_full_finish (self, res, &error)) > - g_simple_async_result_take_error (ctx->result, error); > - else > - g_simple_async_result_set_op_res_gboolean (ctx->result, TRUE); > + g_simple_async_result_set_op_res_gboolean (ctx->result, TRUE); > > /* Only use the GPS port in NMEA/RAW setups */ > if (ctx->source & (MM_MODEM_LOCATION_SOURCE_GPS_NMEA | > @@ -178,11 +336,57 @@ gps_disabled_ready (MMBaseModem *self, > mm_port_serial_close (MM_PORT_SERIAL (gps_port)); > } > > - location_gathering_context_complete_and_free (ctx); > + location_gathering_context_complete_and_free_full (ctx); > +} > + > +static void > +try_gps_disable (MMIfaceModemLocation *self, > + LocationGatheringContext *ctx) > +{ > + gchar *gps_command; > + > + switch (ctx->disable_state) { > + > + //The old command to enable GPS, works on at least PSX8 > + case GPS_CONTEXT_STEP_SGPSS: { > + gps_command = "^SGPSS=0"; > + break; > + } > + //Commands Power/Ant, Engine & Output w/ sgpsc are > + //used by PLS8-X & E. Since around versions 2.0+ > + case GPS_CONTEXT_STEP_SGPSC_ANTENNA: { > + gps_command = "^SGPSC=\"Power/Antenna\",\"off\""; > + break; > + } > + case GPS_CONTEXT_STEP_SGPSC_ENGINE: { > + gps_command = "^SGPSC=\"Engine\",\"0\""; > + break; > + } > + case GPS_CONTEXT_STEP_SGPSC_OUTPUT: { > + gps_command = "^SGPSC=\"NMEA/Output\",\"off\""; > + break; > + } > + case GPS_CONTEXT_STEP_DONE: { > + gps_disabled_ready(MM_BASE_MODEM (self), ctx); > + return; > + } > + default: { > + g_simple_async_result_set_error (ctx->result, > + MM_CORE_ERROR, > + MM_CORE_ERROR_WRONG_STATE, > + "Unknown gps state during tear down."); > + > + location_gathering_context_complete_and_free_full (ctx); > + return; > + } > + } > + > + send_gps_command(self, ctx, &gps_command); > + return; > } > > static void > -internal_disable_location_gathering (LocationGatheringContext *ctx) > +internal_disable_location_gathering (MMIfaceModemLocation *self, LocationGatheringContext *ctx) > { > LocationContext *location_ctx; > gboolean stop_gps = FALSE; > @@ -202,21 +406,30 @@ internal_disable_location_gathering (LocationGatheringContext *ctx) > } > > if (stop_gps) { > - /* We disable continuous GPS fixes */ > - mm_base_modem_at_command_full (MM_BASE_MODEM (ctx->self), > - mm_base_modem_peek_best_at_port (MM_BASE_MODEM (ctx->self), NULL), > - "AT^SGPSS=0", > - 3, > - FALSE, > - FALSE, /* raw */ > - NULL, /* cancellable */ > - (GAsyncReadyCallback)gps_disabled_ready, > - ctx); > + //Don't allow disconnects while trying to connect. > + if (ctx->connect_pending != NULL) > + { > + g_simple_async_result_set_error (ctx->result, > + MM_CORE_ERROR, > + MM_CORE_ERROR_IN_PROGRESS, > + "Can't disconnect GPS while another process is trying to connect it."); > + > + //*_complete_and_free_full is safe to use for the disconnect flow after this point. > + ctx->disconnect_pending = NULL; > + location_gathering_context_complete_and_free (ctx); > + return; > + } > + > + ctx->disable_state = GPS_CONTEXT_STEP_SGPSS; > + ctx->disconnect_pending = self; > + try_gps_disable(self, ctx); > + > return; > } > > /* For any other location (e.g. 3GPP), or if still some GPS needed, just return */ > g_simple_async_result_set_op_res_gboolean (ctx->result, TRUE); > + ctx->disconnect_pending = NULL; > location_gathering_context_complete_and_free (ctx); > } > > @@ -236,12 +449,13 @@ parent_disable_location_gathering_ready (MMIfaceModemLocation *self, > } else { > /* Fatal */ > g_simple_async_result_take_error (ctx->result, error); > + ctx->disconnect_pending = NULL; > location_gathering_context_complete_and_free (ctx); > return; > } > } > > - internal_disable_location_gathering (ctx); > + internal_disable_location_gathering (self, ctx); > } > > void > @@ -259,6 +473,9 @@ mm_common_cinterion_disable_location_gathering (MMIfaceModemLocation *self, > user_data, > mm_common_cinterion_disable_location_gathering); > ctx->source = source; > + ctx->connect_pending = NULL; > + ctx->disconnect_pending = NULL; > + ctx->gps_retry = 0; > > /* Chain up parent's gathering enable */ > if (iface_modem_location_parent->disable_location_gathering) { > @@ -270,7 +487,7 @@ mm_common_cinterion_disable_location_gathering (MMIfaceModemLocation *self, > return; > } > > - internal_disable_location_gathering (ctx); > + internal_disable_location_gathering (self, ctx); > } > > /*****************************************************************************/ > @@ -286,16 +503,10 @@ mm_common_cinterion_enable_location_gathering_finish (MMIfaceModemLocation *self > > static void > gps_enabled_ready (MMBaseModem *self, > - GAsyncResult *res, > LocationGatheringContext *ctx) > { > GError *error = NULL; > > - if (!mm_base_modem_at_command_full_finish (self, res, &error)) { > - g_simple_async_result_take_error (ctx->result, error); > - location_gathering_context_complete_and_free (ctx); > - return; > - } > > /* Only use the GPS port in NMEA/RAW setups */ > if (ctx->source & (MM_MODEM_LOCATION_SOURCE_GPS_NMEA | > @@ -312,12 +523,60 @@ gps_enabled_ready (MMBaseModem *self, > MM_CORE_ERROR, > MM_CORE_ERROR_FAILED, > "Couldn't open raw GPS serial port"); > - } else > + } > + else > g_simple_async_result_set_op_res_gboolean (ctx->result, TRUE); > - } else > + > + } > + else > g_simple_async_result_set_op_res_gboolean (ctx->result, TRUE); > > - location_gathering_context_complete_and_free (ctx); > + location_gathering_context_complete_and_free_full (ctx); > +} > + > +static void > +try_gps_enable (MMIfaceModemLocation *self, > + LocationGatheringContext *ctx) > +{ > + gchar *gps_command; > + > + switch (ctx->enable_state) { > + > + //The old command to enable GPS, works on at least PSX8 > + case GPS_CONTEXT_STEP_SGPSS: { > + gps_command = "^SGPSS=4"; > + break; > + } > + //Commands Power/Ant, Engine & Output w/ sgpsc are > + //used by PLS8-X & E. Since versions 2.0+ > + case GPS_CONTEXT_STEP_SGPSC_ANTENNA: { > + gps_command = "^SGPSC=\"Power/Antenna\",\"on\""; > + break; > + } > + case GPS_CONTEXT_STEP_SGPSC_ENGINE: { > + gps_command = "^SGPSC=\"Engine\",\"1\""; > + break; > + } > + case GPS_CONTEXT_STEP_SGPSC_OUTPUT: { > + gps_command = "^SGPSC=\"NMEA/Output\",\"on\""; > + break; > + } > + case GPS_CONTEXT_STEP_DONE: { > + gps_enabled_ready(MM_BASE_MODEM (self), ctx); > + return; > + } > + default: { > + g_simple_async_result_set_error (ctx->result, > + MM_CORE_ERROR, > + MM_CORE_ERROR_WRONG_STATE, > + "Unknown gps state during setup."); > + location_gathering_context_complete_and_free_full (ctx); > + return; > + } > + } > + > + send_gps_command(self, ctx, &gps_command); > + return; > } > > static void > @@ -338,13 +597,13 @@ parent_enable_location_gathering_ready (MMIfaceModemLocation *self, > } else { > /* Fatal */ > g_simple_async_result_take_error (ctx->result, error); > + ctx->connect_pending = NULL; > location_gathering_context_complete_and_free (ctx); > return; > } > } > > /* Now our own enabling */ > - > location_ctx = get_location_context (MM_BASE_MODEM (self)); > > /* NMEA and RAW are both enabled in the same way */ > @@ -359,22 +618,33 @@ parent_enable_location_gathering_ready (MMIfaceModemLocation *self, > location_ctx->enabled_sources |= ctx->source; > } > > + > if (start_gps) { > - /* We enable continuous GPS fixes */ > - mm_base_modem_at_command_full (MM_BASE_MODEM (self), > - mm_base_modem_peek_best_at_port (MM_BASE_MODEM (self), NULL), > - "AT^SGPSS=4", > - 3, > - FALSE, > - FALSE, /* raw */ > - NULL, /* cancellable */ > - (GAsyncReadyCallback)gps_enabled_ready, > - ctx); > + //Can only setup/tear down one thing at a time. > + if (ctx->disconnect_pending != NULL) > + { > + g_simple_async_result_set_error (ctx->result, > + MM_CORE_ERROR, > + MM_CORE_ERROR_IN_PROGRESS, > + "Can't setup GPS while another process is trying to disconnect it."); > + > + //*_complete_and_free_full is safe to use for the connect flow after this point. > + ctx->connect_pending = NULL; > + location_gathering_context_complete_and_free (ctx); > + return; > + } > + > + ctx->enable_state = GPS_CONTEXT_STEP_SGPSS; > + ctx->connect_pending = self; > + ctx->gps_retry = 0; > + try_gps_enable(self, ctx); > + > return; > } > > /* For any other location (e.g. 3GPP), or if GPS already running just return */ > g_simple_async_result_set_op_res_gboolean (ctx->result, TRUE); > + ctx->connect_pending = NULL; > location_gathering_context_complete_and_free (ctx); > } > > @@ -393,6 +663,8 @@ mm_common_cinterion_enable_location_gathering (MMIfaceModemLocation *self, > user_data, > mm_common_cinterion_enable_location_gathering); > ctx->source = source; > + ctx->connect_pending = NULL; > + ctx->disconnect_pending = NULL; > > /* Chain up parent's gathering enable */ > iface_modem_location_parent->enable_location_gathering ( > @@ -443,11 +715,41 @@ mm_common_cinterion_setup_gps_port (MMBroadbandModem *self) > if (gps_data_port) { > /* It may happen that the modem was started with GPS already enabled, or > * maybe ModemManager got rebooted and it was left enabled before. We'll make > - * sure that it is disabled when we initialize the modem */ > + * sure that it is disabled when we initialize the modem. > + */ > + > + //It may seem odd that we are turning Antenna & Engine on but it is critical for the PLS8 > + //that this happen here. There is a bug that keeps 'Engine on' from succeding on > + //the first go around in the 'try_gps_enable' flow. Cinterion confirmed this a bug. Retry's in > + //the connect flow won't help and the only way found so far is to enable the engine here, > + //before the modem is enabled. This is not ideal and should be redone as soon as the root > + //cause of the bug is identified. This bug does not appear on my x86 platform. -Matt Stanger > + //TODO: Fix bug in 'try_gps_enable' where 'Engine on' command gets stuck returning 767 errors > + //on ARM AT91SAM9263 processor then change these to default off/0. > mm_base_modem_at_command_full (MM_BASE_MODEM (self), > mm_base_modem_peek_best_at_port (MM_BASE_MODEM (self), NULL), > "AT^SGPSS=0", > 3, FALSE, FALSE, NULL, NULL, NULL); > + g_usleep (100); > + > + mm_base_modem_at_command_full (MM_BASE_MODEM (self), > + mm_base_modem_peek_best_at_port (MM_BASE_MODEM (self), NULL), > + "^SGPSC=\"Power/Antenna\",\"on\"", > + 3, FALSE, FALSE, NULL, NULL, NULL); > + g_usleep (100); > + > + mm_base_modem_at_command_full (MM_BASE_MODEM (self), > + mm_base_modem_peek_best_at_port (MM_BASE_MODEM (self), NULL), > + "^SGPSC=\"Engine\",\"1\"", > + 3, FALSE, FALSE, NULL, NULL, NULL); > + > + g_usleep (100); > + > + mm_base_modem_at_command_full (MM_BASE_MODEM (self), > + mm_base_modem_peek_best_at_port (MM_BASE_MODEM (self), NULL), > + "^SGPSC=\"NMEA/Output\",\"off\"", > + 3, FALSE, FALSE, NULL, NULL, NULL); > + > > /* Add handler for the NMEA traces */ > mm_port_serial_gps_add_trace_handler (gps_data_port, > diff --git a/plugins/cinterion/mm-modem-helpers-cinterion.c b/plugins/cinterion/mm-modem-helpers-cinterion.c > index bffe00a..14b18dd 100644 > --- a/plugins/cinterion/mm-modem-helpers-cinterion.c > +++ b/plugins/cinterion/mm-modem-helpers-cinterion.c > @@ -10,12 +10,15 @@ > * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > * GNU General Public License for more details: > * > + * Copyright (C) 2016 Trimble Navigation Limited > * Copyright (C) 2014 Aleksander Morgado <<a href="mailto:aleksander@aleksander.es">aleksander@aleksander.es</a>> > + * Contributor: Matthew Stanger <<a href="mailto:matthew_stanger@trimble.com">matthew_stanger@trimble.com</a>> > */ > > #include <config.h> > #include <string.h> > #include <stdlib.h> > +#include <stdio.h> > > #include "ModemManager.h" > #define _LIBMM_INSIDE_MM > @@ -24,6 +27,7 @@ > #include "mm-charsets.h" > #include "mm-errors-types.h" > #include "mm-modem-helpers-cinterion.h" > +#include "mm-modem-helpers.h" > > /* Setup relationship between the 3G band bitmask in the modem and the bitmask > * in ModemManager. */ > @@ -482,3 +486,187 @@ mm_cinterion_parse_sind_response (const gchar *response, > > return TRUE; > } > + > +/*****************************************************************************/ > +/* ^DHCP response parser */ > + > +static gboolean > +match_info_to_ip4_addr (GMatchInfo *match_info, > + guint match_index, > + guint *out_addr) > +{ > + gchar *s, *bin = NULL; > + gchar buf[9]; > + gsize len, bin_len; > + gboolean success = FALSE; > + > + s = g_match_info_fetch (match_info, match_index); > + g_return_val_if_fail (s != NULL, FALSE); > + > + len = strlen (s); > + if (len == 1 && s[0] == '0') { > + *out_addr = 0; > + success = TRUE; > + goto done; > + } > + if (len < 7 || len > 8) > + goto done; > + > + /* Handle possibly missing leading zero */ > + memset (buf, 0, sizeof (buf)); > + if (len == 7) { > + strcpy (&buf[1], s); > + buf[0] = '0'; > + } else if (len == 8) > + strcpy (buf, s); > + else > + g_assert_not_reached (); > + > + bin = mm_utils_hexstr2bin (buf, &bin_len); > + if (!bin || bin_len != 4) > + goto done; > + > + *out_addr = GUINT32_SWAP_LE_BE (*((guint32 *) bin)); > + success = TRUE; > + > +done: > + g_free (s); > + g_free (bin); > + return success; > +} > + > +gboolean > +mm_cinterion_parse_dhcp_response (const char *reply, > + guint *out_address, > + guint *out_prefix, > + guint *out_gateway, > + guint *out_dns1, > + guint *out_dns2, > + GError **error) > +{ > + gboolean matched; > + GRegex *r; > + GMatchInfo *match_info = NULL; > + GError *match_error = NULL; > + > + g_assert (reply != NULL); > + g_assert (out_address != NULL); > + g_assert (out_prefix != NULL); > + g_assert (out_gateway != NULL); > + g_assert (out_dns1 != NULL); > + g_assert (out_dns2 != NULL); > + > + /* Format: > + * > + * ^DHCP: <address>,<netmask>,<gateway>,<?>,<dns1>,<dns2>,<uplink>,<downlink> > + * > + * All numbers are hexadecimal representations of IPv4 addresses, with > + * least-significant byte first. eg, 192.168.50.32 is expressed as > + * "2032A8C0". Sometimes leading zeros are stripped, so "1010A0A" is > + * actually 10.10.1.1. > + */ > + > + r = g_regex_new ("\\^DHCP:\\s*([0-9a-fA-F]+),([0-9a-fA-F]+),([0-9a-fA-F]+),([0-9a-fA-F]+),([0-9a-fA-F]+),([0-9a-fA-F]+),.*$", 0, 0, NULL); > + g_assert (r != NULL); > + > + matched = g_regex_match_full (r, reply, -1, 0, 0, &match_info, &match_error); > + if (!matched) { > + if (match_error) { > + g_propagate_error (error, match_error); > + g_prefix_error (error, "Could not parse ^DHCP results: "); > + } else { > + g_set_error_literal (error, > + MM_CORE_ERROR, > + MM_CORE_ERROR_FAILED, > + "Couldn't match ^DHCP reply"); > + } > + } else { > + guint netmask; > + > + if (match_info_to_ip4_addr (match_info, 1, out_address) && > + match_info_to_ip4_addr (match_info, 2, &netmask) && > + match_info_to_ip4_addr (match_info, 3, out_gateway) && > + match_info_to_ip4_addr (match_info, 5, out_dns1) && > + match_info_to_ip4_addr (match_info, 6, out_dns2)) { > + *out_prefix = mm_count_bits_set (netmask); > + matched = TRUE; > + } > + } > + > + if (match_info) > + g_match_info_free (match_info); > + g_regex_unref (r); > + return matched; > +} > + > +/*****************************************************************************/ > +/* ^SWWAN read parser > + * Description: Parses <cid>, <state>[, <WWAN adapter>] or CME ERROR from SWWAN. > + * Return: False if error occured while trying to parse SWWAN. > + * Requires: Check for AT Error response before calling. > + * Read Command > + * AT^SWWAN? > + * Response(s)  > + * [^SWWAN: <cid>, <state>[, <WWAN adapter>]] > + * [^SWWAN: ...] > + * OK > + * ERROR > + * +CME ERROR: <err> > + * > + * Examples: > + * OK - If no WWAN connection is active, then read command just returns OK > + * ^SWWAN: 3,1,1 - 3rd PDP Context, Activated, First WWAN Adaptor > + * +CME ERROR: ? - > +*/ > + This is much better than returning a blind GList of integers where the caller needs to know which integer is which: gboolean mm_cinterion_parse_swwan_response ( const gchar *response, guint *cid, guint *state, guint *wwan_adapter, GError **error); Also, please write unit tests for the newly written parsers. > +gboolean > +mm_cinterion_parse_swwan_response (const gchar *response, > + GList **result, > + GError **error) > +{ > + if (!response){ > + g_set_error (error, > + MM_CORE_ERROR, > + MM_CORE_ERROR_FAILED, > + "Recieved NULL from SWWAN response."); > + return FALSE; > + } > + > + //Parse for [^SWWAN: <cid>, <state>[, <WWAN adapter>]] > + if (strcasestr (response, "SWWAN")) > + { > + gint matched; > + guint cid, state, wwan_adapter; > + matched = sscanf (response, "^SWWAN: %d,%d,%d", > + &cid, > + &state, > + &wwan_adapter); > + > + if (matched != 3) { > + g_set_error (error, > + MM_CORE_ERROR, > + MM_CORE_ERROR_FAILED, > + "Could not parse SWWAN response: '%s'", > + response); > + return FALSE; > + } > + > + *result = g_list_append (*result, GINT_TO_POINTER(cid)); > + *result = g_list_append (*result, GINT_TO_POINTER(state)); > + *result = g_list_append (*result, GINT_TO_POINTER(wwan_adapter)); > + } > + //TODO: It'd be nice to get 'OK' from response so we don't have to assume that > + //zero length response means 'OK' or am I doing it wrong... > + else if (!g_utf8_strlen (response, 100)) > + *result = g_list_append (*result, GINT_TO_POINTER(0)); > + else { > + g_set_error (error, > + MM_CORE_ERROR, > + MM_CORE_ERROR_FAILED, > + "Unknown SWWAN response, unable to parse."); > + return FALSE; > + } > + > + > + return TRUE; > +} > diff --git a/plugins/cinterion/mm-modem-helpers-cinterion.h b/plugins/cinterion/mm-modem-helpers-cinterion.h > index d37bcb0..11754ae 100644 > --- a/plugins/cinterion/mm-modem-helpers-cinterion.h > +++ b/plugins/cinterion/mm-modem-helpers-cinterion.h > @@ -10,7 +10,9 @@ > * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > * GNU General Public License for more details: > * > + * Copyright (C) 2016 Trimble Navigation Limited > * Copyright (C) 2014 Aleksander Morgado <<a href="mailto:aleksander@aleksander.es">aleksander@aleksander.es</a>> > + * Contributor: Matthew Stanger <<a href="mailto:matthew_stanger@trimble.com">matthew_stanger@trimble.com</a>> > */ > > #ifndef MM_MODEM_HELPERS_CINTERION_H > @@ -63,4 +65,19 @@ gboolean mm_cinterion_parse_sind_response (const gchar *response, > guint *value, > GError **error); > > +/*****************************************************************************/ > +/* ^DHCP response parser */ > +gboolean mm_cinterion_parse_dhcp_response (const char *reply, > + guint *out_address, > + guint *out_prefix, > + guint *out_gateway, > + guint *out_dns1, > + guint *out_dns2, > + GError **error); > + > +/*****************************************************************************/ > +/* ^SWWAN response parser */ > +gboolean mm_cinterion_parse_swwan_response (const gchar *response, > + GList **result, > + GError **error); > #endif /* MM_MODEM_HELPERS_CINTERION_H */ > diff --git a/plugins/cinterion/mm-plugin-cinterion.c b/plugins/cinterion/mm-plugin-cinterion.c > index 8af0a2a..d944c00 100644 > --- a/plugins/cinterion/mm-plugin-cinterion.c > +++ b/plugins/cinterion/mm-plugin-cinterion.c > @@ -18,7 +18,9 @@ > * > * Copyright (C) 2011 Ammonit Measurement GmbH > * Copyright (C) 2011 - 2012 Google Inc. > + * Copyright (C) 2016 Trimble Navigation Limited > * Author: Aleksander Morgado <<a href="mailto:aleksander@lanedo.com">aleksander@lanedo.com</a>> > + * Contributor: Matthew Stanger <<a href="mailto:matthew_stanger@trimble.com">matthew_stanger@trimble.com</a>> > */ > > #include <string.h> > @@ -45,6 +47,7 @@ int mm_plugin_minor_version = MM_PLUGIN_MINOR_VERSION; > > #define TAG_CINTERION_APP_PORT "cinterion-app-port" > #define TAG_CINTERION_MODEM_PORT "cinterion-modem-port" > +#define TAG_CINTERION_GPS_PORT "ID_MM_CINTERION_PORT_TYPE_GPS" > > typedef struct { > MMPortProbe *probe; > @@ -175,9 +178,14 @@ grab_port (MMPlugin *self, > mm_dbg ("(%s/%s)' Port flagged as PPP", > mm_port_probe_get_port_subsys (probe), > mm_port_probe_get_port_name (probe)); > - pflags = MM_PORT_SERIAL_AT_FLAG_PPP; > + > + //Assuming PLS8 will stay idProduct 0061 and for that product we'll use SWWAN, not PPP. > + if ((g_strcmp0 (g_udev_device_get_property(mm_port_probe_peek_port (probe),"ID_MODEL_ID"),"0061") == 0)) > + pflags = MM_PORT_SERIAL_AT_FLAG_SECONDARY; > + else > + pflags = MM_PORT_SERIAL_AT_FLAG_PPP; > } else if (g_udev_device_get_property_as_boolean (mm_port_probe_peek_port (probe), > - "ID_MM_CINTERION_PORT_TYPE_GPS")) { > + TAG_CINTERION_GPS_PORT)) { > mm_dbg ("(%s/%s)' Port flagged as GPS", Ignore the GPS flag for now in this patch. > mm_port_probe_get_port_subsys (probe), > mm_port_probe_get_port_name (probe)); > @@ -185,6 +193,135 @@ grab_port (MMPlugin *self, > g_warn_if_fail (ptype == MM_PORT_TYPE_UNKNOWN); > ptype = MM_PORT_TYPE_GPS; > } > + //TODO: This should be handled by uDev w/ cinterion-port-types.rules but I can't get > + //it to work. Added the rule in there to set the ENV variable TAG_CINTERION_GPS_PORT. > + //Once that is fixed, it will set the ENV var above & you can delete this else if. > + else if ((g_strcmp0 (g_udev_device_get_property(mm_port_probe_peek_port (probe),"ID_USB_INTERFACE_NUM"),"04") == 0) && > + (g_strcmp0 (g_udev_device_get_property(mm_port_probe_peek_port (probe),"ID_MODEL_ID"),"0061") == 0)) { > + mm_dbg ("(%s/%s)' Port flagged as GPS", > + mm_port_probe_get_port_subsys (probe), > + mm_port_probe_get_port_name (probe)); > + /* Not an AT port, but the port to grab GPS traces */ > + g_warn_if_fail (ptype == MM_PORT_TYPE_UNKNOWN); > + ptype = MM_PORT_TYPE_GPS; Ignore the GPS flag for now in this patch. > + } > + > + /* udev info for PLS8-X of GPS Port, for troubleshooting above > + looking at device '/devices/pci0000:00/0000:00:14.0/usb3/3-2/3-2:1.4/tty/ttyACM2': > + KERNEL=="ttyACM2" > + SUBSYSTEM=="tty" > + DRIVER=="" > + > + looking at parent device '/devices/pci0000:00/0000:00:14.0/usb3/3-2/3-2:1.4': > + KERNELS=="3-2:1.4" > + SUBSYSTEMS=="usb" > + DRIVERS=="cdc_acm" > + ATTRS{bInterfaceClass}=="02" > + ATTRS{iad_bFunctionClass}=="02" > + ATTRS{bmCapabilities}=="2" > + ATTRS{iad_bFirstInterface}=="04" > + ATTRS{bInterfaceSubClass}=="02" > + ATTRS{bInterfaceProtocol}=="01" > + ATTRS{bNumEndpoints}=="01" > + ATTRS{iad_bFunctionSubClass}=="02" > + ATTRS{iad_bFunctionProtocol}=="01" > + ATTRS{supports_autosuspend}=="1" > + ATTRS{iad_bInterfaceCount}=="02" > + ATTRS{bAlternateSetting}==" 0" > + ATTRS{bInterfaceNumber}=="04" > + ATTRS{interface}=="CDC Abstract Control Model (ACM)" > + > + looking at parent device '/devices/pci0000:00/0000:00:14.0/usb3/3-2': > + KERNELS=="3-2" > + SUBSYSTEMS=="usb" > + DRIVERS=="usb" > + ATTRS{bDeviceSubClass}=="00" > + ATTRS{bDeviceProtocol}=="00" > + ATTRS{devpath}=="2" > + ATTRS{idVendor}=="1e2d" > + ATTRS{speed}=="480" > + ATTRS{bNumInterfaces}=="14" > + ATTRS{bConfigurationValue}=="1" > + ATTRS{bMaxPacketSize0}=="64" > + ATTRS{busnum}=="3" > + ATTRS{devnum}=="30" > + ATTRS{configuration}=="" > + ATTRS{bMaxPower}=="20mA" > + ATTRS{authorized}=="1" > + ATTRS{bmAttributes}=="e0" > + ATTRS{bNumConfigurations}=="1" > + ATTRS{maxchild}=="0" > + ATTRS{bcdDevice}=="0232" > + ATTRS{avoid_reset_quirk}=="0" > + ATTRS{quirks}=="0x0" > + ATTRS{version}==" 2.00" > + ATTRS{urbnum}=="3395" > + ATTRS{ltm_capable}=="no" > + ATTRS{manufacturer}=="Cinterion" > + ATTRS{removable}=="removable" > + ATTRS{idProduct}=="0061" > + ATTRS{bDeviceClass}=="00" > + ATTRS{product}=="LTE Modem" > + We don't need your usb host controller details... :) > + looking at parent device '/devices/pci0000:00/0000:00:14.0/usb3': > + KERNELS=="usb3" > + SUBSYSTEMS=="usb" > + DRIVERS=="usb" > + ATTRS{bDeviceSubClass}=="00" > + ATTRS{bDeviceProtocol}=="01" > + ATTRS{devpath}=="0" > + ATTRS{idVendor}=="1d6b" > + ATTRS{speed}=="480" > + ATTRS{bNumInterfaces}==" 1" > + ATTRS{bConfigurationValue}=="1" > + ATTRS{bMaxPacketSize0}=="64" > + ATTRS{authorized_default}=="1" > + ATTRS{busnum}=="3" > + ATTRS{devnum}=="1" > + ATTRS{configuration}=="" > + ATTRS{bMaxPower}=="0mA" > + ATTRS{authorized}=="1" > + ATTRS{bmAttributes}=="e0" > + ATTRS{bNumConfigurations}=="1" > + ATTRS{maxchild}=="4" > + ATTRS{bcdDevice}=="0313" > + ATTRS{avoid_reset_quirk}=="0" > + ATTRS{quirks}=="0x0" > + ATTRS{serial}=="0000:00:14.0" > + ATTRS{version}==" 2.00" > + ATTRS{urbnum}=="516" > + ATTRS{ltm_capable}=="no" > + ATTRS{manufacturer}=="Linux 3.13.0-48-generic xhci_hcd" > + ATTRS{removable}=="unknown" > + ATTRS{idProduct}=="0002" > + ATTRS{bDeviceClass}=="09" > + ATTRS{product}=="xHCI Host Controller" > + > + looking at parent device '/devices/pci0000:00/0000:00:14.0': > + KERNELS=="0000:00:14.0" > + SUBSYSTEMS=="pci" > + DRIVERS=="xhci_hcd" > + ATTRS{irq}=="42" > + ATTRS{subsystem_vendor}=="0x1028" > + ATTRS{broken_parity_status}=="0" > + ATTRS{class}=="0x0c0330" > + ATTRS{consistent_dma_mask_bits}=="64" > + ATTRS{dma_mask_bits}=="64" > + ATTRS{local_cpus}=="00000000,00000000,00000000,00000000,00000000,00000000,00000000,000000ff" > + ATTRS{device}=="0x1e31" > + ATTRS{enable}=="1" > + ATTRS{msi_bus}=="" > + ATTRS{local_cpulist}=="0-7" > + ATTRS{vendor}=="0x8086" > + ATTRS{subsystem_device}=="0x0577" > + ATTRS{numa_node}=="-1" > + ATTRS{d3cold_allowed}=="1" > + > + looking at parent device '/devices/pci0000:00': > + KERNELS=="pci0000:00" > + SUBSYSTEMS=="" > + DRIVERS=="" > + */ > > return mm_base_modem_grab_port (modem, > mm_port_probe_get_port_subsys (probe), > diff --git a/src/mm-base-modem.c b/src/mm-base-modem.c > index 9cd7c5f..449dddc 100644 > --- a/src/mm-base-modem.c > +++ b/src/mm-base-modem.c > @@ -674,6 +674,26 @@ mm_base_modem_peek_best_data_port (MMBaseModem *self, > return NULL; > } > > +MMPort * > +mm_base_modem_peek_current_data_port (MMBaseModem *self, > + MMPortType type) > +{ > + GList *l; > + > + g_return_val_if_fail (MM_IS_BASE_MODEM (self), NULL); > + > + /* Return first connected data port */ > + for (l = self->priv->data; l; l = g_list_next (l)) { > + if (mm_port_get_connected ((MMPort *)l->data) && > + (mm_port_get_port_type ((MMPort *)l->data) == type || > + type == MM_PORT_TYPE_UNKNOWN)) { > + return (MMPort *)l->data; > + } > + } > + > + return NULL; > +} > + > GList * > mm_base_modem_get_data_ports (MMBaseModem *self) > { > diff --git a/src/mm-base-modem.h b/src/mm-base-modem.h > index 3c0d16f..bbea52d 100644 > --- a/src/mm-base-modem.h > +++ b/src/mm-base-modem.h > @@ -131,6 +131,7 @@ MMPortMbim *mm_base_modem_peek_port_mbim_for_data (MMBaseModem *self, MMPo > #endif > MMPortSerialAt *mm_base_modem_peek_best_at_port (MMBaseModem *self, GError **error); > MMPort *mm_base_modem_peek_best_data_port (MMBaseModem *self, MMPortType type); > +MMPort *mm_base_modem_peek_current_data_port (MMBaseModem *self, MMPortType type); > GList *mm_base_modem_peek_data_ports (MMBaseModem *self); > > MMPortSerialAt *mm_base_modem_get_port_primary (MMBaseModem *self); > -- > 1.9.1 > > -- Aleksander <a href="https://aleksander.es" rel="noreferrer" target="_blank">https://aleksander.es</a> </blockquote></div> </div>