1069186 - Invalid LTE signal level reported

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[:gerard-majax]

With current gecko/gaia master on a device that supports LTE (Cat3), I always get the maximum level, whatever the position I am within. Specifically, getting such strong network signal within Montparnasse train station is dubious.

As per TS 27.007 (Ver 10.3.0) Sec 8.69, and as documented in https://android.googlesource.com/platform/frameworks/base/+/android-4.4.4_r2.0.1/telephony/java/android/telephony/CellSignalStrengthLte.java, the expected range of valid signal strength values for LTE is [0;97], 99 denoting an unknown value.

However, in http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#3748 we are checking for [0;63], which seems wrong. I also suspect the _processSignalLevel() call to be wrong: we should pass the dBm value (signalStrength) and not the ASU value (signal.lteSignalStrength), and the dBm range [0;12] does not make any sense to me: getAsuLevel in CellSignalStrengthLte.java explicitely shows that the 0 level dbm is -140dBm, and that the max dbm is with -43dBm. So the normalization map should use [-140;-43] for the min/max values.

Comment 1

[:gerard-majax]

After hacking, I'm getting values that make much more sense (Bouygues Telecom SIM within Paris Office):

> I/Gecko   (  244): RIL Worker: [0] signal strength: {"gsmSignalStrength":99,"gsmBitErrorRate":0,"cdmaDBM":-1,"cdmaECIO":-1,"evdoDBM":-1,"evdoECIO":-1,"evdoSNR":-1,"lteSignalStrength":28,"lteRSRP":84,"lteRSRQ":8,"lteRSSNR":78,"lteCQI":2147483647}
> I/Gecko   (  244): RIL Worker: [0] processLteSignal:: signal.lteSignalStrength=28 signalStrength=-112 signalLevel=29
> I/Gecko   (  244): RIL Worker: [0] Queuing signal network info message: {"voice":{"signalStrength":-112,"relSignalStrength":29},"data":{"signalStrength":-112,"relSignalStrength":29},"rilMessageType":"signalstrengthchange"}
> I/Gecko   (  244): -*- RadioInterface[0]: Received message from worker: {"rilMessageType":"networkinfochanged","rilMessageClientId":0,"signal":{"voice":{"signalStrength":-112,"relSignalStrength":29},"data":{"signalStrength":-112,"relSignalStrength":29},"rilMessageType":"signalstrengthchange"}}

Comment 2

[:gerard-majax]

Attachment: Use proper range for LTE signal strength

Comment 3

[:gerard-majax]

Attachment: Use proper range for LTE signal strength

Comment 4

[:gerard-majax]

Comment on attachment 8491616 [details] [diff] [review]
Use proper range for LTE signal strength

Shawn, I changed the valid range, because I was wrong and [0;31] is actually the proper one. I adjusted other computation after some in-the-wild testing, and it seems to more properly reflect the status for now.

Comment 5

[shawn ku [:sku]]

Comment on attachment 8491616 [details] [diff] [review]
Use proper range for LTE signal strength

Review of attachment 8491616 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Alexandre:
 I am not the proper one to review the patch.
FW to Edgar for doing review.

Besides, Yes, I agree with you that LteSignalStrength shoud be ranged 0 - 31 (see [1]).
However, the AOSP source shows me below getLteLevel()@SignalStrength.java,

The LTE network will not report the value greater than 31, that's what we see on real network.

// I can not find it on github anymore, but my AOSP 4.4 did show.
        /* Valid values are (0-63, 99) as defined in TS 36.331 */
        if (mLteSignalStrength > 63) rssiIconLevel = SIGNAL_STRENGTH_NONE_OR_UNKNOWN;
        else if (mLteSignalStrength >= 12) rssiIconLevel = SIGNAL_STRENGTH_GREAT;
        else if (mLteSignalStrength >= 8) rssiIconLevel = SIGNAL_STRENGTH_GOOD;
        else if (mLteSignalStrength >= 5) rssiIconLevel = SIGNAL_STRENGTH_MODERATE;
        else if (mLteSignalStrength >= 0) rssiIconLevel = SIGNAL_STRENGTH_POOR;
        if (DBG) log("getLTELevel - rssi:" + mLteSignalStrength + " rssiIconLevel:"
                + rssiIconLevel);
        return rssiIconLevel;


[1] - https://github.com/android/platform_hardware_ril/blob/master/include/telephony/ril.h#L732

Comment 6

[Edgar Chen [:edgar]]

Comment on attachment 8491616 [details] [diff] [review]
Use proper range for LTE signal strength

Review of attachment 8491616 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for your patch, please see my comments below. :)

