Hey, See comments below.
On Thu, Mar 10, 2016 at 10:19 AM, <[email protected]> wrote: > From: Carlo Lobrano <[email protected]> > > --- > plugins/telit/mm-broadband-modem-telit.c | 98 +++++++++++ > plugins/telit/mm-modem-helpers-telit.c | 85 ++++++++++ > plugins/telit/mm-modem-helpers-telit.h | 1 + > plugins/telit/tests/test-mm-modem-helpers-telit.c | 188 > ++++++++++++++++++++++ > 4 files changed, 372 insertions(+) > > diff --git a/plugins/telit/mm-broadband-modem-telit.c > b/plugins/telit/mm-broadband-modem-telit.c > index 0127314..e0a7c59 100644 > --- a/plugins/telit/mm-broadband-modem-telit.c > +++ b/plugins/telit/mm-broadband-modem-telit.c > @@ -43,6 +43,102 @@ G_DEFINE_TYPE_EXTENDED (MMBroadbandModemTelit, > mm_broadband_modem_telit, MM_TYPE > G_IMPLEMENT_INTERFACE (MM_TYPE_IFACE_MODEM_3GPP, > iface_modem_3gpp_init)); > > > /*****************************************************************************/ > +/* Set current bands (Modem interface) */ > + > +static gboolean > +modem_set_current_bands_finish (MMIfaceModem *self, > + GAsyncResult *res, > + GError **error) > +{ > + return !!mm_base_modem_at_command_finish (MM_BASE_MODEM (self), res, > error); > +} > + > +static void > +modem_set_current_bands (MMIfaceModem *self, > + GArray *bands_array, > + GAsyncReadyCallback callback, > + gpointer user_data) > +{ > + gchar *cmd; > + gint flag2g; > + gint flag3g; > + gint flag4g; > + gboolean is_2g; > + gboolean is_3g; > + gboolean is_4g; > + > + mm_telit_get_band_flag (bands_array, &flag2g, &flag3g, &flag4g); > + > + is_2g = mm_iface_modem_is_2g (self); > + is_3g = mm_iface_modem_is_3g (self); > + is_4g = mm_iface_modem_is_4g (self); > + > + if (is_2g && flag2g == -1) { > + g_simple_async_report_error_in_idle (G_OBJECT (self), > + callback, > + user_data, > + MM_CORE_ERROR, > + MM_CORE_ERROR_NOT_FOUND, > + "None or invalid 2G bands > combination in the provided list"); > + return; > + } > + > + if (is_3g && flag3g == -1) { > + g_simple_async_report_error_in_idle (G_OBJECT (self), > + callback, > + user_data, > + MM_CORE_ERROR, > + MM_CORE_ERROR_NOT_FOUND, > + "None or invalid 3G bands > combination in the provided list"); > + return; > + } > + > + if (is_4g && flag4g == -1) { > + g_simple_async_report_error_in_idle (G_OBJECT (self), > + callback, > + user_data, > + MM_CORE_ERROR, > + MM_CORE_ERROR_NOT_FOUND, > + "None or invalid 4G bands > combination in the provided list"); > + return; > + } > + > + cmd = NULL; > + if (is_2g && !is_3g && !is_4g) > + cmd = g_strdup_printf ("AT#BND=%d", flag2g); > + else if (is_2g && is_3g && !is_4g) > + cmd = g_strdup_printf ("AT#BND=%d,%d", flag2g, flag3g); > + else if (is_2g && is_3g && is_4g) > + cmd = g_strdup_printf ("AT#BND=%d,%d,%d", flag2g, flag3g, flag4g); > + else if (!is_2g && !is_3g && is_4g) > + cmd = g_strdup_printf ("AT#BND=0,0,%d", flag4g); > + else if (!is_2g && is_3g && is_4g) > + cmd = g_strdup_printf ("AT#BND=0,%d,%d", flag3g, flag4g); > + else if (is_2g && !is_3g && is_4g) > + cmd = g_strdup_printf ("AT#BND=%d,0,%d", flag2g, flag4g); > + else { > + g_simple_async_report_error_in_idle (G_OBJECT (self), > + callback, > + user_data, > + MM_CORE_ERROR, > + MM_CORE_ERROR_FAILED, > + "Unexpectd error: could not > compose AT#BND command"); > + return; > + } > + If you use mm_base_modem_at_command_finish() unconditionally in modem_set_current_bands_finish() you shouldn't use g_simple_async_report_error_in_idle(). In other words, if the async operation doesn't always run mm_base_modem_at_command() then you shouldn't use mm_base_modem_at_command_finish() in modem_set_current_bands_finish(). Your logic is assuming that mm_base_modem_at_command () is based on GSimpleAsyncResult, and that may not be true (right now it is, but this is an assumption we don't want to make). You can leave these g_simple_async_report_error_in_idle() calls, but only if you then create a GSimpleAsyncResult, and provide a (GAsyncReadyCallback) to mm_base_at_command() passing the new GSimpleAsyncResult as user_data. You would then call the mm_base_modem_at_command_finish() within the GAsyncReadyCallback, and complete there the GSimpleAsyncResult. Once that is done, all the code paths in the async method would be equivalent and you can now call g_simple_async_result_propagate_error() in modem_set_current_bands_finish(). > + mm_base_modem_at_command (MM_BASE_MODEM (self), > + cmd, > + 20, > + FALSE, > + callback, > + user_data); > + > + g_free (cmd); > +} > + Too many empty lines here, only one enough. > + > + > +/*****************************************************************************/ > /* Load current bands (Modem interface) */ > > typedef struct { > @@ -915,6 +1011,8 @@ iface_modem_init (MMIfaceModem *iface) > { > iface_modem_parent = g_type_interface_peek_parent (iface); > > + iface->set_current_bands = modem_set_current_bands; > + iface->set_current_bands_finish = modem_set_current_bands_finish; > iface->load_current_bands = modem_load_current_bands; > iface->load_current_bands_finish = modem_load_bands_finish; > iface->load_supported_bands = modem_load_supported_bands; > diff --git a/plugins/telit/mm-modem-helpers-telit.c > b/plugins/telit/mm-modem-helpers-telit.c > index ae7f8e7..9b5b956 100644 > --- a/plugins/telit/mm-modem-helpers-telit.c > +++ b/plugins/telit/mm-modem-helpers-telit.c > @@ -26,6 +26,91 @@ > #include "mm-modem-helpers.h" > #include "mm-modem-helpers-telit.h" > > + > +/*****************************************************************************/ > +/* Set current bands helpers */ > + > +void > +mm_telit_get_band_flag (GArray *bands_array, > + gint *flag2g, > + gint *flag3g, > + gint *flag4g) { The open-brace in a new line. > + guint mask_2g = 0; > + guint mask3g = 0; > + guint mask4g = 0; > + guint found4g = FALSE; > + guint i; > + > + for (i = 0; i < bands_array->len; i++) { > + MMModemBand band = g_array_index(bands_array, MMModemBand, i); > + > + if (flag2g != NULL && > + band > MM_MODEM_BAND_UNKNOWN && band <= MM_MODEM_BAND_G850) { > + mask_2g += 1 << band; > + } > + > + if (flag3g != NULL && > + band >= MM_MODEM_BAND_U2100 && band <= MM_MODEM_BAND_U2600) { > + mask3g += 1 << band; > + } > + > + if (flag4g != NULL && > + band >= MM_MODEM_BAND_EUTRAN_I && band <= > MM_MODEM_BAND_EUTRAN_XLIV) { > + mask4g += 1 << (band - MM_MODEM_BAND_EUTRAN_I); > + found4g = TRUE; > + } > + } > + > + /* Get 2G flag */ > + if (flag2g != NULL) { > + if (mask_2g == ((1 << MM_MODEM_BAND_EGSM) + (1 << > MM_MODEM_BAND_DCS))) > + *flag2g = 0; > + else if (mask_2g == ((1 << MM_MODEM_BAND_EGSM) + (1 << > MM_MODEM_BAND_PCS))) > + *flag2g = 1; > + else if (mask_2g == ((1 << MM_MODEM_BAND_G850) + (1 << > MM_MODEM_BAND_DCS))) > + *flag2g = 2; > + else if (mask_2g == ((1 << MM_MODEM_BAND_G850) + (1 << > MM_MODEM_BAND_PCS))) > + *flag2g = 3; > + else > + *flag2g = -1; > + } > + > + > + /* Get 3G flag */ > + if (flag3g != NULL) { > + if (mask3g == (1 << MM_MODEM_BAND_U2100)) > + *flag3g = 0; > + else if (mask3g == (1 << MM_MODEM_BAND_U1900)) > + *flag3g = 1; > + else if (mask3g == (1 << MM_MODEM_BAND_U850)) > + *flag3g = 2; > + else if (mask3g == ((1 << MM_MODEM_BAND_U2100) + > + (1 << MM_MODEM_BAND_U1900) + > + (1 << MM_MODEM_BAND_U850))) > + *flag3g = 3; > + else if (mask3g == ((1 << MM_MODEM_BAND_U1900) + > + (1 << MM_MODEM_BAND_U850))) > + *flag3g = 4; > + else if (mask3g == (1 << MM_MODEM_BAND_U900)) > + *flag3g = 5; > + else if (mask3g == ((1 << MM_MODEM_BAND_U2100) + > + (1 << MM_MODEM_BAND_U900))) > + *flag3g = 6; > + else if (mask3g == (1 << MM_MODEM_BAND_U17IV)) > + *flag3g = 7; > + else > + *flag3g = -1; > + } > + > + /* 4G flag correspond to the mask */ > + if (flag4g != NULL) { > + if (found4g) > + *flag4g = mask4g; > + else > + *flag4g = -1; > + } > +} > + > > /*****************************************************************************/ > /* +CSIM response parser */ > > diff --git a/plugins/telit/mm-modem-helpers-telit.h > b/plugins/telit/mm-modem-helpers-telit.h > index a7e250e..75bd6ad 100644 > --- a/plugins/telit/mm-modem-helpers-telit.h > +++ b/plugins/telit/mm-modem-helpers-telit.h > @@ -97,4 +97,5 @@ gboolean mm_telit_update_2g_bands(gchar *band_list, > GMatchInfo **match_info, GAr > gboolean mm_telit_update_3g_bands(gchar *band_list, GMatchInfo **match_info, > GArray **bands, GError **error); > gboolean mm_telit_update_4g_bands(GArray** bands, GMatchInfo *match_info, > GError **error); > > +void mm_telit_get_band_flag (GArray *bands_array, gint *flag_2g, gint > *flag_3g, gint *flag_4g); A whiteline here. > #endif /* MM_MODEM_HELPERS_TELIT_H */ > diff --git a/plugins/telit/tests/test-mm-modem-helpers-telit.c > b/plugins/telit/tests/test-mm-modem-helpers-telit.c > index cac10fb..e5a30e4 100644 > --- a/plugins/telit/tests/test-mm-modem-helpers-telit.c > +++ b/plugins/telit/tests/test-mm-modem-helpers-telit.c > @@ -313,7 +313,192 @@ test_parse_current_bands_response (void) { > } > } > > +static void > +test_telit_get_2g_bnd_flag (void) { The open-brace in a new line. > + GArray *bands_array; > + gint flag2g; > + MMModemBand egsm = MM_MODEM_BAND_EGSM; > + MMModemBand dcs = MM_MODEM_BAND_DCS; > + MMModemBand pcs = MM_MODEM_BAND_PCS; > + MMModemBand g850 = MM_MODEM_BAND_G850; > + > + // Test Flag 0 We do comments /* like this */ even if it's a single line. Multiple places to fix in the patch. > + bands_array = g_array_sized_new (FALSE, FALSE, sizeof (MMModemBand), 2); > + g_array_append_val (bands_array, egsm); > + g_array_append_val (bands_array, dcs); > + > + mm_telit_get_band_flag (bands_array, &flag2g, NULL, NULL); > + g_assert_cmpuint (flag2g, ==, 0); > + g_array_free (bands_array, TRUE); > + > + // Test flag 1 > + bands_array = g_array_sized_new (FALSE, FALSE, sizeof (MMModemBand), 2); > + g_array_append_val (bands_array, egsm); > + g_array_append_val (bands_array, pcs); > + > + mm_telit_get_band_flag (bands_array, &flag2g, NULL, NULL); > + g_assert_cmpuint (flag2g, ==, 1); > + g_array_free (bands_array, TRUE); > + > + // Test flag 2 > + bands_array = g_array_sized_new (FALSE, FALSE, sizeof (MMModemBand), 2); > + g_array_append_val (bands_array, g850); > + g_array_append_val (bands_array, dcs); > + > + mm_telit_get_band_flag (bands_array, &flag2g, NULL, NULL); > + g_assert_cmpuint (flag2g, ==, 2); > + g_array_free (bands_array, TRUE); > + > + // Test flag 3 > + bands_array = g_array_sized_new (FALSE, FALSE, sizeof (MMModemBand), 2); > + g_array_append_val (bands_array, g850); > + g_array_append_val (bands_array, pcs); > + > + mm_telit_get_band_flag (bands_array, &flag2g, NULL, NULL); > + g_assert_cmpuint (flag2g, ==, 3); > + g_array_free (bands_array, TRUE); > + > + // Test invalid band array > + bands_array = g_array_sized_new (FALSE, FALSE, sizeof (MMModemBand), 2); > + g_array_append_val (bands_array, g850); > + g_array_append_val (bands_array, egsm); > + > + mm_telit_get_band_flag (bands_array, &flag2g, NULL, NULL); > + g_assert_cmpuint (flag2g, ==, -1); > + g_array_free (bands_array, TRUE); > +} > + > + > +static void > +test_telit_get_3g_bnd_flag (void) { The open-brace in a new line. > + GArray *bands_array; > + MMModemBand u2100 = MM_MODEM_BAND_U2100; > + MMModemBand u1900 = MM_MODEM_BAND_U1900; > + MMModemBand u850 = MM_MODEM_BAND_U850; > + MMModemBand u900 = MM_MODEM_BAND_U900; > + MMModemBand u17iv = MM_MODEM_BAND_U17IV; > + MMModemBand u17ix = MM_MODEM_BAND_U17IX; > + gint flag; > + > + // Test flag 0 > + flag = -1; > + bands_array = g_array_sized_new (FALSE, FALSE, sizeof (MMModemBand), 1); > + g_array_append_val (bands_array, u2100); > + > + mm_telit_get_band_flag (bands_array, NULL, &flag, NULL); > + g_assert_cmpint (flag, ==, 0); > + g_array_free (bands_array, TRUE); > + > + // Test flag 1 > + flag = -1; > + bands_array = g_array_sized_new (FALSE, FALSE, sizeof (MMModemBand), 1); > + g_array_append_val (bands_array, u1900); > + > + mm_telit_get_band_flag (bands_array, NULL, &flag, NULL); > + g_assert_cmpint (flag, ==, 1); > + g_array_free (bands_array, TRUE); > + > + // Test flag 2 > + flag = -1; > + bands_array = g_array_sized_new (FALSE, FALSE, sizeof (MMModemBand), 1); > + g_array_append_val (bands_array, u850); > + > + mm_telit_get_band_flag (bands_array, NULL, &flag, NULL); > + g_assert_cmpint (flag, ==, 2); > + g_array_free (bands_array, TRUE); > + > + // Test flag 3 > + flag = -1; > + bands_array = g_array_sized_new (FALSE, FALSE, sizeof (MMModemBand), 3); > + g_array_append_val (bands_array, u2100); > + g_array_append_val (bands_array, u1900); > + g_array_append_val (bands_array, u850); > + > + mm_telit_get_band_flag (bands_array, NULL, &flag, NULL); > + g_assert_cmpint (flag, ==, 3); > + g_array_free (bands_array, TRUE); > + > + // Test flag 4 > + flag = -1; > + bands_array = g_array_sized_new (FALSE, FALSE, sizeof (MMModemBand), 2); > + g_array_append_val (bands_array, u1900); > + g_array_append_val (bands_array, u850); > + > + mm_telit_get_band_flag (bands_array, NULL, &flag, NULL); > + g_assert_cmpint (flag, ==, 4); > + g_array_free (bands_array, TRUE); > + > + // Test flag 5 > + flag = -1; > + bands_array = g_array_sized_new (FALSE, FALSE, sizeof (MMModemBand), 1); > + g_array_append_val (bands_array, u900); > + > + mm_telit_get_band_flag (bands_array, NULL, &flag, NULL); > + g_assert_cmpint (flag, ==, 5); > + g_array_free (bands_array, TRUE); > + > + // Test flag 6 > + flag = -1; > + bands_array = g_array_sized_new (FALSE, FALSE, sizeof (MMModemBand), 2); > + g_array_append_val (bands_array, u2100); > + g_array_append_val (bands_array, u900); > + > + mm_telit_get_band_flag (bands_array, NULL, &flag, NULL); > + g_assert_cmpint (flag, ==, 6); > + g_array_free (bands_array, TRUE); > + > + // Test flag 7 > + flag = -1; > + bands_array = g_array_sized_new (FALSE, FALSE, sizeof (MMModemBand), 1); > + g_array_append_val (bands_array, u17iv); > + > + mm_telit_get_band_flag (bands_array, NULL, &flag, NULL); > + g_assert_cmpint (flag, ==, 7); > + g_array_free (bands_array, TRUE); > + > + // Test invalid band array > + flag = -1; > + bands_array = g_array_sized_new (FALSE, FALSE, sizeof (MMModemBand), 1); > + g_array_append_val (bands_array, u17ix); > + > + mm_telit_get_band_flag (bands_array, NULL, &flag, NULL); > + g_assert_cmpint (flag, ==, -1); > + g_array_free (bands_array, TRUE); > +} > > +static void > +test_telit_get_4g_bnd_flag (void) { The open-brace in a new line. > + GArray *bands_array; > + MMModemBand eutran_i = MM_MODEM_BAND_EUTRAN_I; > + MMModemBand eutran_ii = MM_MODEM_BAND_EUTRAN_II; > + MMModemBand egsm = MM_MODEM_BAND_EGSM; > + gint flag = -1; > + > + // Test flag 1 > + bands_array = g_array_sized_new (FALSE, FALSE, sizeof (MMModemBand), 1); > + g_array_append_val (bands_array, eutran_i); > + > + mm_telit_get_band_flag (bands_array, NULL, NULL, &flag); > + g_assert_cmpint (flag, ==, 1); > + g_array_free (bands_array, TRUE); > + > + // Test flag 3 > + bands_array = g_array_sized_new (FALSE, FALSE, sizeof (MMModemBand), 2); > + g_array_append_val (bands_array, eutran_i); > + g_array_append_val (bands_array, eutran_ii); > + > + mm_telit_get_band_flag (bands_array, NULL, NULL, &flag); > + g_assert_cmpint (flag, ==, 3); > + g_array_free (bands_array, TRUE); > + > + // Test invalid bands array > + bands_array = g_array_sized_new (FALSE, FALSE, sizeof (MMModemBand), 1); > + g_array_append_val (bands_array, egsm); > + > + mm_telit_get_band_flag (bands_array, NULL, NULL, &flag); > + g_assert_cmpint (flag, ==, -1); > + g_array_free (bands_array, TRUE); > +} > > int main (int argc, char **argv) > { > @@ -327,5 +512,8 @@ int main (int argc, char **argv) > g_test_add_func ("/MM/telit/bands/supported/parse_band_flag", > test_parse_band_flag_str); > g_test_add_func ("/MM/telit/bands/supported/parse_bands_response", > test_parse_supported_bands_response); > g_test_add_func ("/MM/telit/bands/current/parse_bands_response", > test_parse_current_bands_response); > + g_test_add_func ("/MM/telit/bands/current/set_bands/2g", > test_telit_get_2g_bnd_flag); > + g_test_add_func ("/MM/telit/bands/current/set_bands/3g", > test_telit_get_3g_bnd_flag); > + g_test_add_func ("/MM/telit/bands/current/set_bands/4g", > test_telit_get_4g_bnd_flag); > return g_test_run (); > } > -- > 2.1.4 > Very nice to see lots of unit tests being added :) -- Aleksander https://aleksander.es _______________________________________________ ModemManager-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
