<div dir="ltr">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:<div><br><div><div>Converted Enum types from all caps to camel case.</div><div>Changed pdp_cid from gchar to guint</div><div>Moved retry_count from ..CinterionPrivate to 3gppContext</div><div>Changed all single like '//' to block '/*' comments</div><div>Removed brackets from any case stamement which was not defining variables</div><div>All state machines refactored so default state results in assert exit</div><div>Changed states which were programing failure states from async errors to assert failures</div><div>Replaced gusleep calls with g_timeout_add callbacks</div><div>Fixed pdp_cid to map g_udev_device_get_property_as_int->ID_USB_INTERFACE_NUM to context instead of hardcoded usb# interface.</div><div>Connect/Disconnect logic now has different contexts in addition to complete_and_free()</div><div>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.</div><div>Changed pdp_cid matching logic from hard mapped USB0/1 strings to use udev</div><div>Changed pdp_cid to read as hex value</div><div>Removed retry logic, that logic should be left up to calling program</div><div>Updated parenthesis spacing</div><div>Provided context on Cinterion pdp_cid mapping in pdp_cid_map() function</div><div>Added SWWAN parser unit test</div><div>Moved SWWAN test back to bearer to avoid MM enable init issue</div><div>SWWAN test for bearer creation will now cache</div><div>Updated SWWAN parser to use Glist of struct for dual connections</div><div>Provided context on optional <WWAN adapter> in code</div><div>Deleted unnecessary peek_current_data_port in disconnect flow.</div><div>Fixed send_write_command_ctx_connect pass by ref, when it should have been const</div><div>Changed bearer_interface ctx from string cmp to int</div><div>Fixed parenthesis & var spacing issues</div></div></div><div>Fixed alignment issues</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Oct 26, 2016 at 3:10 AM, Aleksander Morgado <span dir="ltr"><<a href="mailto:aleksander@aleksander.es" target="_blank">aleksander@aleksander.es</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hey!<br>
<br>
This looks much much better :)<br>
See comments next, in no particular order.<br>
<span class=""><br>
On Mon, Oct 24, 2016 at 7:13 AM, matthew stanger <<a href="mailto:stangerm2@gmail.com">stangerm2@gmail.com</a>> wrote:<br>
> Here is finally the update... to the above code reviewed patch. A big thank<br>
> you Dan and Aleksander for the detail in the review it was really a great<br>
> learning experience for me. Please don't hold back with this one either :).<br>
> It's a pretty big rewrite so I'm sure there will still be some minor things<br>
> to tidy up still but I think it should be more inline now. There is one<br>
> thing in here for sure that needs fixing still but I wanted your options so<br>
> please see the note at line 1192 and let me know what you think.<br>
<br>
</span>Regarding:<br>
<br>
> + /* TODO: Before upstream merge. This needs check_for_swwan_support to block<br>
> + * until it's finish. Otherwise there is race conidion with swwan_support assert?<br>
> + * I'm really not sure how else to do this other than move it or use sleep :? */<br>
<br>
<br>
The check_for_swwan_support() is calling<br>
mm_base_modem_at_command_full(<wbr>) with a callback, so the whole<br>
"enabling_modem_init" step will be running until the AT command is<br>
finished and you can see whether SWWAN is supported or not. BTW; given<br>
it's a "test" command (i.e. ^SWWAN=? should always reply the same<br>
thing regardless of the modem state), you can use allow_cached=TRUE in<br>
the mm_base_modem_at_command_full(<wbr>) call, so that if it ever gets<br>
called again we don't ask the modem but use the cached response<br>
instead.<br>
<br>
More comments:<br>
<span class=""><br>
> +static void<br>
> +disconnect_3gpp (MMBroadbandBearer *self,<br>
> + MMBroadbandModem *modem,<br>
> + MMPortSerialAt *primary,<br>
> + MMPortSerialAt *secondary,<br>
> + MMPort *data,<br>
> + guint cid,<br>
> + GAsyncReadyCallback callback,<br>
> + gpointer user_data)<br>
> +{<br>
</span>> + Disconnect3gppContext *ctx;<br>
<span class="">> + MMPort *port;<br>
> +<br>
> + g_assert (primary != NULL);<br>
> +<br>
</span>> + /* Note: Not sure how else to get active data port? Can this be done without adding this<br>
> + * function to mm-base-modem.c?<br>
> + * TODO: Dual SIM how do we know which interface to grab/disconnect if two are active? */<br>
> + /* Get the Net port to be torn down */<br>
<span class="">> + port = mm_base_modem_peek_current_<wbr>data_port (MM_BASE_MODEM (modem), MM_PORT_TYPE_NET);<br>
<br>
</span>You don't need this peek_current_data_port() method; the "MMPort<br>
*data" that you get as input is already the NET port that is being<br>
disconnected.<br>
<br>
> +static void<br>
> +send_write_command_ctx_<wbr>connect (Connect3gppContext *ctx,<br>
> + gchar **command)<br>
<br>
You can pass the command as a "const gchar *command" instead of gchar<br>
**, which seems weird because the command string isn't being updated<br>
inside the method.<br>
<br>
> + if (g_strcmp0 (bearer_interface, "0a") == 0)<br>
> + self->priv->pdp_cid = 3;<br>
> + else if (g_strcmp0 (bearer_interface, "0c") == 0)<br>
> + self->priv->pdp_cid = 1;<br>
<br>
I know this works, but to me it looks saner to get the interface<br>
number with mm_kernel_device_get_property_<wbr>as_int() and then directly<br>
do a numeric comparison with 0x0a or 0x0c.<br>
<span class=""><br>
><br>
> +static gint<br>
> +cinterion_parse_auth_type (MMBearerAllowedAuth mm_auth)<br>
<br>
<br>
</span>This can return a BearerCinterionAuthType instead of an int, right?<br>
<br>
> +static void<br>
> +pdp_cid_map (MMBroadbandBearerCinterion *self, const gchar *bearer_interface)<br>
> +{<br>
> + /* Map PDP context from the current Bearer. USB0 -> 3rd context, USB1 -> 1st context.<br>
> + * Note - Cinterion told me specifically to map the contexts to the interfaces this way, though<br>
> + * I've seen that SWWAN appear's to work fine with any PDP to any interface */<br>
<br>
I would ask Cinterion for confirmation on why this mapping is needed.<br>
Maybe those are just the defaults if the <WWAN adapter> parameter in<br>
SWWAN isn't given?<br>
<br>
<br>
> + g_list_free(response_parsed);<br>
<br>
Note we include a whitespace always before every parenthesis, even in<br>
method calls. There are multiple places in the code where this is<br>
still being done.<br>
<br>
> + /* Get the SWWAN read response */<br>
<span class="">> + response = mm_base_modem_at_command_<wbr>finish (modem, res, &error);<br>
> +<br>
</span>> + /* Error if parsing SWWAN read response fails */<br>
> + if (!mm_cinterion_parse_swwan_<wbr>response (response, &response_parsed, &error)) {<br>
<span class="">> + g_simple_async_result_take_<wbr>error (ctx->result, error);<br>
> + connect_3gpp_context_complete_<wbr>and_free (ctx);<br>
</span>> + return;<br>
> + }<br>
<br>
You shouldn't do this. Before calling<br>
mm_cinterion_parse_swwan_<wbr>response<br>
(), first check whether you have a NULL response and therefore error<br>
already set, e.g.:<br>
<span class=""><br>
response = mm_base_modem_at_command_<wbr>finish (modem, res, &error);<br>
</span>if (!response || !mm_cinterion_parse_swwan_<wbr>response (response,<br>
&response_parsed, &error)) {<br>
g_simple_async_result_take_<wbr>error (ctx->result, error);<br>
connect_3gpp_context_complete_<wbr>and_free (ctx);<br>
return;<br>
<span class="">}<br>
<br>
> +gboolean<br>
> +mm_cinterion_parse_swwan_<wbr>response (const gchar *response,<br>
> + GList **result,<br>
</span>> + GError **error)<br>
> +{<br>
> + if (*error)<br>
> + return FALSE;<br>
> +<br>
><br>
<span class="">> + if (!response) {<br>
> + g_set_error (error,<br>
</span>> + MM_CORE_ERROR,<br>
> + MM_CORE_ERROR_FAILED,<br>
> + "Recieved NULL from SWWAN response.");<br>
> + return FALSE;<br>
> + }<br>
<br>
I would actually remove this two checks (the *error and the !response<br>
one) all together. You don't need them if you're making sure response<br>
is set in the caller. You can g_assert (response); if you want, but<br>
just ignore the state of error, error should always be considered a<br>
pointer to a GError which is set to NULL, that is the convention used<br>
for all methods accepting a GError.<br>
<span class=""><br>
> +gboolean<br>
> +mm_cinterion_parse_swwan_<wbr>response (const gchar *response,<br>
> + GList **result,<br>
> + GError **error)<br>
</span>> ...<br>
<span class="">> + *result = g_list_append (*result, GINT_TO_POINTER(cid));<br>
> + *result = g_list_append (*result, GINT_TO_POINTER(state));<br>
> + *result = g_list_append (*result, GINT_TO_POINTER(wwan_adapter))<wbr>;<br>
<br>
</span>Instead of returning a blind list of integers, let's do it in some<br>
other way. You can define in the API of the helpers a struct like<br>
this:<br>
typedef struct {<br>
guint cid;<br>
guint state;<br>
guint wwan_adapter;<br>
} SwwanResponseItem;<br>
<br>
Your parse_swwan_response() could then be like you had it implemented,<br>
but instead of a list of integers, you would return a list of<br>
SwwanResponseItem structs. This will also support the SWWAN responses<br>
with multiple lines, which is what the command would do:<br>
AT^SWWAN?<br>
<span class=""> [^SWWAN: <cid>, <state>[, <WWAN adapter>]]<br>
</span> [^SWWAN: ...]<br>
<br>
> + /* TODO: It'd be nice to get 'OK' from response so we don't have to assume that<br>
> + * zero length response means 'OK' or am I doing it wrong?... */<br>
<span class="">> + else if (!g_utf8_strlen (response, 100))<br>
> + *result = g_list_append (*result, GINT_TO_POINTER(0));<br>
<br>
</span>No need for that g_utf8_strlen(); if you get an empty string that was<br>
because the response was just "OK". With the new API with the list of<br>
SwwanResponseItem items, you would be returning TRUE in the method and<br>
a NULL list (i.e. empty list).<br>
<br>
Also, note that the command response format says that <WWAN adapter><br>
is optional. Does that mean that there are cases where SWWAN doesn't<br>
return the WWAN adapter? If so, the sscanf() won't return a 3 and you<br>
would be returning an error.<br>
<br>
> + /* 3rd context -> 1st wwan adapt / 1st context -> 2nd wwan adapt */<br>
> + gchar *command;<br>
<br>
Just one single whitespace in between the type and name is enough,<br>
unless you're aligning things, which doesn't seem the case here. There<br>
are other places in the patch where the same happens I think.<br>
<span class="HOEnZb"><font color="#888888"><br>
<br>
--<br>
Aleksander<br>
<a href="https://aleksander.es" rel="noreferrer" target="_blank">https://aleksander.es</a><br>
</font></span></blockquote></div><br></div>