Bug/Oversight in gatchat/gatresult.c with negative numbers

Bug/Oversight in gatchat/gatresult.c with negative numbers

[Alex J Lennon]

[-- Attachment #1: Type: text/plain, Size: 1398 bytes --]

Hi all,

I'm doing a bit of work with Ofono again, extending support for a 
Quectel EG91 - handling additional cell strength information which seems 
necessary for LTE.

I'm using Quectel's AT+QCOPS which is returning some negative signal 
strengths and after some investigation I've spotted that 
gatresult::g_at_result_iter_next_number() doesn't handle negative numbers.

No doubt there's a better solution than mine, but this does appear to work.

Cheers,

Alex

--- ofono-1.22.org/gatchat/gatresult.c  2011-10-11 18:21:45.000000000 +0000
+++ ofono-1.22/gatchat/gatresult.c      2021-07-20 15:50:06.300001000 +0000
@@ -262,6 +262,7 @@
         int end;
         int len;
         int value = 0;
+       int sign = 1;
         char *line;

         if (iter == NULL)
@@ -276,10 +277,16 @@
         pos = iter->line_pos;
         end = pos;

+       if(line[end] == '-') {
+               sign = -1;
+               end += 1;
+       }
+
         while (line[end] >= '0' && line[end] <= '9') {
                 value = value * 10 + (int)(line[end] - '0');
                 end += 1;
         }
+       value = sign * value;

         if (pos == end)
                 return FALSE;

Re: Bug/Oversight in gatchat/gatresult.c with negative numbers

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]

Hi Alex,

On 7/20/21 11:11 AM, Alex J Lennon wrote:
> Hi all,
> 
> I'm doing a bit of work with Ofono again, extending support for a Quectel EG91 - 
> handling additional cell strength information which seems necessary for LTE.
> 
> I'm using Quectel's AT+QCOPS which is returning some negative signal strengths 
> and after some investigation I've spotted that 
> gatresult::g_at_result_iter_next_number() doesn't handle negative numbers.

AT commands do not have a concept of negative numbers.  v.250 Section 5.3.1:
<number> may be a string of one or more characters from "0" through "9" 
representing a decimal
integer value. Commands that expect a <number> are noted in the description of 
the command (see
clause 6).

I don't recall any other vendor using negative values in AT commands.

If you want to add a negative number parser to GAtChat, then please make it a 
separate method, so that it is very clear that a vendor 'extension' is being 
worked around.

Regards,
-Denis

Re: Bug/Oversight in gatchat/gatresult.c with negative numbers

[Alex J Lennon]

[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]

Hi Sebastian,

Please see below:

On 27/07/2021 15:38, Denis Kenzior wrote:
> Hi Alex,
>
> On 7/20/21 11:11 AM, Alex J Lennon wrote:
>> Hi all,
>>
>> I'm doing a bit of work with Ofono again, extending support for a 
>> Quectel EG91 - handling additional cell strength information which 
>> seems necessary for LTE.
>>
>> I'm using Quectel's AT+QCOPS which is returning some negative signal 
>> strengths and after some investigation I've spotted that 
>> gatresult::g_at_result_iter_next_number() doesn't handle negative 
>> numbers.
>
> AT commands do not have a concept of negative numbers.  v.250 Section 
> 5.3.1:
> <number> may be a string of one or more characters from "0" through 
> "9" representing a decimal
> integer value. Commands that expect a <number> are noted in the 
> description of the command (see
> clause 6).
>
> I don't recall any other vendor using negative values in AT commands.
>
> If you want to add a negative number parser to GAtChat, then please 
> make it a separate method, so that it is very clear that a vendor 
> 'extension' is being worked around.
>
> Regards,
> -Denis


The team at Ofono are suggesting that Quectel (EG91) should not be 
returning negative numbers in AT command responses

These are returned to an AT+QCOPS command where signal strength is given.

Can you comment?

Thanks,

Alex

Re: Bug/Oversight in gatchat/gatresult.c with negative numbers

[Alex J Lennon]