::: dom/system/gonk/ril_worker.js
@@ +3744,5 @@
>     * @return The object of signal strength info.
>     *         Or null if invalid signal input.
>     */
>    _processLteSignal: function(signal) {
> +    // Valid values are 0-31 as defined in TS 27.007 clause 8.69.

Comment seems wrong. Could you help to correct it as well?
According to ril.h [1], it should be TS 27.007 clause 8.5.

[1] https://github.com/android/platform_hardware_ril/blob/master/include/telephony/ril.h#L713

@@ +3764,5 @@
>      };
>  
>      // TODO: Bug 982013: reconsider signalStrength/relSignalStrength APIs for
>      //       GSM/CDMA/LTE, and take rsrp/rssnr into account for LTE case then.
> +    let signalStrength = -140 + 3*signal.lteSignalStrength;

The range for letSignalStrength should be [-113;-51] according to TS 27.007 clause 8.5.

nit: spaces around operators.

@@ +3769,4 @@
>      info.voice.signalStrength = info.data.signalStrength = signalStrength;
>      // 0 and 12 are referred to AOSP's implementation. These values are not
>      // constants and can be customized based on different requirements.
> +    let signalLevel = this._processSignalLevel(signalStrength, -140, -77);

The range [-140;-43] is for "rsrp" [1], not "signalStrength".
And AOSP's implementation use "rsrp" to calculate signal level [2].

if (mRsrp == Integer.MAX_VALUE) levelRsrp = 0;
else if (mRsrp >= -95) levelRsrp = SIGNAL_STRENGTH_GREAT;
else if (mRsrp >= -105) levelRsrp = SIGNAL_STRENGTH_GOOD;
else if (mRsrp >= -115) levelRsrp = SIGNAL_STRENGTH_MODERATE;
else levelRsrp = SIGNAL_STRENGTH_POOR;

And maybe we could just follow the AOSP's implementation, uses "rsrp" for lte and takes [-115;-95] as our normalization configuration.

But I am not sure if this works good in real network, could you help to try this if you have available time?

[1] https://github.com/android/platform_hardware_ril/blob/master/include/telephony/ril.h#L714-L717
[2] https://android.googlesource.com/platform/frameworks/base/+/android-4.4.4_r2.0.1/telephony/java/android/telephony/CellSignalStrengthLte.java

Comment 7

[:gerard-majax]

Attachment: Use proper range for LTE signal strength

Comment 8

[:gerard-majax]

Comment on attachment 8498006 [details] [diff] [review]
Use proper range for LTE signal strength

Thanks Edgar. I got mislead with all the AOSP references :)

This should address all your remarks. I also took the liberty to not only use RSRP but also RSSNR, the same way AOSP does. I'm going to test it live on Xperia ZR. If I understand correctly, this would partly fix bug 982013 ?

Comment 9

[:gerard-majax]

Attachment: LTE signal log

Edgar, I added some debug on top of my patch. Here is the result after going out of the office and grabbing something to eat, then coming back.

Comment 10

[Edgar Chen [:edgar]]

Comment on attachment 8498006 [details] [diff] [review]
Use proper range for LTE signal strength

Review of attachment 8498006 [details] [diff] [review]:
-----------------------------------------------------------------

Please see my below comments. Thank you.

::: dom/system/gonk/ril_worker.js
@@ +3554,4 @@
>      info.voice.signalStrength = info.data.signalStrength = signalStrength;
> +    // Use RSRP and RSSNR as in AOSP
> +    let signalLevel;
> +    signalLevel = this._processSignalLevel(signal.lteRSRP, -115, -95);

rsrp is written into the parcel as positive values, need to convert it back to negative values [1].

[1] https://github.com/android/platform_hardware_ril/blob/master/include/telephony/ril.h#L714-L717

@@ +3554,5 @@
>      info.voice.signalStrength = info.data.signalStrength = signalStrength;
> +    // Use RSRP and RSSNR as in AOSP
> +    let signalLevel;
> +    signalLevel = this._processSignalLevel(signal.lteRSRP, -115, -95);
> +    signalLevel = this._processSignalLevel(signal.lteRSSNR, -30, 45);

And I guess you should take the higher one.

signalLevel = Math.max(this._processSignalLevel(signal.lteRSRP * -1, -115, -95), this._processSignalLevel(signal.lteRSSNR, -30, 45));

Comment 11

[Edgar Chen [:edgar]]

And one more thing, we have a marionette test case for signal strength, we may need to modify the test as well.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/mobileconnection/tests/marionette/test_mobile_signal_strength.js

Comment 12

[:gerard-majax]

