Fwd: Cinterion plugin patch
matthew stanger
stangerm2 at gmail.com
Sun Nov 20 18:26:14 UTC 2016
Ok, so clearly I'm not as fast as I wish, but another patch never the less.
This should be pretty close but I'll keep at it if there are any changes
still needed so let me know :). Here are the changes since the first patch
of this chain:
Converted Enum types from all caps to camel case.
Changed pdp_cid from gchar to guint
Moved retry_count from ..CinterionPrivate to 3gppContext
Changed all single like '//' to block '/*' comments
Removed brackets from any case stamement which was not defining variables
All state machines refactored so default state results in assert exit
Changed states which were programing failure states from async errors to
assert failures
Replaced gusleep calls with g_timeout_add callbacks
Fixed pdp_cid to map
g_udev_device_get_property_as_int->ID_USB_INTERFACE_NUM to context instead
of hardcoded usb# interface.
Connect/Disconnect logic now has different contexts in addition to
complete_and_free()
Connect/Disconnect ctx is no longer stored in private info, and instead is
not passed directly. This may be backed out in the future for a URC
supporting patch.
Changed pdp_cid matching logic from hard mapped USB0/1 strings to use udev
Changed pdp_cid to read as hex value
Removed retry logic, that logic should be left up to calling program
Updated parenthesis spacing
Provided context on Cinterion pdp_cid mapping in pdp_cid_map() function
Added SWWAN parser unit test
Moved SWWAN test back to bearer to avoid MM enable init issue
SWWAN test for bearer creation will now cache
Updated SWWAN parser to use Glist of struct for dual connections
Provided context on optional <WWAN adapter> in code
Deleted unnecessary peek_current_data_port in disconnect flow.
Fixed send_write_command_ctx_connect pass by ref, when it should have been
const
Changed bearer_interface ctx from string cmp to int
Fixed parenthesis & var spacing issues
Fixed alignment issues
On Wed, Oct 26, 2016 at 3:10 AM, Aleksander Morgado <
aleksander at aleksander.es> wrote:
> Hey!
>
> This looks much much better :)
> See comments next, in no particular order.
>
> On Mon, Oct 24, 2016 at 7:13 AM, matthew stanger <stangerm2 at gmail.com>
> wrote:
> > 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.
>
> Regarding:
>
> > + /* TODO: Before upstream merge. This needs check_for_swwan_support
> to block
> > + * until it's finish. Otherwise there is race conidion with
> swwan_support assert?
> > + * I'm really not sure how else to do this other than move it or use
> sleep :? */
>
>
> The check_for_swwan_support() is calling
> mm_base_modem_at_command_full() with a callback, so the whole
> "enabling_modem_init" step will be running until the AT command is
> finished and you can see whether SWWAN is supported or not. BTW; given
> it's a "test" command (i.e. ^SWWAN=? should always reply the same
> thing regardless of the modem state), you can use allow_cached=TRUE in
> the mm_base_modem_at_command_full() call, so that if it ever gets
> called again we don't ask the modem but use the cached response
> instead.
>
> More comments:
>
> > +static void
> > +disconnect_3gpp (MMBroadbandBearer *self,
> > + MMBroadbandModem *modem,
> > + MMPortSerialAt *primary,
> > + MMPortSerialAt *secondary,
> > + MMPort *data,
> > + guint cid,
> > + GAsyncReadyCallback callback,
> > + gpointer user_data)
> > +{
> > + Disconnect3gppContext *ctx;
> > + MMPort *port;
> > +
> > + g_assert (primary != NULL);
> > +
> > + /* Note: 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? */
> > + /* Get the Net port to be torn down */
> > + port = mm_base_modem_peek_current_data_port (MM_BASE_MODEM
> (modem), MM_PORT_TYPE_NET);
>
> You don't need this peek_current_data_port() method; the "MMPort
> *data" that you get as input is already the NET port that is being
> disconnected.
>
> > +static void
> > +send_write_command_ctx_connect (Connect3gppContext *ctx,
> > + gchar **command)
>
> You can pass the command as a "const gchar *command" instead of gchar
> **, which seems weird because the command string isn't being updated
> inside the method.
>
> > + if (g_strcmp0 (bearer_interface, "0a") == 0)
> > + self->priv->pdp_cid = 3;
> > + else if (g_strcmp0 (bearer_interface, "0c") == 0)
> > + self->priv->pdp_cid = 1;
>
> I know this works, but to me it looks saner to get the interface
> number with mm_kernel_device_get_property_as_int() and then directly
> do a numeric comparison with 0x0a or 0x0c.
>
> >
> > +static gint
> > +cinterion_parse_auth_type (MMBearerAllowedAuth mm_auth)
>
>
> This can return a BearerCinterionAuthType instead of an int, right?
>
> > +static void
> > +pdp_cid_map (MMBroadbandBearerCinterion *self, const gchar
> *bearer_interface)
> > +{
> > + /* Map PDP context from the current Bearer. USB0 -> 3rd context,
> USB1 -> 1st context.
> > + * Note - Cinterion told me specifically to map the contexts to the
> interfaces this way, though
> > + * I've seen that SWWAN appear's to work fine with any PDP to any
> interface */
>
> I would ask Cinterion for confirmation on why this mapping is needed.
> Maybe those are just the defaults if the <WWAN adapter> parameter in
> SWWAN isn't given?
>
>
> > + g_list_free(response_parsed);
>
> Note we include a whitespace always before every parenthesis, even in
> method calls. There are multiple places in the code where this is
> still being done.
>
> > + /* Get the SWWAN read response */
> > + response = mm_base_modem_at_command_finish (modem, res, &error);
> > +
> > + /* Error if parsing SWWAN read response fails */
> > + if (!mm_cinterion_parse_swwan_response (response,
> &response_parsed, &error)) {
> > + g_simple_async_result_take_error (ctx->result, error);
> > + connect_3gpp_context_complete_and_free (ctx);
> > + return;
> > + }
>
> You shouldn't do this. Before calling
> mm_cinterion_parse_swwan_response
> (), first check whether you have a NULL response and therefore error
> already set, e.g.:
>
> response = mm_base_modem_at_command_finish (modem, res, &error);
> if (!response || !mm_cinterion_parse_swwan_response (response,
> &response_parsed, &error)) {
> g_simple_async_result_take_error (ctx->result, error);
> connect_3gpp_context_complete_and_free (ctx);
> return;
> }
>
> > +gboolean
> > +mm_cinterion_parse_swwan_response (const gchar *response,
> > + GList **result,
> > + GError **error)
> > +{
> > + if (*error)
> > + return FALSE;
> > +
> >
> > + if (!response) {
> > + g_set_error (error,
> > + MM_CORE_ERROR,
> > + MM_CORE_ERROR_FAILED,
> > + "Recieved NULL from SWWAN response.");
> > + return FALSE;
> > + }
>
> I would actually remove this two checks (the *error and the !response
> one) all together. You don't need them if you're making sure response
> is set in the caller. You can g_assert (response); if you want, but
> just ignore the state of error, error should always be considered a
> pointer to a GError which is set to NULL, that is the convention used
> for all methods accepting a GError.
>
> > +gboolean
> > +mm_cinterion_parse_swwan_response (const gchar *response,
> > + GList **result,
> > + GError **error)
> > ...
> > + *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))
> ;
>
> Instead of returning a blind list of integers, let's do it in some
> other way. You can define in the API of the helpers a struct like
> this:
> typedef struct {
> guint cid;
> guint state;
> guint wwan_adapter;
> } SwwanResponseItem;
>
> Your parse_swwan_response() could then be like you had it implemented,
> but instead of a list of integers, you would return a list of
> SwwanResponseItem structs. This will also support the SWWAN responses
> with multiple lines, which is what the command would do:
> AT^SWWAN?
> [^SWWAN: <cid>, <state>[, <WWAN adapter>]]
> [^SWWAN: ...]
>
> > + /* 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));
>
> No need for that g_utf8_strlen(); if you get an empty string that was
> because the response was just "OK". With the new API with the list of
> SwwanResponseItem items, you would be returning TRUE in the method and
> a NULL list (i.e. empty list).
>
> Also, note that the command response format says that <WWAN adapter>
> is optional. Does that mean that there are cases where SWWAN doesn't
> return the WWAN adapter? If so, the sscanf() won't return a 3 and you
> would be returning an error.
>
> > + /* 3rd context -> 1st wwan adapt / 1st context -> 2nd wwan adapt */
> > + gchar *command;
>
> Just one single whitespace in between the type and name is enough,
> unless you're aligning things, which doesn't seem the case here. There
> are other places in the patch where the same happens I think.
>
>
> --
> Aleksander
> https://aleksander.es
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20161120/ab6f8bdc/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cinterion_swwan_support.patch
Type: text/x-patch
Size: 61189 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20161120/ab6f8bdc/attachment-0001.bin>
More information about the ModemManager-devel
mailing list