[PATCH 2/2] altair-lte: ignore invalid bands in %BANDCAP / %GETCFG="BAND" responses

[Aleksander Morgado]

On 29/05/14 08:13, Ben Chan wrote:
> Due to a firmware issue, the modem may reply an invalid band value, such
> as 0, for the %BANDCAP or %GETCFG="BAND" command. This patch moves
> parse_bands_response() to mm-modem-helpers-altair-lte.c, modifies the
> function to ignore any invalid band outside the range of E-UTRAN
> operating bands, and add unit tests for the function.

Kudos for the unit test! :)

Pushed, thanks.

> ---
>  plugins/altair/mm-broadband-modem-altair-lte.c     | 35 ++-----------------
>  plugins/altair/mm-modem-helpers-altair-lte.c       | 39 ++++++++++++++++++++++
>  plugins/altair/mm-modem-helpers-altair-lte.h       |  3 ++
>  .../altair/tests/test-modem-helpers-altair-lte.c   | 29 ++++++++++++++++
>  4 files changed, 73 insertions(+), 33 deletions(-)
> 
> diff --git a/plugins/altair/mm-broadband-modem-altair-lte.c b/plugins/altair/mm-broadband-modem-altair-lte.c
> index 8240185..d313a51 100644
> --- a/plugins/altair/mm-broadband-modem-altair-lte.c
> +++ b/plugins/altair/mm-broadband-modem-altair-lte.c
> @@ -264,37 +264,6 @@ load_current_capabilities (MMIfaceModem *self,
>  }
>  
>  /*****************************************************************************/
> -/* supported/current Bands helpers*/
> -
> -static GArray *
> -parse_bands_response (const gchar *response)
> -{
> -    guint32 bandval;
> -    MMModemBand band;
> -    gchar **split;
> -    guint i, num_of_bands;
> -    GArray *bands;
> -
> -    split = g_strsplit_set (response, ",", -1);
> -    if (!split)
> -        return NULL;
> -
> -    num_of_bands = g_strv_length (split);
> -
> -    bands = g_array_sized_new (FALSE, FALSE, sizeof (MMModemBand), num_of_bands);
> -
> -    for (i = 0; split[i]; i++) {
> -        bandval = (guint32)strtoul (split[i], NULL, 10);
> -        band = MM_MODEM_BAND_EUTRAN_I - 1 + bandval;
> -        g_array_append_val (bands, band);
> -    }
> -
> -    g_strfreev (split);
> -
> -    return bands;
> -}
> -
> -/*****************************************************************************/
>  /* Load supported bands (Modem interface) */
>  
>  static GArray *
> @@ -334,7 +303,7 @@ load_supported_bands_done (MMIfaceModem *self,
>       */
>      response = mm_strip_tag (response, BANDCAP_TAG);
>  
> -    bands = parse_bands_response (response);
> +    bands = mm_altair_parse_bands_response (response);
>      if (!bands) {
>          mm_dbg ("Failed to parse supported bands response");
>          g_simple_async_result_set_error (
> @@ -415,7 +384,7 @@ load_current_bands_done (MMIfaceModem *self,
>       */
>      response = mm_strip_tag (response, CFGBANDS_TAG);
>  
> -    bands = parse_bands_response (response);
> +    bands = mm_altair_parse_bands_response (response);
>      if (!bands) {
>          mm_dbg ("Failed to parse current bands response");
>          g_simple_async_result_set_error (
> diff --git a/plugins/altair/mm-modem-helpers-altair-lte.c b/plugins/altair/mm-modem-helpers-altair-lte.c
> index e19f147..b369e29 100644
> --- a/plugins/altair/mm-modem-helpers-altair-lte.c
> +++ b/plugins/altair/mm-modem-helpers-altair-lte.c
> @@ -14,6 +14,7 @@
>   *
>   */
>  
> +#include <stdlib.h>
>  #include <string.h>
>  
>  #include <ModemManager.h>
> @@ -23,6 +24,44 @@
>  #include "mm-modem-helpers-altair-lte.h"
>  
>  /*****************************************************************************/
> +/* Bands response parser */
> +
> +GArray *
> +mm_altair_parse_bands_response (const gchar *response)
> +{
> +    gchar **split;
> +    GArray *bands;
> +    guint i;
> +
> +    /*
> +     * Response is "<band>[,<band>...]"
> +     */
> +    split = g_strsplit_set (response, ",", -1);
> +    if (!split)
> +        return NULL;
> +
> +    bands = g_array_sized_new (FALSE, FALSE, sizeof (MMModemBand), g_strv_length (split));
> +
> +    for (i = 0; split[i]; i++) {
> +        guint32 band_value;
> +        MMModemBand band;
> +
> +        band_value = (guint32)strtoul (split[i], NULL, 10);
> +        band = MM_MODEM_BAND_EUTRAN_I - 1 + band_value;
> +
> +        /* Due to a firmware issue, the modem may incorrectly includes 0 in the
> +         * bands response. We thus ignore any band value outside the range of
> +         * E-UTRAN operating bands. */
> +        if (band >= MM_MODEM_BAND_EUTRAN_I && band <= MM_MODEM_BAND_EUTRAN_XLIV)
> +            g_array_append_val (bands, band);
> +    }
> +
> +    g_strfreev (split);
> +
> +    return bands;
> +}
> +
> +/*****************************************************************************/
>  /* +CEER response parser */
>  
>  gchar *
> diff --git a/plugins/altair/mm-modem-helpers-altair-lte.h b/plugins/altair/mm-modem-helpers-altair-lte.h
> index c6fbf34..56a6e15 100644
> --- a/plugins/altair/mm-modem-helpers-altair-lte.h
> +++ b/plugins/altair/mm-modem-helpers-altair-lte.h
> @@ -19,6 +19,9 @@
>  
>  #include <glib.h>
>  
> +/* Bands response parser */
> +GArray *mm_altair_parse_bands_response (const gchar *response);
> +
>  /* +CEER response parser */
>  gchar *mm_altair_parse_ceer_response (const gchar *response,
>                                        GError **error);
> diff --git a/plugins/altair/tests/test-modem-helpers-altair-lte.c b/plugins/altair/tests/test-modem-helpers-altair-lte.c
> index 2df3eef..46f70b6 100644
> --- a/plugins/altair/tests/test-modem-helpers-altair-lte.c
> +++ b/plugins/altair/tests/test-modem-helpers-altair-lte.c
> @@ -20,9 +20,37 @@
>  #include <glib-object.h>
>  #include <locale.h>
>  
> +#include <ModemManager.h>
> +#define _LIBMM_INSIDE_MM
> +#include <libmm-glib.h>
> +
>  #include "mm-modem-helpers-altair-lte.h"
>  
>  /*****************************************************************************/
> +/* Test bands response parsing */
> +
> +static void
> +test_parse_bands (void)
> +{
> +    GArray *bands;
> +
> +    bands = mm_altair_parse_bands_response ("");
> +    g_assert (bands != NULL);
> +    g_assert_cmpuint (bands->len, ==, 0);
> +    g_array_free (bands, TRUE);
> +
> +    /* 0 and 45 are outside the range of E-UTRAN operating bands and should be ignored. */
> +    bands = mm_altair_parse_bands_response ("0, 0, 1, 4,13,44,45");
> +    g_assert (bands != NULL);
> +    g_assert_cmpuint (bands->len, ==, 4);
> +    g_assert_cmpuint (g_array_index (bands, MMModemBand, 0), ==, MM_MODEM_BAND_EUTRAN_I);
> +    g_assert_cmpuint (g_array_index (bands, MMModemBand, 1), ==, MM_MODEM_BAND_EUTRAN_IV);
> +    g_assert_cmpuint (g_array_index (bands, MMModemBand, 2), ==, MM_MODEM_BAND_EUTRAN_XIII);
> +    g_assert_cmpuint (g_array_index (bands, MMModemBand, 3), ==, MM_MODEM_BAND_EUTRAN_XLIV);
> +    g_array_free (bands, TRUE);
> +}
> +
> +/*****************************************************************************/
>  /* Test +CEER responses */
>  
>  typedef struct {
> @@ -105,6 +133,7 @@ int main (int argc, char **argv)
>      g_type_init ();
>      g_test_init (&argc, &argv, NULL);
>  
> +    g_test_add_func ("/MM/altair/parse_bands", test_parse_bands);
>      g_test_add_func ("/MM/altair/ceer", test_ceer);
>      g_test_add_func ("/MM/altair/parse_cid", test_parse_cid);
>      g_test_add_func ("/MM/altair/parse_vendor_pco_info", test_parse_vendor_pco_info);
> 


-- 
Aleksander
https://aleksander.es