(In reply to Edgar Chen [:edgar][:echen] from comment #11)
> And one more thing, we have a marionette test case for signal strength, we
> may need to modify the test as well.
> 
> [1]
> http://dxr.mozilla.org/mozilla-central/source/dom/mobileconnection/tests/
> marionette/test_mobile_signal_strength.js

Ha, excellent, I missed this. I'll update accordingly.

Comment 13

[:gerard-majax]

Attachment: Use proper range for LTE signal strength

Comment 14

[:gerard-majax]

Attachment: Use proper range for LTE signal strength r=echen

Comment 15

[:gerard-majax]

Comment on attachment 8501588 [details] [diff] [review]
Use proper range for LTE signal strength r=echen

Edgar, this new patch should address all your previous comments.
Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=acbdbbbf1728

Comment 16

[:gerard-majax]

Edgar, I could test the latest changes and see proper signal level, matching my user experience: poor signal, poor connection (even dropping back to H+).

Comment 17

[Edgar Chen [:edgar]]

Attachment: Patch for test cases.

Hi Alexandre, I found one issue when running the marionette tests, we should filter out invalid rsrp and rssnr [1]. And I also corrected some values in TEST_DATA to fit new LTE signal calculation.

[1] 0x7FFFFFFF denotes invalid value, https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/include/telephony/ril.h#L664-L682

Comment 18

[Edgar Chen [:edgar]]

Thanks for your previous work, Alexandre.

Comment 19

[Edgar Chen [:edgar]]

Attachment: Use proper range for LTE signal strength, v6

Comment 20

[Hsin-Yi Tsai (she/her) [:hsinyi]]

While working on Shinano porting, I always get "null" signal bar even with the v6 patch applied. Investigating ...

== log snippet ==
06-06 21:20:07.499 I/Gecko   (  275): RIL Worker: [0] signal strength: {"gsmSignalStrength":99,"gsmBitErrorRate":0,"cdmaDBM":127,"cdmaECIO":-1,"evdoDBM":-1,"evdoEC     IO":-1,"evdoSNR":-1,"lteSignalStrength":-1,"lteRSRP":99,"lteRSRQ":2147483647,"lteRSSNR":2147483647,"lteCQI":2147483647}

06-06 21:20:07.499 I/Gecko   (  275): RIL Worker: [0]  _processLteSignal: {"gsmSignalStrength":99,"gsmBitErrorRate":0,"cdmaDBM":127,"cdmaECIO":-1,"evdoDBM":-1,"evd     oECIO":-1,"evdoSNR":-1,"lteSignalStrength":-1,"lteRSRP":99,"lteRSRQ":2147483647,"lteRSSNR":2147483647,"lteCQI":2147483647}

06-06 21:20:07.509 I/Gecko   (  275): -*- RadioInterface[0]: Received message from worker: {"voice":{"signalStrength":null,"relSignalStrength":null},"data":{"signa     lStrength":null,"relSignalStrength":null},"rilMessageType":"signalstrengthchange","rilMessageClientId":0}

Comment 21

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - not use lteSignalStrength, instead of using rsrp

seems the root cause is exactly bug 982013 (or bug 977433 comment 11)...

According to AOSP, in LTE, the signal strength should come from "rsrp" instead of "lteSignalStrength". The relSignalStrength is calculated by rssnr or rsrp.

One thing to note that in shinano, I always get |.lteSignalStrength == -1| though ril.h says the valid value is 0 - 31

Comment 22

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Let me continue on Alexandre and Edgar's contribution ;)

Comment 23

[:gerard-majax]

I've changed one of my SIM to a nano and I get LTE. I can confirm the observation regarding the -1 and RSRP values. As far as I could understand, it looks like the -1 is an expected behavior from those blobs.

Comment 24

[:gerard-majax]

Just so that you know, but it seems like just changing lteSignalStrength with lteRSRP gives the proper behavior. However, I have not checked if it's meant to be correct :)

Comment 25

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Alexandre LISSY :gerard-majax from comment #24)
> Just so that you know, but it seems like just changing lteSignalStrength
> with lteRSRP gives the proper behavior. However, I have not checked if it's
> meant to be correct :)

Good to know that, thanks for the kind heads up. This is definitely the direction my WIP goes!

Comment 26

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Just talked to Arthur regarding the idea of deprecating mobileconnection.signalStrength. Though currently gaia doesn't use that, he had concern that eventually we would want to expose real values to apps, such as utility. I realize that utility apps on market do display the details such as rsrp, rsrq. So in the long term, exposing signalStrength for gsm in addition to rsrp, rsrq, etc for cdma/wcdma could still be the way. 