[-- Attachment #1: Type: text/plain, Size: 1698 bytes --]


On 27/07/2021 15:38, Denis Kenzior wrote:
> Hi Alex,
>
> On 7/20/21 11:11 AM, Alex J Lennon wrote:
>> Hi all,
>>
>> I'm doing a bit of work with Ofono again, extending support for a 
>> Quectel EG91 - handling additional cell strength information which 
>> seems necessary for LTE.
>>
>> I'm using Quectel's AT+QCOPS which is returning some negative signal 
>> strengths and after some investigation I've spotted that 
>> gatresult::g_at_result_iter_next_number() doesn't handle negative 
>> numbers.
>
> AT commands do not have a concept of negative numbers.  v.250 Section 
> 5.3.1:
> <number> may be a string of one or more characters from "0" through 
> "9" representing a decimal
> integer value. Commands that expect a <number> are noted in the 
> description of the command (see
> clause 6).
>
> I don't recall any other vendor using negative values in AT commands.
>
> If you want to add a negative number parser to GAtChat, then please 
> make it a separate method, so that it is very clear that a vendor 
> 'extension' is being worked around.
>
> Regards,
> -Denis

Hi Denis,

I have been chasing this up with Quectel and they tell me this the 
correct implementation.

"QCOPS and QENG command show the RSSI、RSRP and SINR value of the 
network, this is in accordance with the 3GPP TS regulations, so it is 
normal for negative numbers to be displayed.image"

This is quite interesting as aside from the parsing, Ofono seems to rely 
on negative numbers being errors (e.g. src/netmon.c)

What are your thoughts? This being correct, one would have thought other 
modem manufacturers would report in the same manner.

Cheers,

Alex



[-- Attachment #2: attachment.htm --]
[-- Type: text/html, Size: 2522 bytes --]

[-- Attachment #3: dhlplbcpkdpfjdoo.png --]
[-- Type: image/png, Size: 25653 bytes --]

Re: Bug/Oversight in gatchat/gatresult.c with negative numbers

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 874 bytes --]

Hi Alex,

> I have been chasing this up with Quectel and they tell me this the correct 
> implementation.
> 
> "QCOPS and QENG command show the RSSI、RSRP and SINR value of the network, this 
> is in accordance with the 3GPP TS regulations, so it is normal for negative 
> numbers to be displayed.image"


So the image shows the value for <rsrp>

'0' if -140 dbm
...
'95' if dbm is -46 to -45
etc.

<rsrp> (what is actually sent over AT commands) is never negative. Isn't the 
image supporting my earlier assertion:  'AT commands do not have a concept of 
negative numbers'?

oFono exposes the <rsrp> value from 3GPP:

"byte ReferenceSignalReceivedPower [optional, lte]

         Contains the Reference Signal Received Power.  Valid range of values
         is 0-97. Refer to <rsrp> in 27.007, Section 8.69 for more details.
"

Regards,
-Denis

Re: Bug/Oversight in gatchat/gatresult.c with negative numbers

[Alex J Lennon]

[-- Attachment #1: Type: text/plain, Size: 1054 bytes --]


On 03/08/2021 15:28, Denis Kenzior wrote:
> Hi Alex,
>
>> I have been chasing this up with Quectel and they tell me this the 
>> correct implementation.
>>
>> "QCOPS and QENG command show the RSSI、RSRP and SINR value of the 
>> network, this is in accordance with the 3GPP TS regulations, so it is 
>> normal for negative numbers to be displayed.image"
>
>
> So the image shows the value for <rsrp>
>
> '0' if -140 dbm
> ...
> '95' if dbm is -46 to -45
> etc.
>
> <rsrp> (what is actually sent over AT commands) is never negative. 
> Isn't the image supporting my earlier assertion:  'AT commands do not 
> have a concept of negative numbers'?
>
> oFono exposes the <rsrp> value from 3GPP:
>
> "byte ReferenceSignalReceivedPower [optional, lte]
>
>         Contains the Reference Signal Received Power.  Valid range of 
> values
>         is 0-97. Refer to <rsrp> in 27.007, Section 8.69 for more 
> details.
> "
>
> Regards,
> -Denis


Thanks... will go back to Quectel... (!)

Cheers,

Alex

Re: Bug/Oversight in gatchat/gatresult.c with negative numbers

[Alex J Lennon]

[-- Attachment #1: Type: text/plain, Size: 1453 bytes --]


On 03/08/2021 15:28, Denis Kenzior wrote:
> Hi Alex,
>
>> I have been chasing this up with Quectel and they tell me this the 
>> correct implementation.
>>
>> "QCOPS and QENG command show the RSSI、RSRP and SINR value of the 
>> network, this is in accordance with the 3GPP TS regulations, so it is 
>> normal for negative numbers to be displayed.image"
>
>
> So the image shows the value for <rsrp>
>
> '0' if -140 dbm
> ...
> '95' if dbm is -46 to -45
> etc.
>
> <rsrp> (what is actually sent over AT commands) is never negative. 
> Isn't the image supporting my earlier assertion:  'AT commands do not 
> have a concept of negative numbers'?
>
> oFono exposes the <rsrp> value from 3GPP:
>
> "byte ReferenceSignalReceivedPower [optional, lte]
>
>         Contains the Reference Signal Received Power.  Valid range of 
> values
>         is 0-97. Refer to <rsrp> in 27.007, Section 8.69 for more 
> details.
> "
>
> Regards,
> -Denis


Denis - I am being directed to the ITU 36.133 spec here

https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=2420

This document defines this in 9.1.4

I can't see where RSRP_-17 is defined anywhere but the strong 
implication to me is that this number should be -17

Can you confirm where it states that these numbers cannot be negative 
numbers as I cannot find that requirement.

Thanks,

Alex



[-- Attachment #2: attachment.htm --]
[-- Type: text/html, Size: 2499 bytes --]

[-- Attachment #3: omoepmpcdeogdgnp.png --]
[-- Type: image/png, Size: 46406 bytes --]

Re: Bug/Oversight in gatchat/gatresult.c with negative numbers

[Alex J Lennon]

[-- Attachment #1: Type: text/plain, Size: 1648 bytes --]


On 03/08/2021 15:28, Denis Kenzior wrote:
> Hi Alex,
>
>> I have been chasing this up with Quectel and they tell me this the 
>> correct implementation.
>>
>> "QCOPS and QENG command show the RSSI、RSRP and SINR value of the 
>> network, this is in accordance with the 3GPP TS regulations, so it is 
>> normal for negative numbers to be displayed.image"
>
>
> So the image shows the value for <rsrp>
>
> '0' if -140 dbm
> ...
> '95' if dbm is -46 to -45
> etc.
>
> <rsrp> (what is actually sent over AT commands) is never negative. 
> Isn't the image supporting my earlier assertion:  'AT commands do not 
> have a concept of negative numbers'?
>
> oFono exposes the <rsrp> value from 3GPP:
>
> "byte ReferenceSignalReceivedPower [optional, lte]
>
>         Contains the Reference Signal Received Power.  Valid range of 
> values
>         is 0-97. Refer to <rsrp> in 27.007, Section 8.69 for more 
> details.
> "
>
> Regards,
> -Denis


Denis - I am being directed to the ITU 36.133 spec here

https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=2420

This document defines this in 9.1.4

I can't see where RSRP_-17 is defined anywhere but the strong 
implication to me is that this number should be -17

Can you confirm where it states that these numbers cannot be negative 
numbers as I cannot find that requirement.

Thanks,

Alex

------------------------------------------------------------------------

A colleague has just pointed me to a UBlox document which specifies for 
negative values too

Regards,

Alex




[-- Attachment #2: attachment.htm --]
[-- Type: text/html, Size: 2729 bytes --]

[-- Attachment #3: omoepmpcdeogdgnp.png --]
[-- Type: image/png, Size: 46406 bytes --]

[-- Attachment #4: dcofklpjpnbjkooh.png --]
[-- Type: image/png, Size: 93293 bytes --]

Re: Bug/Oversight in gatchat/gatresult.c with negative numbers

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

Hi Alex,

> Denis - I am being directed to the ITU 36.133 spec here
> 
> https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=2420
> 

Sure, same document/section where 27.007 refers to for <rsrp>.  But 
unfortunately 27.007 only defines the range 0..97 where 0 is <rsrp> < 140 dbm. 
How 3GPP decides to represent <rsrp> values between 156 and 140 is up to 3GPP. 
If your vendor is using negative integers in AT commands, then that is a vendor 
extension and not something that is permitted by ITU v.250.

> This document defines this in 9.1.4
> 
> I can't see where RSRP_-17 is defined anywhere but the strong implication to me 
> is that this number should be -17
> 
> Can you confirm where it states that these numbers cannot be negative numbers as 
> I cannot find that requirement.
> 

V.250 Section 5.3.1.  Notice that 27.007 never uses negative integers, there's a 
reason for that.

Regards,
-Denis

Re: Bug/Oversight in gatchat/gatresult.c with negative numbers

[Alex J Lennon]

[-- Attachment #1: Type: text/plain, Size: 1291 bytes --]


On 06/08/2021 15:20, Denis Kenzior wrote:
> Hi Alex,
>
>> Denis - I am being directed to the ITU 36.133 spec here
>>
>> https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=2420 
>>
>>
>
> Sure, same document/section where 27.007 refers to for <rsrp>.  But 
> unfortunately 27.007 only defines the range 0..97 where 0 is <rsrp> < 
> 140 dbm. How 3GPP decides to represent <rsrp> values between 156 and 
> 140 is up to 3GPP. If your vendor is using negative integers in AT 
> commands, then that is a vendor extension and not something that is 
> permitted by ITU v.250.
>
>> This document defines this in 9.1.4
>>
>> I can't see where RSRP_-17 is defined anywhere but the strong 
>> implication to me is that this number should be -17
>>
>> Can you confirm where it states that these numbers cannot be negative 
>> numbers as I cannot find that requirement.
>>
>
> V.250 Section 5.3.1.  Notice that 27.007 never uses negative integers, 
> there's a reason for that.
>
> Regards,
> -Denis


OK - they say it's fine and you say it's not. I don't have the time to 
be the middle-man here.

Although I'd like to, clearly I won't be able to contribute this Quectel 
support upstream.

Thanks anyway,

Alex

Re: Bug/Oversight in gatchat/gatresult.c with negative numbers

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 547 bytes --]

Hi Alex,

> 
> OK - they say it's fine and you say it's not. I don't have the time to be the 
> middle-man here.
> 

I'm fairly certain your vendor's firmware is not following the relevant 
standards, but it sounds like they don't want to fix it.  What can I do? :)

> Although I'd like to, clearly I won't be able to contribute this Quectel support 
> upstream.
> 

Why does it even matter?  I told you very early on that it would be acceptable 
to add a new g_at_result_* method that handles negative integers.

Regards,
-Denis

Re: Bug/Oversight in gatchat/gatresult.c with negative numbers

[Alex J Lennon]

[-- Attachment #1: Type: text/plain, Size: 1217 bytes --]


On 06/08/2021 15:44, Denis Kenzior wrote:
> Hi Alex,
>
>>
>> OK - they say it's fine and you say it's not. I don't have the time 
>> to be the middle-man here.
>>
>
> I'm fairly certain your vendor's firmware is not following the 
> relevant standards, but it sounds like they don't want to fix it. What 
> can I do? :)

Yeah I think you are right and I am getting really irritated with them 
giving me the run-around tbh.

>
>> Although I'd like to, clearly I won't be able to contribute this 
>> Quectel support upstream.
>>
>
> Why does it even matter?  I told you very early on that it would be 
> acceptable to add a new g_at_result_* method that handles negative 
> integers.
>
> Regards,
> -Denis

Yeah ok - what name do you suggest?

Also there's a bigger problem here I think, which is that if I parse 
negative values and try to pass them back via e.g. netmon interface then 
Ofono is treating anything negative in there as an error.

And the DBUS implementation is unsigned for values based on the thinking 
that AT responses are non-negative.

So actually there are a number of invasive changes that seem to need to 
be made to support negative numbers?

Alex

Re: Bug/Oversight in gatchat/gatresult.c with negative numbers

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1722 bytes --]

Hi Alex,

> Yeah I think you are right and I am getting really irritated with them giving me 
> the run-around tbh.
> 

I sympathize, been there ;)

> 
> Yeah ok - what name do you suggest?

So there's a few things you can do actually:
- We do have g_at_result_next_unquoted_string.  You could try using that to 
obtain the negative <rsrp> and then just sscanf/strtol or whatever to convert it.
- We also have g_at_result_iter_raw_line() which can give you the raw result 
line.  It should be trivial to come up with some sscanf magic to parse the result.
- Just add a new g_at_result_iter_next_negative_number.  Or if you come up with 
a more descriptive word than negative, feel free.

> 
> Also there's a bigger problem here I think, which is that if I parse negative 
> values and try to pass them back via e.g. netmon interface then Ofono is 
> treating anything negative in there as an error.

oFono uses 27.007 as the interface between the driver and the core (and also for 
DBus API).  If it isn't defined in 27.007, then your driver has to sanitize it 
to a range that is defined in 27.007.  In your <rsrp> case any negative value 
would just be converted to a '0' which means < -140.

> 
> And the DBUS implementation is unsigned for values based on the thinking that AT 
> responses are non-negative.
> 

See above, the DBus implementation would be untouched.  If you need to 
*actually* know whether <rsrp> is -142 or other range that is not currently 
supported by 3GPP 27.007, then that is a bit nastier problem.

> So actually there are a number of invasive changes that seem to need to be made 
> to support negative numbers?
> 

Hopefully not?

Regards,
-Denis

Re: Bug/Oversight in gatchat/gatresult.c with negative numbers

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]

On Fri, Aug 06, 2021 at 10:02:18AM -0500, Denis Kenzior wrote:
> > Yeah ok - what name do you suggest?
> 
> So there's a few things you can do actually:
> - We do have g_at_result_next_unquoted_string.  You could try using that to
> obtain the negative <rsrp> and then just sscanf/strtol or whatever to
> convert it.
> - We also have g_at_result_iter_raw_line() which can give you the raw result
> line.  It should be trivial to come up with some sscanf magic to parse the
> result.
> - Just add a new g_at_result_iter_next_negative_number.  Or if you come up
> with a more descriptive word than negative, feel free.

I would go with g_at_result_iter_next_signed_number() to reflect that
the value isn't necessarily negative.

// Martin