Re: [PATCH 1/2] broadband-modem, helpers: implement AT+WS46=? response parser

[Aleksander Morgado]

On Fri, Mar 24, 2017 at 9:22 PM, Aleksander Morgado
 wrote:
>>> We want a parser that returns the full list of combinations found.
>>
>> Looks good to me except the comment:
>>
>> +/* Fixup the ANY value, based on whether LTE is supported or not
>>
>> the code under that comment seems to use all modes, not just LTE/4G.
>
> Ah, good catch, yes, first version of the patch was only for LTE and
> then I updated it without updating the comment :) Will fix that before
> merging.

Pushed to git master after changing the comment text to:
/* Fixup the ANY value, based on which are the supported modes */


-- 
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: [PATCH 1/2] broadband-modem,helpers: implement AT+WS46=? response parser

[Dan Williams]

On Sun, 2017-03-12 at 21:05 +0100, Aleksander Morgado wrote:
> We want a parser that returns the full list of combinations found.

Looks good to me except the comment:

+/* Fixup the ANY value, based on whether LTE is supported or not

the code under that comment seems to use all modes, not just LTE/4G.

Dan


> ---
>  src/mm-broadband-modem.c   | 111 ---
> -
>  src/mm-modem-helpers.c | 143
> +
>  src/mm-modem-helpers.h |   3 +
>  src/tests/test-modem-helpers.c | 102 +
>  4 files changed, 290 insertions(+), 69 deletions(-)
> 
> diff --git a/src/mm-broadband-modem.c b/src/mm-broadband-modem.c
> index 17b3253a..aa84fc7e 100644
> --- a/src/mm-broadband-modem.c
> +++ b/src/mm-broadband-modem.c
> @@ -378,17 +378,28 @@ current_capabilities_ws46_test_ready
> (MMBaseModem *self,
>    LoadCapabilitiesContext *ctx)
>  {
>  const gchar *response;
> +GArray  *modes;
> +guinti;
>  
>  /* Completely ignore errors in AT+WS46=? */
>  response = mm_base_modem_at_command_finish (self, res, NULL);
> -if (response &&
> -(strstr (response, "28") != NULL ||   /* 4G only */
> - strstr (response, "30") != NULL ||   /* 2G/4G */
> - strstr (response, "31") != NULL)) {  /* 3G/4G */
> -/* Add LTE caps */
> -ctx->caps |= MM_MODEM_CAPABILITY_LTE;
> +if (!response)
> +goto out;
> +
> +modes = mm_3gpp_parse_ws46_test_response (response, NULL);
> +if (!modes)
> +goto out;
> +
> +/* Add LTE caps if any of the reported modes supports 4G */
> +for (i = 0; i < modes->len; i++) {
> +if (g_array_index (modes, MMModemMode, i) &
> MM_MODEM_MODE_4G) {
> +ctx->caps |= MM_MODEM_CAPABILITY_LTE;
> +break;
> +}
>  }
> +g_array_unref (modes);
>  
> +out:
>  g_simple_async_result_set_op_res_gpointer (
>  ctx->result,
>  GUINT_TO_POINTER (ctx->caps),
> @@ -1430,78 +1441,40 @@ supported_modes_ws46_test_ready
> (MMBroadbandModem *self,
>   LoadSupportedModesContext *ctx)
>  {
>  const gchar *response;
> -GError *error = NULL;
> +GArray  *modes;
> +GError  *error = NULL;
> +guinti;
>  
>  response = mm_base_modem_at_command_finish (MM_BASE_MODEM
> (self), res, );
> -if (!error) {
> -MMModemMode mode = MM_MODEM_MODE_NONE;
> -
> -/*
> - * More than one numeric ID may appear in the list, that's
> why
> - * they are checked separately.
> - *
> - * NOTE: Do not skip WS46 prefix; it would break Cinterion
> handling.
> - *
> - * From 3GPP TS 27.007 v.11.2.0, section 5.9
> - * 12GSM Digital Cellular Systems (GERAN only)
> - * 22UTRAN only
> - * 253GPP Systems (GERAN, UTRAN and E-UTRAN)
> - * 28E-UTRAN only
> - * 29GERAN and UTRAN
> - * 30GERAN and E-UTRAN
> - * 31UTRAN and E-UTRAN
> - */
> -
> -if (strstr (response, "12") != NULL) {
> -mm_dbg ("Device allows (3GPP) 2G-only network mode");
> -mode |= MM_MODEM_MODE_2G;
> -}
> -
> -if (strstr (response, "22") != NULL) {
> -mm_dbg ("Device allows (3GPP) 3G-only network mode");
> -mode |= MM_MODEM_MODE_3G;
> -}
> -
> -if (strstr (response, "28") != NULL) {
> -mm_dbg ("Device allows (3GPP) 4G-only network mode");
> -mode |= MM_MODEM_MODE_4G;
> -}
> +if (error) {
> +mm_dbg ("Generic query of supported 3GPP networks with
> WS46=? failed: '%s'", error->message);
> +g_error_free (error);
> +goto out;
> +}
>  
> -if (strstr (response, "29") != NULL) {
> -mm_dbg ("Device allows (3GPP) 2G/3G network mode");
> -mode |= (MM_MODEM_MODE_2G | MM_MODEM_MODE_3G);
> -}
> +modes = mm_3gpp_parse_ws46_test_response (response, );
> +if (!modes) {
> +mm_dbg ("Parsing WS46=? response failed: '%s'", error-
> >message);
> +g_error_free (error);
> +goto out;
> +}
>  
> -if (strstr (response, "30") != NULL) {
> -mm_dbg ("Device allows (3GPP) 2G/4G network mode");
> -mode |= (MM_MODEM_MODE_2G | MM_MODEM_MODE_4G);
> -}
> +for (i = 0; i < modes->len; i++) {
> +MMModemMode  mode;
> +gchar   *str;
>  
> -if (strstr (response, "31") != NULL) {
> -mm_dbg ("Device allows (3GPP) 3G/4G network mode");
> -mode |= (MM_MODEM_MODE_3G | MM_MODEM_MODE_4G);
> -}
> +mode = g_array_index (modes, MMModemMode, i);
>  
> -if (strstr (response, "25") != NULL) {
> -if (mm_iface_modem_is_3gpp_lte (MM_IFACE_MODEM (self)))
> {
> -