Re: [PATCH 1/2] atmodem: Detect usage of AT+CGEREP
On Tue, Aug 25, 2020 at 11:49:02AM -0500, Denis Kenzior wrote:
> Hi Lars,
>
> > > So in theory this could be written like:
> > > int max = -1;
> > > iter_open_list(&iter);
> > >
> > > while (iter_next_range(&iter, &max1, &max2)) {
> > > if (max2 > max)
> > > max = max2;
> > > }
> > >
> > > iter_close_list(&iter);
> >
> > I can do this and it does indeed work! :-)
> >
> > This may be my personal thing, but I find this not very intuitive and
> > hard to read.
> >
> > To make this more intuitive can we:
> > * move your proposed while loop to a new function
> > * name that function ...iter_next_range_or_list (or something like that)
> > * put that function into gatresult.c
>
> I'm confused. iter_next_range specifically handles ranges as defined by ITU
> v.250. See section 5.7.3.1 "Range of Values".
>
> Here's a small quote:
> "If more than one value is supported, then the values may be listed
> individually, separated by comma characters (IA5 2/12), or, when a
> continuous range of values is supported, by the first value in the range,
> followed by a hyphen character (IA5 2/13), followed by the last value in the
> range. The specification of single values and ranges of values may be
> intermixed within a single information text. In all cases, the supported
> values shall be indicated in ascending order."
>
> The only thing the user has to do is open up a list, since some modems get
> this wrong and don't enclose the entire set in '()'
>
> > * and in the new at_cgerep_test_cb just use this function ?
>
> How would you return the set of supported settings? I guess you could use
> something like l_uintset, but even knowing the valid theoretical range is
> tricky.
>
> >
> > I can imagine this new function can be of use at other places as well.
> > What do you think ?
>
> Given my confusion above, no idea?
Well, given your various hints I think I slowly get how this is supposed
to work.
The main misunderstanding I had was about the role of iter_next_range().
I thought, it should iterate over the whole thing inside the paranthesis
'()' and give me the min and max of this thing. And it did not make
sense to me that it does behave differently for (0,1) and (0-1). Why
should I iterate multiple times inside of the paranthesis with its min -
max calling interface ?
This was what I meant with the function iter_next_range_or_list() as I
called it above that does what I originally expected from the
iter_next_range().
Ok, I will post a solution with your proposed while-loop.
Many thanks for your hints Denis!
Regards,
Lars
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org
Re: [PATCH 1/2] atmodem: Detect usage of AT+CGEREP
Hi Lars,
So in theory this could be written like:
int max = -1;
iter_open_list(&iter);
while (iter_next_range(&iter, &max1, &max2)) {
if (max2 > max)
max = max2;
}
iter_close_list(&iter);
I can do this and it does indeed work! :-)
This may be my personal thing, but I find this not very intuitive and
hard to read.
To make this more intuitive can we:
* move your proposed while loop to a new function
* name that function ...iter_next_range_or_list (or something like that)
* put that function into gatresult.c
I'm confused. iter_next_range specifically handles ranges as defined by ITU
v.250. See section 5.7.3.1 "Range of Values".
Here's a small quote:
"If more than one value is supported, then the values may be listed
individually, separated by comma characters (IA5 2/12), or, when a continuous
range of values is supported, by the first value in the range, followed by a
hyphen character (IA5 2/13), followed by the last value in the range. The
specification of single values and ranges of values may be intermixed within a
single information text. In all cases, the supported values shall be indicated
in ascending order."
The only thing the user has to do is open up a list, since some modems get this
wrong and don't enclose the entire set in '()'
* and in the new at_cgerep_test_cb just use this function ?
How would you return the set of supported settings? I guess you could use
something like l_uintset, but even knowing the valid theoretical range is tricky.
I can imagine this new function can be of use at other places as well.
What do you think ?
Given my confusion above, no idea?
Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org
Re: [PATCH 1/2] atmodem: Detect usage of AT+CGEREP
Hi Denis,
On Thu, Aug 20, 2020 at 09:41:28AM -0500, Denis Kenzior wrote:
> > > So why not just run iter_next_range in a loop? it actually understands
> > > both
> > > lists and ranges. See cind_support_cb() for an example.
> >
> > Ok, I can do this i a loop if you want.
> >
> > > > + /* if min1 == max1 we had no range but a list and that
> > > > +* means we are interested in the last number of that list*/
> > > > + if (min1 == max1) {
> > > > + while (!g_at_result_iter_close_list(&iter)) {
> > > > + if (!g_at_result_iter_next_number(&iter, &max1))
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + if (!g_at_result_iter_skip_next(&iter)) {
> > > > + two_arguments = false;
> > > > + goto out;
> > > > + }
> > >
> > > Not sure what this is doing? isn't +CGEREP just two lists ? According
> > > to 27.007:
> > > "+CGEREP: (list of supported s),(list of supported s)"
> >
> > Well, we have to deal with very different +CGEREP results. For example
> > on the quectel EC21 I get this:
> >
> > "+CGEREP: (0-2),(0,1)"
> >
> > and on the quectel M95 I get this:
> >
> > "+CGEREP: (0,1)"
> >
> > So what the code does is this:
> > It tries to parse a range with
> >
> > g_at_result_iter_next_range(&iter, &min1, &max1)
> >
> > Now two things can happen: It either parsed the range "(0-2)" then we
> > have min1 != max1, or it tried to parse a list "(0,1)". This time
> > iter_next_range() breaks on the comma ',' and exits with
> > min1 == max1 == 0. Then we know, we did not find the maximum value yet
> > and we enter the loop:
> >
> > ▸··▸···while (!g_at_result_iter_close_list(&iter)) {
> > ▸··▸···▸···if (!g_at_result_iter_next_number(&iter, &max1))
> > ▸··▸···▸···▸···break;
> > ▸··▸···}
> >
> > This does then loop to the end of the list until iter_close_list()
> > becomes true, which is at the closing paranthesis ')'. max1 then
> > contains the last item in that list (which we suspect to be the
> > maximum value).
>
> So in theory this could be written like:
> int max = -1;
> iter_open_list(&iter);
>
> while (iter_next_range(&iter, &max1, &max2)) {
> if (max2 > max)
> max = max2;
> }
>
> iter_close_list(&iter);
I can do this and it does indeed work! :-)
This may be my personal thing, but I find this not very intuitive and
hard to read.
To make this more intuitive can we:
* move your proposed while loop to a new function
* name that function ...iter_next_range_or_list (or something like that)
* put that function into gatresult.c
* and in the new at_cgerep_test_cb just use this function ?
I can imagine this new function can be of use at other places as well.
What do you think ?
Regards,
Lars
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org
Re: [PATCH 1/2] atmodem: Detect usage of AT+CGEREP
Hi Lars,
So why not just run iter_next_range in a loop? it actually understands both
lists and ranges. See cind_support_cb() for an example.
Ok, I can do this i a loop if you want.
+ /* if min1 == max1 we had no range but a list and that
+* means we are interested in the last number of that list*/
+ if (min1 == max1) {
+ while (!g_at_result_iter_close_list(&iter)) {
+ if (!g_at_result_iter_next_number(&iter, &max1))
+ break;
+ }
+ }
+
+ if (!g_at_result_iter_skip_next(&iter)) {
+ two_arguments = false;
+ goto out;
+ }
Not sure what this is doing? isn't +CGEREP just two lists ? According to
27.007:
"+CGEREP: (list of supported s),(list of supported s)"
Well, we have to deal with very different +CGEREP results. For example
on the quectel EC21 I get this:
"+CGEREP: (0-2),(0,1)"
and on the quectel M95 I get this:
"+CGEREP: (0,1)"
So what the code does is this:
It tries to parse a range with
g_at_result_iter_next_range(&iter, &min1, &max1)
Now two things can happen: It either parsed the range "(0-2)" then we
have min1 != max1, or it tried to parse a list "(0,1)". This time
iter_next_range() breaks on the comma ',' and exits with
min1 == max1 == 0. Then we know, we did not find the maximum value yet
and we enter the loop:
▸··▸···while (!g_at_result_iter_close_list(&iter)) {
▸··▸···▸···if (!g_at_result_iter_next_number(&iter, &max1))
▸··▸···▸···▸···break;
▸··▸···}
This does then loop to the end of the list until iter_close_list()
becomes true, which is at the closing paranthesis ')'. max1 then
contains the last item in that list (which we suspect to be the
maximum value).
So in theory this could be written like:
int max = -1;
iter_open_list(&iter);
while (iter_next_range(&iter, &max1, &max2)) {
if (max2 > max)
max = max2;
}
iter_close_list(&iter);
Now we have min1 and max1 in both cases(list and range). Now if
g_at_result_iter_skip_next()
fails, we are at the end of the AT result and we had only one argument
to +CGEREP or we start trying to parse the second argument also. (See
below.)
At the end we construct our AT+CGEREP= string depending on if we had
two or only one argument.
or you could take the above code I suggested followed by something like:
if (!iter_open_list(&iter)) {
twoargs = false;
goto done;
}
while (iter_next_range(...)) {
...
}
iter_close_list(&iter);
...
Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org
Re: [PATCH 1/2] atmodem: Detect usage of AT+CGEREP
On Wed, Aug 19, 2020 at 10:07:58AM -0500, Denis Kenzior wrote:
> Hi Lars,
>
> On 8/19/20 7:13 AM, poesc...@lemonage.de wrote:
> > From: Lars Poeschel
> >
> > Currently AT+CGEREP=2,1 is sent in case we don't know what the modem
> > needs. (default case) Not all modems understand this. So, we first query
> > what the modem supports with AT+CGEREP=? and then use this information
> > to be nice to the modem. This way modems, like the Quectel M95 that do
> > only understand AT+CGEREP=1 do also work nicely.
>
> I think this is a nice improvement, couple of comments below:
Thanks. :-)
> > ---
> > drivers/atmodem/gprs.c | 75 --
> > 1 file changed, 73 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/atmodem/gprs.c b/drivers/atmodem/gprs.c
> > index d829e2e6..3900b95b 100644
> > --- a/drivers/atmodem/gprs.c
> > +++ b/drivers/atmodem/gprs.c
> > @@ -45,6 +45,7 @@
> > #define MAX_CONTEXTS 255
> > static const char *cgreg_prefix[] = { "+CGREG:", NULL };
> > +static const char *cgerep_prefix[] = { "+CGEREP:", NULL };
> > static const char *cgdcont_prefix[] = { "+CGDCONT:", NULL };
> > static const char *cgact_prefix[] = { "+CGACT:", NULL };
> > static const char *none_prefix[] = { NULL };
> > @@ -647,6 +648,76 @@ static void gprs_initialized(gboolean ok, GAtResult
> > *result, gpointer user_data)
> > ofono_gprs_register(gprs);
> > }
> > +static void at_cgerep_test_cb(gboolean ok, GAtResult *result,
> > + gpointer user_data)
> > +{
> > + struct ofono_gprs *gprs = user_data;
> > + struct gprs_data *gd = ofono_gprs_get_data(gprs);
> > + GAtResultIter iter;
> > + int min1, max1 = 1, min2, max2 = 1;
> > + gboolean two_arguments = true;
> > + char buf[20];
> > +
> > + if (!ok)
> > + return;
>
> Hmm, maybe a good idea here is to print a warning and call
> ofono_gprs_remove(). Otherwise this will just fail silently for someone.
Ok, I will update this.
> > +
> > + g_at_result_iter_init(&iter, result);
> > +
> > + g_at_result_iter_next(&iter, "+CGEREP:");
> > +
> > + if (!g_at_result_iter_open_list(&iter))
> > + return;
>
> Ditto here
Same here.
> > +
> > + if (!g_at_result_iter_next_range(&iter, &min1, &max1))
> > + return;
> > +
>
> So why not just run iter_next_range in a loop? it actually understands both
> lists and ranges. See cind_support_cb() for an example.
Ok, I can do this i a loop if you want.
> > + /* if min1 == max1 we had no range but a list and that
> > +* means we are interested in the last number of that list*/
> > + if (min1 == max1) {
> > + while (!g_at_result_iter_close_list(&iter)) {
> > + if (!g_at_result_iter_next_number(&iter, &max1))
> > + break;
> > + }
> > + }
> > +
> > + if (!g_at_result_iter_skip_next(&iter)) {
> > + two_arguments = false;
> > + goto out;
> > + }
>
> Not sure what this is doing? isn't +CGEREP just two lists ? According to
> 27.007:
> "+CGEREP: (list of supported s),(list of supported s)"
Well, we have to deal with very different +CGEREP results. For example
on the quectel EC21 I get this:
"+CGEREP: (0-2),(0,1)"
and on the quectel M95 I get this:
"+CGEREP: (0,1)"
So what the code does is this:
It tries to parse a range with
g_at_result_iter_next_range(&iter, &min1, &max1)
Now two things can happen: It either parsed the range "(0-2)" then we
have min1 != max1, or it tried to parse a list "(0,1)". This time
iter_next_range() breaks on the comma ',' and exits with
min1 == max1 == 0. Then we know, we did not find the maximum value yet
and we enter the loop:
▸··▸···while (!g_at_result_iter_close_list(&iter)) {
▸··▸···▸···if (!g_at_result_iter_next_number(&iter, &max1))
▸··▸···▸···▸···break;
▸··▸···}
This does then loop to the end of the list until iter_close_list()
becomes true, which is at the closing paranthesis ')'. max1 then
contains the last item in that list (which we suspect to be the
maximum value).
Now we have min1 and max1 in both cases(list and range). Now if
g_at_result_iter_skip_next()
fails, we are at the end of the AT result and we had only one argument
to +CGEREP or we start trying to parse the second argument also. (See
below.)
At the end we construct our AT+CGEREP= string depending on if we had
two or only one argument.
Does this sound reasonable ?
> > +
> > + if (!g_at_result_iter_open_list(&iter)) {
> > + two_arguments = false;
> > + goto out;
> > + }
> > +
> > + if (!g_at_result_iter_next_range(&iter, &min2, &max2)) {
> > + two_arguments = false;
> > + goto out;
> > + }
> > +
> > + if (min2 == max2) {
> > + while (!g_at_result_iter_close_list(&iter)) {
> > + if (!g_at_result_iter_next_number(&iter, &max2)) {
> > + break;
> > + }
> > +
Re: [PATCH 1/2] atmodem: Detect usage of AT+CGEREP
Hi Lars,
On 8/19/20 7:13 AM, poesc...@lemonage.de wrote:
From: Lars Poeschel
Currently AT+CGEREP=2,1 is sent in case we don't know what the modem
needs. (default case) Not all modems understand this. So, we first query
what the modem supports with AT+CGEREP=? and then use this information
to be nice to the modem. This way modems, like the Quectel M95 that do
only understand AT+CGEREP=1 do also work nicely.
I think this is a nice improvement, couple of comments below:
---
drivers/atmodem/gprs.c | 75 --
1 file changed, 73 insertions(+), 2 deletions(-)
diff --git a/drivers/atmodem/gprs.c b/drivers/atmodem/gprs.c
index d829e2e6..3900b95b 100644
--- a/drivers/atmodem/gprs.c
+++ b/drivers/atmodem/gprs.c
@@ -45,6 +45,7 @@
#define MAX_CONTEXTS 255
static const char *cgreg_prefix[] = { "+CGREG:", NULL };
+static const char *cgerep_prefix[] = { "+CGEREP:", NULL };
static const char *cgdcont_prefix[] = { "+CGDCONT:", NULL };
static const char *cgact_prefix[] = { "+CGACT:", NULL };
static const char *none_prefix[] = { NULL };
@@ -647,6 +648,76 @@ static void gprs_initialized(gboolean ok, GAtResult
*result, gpointer user_data)
ofono_gprs_register(gprs);
}
+static void at_cgerep_test_cb(gboolean ok, GAtResult *result,
+ gpointer user_data)
+{
+ struct ofono_gprs *gprs = user_data;
+ struct gprs_data *gd = ofono_gprs_get_data(gprs);
+ GAtResultIter iter;
+ int min1, max1 = 1, min2, max2 = 1;
+ gboolean two_arguments = true;
+ char buf[20];
+
+ if (!ok)
+ return;
Hmm, maybe a good idea here is to print a warning and call ofono_gprs_remove().
Otherwise this will just fail silently for someone.
+
+ g_at_result_iter_init(&iter, result);
+
+ g_at_result_iter_next(&iter, "+CGEREP:");
+
+ if (!g_at_result_iter_open_list(&iter))
+ return;
Ditto here
+
+ if (!g_at_result_iter_next_range(&iter, &min1, &max1))
+ return;
+
So why not just run iter_next_range in a loop? it actually understands both
lists and ranges. See cind_support_cb() for an example.
+ /* if min1 == max1 we had no range but a list and that
+* means we are interested in the last number of that list*/
+ if (min1 == max1) {
+ while (!g_at_result_iter_close_list(&iter)) {
+ if (!g_at_result_iter_next_number(&iter, &max1))
+ break;
+ }
+ }
+
+ if (!g_at_result_iter_skip_next(&iter)) {
+ two_arguments = false;
+ goto out;
+ }
Not sure what this is doing? isn't +CGEREP just two lists ? According to
27.007:
"+CGEREP: (list of supported s),(list of supported s)"
+
+ if (!g_at_result_iter_open_list(&iter)) {
+ two_arguments = false;
+ goto out;
+ }
+
+ if (!g_at_result_iter_next_range(&iter, &min2, &max2)) {
+ two_arguments = false;
+ goto out;
+ }
+
+ if (min2 == max2) {
+ while (!g_at_result_iter_close_list(&iter)) {
+ if (!g_at_result_iter_next_number(&iter, &max2)) {
+ break;
+ }
+ }
+ }
+
+out:
+ if (max1 > 2)
+ max1 = 2;
+
+ if (max2 > 1)
+ max2 = 1;
+
+ if (two_arguments)
+ sprintf(buf, "AT+CGEREP=%u,%u", max1, max2);
+ else
+ sprintf(buf, "AT+CGEREP=%u", max1);
+
+ g_at_chat_send(gd->chat, buf, none_prefix, gprs_initialized, gprs,
NULL);
+}
+
static void at_cgreg_test_cb(gboolean ok, GAtResult *result,
gpointer user_data)
{
@@ -701,8 +772,8 @@ retry:
gprs_initialized, gprs, NULL);
break;
default:
- g_at_chat_send(gd->chat, "AT+CGEREP=2,1", none_prefix,
- gprs_initialized, gprs, NULL);
+ g_at_chat_send(gd->chat, "AT+CGEREP=?", cgerep_prefix,
+ at_cgerep_test_cb, gprs, NULL);
break;
}
Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org
[PATCH 1/2] atmodem: Detect usage of AT+CGEREP
From: Lars Poeschel
Currently AT+CGEREP=2,1 is sent in case we don't know what the modem
needs. (default case) Not all modems understand this. So, we first query
what the modem supports with AT+CGEREP=? and then use this information
to be nice to the modem. This way modems, like the Quectel M95 that do
only understand AT+CGEREP=1 do also work nicely.
---
drivers/atmodem/gprs.c | 75 --
1 file changed, 73 insertions(+), 2 deletions(-)
diff --git a/drivers/atmodem/gprs.c b/drivers/atmodem/gprs.c
index d829e2e6..3900b95b 100644
--- a/drivers/atmodem/gprs.c
+++ b/drivers/atmodem/gprs.c
@@ -45,6 +45,7 @@
#define MAX_CONTEXTS 255
static const char *cgreg_prefix[] = { "+CGREG:", NULL };
+static const char *cgerep_prefix[] = { "+CGEREP:", NULL };
static const char *cgdcont_prefix[] = { "+CGDCONT:", NULL };
static const char *cgact_prefix[] = { "+CGACT:", NULL };
static const char *none_prefix[] = { NULL };
@@ -647,6 +648,76 @@ static void gprs_initialized(gboolean ok, GAtResult
*result, gpointer user_data)
ofono_gprs_register(gprs);
}
+static void at_cgerep_test_cb(gboolean ok, GAtResult *result,
+ gpointer user_data)
+{
+ struct ofono_gprs *gprs = user_data;
+ struct gprs_data *gd = ofono_gprs_get_data(gprs);
+ GAtResultIter iter;
+ int min1, max1 = 1, min2, max2 = 1;
+ gboolean two_arguments = true;
+ char buf[20];
+
+ if (!ok)
+ return;
+
+ g_at_result_iter_init(&iter, result);
+
+ g_at_result_iter_next(&iter, "+CGEREP:");
+
+ if (!g_at_result_iter_open_list(&iter))
+ return;
+
+ if (!g_at_result_iter_next_range(&iter, &min1, &max1))
+ return;
+
+ /* if min1 == max1 we had no range but a list and that
+* means we are interested in the last number of that list*/
+ if (min1 == max1) {
+ while (!g_at_result_iter_close_list(&iter)) {
+ if (!g_at_result_iter_next_number(&iter, &max1))
+ break;
+ }
+ }
+
+ if (!g_at_result_iter_skip_next(&iter)) {
+ two_arguments = false;
+ goto out;
+ }
+
+ if (!g_at_result_iter_open_list(&iter)) {
+ two_arguments = false;
+ goto out;
+ }
+
+ if (!g_at_result_iter_next_range(&iter, &min2, &max2)) {
+ two_arguments = false;
+ goto out;
+ }
+
+ if (min2 == max2) {
+ while (!g_at_result_iter_close_list(&iter)) {
+ if (!g_at_result_iter_next_number(&iter, &max2)) {
+ break;
+ }
+ }
+ }
+
+out:
+ if (max1 > 2)
+ max1 = 2;
+
+ if (max2 > 1)
+ max2 = 1;
+
+ if (two_arguments)
+ sprintf(buf, "AT+CGEREP=%u,%u", max1, max2);
+ else
+ sprintf(buf, "AT+CGEREP=%u", max1);
+
+ g_at_chat_send(gd->chat, buf, none_prefix, gprs_initialized, gprs,
NULL);
+}
+
static void at_cgreg_test_cb(gboolean ok, GAtResult *result,
gpointer user_data)
{
@@ -701,8 +772,8 @@ retry:
gprs_initialized, gprs, NULL);
break;
default:
- g_at_chat_send(gd->chat, "AT+CGEREP=2,1", none_prefix,
- gprs_initialized, gprs, NULL);
+ g_at_chat_send(gd->chat, "AT+CGEREP=?", cgerep_prefix,
+ at_cgerep_test_cb, gprs, NULL);
break;
}
--
2.27.0
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org
