[PATCH 1/2] modem response parsing updates needed for thuraya
Aleksander Morgado
aleksander at aleksander.es
Thu Feb 11 08:29:03 UTC 2016
Hey Thomas,
On Tue, Feb 2, 2016 at 6:13 PM, Thomas Sailer
<sailer at sailer.dynip.lugs.ch> wrote:
> From: Thomas Sailer <t.sailer at alumni.ethz.ch>
>
> Signed-off-by: Thomas Sailer <t.sailer at alumni.ethz.ch>
> ---
Too many different changes in the same patch it seems. Could you split
it into different logical patches? At least one for the CPMS thing,
another one for the CREG thing and another one for the CGDCONT thing.
Also, for each of the changes, if you could explain what was done in
the commit log, that would be helpful; e.g. the CGDCONT fixes seem to
be related to adding additional optional whitespaces in the regex,
right?
See some comments below as well.
> src/mm-modem-helpers.c | 86 ++++++++++++++++++++++++++++++++++++++----
> src/tests/test-modem-helpers.c | 81 ++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 158 insertions(+), 9 deletions(-)
>
> diff --git a/src/mm-modem-helpers.c b/src/mm-modem-helpers.c
> index 17ad8d2..a9b1c71 100644
> --- a/src/mm-modem-helpers.c
> +++ b/src/mm-modem-helpers.c
> @@ -131,6 +131,58 @@ mm_split_string_groups (const gchar *str)
>
> /*****************************************************************************/
>
> +static gchar **
> +mm_split_string_groups_space (const gchar *str)
> +{
> + GPtrArray *array;
> + const gchar *start;
> + const gchar *next;
> +
> + array = g_ptr_array_new ();
> +
> + /*
> + * Manually parse splitting groups. Groups are separated by space
> + */
> +
> + /* Iterate string splitting groups */
> + for (start = str; start; start = next) {
> + gchar *item;
> + gssize len = -1;
> +
> + /* skip leading whitespaces */
> + while (*start == ' ')
> + start++;
> +
> + if (!*start)
> + break;
> +
> + next = strchr (start, ' ');
> + if (next) {
> + len = next - start;
> + if (next[-1] == ',')
> + len--;
> + next++;
> + }
> +
> + if (len < 0)
> + item = g_strdup (start);
> + else
> + item = g_strndup (start, len);
> +
> + g_ptr_array_add (array, item);
> + }
> +
> + if (array->len > 0) {
> + g_ptr_array_add (array, NULL);
> + return (gchar **) g_ptr_array_free (array, FALSE);
> + }
> +
> + g_ptr_array_unref (array);
> + return NULL;
> +}
The original mm_split_string_groups() was assuming that groups were
separated by commas, like this:
/*
* Manually parse splitting groups. Groups may be single elements,
or otherwise
* lists given between parenthesis, e.g.:
*
* ("SM","ME"),("SM","ME"),("SM","ME")
* "SM","SM","SM"
* "SM",("SM","ME"),("SM","ME")
*/
This new method assumes the groups are separated by spaces, which
according to the example you added as unit test looks like this:
* "SM","ME", "SM","ME", "SM","ME"
Someone at Thuraya must be very very ashamed for doing something so
unreadable like this...
Anyway, given that the space is the one splitting groups, why not just
g_str_split (str, " ", -1);?
> +
> +/*****************************************************************************/
> +
> guint
> mm_count_bits_set (gulong number)
> {
> @@ -434,6 +486,7 @@ mm_voice_clip_regex_get (void)
>
> /* +CREG: <stat>,<lac>,<ci> (GSM 07.07 CREG=2 unsolicited) */
> #define CREG3 "\\+(CREG|CGREG|CEREG):\\s*0*([0-9]),\\s*([^,\\s]*)\\s*,\\s*([^,\\s]*)"
> +#define CREG11 "\\+(CREG|CGREG|CEREG):\\s*0*([0-9]),\\s*(\"[^\"\\s]*\")\\s*,\\s*(\"[^\"\\s]*\")"
Each of the CREG regex defines we have has a comment line before
explaining the format expected, like an example to easily understand
what's being parsed. Could you add that? Also, you can put it after
CREG10 just fine, once you add the comment line before.
Now that I see it, is it just adding some additional quotes in the
elements? Maybe the CREG3 could be updated to support optional quotes,
like \"?, what do you think?
>
> /* +CREG: <n>,<stat>,<lac>,<ci> (GSM 07.07 solicited and some CREG=2 unsolicited) */
> #define CREG4 "\\+(CREG|CGREG|CEREG):\\s*([0-9]),\\s*([0-9])\\s*,\\s*([^,]*)\\s*,\\s*([^,\\s]*)"
> @@ -545,6 +598,14 @@ mm_3gpp_creg_regex_get (gboolean solicited)
> g_assert (regex);
> g_ptr_array_add (array, regex);
>
> + /* #11 */
> + if (solicited)
> + regex = g_regex_new (CREG11 "$", G_REGEX_RAW | G_REGEX_OPTIMIZE, 0, NULL);
> + else
> + regex = g_regex_new ("\\r\\n" CREG11 "\\r\\n", G_REGEX_RAW | G_REGEX_OPTIMIZE, 0, NULL);
> + g_assert (regex);
> + g_ptr_array_add (array, regex);
> +
> /* CEREG #1 */
> if (solicited)
> regex = g_regex_new (CEREG1 "$", G_REGEX_RAW | G_REGEX_OPTIMIZE, 0, NULL);
> @@ -867,7 +928,7 @@ mm_3gpp_parse_cgdcont_test_response (const gchar *response,
> return NULL;
> }
>
> - r = g_regex_new ("\\+CGDCONT:\\s*\\((\\d+)-?(\\d+)?\\),\\(?\"(\\S+)\"",
> + r = g_regex_new ("\\+CGDCONT:\\s*\\(\\s*(\\d+)\\s*-?\\s*(\\d+)?\\s*\\)\\s*,\\s*\\(?\"(\\S+)\"",
> G_REGEX_DOLLAR_ENDONLY | G_REGEX_RAW,
> 0, &inner_error);
> g_assert (r != NULL);
> @@ -955,7 +1016,7 @@ mm_3gpp_parse_cgdcont_read_response (const gchar *reply,
> return NULL;
>
> list = NULL;
> - r = g_regex_new ("\\+CGDCONT:\\s*(\\d+)\\s*,([^,\\)]*),([^,\\)]*),([^,\\)]*)",
> + r = g_regex_new ("\\+CGDCONT:\\s*(\\d+)\\s*,([^, \\)]*)\\s*,([^, \\)]*)\\s*,([^, \\)]*)",
> G_REGEX_DOLLAR_ENDONLY | G_REGEX_RAW,
> 0, &inner_error);
> if (r) {
> @@ -1359,12 +1420,14 @@ mm_3gpp_parse_cpms_test_response (const gchar *reply,
> GArray **mem2,
> GArray **mem3)
> {
> + const gchar *reply_notag;
> GRegex *r;
> gchar **split;
> guint i;
> GArray *tmp1 = NULL;
> GArray *tmp2 = NULL;
> GArray *tmp3 = NULL;
> + guint n_groups;
>
> g_assert (mem1 != NULL);
> g_assert (mem2 != NULL);
> @@ -1372,15 +1435,24 @@ mm_3gpp_parse_cpms_test_response (const gchar *reply,
>
> #define N_EXPECTED_GROUPS 3
>
> - split = mm_split_string_groups (mm_strip_tag (reply, "+CPMS:"));
> + reply_notag = mm_strip_tag (reply, "+CPMS:");
You can do reply = mm_strip_tag (reply, "+CPMS:"); and avoid the extra
"reply_notag" variable.
> + split = mm_split_string_groups (reply_notag);
> if (!split)
> return FALSE;
>
> - if (g_strv_length (split) != N_EXPECTED_GROUPS) {
> - mm_warn ("Cannot parse +CPMS test response: invalid number of groups (%u != %u)",
> - g_strv_length (split), N_EXPECTED_GROUPS);
> + n_groups = g_strv_length (split);
> + if (n_groups != N_EXPECTED_GROUPS) {
> g_strfreev (split);
> - return FALSE;
> + split = mm_split_string_groups_space (reply_notag);
> + if (split && g_strv_length (split) != N_EXPECTED_GROUPS) {
> + g_strfreev (split);
> + split = 0;
> + }
> + if (!split) {
> + mm_warn ("Cannot parse +CPMS test response: invalid number of groups (%u != %u)",
> + n_groups, N_EXPECTED_GROUPS);
> + return FALSE;
> + }
> }
Hum... I don't think the space-separated-groups thing is a very common
thing. Maybe we should move it to a "helpers" file in the Thuraya
plugin itself, and provide a new plugin specific parser for the +CPMS
response testing?
>
> r = g_regex_new ("\\s*\"([^,\\)]+)\"\\s*", 0, 0, NULL);
> diff --git a/src/tests/test-modem-helpers.c b/src/tests/test-modem-helpers.c
> index bd7f688..a85709a 100644
> --- a/src/tests/test-modem-helpers.c
> +++ b/src/tests/test-modem-helpers.c
> @@ -1135,7 +1135,7 @@ test_cereg2_novatel_lte_solicited (void *f, gpointer d)
> {
> RegTestData *data = (RegTestData *) d;
> const char *reply = "\r\n+CEREG: 2,1, 1F00, 20 ,79D903 ,7\r\n";
> - const CregResult result = { 1, 0x1F00, 0x79D903, MM_MODEM_ACCESS_TECHNOLOGY_LTE, 12, FALSE, TRUE };
> + const CregResult result = { 1, 0x1F00, 0x79D903, MM_MODEM_ACCESS_TECHNOLOGY_LTE, 13, FALSE, TRUE };
>
> test_creg_match ("Novatel LTE E362 CEREG=2", TRUE, reply, data, &result);
> }
> @@ -1145,11 +1145,31 @@ test_cereg2_novatel_lte_unsolicited (void *f, gpointer d)
> {
> RegTestData *data = (RegTestData *) d;
> const char *reply = "\r\n+CEREG: 1, 1F00, 20 ,79D903 ,7\r\n";
> - const CregResult result = { 1, 0x1F00, 0x79D903, MM_MODEM_ACCESS_TECHNOLOGY_LTE, 11, FALSE, TRUE };
> + const CregResult result = { 1, 0x1F00, 0x79D903, MM_MODEM_ACCESS_TECHNOLOGY_LTE, 12, FALSE, TRUE };
>
> test_creg_match ("Novatel LTE E362 CEREG=2", FALSE, reply, data, &result);
> }
>
> +static void
> +test_cgreg2_thuraya_solicited (void *f, gpointer d)
> +{
> + RegTestData *data = (RegTestData *) d;
> + const char *reply = "+CGREG: 1, \"0426\", \"F0,0F\"";
> + const CregResult result = { 1, 0x0426, 0x00F0, MM_MODEM_ACCESS_TECHNOLOGY_UNKNOWN, 11, TRUE, FALSE };
> +
> + test_creg_match ("Thuraya solicited CREG=2", TRUE, reply, data, &result);
> +}
> +
> +static void
> +test_cgreg2_thuraya_unsolicited (void *f, gpointer d)
> +{
> + RegTestData *data = (RegTestData *) d;
> + const char *reply = "\r\n+CGREG: 1, \"0426\", \"F0,0F\"\r\n";
> + const CregResult result = { 1, 0x0426, 0x00F0, MM_MODEM_ACCESS_TECHNOLOGY_UNKNOWN, 11, TRUE, FALSE };
> +
> + test_creg_match ("Thuraya unsolicited CREG=2", FALSE, reply, data, &result);
> +}
> +
> /*****************************************************************************/
> /* Test CSCS responses */
>
> @@ -1798,6 +1818,19 @@ test_cgdcont_test_response_single_context (void *f, gpointer d)
> test_cgdcont_test_results ("Single Context", reply, &expected[0], G_N_ELEMENTS (expected));
> }
>
> +static void
> +test_cgdcont_test_response_thuraya (void *f, gpointer d)
> +{
> + const gchar *reply =
> + "+CGDCONT: ( 1 ) , \"IP\" ,,, (0-2),(0-3)\r\n"
> + "+CGDCONT: , \"PPP\" ,,, (0-2),(0-3)\r\n";
> + static MM3gppPdpContextFormat expected[] = {
> + { 1, 1, MM_BEARER_IP_FAMILY_IPV4 }
> + };
> +
> + test_cgdcont_test_results ("Thuraya", reply, &expected[0], G_N_ELEMENTS (expected));
> +}
> +
> /*****************************************************************************/
> /* Test CGDCONT read responses */
>
> @@ -2037,6 +2070,46 @@ test_cpms_response_empty_fields (void *f, gpointer d)
> g_array_unref (mem3);
> }
>
> +static void
> +test_cpms_response_thuraya (void *f, gpointer d)
> +{
> + /*
> + * First: ("ME","MT") 2-item group
> + * Second: "ME" 1 item
> + * Third: ("SM") 1-item group
> + */
This previous comment block doesn't apply to this test.
> + const gchar *reply = "+CPMS: \"MT\",\"SM\",\"BM\",\"ME\",\"SR\", \"MT\",\"SM\",\"BM\",\"ME\",\"SR\", \"MT\",\"SM\",\"BM\",\"ME\",\"SR\" ";
> + GArray *mem1 = NULL;
> + GArray *mem2 = NULL;
> + GArray *mem3 = NULL;
> +
> + trace ("\nTesting thuraya +CPMS=? response...\n");
> +
> + g_assert (mm_3gpp_parse_cpms_test_response (reply, &mem1, &mem2, &mem3));
> + g_assert_cmpuint (mem1->len, ==, 5);
> + g_assert (is_storage_supported (mem1, MM_SMS_STORAGE_MT));
> + g_assert (is_storage_supported (mem1, MM_SMS_STORAGE_SM));
> + g_assert (is_storage_supported (mem1, MM_SMS_STORAGE_BM));
> + g_assert (is_storage_supported (mem1, MM_SMS_STORAGE_ME));
> + g_assert (is_storage_supported (mem1, MM_SMS_STORAGE_SR));
> + g_assert_cmpuint (mem2->len, ==, 5);
> + g_assert (is_storage_supported (mem2, MM_SMS_STORAGE_MT));
> + g_assert (is_storage_supported (mem2, MM_SMS_STORAGE_SM));
> + g_assert (is_storage_supported (mem2, MM_SMS_STORAGE_BM));
> + g_assert (is_storage_supported (mem2, MM_SMS_STORAGE_ME));
> + g_assert (is_storage_supported (mem2, MM_SMS_STORAGE_SR));
> + g_assert_cmpuint (mem3->len, ==, 5);
> + g_assert (is_storage_supported (mem3, MM_SMS_STORAGE_MT));
> + g_assert (is_storage_supported (mem3, MM_SMS_STORAGE_SM));
> + g_assert (is_storage_supported (mem3, MM_SMS_STORAGE_BM));
> + g_assert (is_storage_supported (mem3, MM_SMS_STORAGE_ME));
> + g_assert (is_storage_supported (mem3, MM_SMS_STORAGE_SR));
> +
> + g_array_unref (mem1);
> + g_array_unref (mem2);
> + g_array_unref (mem3);
> +}
> +
> /*****************************************************************************/
> /* Test CNUM responses */
>
> @@ -2693,6 +2766,8 @@ int main (int argc, char **argv)
> g_test_suite_add (suite, TESTCASE (test_cgreg2_md400_unsolicited, reg_data));
> g_test_suite_add (suite, TESTCASE (test_cgreg2_x220_unsolicited, reg_data));
> g_test_suite_add (suite, TESTCASE (test_cgreg2_unsolicited_with_rac, reg_data));
> + g_test_suite_add (suite, TESTCASE (test_cgreg2_thuraya_solicited, reg_data));
> + g_test_suite_add (suite, TESTCASE (test_cgreg2_thuraya_unsolicited, reg_data));
>
> g_test_suite_add (suite, TESTCASE (test_cereg1_solicited, reg_data));
> g_test_suite_add (suite, TESTCASE (test_cereg1_unsolicited, reg_data));
> @@ -2734,11 +2809,13 @@ int main (int argc, char **argv)
> g_test_suite_add (suite, TESTCASE (test_cpms_response_mixed, NULL));
> g_test_suite_add (suite, TESTCASE (test_cpms_response_mixed_spaces, NULL));
> g_test_suite_add (suite, TESTCASE (test_cpms_response_empty_fields, NULL));
> + g_test_suite_add (suite, TESTCASE (test_cpms_response_thuraya, NULL));
>
> g_test_suite_add (suite, TESTCASE (test_cgdcont_test_response_single, NULL));
> g_test_suite_add (suite, TESTCASE (test_cgdcont_test_response_multiple, NULL));
> g_test_suite_add (suite, TESTCASE (test_cgdcont_test_response_multiple_and_ignore, NULL));
> g_test_suite_add (suite, TESTCASE (test_cgdcont_test_response_single_context, NULL));
> + g_test_suite_add (suite, TESTCASE (test_cgdcont_test_response_thuraya, NULL));
>
> g_test_suite_add (suite, TESTCASE (test_cgdcont_read_response_nokia, NULL));
> g_test_suite_add (suite, TESTCASE (test_cgdcont_read_response_samsung, NULL));
--
Aleksander
https://aleksander.es
More information about the ModemManager-devel
mailing list