In sum, in this bug, I plan to not change any WebAPI, and still leave that on bug 982013. What I am going to do is very simple, i.e. use "lteRsrp" for mobileConnection.signalStrength, and use "rsrp or rssnr" for .relSignalStrenght as my WIP does. Sounds good?

Comment 27

[:gerard-majax]

Hsin-Yi, that's exactly what I currently have locally: my previous patch with signalStrength computed from lteRSRP :).

Comment 28

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: patch (v7) - consider lteRsrp and lteRssnr in addition to lteSignalStrength

In this revision, I basically follow the logic AOSP does, i.e. determine relSignalStrength (level) firstly based on rsrp/rssnr, then on lteSignalStrength. Also, as AOSP, I simply assign "rsrp" to signalStrength.

Note that in this patch, I use rather loose checks. For example, I don't check the boundary (44, 140) defined in ril.h for rsrp. I only check if it's 0x7FFFFFFF. The reason is I always get a value out of boundary on shinano that has its own definitions. :(

Hi Edgar,
I will need to work on marionette tests. But could you provide feedback first to see if you have concerns on the direction? Thank you :)

Comment 29

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai (Chinese new year OOO Feb. 18 ~ Feb. 27) [:hsinyi] from comment #28)
> Created attachment 8564985 [details] [diff] [review]
> patch (v7) - consider lteRsrp and lteRssnr in addition to lteSignalStrength
> 
> In this revision, I basically follow the logic AOSP does, i.e. determine
> relSignalStrength (level) firstly based on rsrp/rssnr, then on
> lteSignalStrength. Also, as AOSP, I simply assign "rsrp" to signalStrength.
> 

AOSP reference: http://androidxref.com/5.0.0_r2/xref/frameworks/base/telephony/java/android/telephony/SignalStrength.java#739

> Note that in this patch, I use rather loose checks. For example, I don't
> check the boundary (44, 140) defined in ril.h for rsrp. I only check if it's
> 0x7FFFFFFF. The reason is I always get a value out of boundary on shinano
> that has its own definitions. :(
> 
> Hi Edgar,
> I will need to work on marionette tests. But could you provide feedback
> first to see if you have concerns on the direction? Thank you :)

Comment 30

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8564985 [details] [diff] [review]
patch (v7) - consider lteRsrp and lteRssnr in addition to lteSignalStrength

Review of attachment 8564985 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/ril_worker.js
@@ +3417,5 @@
>  
> +    // Referring to AOSP, use lteRSRP for signalStrength in dBm.
> +    let signalStrength = (signal.lteRSRP === undefined || signal.lteRSRP === 0x7FFFFFFF) ?
> +                         null : signal.lteRSRP;
> +    info.voice.signalStrength = info.data.signalStrength = signal.lteRSRP;

My fault, I should replace |signal.lteRSRP| with signalStrength. Will address in the next version.

@@ +3445,5 @@
> +      info.voice.relSignalStrength = info.data.relSignalStrength = level;
> +      return info;
> +    }
> +
> +    if (signal.lteSignalStrength !== undefined) {

I plan to make this a little bit strict -- check the valid values 0 - 31 defined in ril.h

Comment 31

[Edgar Chen [:edgar]]

Comment on attachment 8564985 [details] [diff] [review]
patch (v7) - consider lteRsrp and lteRssnr in addition to lteSignalStrength

Review of attachment 8564985 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thank you.

::: dom/system/gonk/ril_worker.js
@@ +3421,5 @@
> +    info.voice.signalStrength = info.data.signalStrength = signal.lteRSRP;
> +
> +    // Referring to AOSP, first determine signalLevel based on RSRP and RSSNR,
> +    // then on lteSignalStrength if RSRP and RSSNR are invalid.
> +    let rsrpLevel = -1, rssnrLevel = -1;

Nit: One declaration per line.

let rsrpLevel = -1;
let rssnrLevel = -1;

Comment 32

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: patch (v8) - consider lteRsrp and lteRssnr in addition to lteSignalStrength

review comments addressed.

Comment 33

[:gerard-majax]

Hsin-Yi, I'm not sure your patch is currently good: I gave it a try, and the signal was constantly reported as 3 bars on LTE yesterday when I run my test, whatever the place, absolutely no change. Even in places where the signal would drop for sure.

Comment 34

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Alexandre LISSY :gerard-majax from comment #33)
> Hsin-Yi, I'm not sure your patch is currently good: I gave it a try, and the
> signal was constantly reported as 3 bars on LTE yesterday when I run my
> test, whatever the place, absolutely no change. Even in places where the
> signal would drop for sure.

Alexandre,
Could you provide me ril debug messages? I could take a look! Thank you.

Comment 35

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 1 (v9) - consider lteRsrp and lteRssnr in addition to lteSignalStrength

Compared to v8, I did modifications in the revision:
1) I still follow 27.007 clause 8.69 to determine the valid values of "lteSingnalStrength." That is, the valid numbers are within 0 - 63.
2) Based on [1], I slightly modified the normalization formula for rsrpLevel and rssnrLevel. The range for processing rsrpLevel is now -115 ~ -85, and that for rssnrLevel is -30 ~ 130. The previous formula seems too coarse-grained that might explain why Alexendra always gets 3 bars. 


