[PATCH] [TELIT] implemented set current bands interface
Aleksander Morgado
aleksander at aleksander.es
Thu Mar 10 09:45:48 UTC 2016
Hey,
See comments below.
On Thu, Mar 10, 2016 at 10:19 AM, <c.lobrano at gmail.com> wrote:
> From: Carlo Lobrano <c.lobrano at gmail.com>
>
> ---
> 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
More information about the ModemManager-devel
mailing list