[1] http://androidxref.com/5.0.0_r2/xref/frameworks/base/telephony/java/android/telephony/SignalStrength.java#739

Comment 36

[Hsin-Yi Tsai (she/her) [:hsinyi]]

treeherder looks good https://treeherder.mozilla.org/#/jobs?repo=try&revision=19d592c7cc6e

Comment 37

[:gerard-majax]

I'll give a try to the new patch :)

Comment 38

[:gerard-majax]

For now I see only two bars, I'll check over time and places. Meanwhile, I have an issue with ril.debugging.enabled: when I set it to true in my profile's prefs.js, it's working for a couple of seconds and then it stops working. And when I re-check with ./edit-prefs.sh the user_pref line disappeared.

Comment 39

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Alexandre LISSY :gerard-majax from comment #38)
> For now I see only two bars, I'll check over time and places. Meanwhile, I
> have an issue with ril.debugging.enabled: when I set it to true in my
> profile's prefs.js, it's working for a couple of seconds and then it stops
> working. And when I re-check with ./edit-prefs.sh the user_pref line
> disappeared.

I don't have ideas, sorry.

But from now, you could simply turn on ril debugger via Settings App --> Developer --> RIL output in ADB

Comment 40

[:gerard-majax]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #39)
> (In reply to Alexandre LISSY :gerard-majax from comment #38)
> > For now I see only two bars, I'll check over time and places. Meanwhile, I
> > have an issue with ril.debugging.enabled: when I set it to true in my
> > profile's prefs.js, it's working for a couple of seconds and then it stops
> > working. And when I re-check with ./edit-prefs.sh the user_pref line
> > disappeared.
> 
> I don't have ideas, sorry.
> 
> But from now, you could simply turn on ril debugger via Settings App -->
> Developer --> RIL output in ADB

Oh nice, I did check there but I missed the entry. It's disabled even when I turned the pref on, that's probably the reason why I have this behavior.

Comment 41

[:gerard-majax]

Attachment: ril-signal-z3c_lte_edge_hsdpa.log.gz with patch v9

Here is logcat -v threadtime with RIL debugging enabled. This was taken during some minutes, in the train: LTE, EDGE and HSDPA are there.

Signal level is totally wrong for all those:
 - LTE is stuck with two bars, and never moving. According to the logs, I see that there are variations that we should see in the statusbar and in the relSignalStrength
 - EDGE/HSDPA is the same, but the level is constantly stuck at three bars instead of two

Comment 42

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Alexandre LISSY :gerard-majax from comment #41)
> Created attachment 8569707 [details]
> ril-signal-z3c_lte_edge_hsdpa.log.gz with patch v9
> 
> Here is logcat -v threadtime with RIL debugging enabled. This was taken
> during some minutes, in the train: LTE, EDGE and HSDPA are there.
> 
> Signal level is totally wrong for all those:
>  - LTE is stuck with two bars, and never moving. According to the logs, I
> see that there are variations that we should see in the statusbar and in the
> relSignalStrength


Thanks for testing.

The normalization formula mozril uses isn't exactly the same as AOSP. If we adopt AOSP implementation, in your situation, you could get 1 or 2 bars. After discussing with Edgar, I will change our implementation. Hopefully the code could detect fine changes.

>  - EDGE/HSDPA is the same, but the level is constantly stuck at three bars
> instead of two

I doubt if shinano follows the definition in ril.h. According to the log, even the device camped on gsm/umts network (not lte), the lte signal attributes are still valid that is however not what our code expects to rely on. Seems shinano uses |"lteRSRP":99| to indicate an invalid value but 99 is supposed to mean "valid" per ril.h.

To adapt to shinano behaviour, I will check "radioTech" before parsing signal attributes. Let's see if it works fine.

Comment 43

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8569633 [details] [diff] [review]
part 1 (v9) - consider lteRsrp and lteRssnr in addition to lteSignalStrength

Review of attachment 8569633 [details] [diff] [review]:
-----------------------------------------------------------------

Let me keep working for comment 42.

Comment 44

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #42)
> (In reply to Alexandre LISSY :gerard-majax from comment #41)
> > Created attachment 8569707 [details]
> > ril-signal-z3c_lte_edge_hsdpa.log.gz with patch v9
> > 
> > Here is logcat -v threadtime with RIL debugging enabled. This was taken
> > during some minutes, in the train: LTE, EDGE and HSDPA are there.
> > 
> > Signal level is totally wrong for all those:
> >  - LTE is stuck with two bars, and never moving. According to the logs, I
> > see that there are variations that we should see in the statusbar and in the
> > relSignalStrength
> 
> 
> Thanks for testing.
> 
> The normalization formula mozril uses isn't exactly the same as AOSP. If we
> adopt AOSP implementation, in your situation, you could get 1 or 2 bars.
> After discussing with Edgar, I will change our implementation. Hopefully the
> code could detect fine changes.
> 
> >  - EDGE/HSDPA is the same, but the level is constantly stuck at three bars
> > instead of two
> 
> I doubt if shinano follows the definition in ril.h. According to the log,
> even the device camped on gsm/umts network (not lte), the lte signal
> attributes are still valid that is however not what our code expects to rely
> on. Seems shinano uses |"lteRSRP":99| to indicate an invalid value but 99 is
> supposed to mean "valid" per ril.h.
> 
> To adapt to shinano behaviour, I will check "radioTech" before parsing
> signal attributes. Let's see if it works fine.

Observation on nexus-5: if the device camped on gsm/umts network (not lte), the value of "lteRSRP" was set to 0x7FFFFFF as defined in ril.h.

Comment 45

[:gerard-majax]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #42)

[...]

> 
> >  - EDGE/HSDPA is the same, but the level is constantly stuck at three bars
> > instead of two
> 
> I doubt if shinano follows the definition in ril.h. According to the log,
> even the device camped on gsm/umts network (not lte), the lte signal
> attributes are still valid that is however not what our code expects to rely
> on. Seems shinano uses |"lteRSRP":99| to indicate an invalid value but 99 is
> supposed to mean "valid" per ril.h.
> 
> To adapt to shinano behaviour, I will check "radioTech" before parsing
> signal attributes. Let's see if it works fine.

I'm sorry, but when I see this, I don't understand.

> 02-26 10:37:29.945  1959  2007 I Gecko   : RIL Worker: [0] signal strength: {"gsmSignalStrength":4,"gsmBitErrorRate":0,"cdmaDBM":105,"cdmaECIO":-1,"evdoDBM":-1,"evdoECIO":-1,"evdoSNR":-1,"lteSignalStrength":-1,"lteRSRP":99,"lteRSRQ":2147483647,"lteRSSNR":2147483647,"lteCQI":2147483647}
> 02-26 10:37:29.945  1959  2007 I Gecko   : RIL Worker: [0] Queuing signal network info message: {"voice":{"signalStrength":99,"relSignalStrength":53},"data":{"signalStrength":99,"relSignalStrength":53},"rilMessageType":"signalstrengthchange"}

relSignalStrength is a 0-100% value, as far as I understand. Raw value is 4 in this case. 
This means we are passing -113+2*4=-105 to |this._processSignalLevel(signalStrength, -110, -85);|

So, as far as I understand, we will produce a percentage value within the range of [-110;-85], with -110 indicating a very bad signal and -85 indicating a very good one. With -105, I'm not sure I should get relSignalStrength=53.

And with:
> 02-26 10:26:50.465  1959  2007 I Gecko   : RIL Worker: [0] signal strength: {"gsmSignalStrength":18,"gsmBitErrorRate":0,"cdmaDBM":78,"cdmaECIO":-1,"evdoDBM":-1,"evdoECIO":-1,"evdoSNR":-1,"lteSignalStrength":-1,"lt
eRSRP":99,"lteRSRQ":2147483647,"lteRSSNR":2147483647,"lteCQI":2147483647}
> 02-26 10:26:50.475  1959  1959 I Gecko   : -*- RadioInterface[0]: Received message from worker: {"voice":{"signalStrength":99,"relSignalStrength":53},"data":{"signalStrength":99,"relSignalStrength":53},"rilMessage
Type":"signalstrengthchange","rilMessageClientId":0}

We have 18, so we are calling processSignalLevel() with -113+2*18=-77, which should get us a very strong signal.

Comment 46

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Alexandre LISSY :gerard-majax from comment #45)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #42)
> 
> [...]
> 
> > 
> > >  - EDGE/HSDPA is the same, but the level is constantly stuck at three bars
> > > instead of two
> > 
> > I doubt if shinano follows the definition in ril.h. According to the log,
> > even the device camped on gsm/umts network (not lte), the lte signal
> > attributes are still valid that is however not what our code expects to rely
> > on. Seems shinano uses |"lteRSRP":99| to indicate an invalid value but 99 is
> > supposed to mean "valid" per ril.h.
> > 
> > To adapt to shinano behaviour, I will check "radioTech" before parsing
> > signal attributes. Let's see if it works fine.
> 
> I'm sorry, but when I see this, I don't understand.
> 
> > 02-26 10:37:29.945  1959  2007 I Gecko   : RIL Worker: [0] signal strength: {"gsmSignalStrength":4,"gsmBitErrorRate":0,"cdmaDBM":105,"cdmaECIO":-1,"evdoDBM":-1,"evdoECIO":-1,"evdoSNR":-1,"lteSignalStrength":-1,"lteRSRP":99,"lteRSRQ":2147483647,"lteRSSNR":2147483647,"lteCQI":2147483647}
> > 02-26 10:37:29.945  1959  2007 I Gecko   : RIL Worker: [0] Queuing signal network info message: {"voice":{"signalStrength":99,"relSignalStrength":53},"data":{"signalStrength":99,"relSignalStrength":53},"rilMessageType":"signalstrengthchange"}
> 
> relSignalStrength is a 0-100% value, as far as I understand. Raw value is 4
> in this case. 
> This means we are passing -113+2*4=-105 to
> |this._processSignalLevel(signalStrength, -110, -85);|
> 
> So, as far as I understand, we will produce a percentage value within the
> range of [-110;-85], with -110 indicating a very bad signal and -85
> indicating a very good one. With -105, I'm not sure I should get
> relSignalStrength=53.
> 

The logic is: if the radioTech is in GsmFamily, i.e. gsm, umts, lte, then we will check lteSignal attributes first. And in this case, lteRSRP (99) was viewed a valid number per ril.h. So, the code calculated relSignalStrength based on lteRSRP, instead of gsmSignalStrength.

> And with:
> > 02-26 10:26:50.465  1959  2007 I Gecko   : RIL Worker: [0] signal strength: {"gsmSignalStrength":18,"gsmBitErrorRate":0,"cdmaDBM":78,"cdmaECIO":-1,"evdoDBM":-1,"evdoECIO":-1,"evdoSNR":-1,"lteSignalStrength":-1,"lt
> eRSRP":99,"lteRSRQ":2147483647,"lteRSSNR":2147483647,"lteCQI":2147483647}
> > 02-26 10:26:50.475  1959  1959 I Gecko   : -*- RadioInterface[0]: Received message from worker: {"voice":{"signalStrength":99,"relSignalStrength":53},"data":{"signalStrength":99,"relSignalStrength":53},"rilMessage
> Type":"signalstrengthchange","rilMessageClientId":0}
> 
> We have 18, so we are calling processSignalLevel() with -113+2*18=-77, which
> should get us a very strong signal.

Same as my explanation above.

Per AOSP reference, modem/rild should have reported us 2147483647 (0x7fffffff) in this case since now the network is not lte. The device behaviour on shinano seems not exactly the same as AOSP/Nexus.

Is it clearer to you now?

Comment 47

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #46)
> > And with:
> > > 02-26 10:26:50.465  1959  2007 I Gecko   : RIL Worker: [0] signal strength: {"gsmSignalStrength":18,"gsmBitErrorRate":0,"cdmaDBM":78,"cdmaECIO":-1,"evdoDBM":-1,"evdoECIO":-1,"evdoSNR":-1,"lteSignalStrength":-1,"lt
> > eRSRP":99,"lteRSRQ":2147483647,"lteRSSNR":2147483647,"lteCQI":2147483647}
> > > 02-26 10:26:50.475  1959  1959 I Gecko   : -*- RadioInterface[0]: Received message from worker: {"voice":{"signalStrength":99,"relSignalStrength":53},"data":{"signalStrength":99,"relSignalStrength":53},"rilMessage
> > Type":"signalstrengthchange","rilMessageClientId":0}
> > 
> > We have 18, so we are calling processSignalLevel() with -113+2*18=-77, which
> > should get us a very strong signal.
> 
> Same as my explanation above.
> 
> Per AOSP reference, modem/rild should have reported us 
> (0x7fffffff) 

reported us "lteRSRP" with the value 2147483647 ...

> in this case since now the network is not lte. The device
> behaviour on shinano seems not exactly the same as AOSP/Nexus.
> 
> Is it clearer to you now?

Comment 48

[:gerard-majax]

Yes, thanks, that's the point I was missing to understand :). I have heard from people of the FreeXperia team that current blobs for this device have some little differences around this kind of stuff, compared to AOSP.

Comment 49

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Alexandre pointed me to https://github.com/CyanogenMod/android_device_sony_shinano-common/commit/8dd386021b3182933fac8ec141492b31af873ec5

Looks shinano has it's parcel definition. We might need a quirk so that RIL code could read signal attributes varied with devices.

Comment 50

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 2 - WIP - shinano-specific

This is an ugly WIP to prove the concept.

Comment 51

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Alexandre, with part2 WIP, I could get signal attributes with valid values defined in ril.h. Could you see how things go with additional part2 WIP?

Comment 52

[:gerard-majax]

Thanks. I'm testing. For now, under HSDPA coverage, signal level is consistent.

Comment 53

[:gerard-majax]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #51)
> Alexandre, with part2 WIP, I could get signal attributes with valid values
> defined in ril.h. Could you see how things go with additional part2 WIP?

So, as expected with this hack, we get consistent network signal strength reported: values are changing, in a way that makes sense regarding each radio technology, current coverage, and my moving sped.

Comment 54

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Alexandre LISSY :gerard-majax from comment #53)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #51)
> > Alexandre, with part2 WIP, I could get signal attributes with valid values
> > defined in ril.h. Could you see how things go with additional part2 WIP?
> 
> So, as expected with this hack, we get consistent network signal strength
> reported: values are changing, in a way that makes sense regarding each
> radio technology, current coverage, and my moving sped.

Yay :)

I will provide a formal patch in addition to PR for adding a quirk. I need to prepare PR to both device_shinano and device_aries, right?

Comment 55

[:gerard-majax]

No, since those devices are really very very very similar, we have a common repository for the shinano platform they are based on. So you should add your quirks against the device-shinano-common repository, as in https://github.com/mozilla-b2g/device-shinano-common/blob/master/device.mk#L16

While you're at it, if you see any quirks property that are missing, feel free to add them :)

Comment 56

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Quirks to add for shinano: signalStrength and have_ipv6

Comment 57

[:gerard-majax]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #56)
> Quirks to add for shinano: signalStrength and have_ipv6

Are you doing the patch ? In which bug ?

Comment 58

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Alexandre LISSY :gerard-majax from comment #57)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #56)
> > Quirks to add for shinano: signalStrength and have_ipv6
> 
> Are you doing the patch ? In which bug ?

I am going to do that in this bug. I wasn't able to do that yesterday but will try to provide an update today.

Comment 59

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #58)
> (In reply to Alexandre LISSY :gerard-majax from comment #57)
> > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #56)
> > > Quirks to add for shinano: signalStrength and have_ipv6
> > 
> > Are you doing the patch ? In which bug ?
> 
> I am going to do that in this bug. I wasn't able to do that yesterday but
> will try to provide an update today.

shinano quirks https://github.com/hsinyi/device-shinano-common/commit/ba0395668b240b57b27623ed5cd9073934e7f350

Comment 60

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: patch (v10) - consider lteRsrp and lteRssnr in addition to lteSignalStrength

Depends on a new ril quirk to parse shinano-specific parcels

Comment 61

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: PR to shinano-common

Comment 62

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8572414 [details] [diff] [review]
patch (v10) - consider lteRsrp and lteRssnr in addition to lteSignalStrength

Review of attachment 8572414 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Edgar,
With attachment 8573122 [details] [review] we can parse signal parcel on shinano correctly. Could you help review this? Thank you thank you!

Comment 63

[Edgar Chen [:edgar]]

Comment on attachment 8572414 [details] [diff] [review]
patch (v10) - consider lteRsrp and lteRssnr in addition to lteSignalStrength

Review of attachment 8572414 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thank you.

Comment 64

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8573122 [details] [review]
PR to shinano-common

Hi Michael,
I add two ril quirks for shinano and aries. Please help review it, thank you.

Comment 65

[:gerard-majax]

> $ adb shell getprop | grep ro.moz.ril
> [ro.moz.ril.ipv6]: [true]
> [ro.moz.ril.signal_extra_int]: [true]

And now signal seems ok for LTE and HSPA+ !

Comment 66

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Thank you all for the review and investigation. Try result looks good. Will be checking in the patch!

Comment 67

[Hsin-Yi Tsai (she/her) [:hsinyi]]

https://hg.mozilla.org/integration/b2g-inbound/rev/d971715592f0

Comment 68

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d971715592f0

Comment 69

[:gerard-majax]

https://github.com/mozilla-b2g/device-shinano-common/commit/1e35c81a7325de586e277b4bb8852809eb8414e3

Sorry for forgetting this, I was waiting for you to merge it :)