835237 - B2G CDMA: Add signal notification function for CDMA/EVDO network

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Ken Chang[:kenkai]]

The meaning of signal quality values of CDMA network is different from the one of GSM network in CDMA specification. 
In CDMA specification, C.S0017, defines the signal values are 0-31 and 99, but it doesn't define the unit of these signal values, which means we can not know what is the exact meaning of these signal values, and moreover, this specification doesn't define received signal quality measure values for EVDO.
But in order to let upper layer knows the signal value of current CDMA/EVDO network, we still need create the signal notification function for CDMA/EVDO network.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

In Bug 729173, there are already some discussions about signal strength for GSM and CDMA.

Comment 2

[Ken Chang[:kenkai]]

Attachment: Add nsIDOMMozMobileSignalInfo

Comment 3

[Ken Chang[:kenkai]]

Attachment: Getting all rf signal values from radiointerface

Getting all rf signal values from radiointerfacelayer

Comment 4

[Ken Chang[:kenkai]]

Attachment: Setting all rf signal values in radiointerfacelayer

Setting all rf signal values in radiointerfacelayer

Comment 5

[Ken Chang[:kenkai]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #1)
> In Bug 729173, there are already some discussions about signal strength for
> GSM and CDMA.

Thanks for the information. 
I think we can provide all signal values to upper layer and let UX designer freely decide how many signal strength bars to be showed in UX.

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Ken Chang from comment #5)
> (In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #1)
> > In Bug 729173, there are already some discussions about signal strength for
> > GSM and CDMA.
> 
> Thanks for the information. 
> I think we can provide all signal values to upper layer and let UX designer
> freely decide how many signal strength bars to be showed in UX.

And I think the original thread has the same conclusion: just let UI designers decide what they want. We just provide every number you can find on specification.

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 709321 [details] [diff] [review]
Add nsIDOMMozMobileSignalInfo

>diff --git a/dom/network/interfaces/nsIDOMMobileConnection.idl b/dom/network/interfaces/nsIDOMMobileConnection.idl
>--- a/dom/network/interfaces/nsIDOMMobileConnection.idl
>+++ b/dom/network/interfaces/nsIDOMMobileConnection.idl
>@@ -370,16 +371,21 @@ interface nsIDOMMozMobileConnectionInfo 
>    */
>   readonly attribute jsval relSignalStrength;

A lot trailing spaces :(
And do we still need |signalStrength| and |relSignalStrength|?
Please also include the max/min values in IDL comments.

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

Include all people joined signalStrength discuss on bug 729173.

Comment 9

[Mounir Lamouri (:mounir)]

Comment on attachment 709321 [details] [diff] [review]
Add nsIDOMMozMobileSignalInfo

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

Shouldn't you move all signal related stuff from nsIDOMMobileConnection to nsIDOMMozMobileSignalInfo?

Comment 10

[Ken Chang[:kenkai]]

(In reply to Mounir Lamouri (:mounir) from comment #9)
> Comment on attachment 709321 [details] [diff] [review]
> Add nsIDOMMozMobileSignalInfo
> 
> Review of attachment 709321 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Shouldn't you move all signal related stuff from nsIDOMMobileConnection to
> nsIDOMMozMobileSignalInfo?

Because we had released firefoxOS V1.0 last month, I keep the |signalStrength| and |relSignalStrength| for backward compatible.

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to Ken Chang from comment #10)
> (In reply to Mounir Lamouri (:mounir) from comment #9)
> > Comment on attachment 709321 [details] [diff] [review]
> > Add nsIDOMMozMobileSignalInfo
> > 
> > Review of attachment 709321 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Shouldn't you move all signal related stuff from nsIDOMMobileConnection to
> > nsIDOMMozMobileSignalInfo?
> 
> Because we had released firefoxOS V1.0 last month, I keep the
> |signalStrength| and |relSignalStrength| for backward compatible.

This API is for Certified Apps only, right? We should just make sure that Gaia trunk is updated appropriately. I don't think we should care too much of backward compatibility for APIs that are not used by third party apps.

Comment 12

[Mounir Lamouri (:mounir)]

Comment on attachment 709321 [details] [diff] [review]
Add nsIDOMMozMobileSignalInfo

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

Cancelling review based on previous comment.

Really sorry for the delay.

Comment 13

[Ken Chang[:kenkai]]

(In reply to Mounir Lamouri (:mounir) from comment #11)
> (In reply to Ken Chang from comment #10)
> > (In reply to Mounir Lamouri (:mounir) from comment #9)
> > > Comment on attachment 709321 [details] [diff] [review]
> > > Add nsIDOMMozMobileSignalInfo
> > > 
> > > Review of attachment 709321 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Shouldn't you move all signal related stuff from nsIDOMMobileConnection to
> > > nsIDOMMozMobileSignalInfo?
> > 
> > Because we had released firefoxOS V1.0 last month, I keep the
> > |signalStrength| and |relSignalStrength| for backward compatible.
> 
> This API is for Certified Apps only, right? We should just make sure that
> Gaia trunk is updated appropriately. I don't think we should care too much
> of backward compatibility for APIs that are not used by third party apps.

I have created the bug 842220 for tracking this implementation in gaia.

Comment 14

[Ken Chang[:kenkai]]

Attachment: Add nsIDOMMozMobileSignalInfo - WIP V2

Add |signal| and remove |signalStrength| and |relSignalStrength|.

Comment 15

[Ken Chang[:kenkai]]

Attachment: Getting all rf signal values from radiointerface

Comment 16

[Ken Chang[:kenkai]]

Attachment: Updating all rf signal values in radiointerfacelayer

Comment 17

[Ken Chang[:kenkai]]

Attachment: Updating signal level in BT for new signal architecture - WIP V1.

Comment 18

[Shian-Yow Wu [:swu]]

Comment on attachment 717790 [details] [diff] [review]
Updating all rf signal values in radiointerfacelayer

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

Thanks for the patch.  Some values are different from the comment, could you check it?

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1041,5 @@
> +    }
> +    else if (rfValue < -startValue && rfValue >= -endValue) {
> +      return rfValue;
> +    }
> +    return -(endValue+1);

Nit: space around operator, please.

@@ +1049,5 @@
>      let voiceInfo = this.rilContext.voice;
> +    let dataInfo = this.rilContext.data;
> +    let voiceSignal = voiceInfo.signal;
> +    let dataSignal = dataInfo.signal;
> +    let updateRFValues = false;

updateRFValues is set but not been used, what is the purpose of this value?

@@ +1050,5 @@
> +    let dataInfo = this.rilContext.data;
> +    let voiceSignal = voiceInfo.signal;
> +    let dataSignal = dataInfo.signal;
> +    let updateRFValues = false;
> +    let rfValue;

Adding an empty line here improves readability.

@@ +1060,5 @@
> +              : -255;
> +    if (voiceInfo.gsmSignal != rfValue) {
> +      voiceSignal.gsmSignal = dataSignal.gsmSignal = rfValue;
> +      updateRFValues = true;
> +    }

Adding an empty line between setting of each signal value in this function improves readability.

@@ +1072,5 @@
> +    }
> +    /* In 3GPP2 C.S0005, receive power = 10 * log(SD) + 120.
> +     * But it is negative, its range should be -1 ~ -120
> +     */
> +    rfValue = this.validateRFValue(message.cdmaDBM, 0, 120);

Some values are checked with >= against start value, and values passed to this.validateRFValue() is checked with > against (start value - 1).  Why not use same way for consistency?

@@ +1082,5 @@
> +     * which means its range is -0.5 ~ -31.5db. And vendors usually change
> +     * this value to be db*10, so the ecio value is -5 ~ - 315 in
> +     * vendor's definition.
> +     */
> +    rfValue = this.validateRFValue(message.cdmaECIO, 0, 315);

Wrong start value, according to comment above.

@@ +1100,5 @@
> +     * which means its range is -0.5 ~ -31.5db. And vendors usually change
> +     * this value to be db*10, so the ecio value is -5 ~ - 315 in
> +     * vendor's definition.
> +     */
> +    rfValue = this.validateRFValue(message.evdoECIO, 0, 315);

Wrong start value, according to comment above.

@@ +1116,5 @@
> +    }
> +    /* Valid values are -43 ~ -140 as defined in TS 27.007.
> +     * And the value, -255, is to represent no-service.
> +     */
> +    rfValue = (message.lteSignalStrength >= 0 && 

Nit: trailing space.

@@ +1127,5 @@
> +    }
> +    // In TS 36.133, -44 dbm >= RSRP >= -140 dbm
> +    rfValue = this.validateRFValue(message.lteRSRP, 43, 140);
> +    if (voiceSignal.lteRSRP != rfValue) {
> +      voiceSignal.lteRSRP= dataSignal.lteRSRP = rfValue;

Nit: add space before =.

Comment 19

[Shian-Yow Wu [:swu]]

Comment on attachment 717788 [details] [diff] [review]
Getting all rf signal values from radiointerface

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

::: dom/system/gonk/RILContentHelper.js
@@ +401,1 @@
>        return;

In case srcInfo.network is null, code will return here and the destInfo.signal won't be updated.

Comment 20

[Ken Chang[:kenkai]]

(In reply to Shian-Yow Wu from comment #19)
> Comment on attachment 717788 [details] [diff] [review]
> Getting all rf signal values from radiointerface
> 
> Review of attachment 717788 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RILContentHelper.js
> @@ +401,1 @@
> >        return;
> 
> In case srcInfo.network is null, code will return here and the
> destInfo.signal won't be updated.

Thanks for your review. I will modify it.

Comment 21

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

Comment on attachment 717788 [details] [diff] [review]
Getting all rf signal values from radiointerface

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

Agree with Shianyow's comment.

Comment 22

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

Comment on attachment 717790 [details] [diff] [review]
Updating all rf signal values in radiointerfacelayer

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

ril_worker.js has mentioned valid values of dbm, EcO, RSRP, RSRQ, SNR. Please be consistent with those, which are defined in hardware/ril/include/telephony/ril.h.

Also, what is relative signal strength which was designed for signal status bar? Looks like that you're gonna abandon that but make signal strength refactored? Not sure I got your idea right, but if you are going to do what I guess, then I would say I am inclined to the the method we have now, i.e. return signal strength that we received from RIL but keep relSignalStrength as the reference for status bar. Maybe we should have discussion about this API change first.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1033,5 @@
>  
> +  /*
> +   * The values of dbm, EcIo, RSRP, and RSRQ, should be negative.
> +   * And we set the value to be -(endValue+1) for no service.
> +   */

The comment is really ambiguous to me. What does the parameter 'rfValue' mean?

Also, looks like startValue and negativeValue should be positive? Please give a comment.

@@ +1038,5 @@
> +  validateRFValue: function validateRFValue(rfValue, startValue, endValue) {
> +    if (rfValue > startValue && rfValue <= endValue) {
> +      return -rfValue;
> +    }
> +    else if (rfValue < -startValue && rfValue >= -endValue) {

nit: put 'else if' right after the previous "}"

This article might help.
https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style

@@ +1051,5 @@
> +    let voiceSignal = voiceInfo.signal;
> +    let dataSignal = dataInfo.signal;
> +    let updateRFValues = false;
> +    let rfValue;
> +    /* Valid values are -51 ~ -113 as defined in TS 27.007.

Valid values of 'gsmDBM' are ... ...

@@ +1061,5 @@
> +    if (voiceInfo.gsmSignal != rfValue) {
> +      voiceSignal.gsmSignal = dataSignal.gsmSignal = rfValue;
> +      updateRFValues = true;
> +    }
> +    // Valid values are (0-7, 99) as defined in TS 27.007.

Valid values of 'gsmBER' are ...

@@ +1069,5 @@
> +    if (voiceSignal.gsmBER != rfValue) {
> +      voiceSignal.gsmBER = dataSignal.gsmBER = rfValue;
> +      updateRFValues = true;
> +    }
> +    /* In 3GPP2 C.S0005, receive power = 10 * log(SD) + 120.

Does 'receive power' stand for 'cdmaDBM'? If yes, please replace with that to make the comment consistent with the code.

@@ +1072,5 @@
> +    }
> +    /* In 3GPP2 C.S0005, receive power = 10 * log(SD) + 120.
> +     * But it is negative, its range should be -1 ~ -120
> +     */
> +    rfValue = this.validateRFValue(message.cdmaDBM, 0, 120);

why is startValue 0 instead of 1?

@@ +1077,5 @@
> +    if (voiceSignal.cdmaDBM != rfValue) {
> +      voiceSignal.cdmaDBM = dataSignal.cdmaDBM = rfValue;
> +      updateRFValues = true;
> +    }
> +    /* In 3GPP2 C.S0005, this value = - 0.5 * X, where X's value is 1 ~ 63

s/this value/cdmaCEIO

@@ +1087,5 @@
> +    if (voiceSignal.cdmaECIO != rfValue) {
> +      voiceSignal.cdmaECIO = dataSignal.cdmaECIO = rfValue;
> +      updateRFValues = true;
> +    }
> +    /* In 3GPP2 C.S0005, receive power = 10 * log(SD) + 120.

s/receive power/evdoDBM

@@ +1095,5 @@
> +    if (voiceSignal.evdoDBM != rfValue) {
> +      voiceSignal.evdoDBM = dataSignal.evdoDBM = rfValue;
> +      updateRFValues = true;
> +    }
> +    /* In 3GPP2 C.S0005, this value = - 0.5 * X, where X's value are 1 ~ 63

s/this value/evdoECIO

@@ +1117,5 @@
> +    /* Valid values are -43 ~ -140 as defined in TS 27.007.
> +     * And the value, -255, is to represent no-service.
> +     */
> +    rfValue = (message.lteSignalStrength >= 0 && 
> +               message.lteSignalStrength <= 97)

Where does 97 come from?

Comment 23

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #22)
> Comment on attachment 717790 [details] [diff] [review]
> Updating all rf signal values in radiointerfacelayer
> 
> Review of attachment 717790 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ril_worker.js has mentioned valid values of dbm, EcO, RSRP, RSRQ, SNR.
> Please be consistent with those, which are defined in
> hardware/ril/include/telephony/ril.h.
> 
> Also, what is relative signal strength which was designed for signal status
> bar? Looks like that you're gonna abandon that but make signal strength
> refactored? Not sure I got your idea right, but if you are going to do what
> I guess, then I would say I am inclined to the the method we have now, i.e.
> return signal strength that we received from RIL but keep relSignalStrength
> as the reference for status bar. Maybe we should have discussion about this
> API change first.
> 
To be clear, I don't insist we should keep 'relSingalStrength.' What I meant was, if we do need to provide 'refactored' signal strength rather than what we received from rild, then I vote for additional relSignalStrength. Otherwise, simply as comment 6 said, we just provide everything truly on specification.

Comment 24

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 717790 [details] [diff] [review]
Updating all rf signal values in radiointerfacelayer

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

Except for the coding style is mess,
why do you do these things in RadioInterfaceLayer?
Unless you got strong reason, otherwise we should move those code in ril_worker.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +246,5 @@
> +                                evdoECIO: null,
> +                                evdoSNR: null,
> +                                lteSignal: null,
> +                                lteRSRP: null,
> +                                lteSNR: null}},

The change here is strange to me.
We're writing Javascript, it's not Java nor cpp, why do you want to initialize every member here?
Also these members won't exist at the same time.

Comment 25

[Ken Chang[:kenkai]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #22)
Thanks for your review.

> @@ +1117,5 @@
> > +    /* Valid values are -43 ~ -140 as defined in TS 27.007.
> > +     * And the value, -255, is to represent no-service.
> > +     */
> > +    rfValue = (message.lteSignalStrength >= 0 && 
> > +               message.lteSignalStrength <= 97)
> 
> Where does 97 come from?
In TS 27.007, the formula is letDBM = -140 + lteSignalStrength, where the value range of lteSignalStrength is from 0 to 97.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #23)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #22)
> > Comment on attachment 717790 [details] [diff] [review]
> > Updating all rf signal values in radiointerfacelayer
> > 
> > Review of attachment 717790 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ril_worker.js has mentioned valid values of dbm, EcO, RSRP, RSRQ, SNR.
> > Please be consistent with those, which are defined in
> > hardware/ril/include/telephony/ril.h.
> > 
> > Also, what is relative signal strength which was designed for signal status
> > bar? Looks like that you're gonna abandon that but make signal strength
> > refactored? Not sure I got your idea right, but if you are going to do what
> > I guess, then I would say I am inclined to the the method we have now, i.e.
> > return signal strength that we received from RIL but keep relSignalStrength
> > as the reference for status bar. Maybe we should have discussion about this
> > API change first.
First, because of comment 11, I remove |relSignalStrength| in code.
Second, we use linear values to express signal strength. Here are the codes, in ril_worker.js, 
obj.gsmRelative = Math.floor(obj.gsmSignalStrength * 100 / 31); 
In apps/system/js/statusbar.js, 
icon.dataset.level = Math.ceil(voice.relSignalStrength / 20); // 0-5 
(The value of voice.relSignalStrength is from obj.gsmRelative)
However, the signal strength is exponential value, I don't think it is appropriate to use linear values in |relSignalStrength|.
Third, every rf value is able to be gotten from |signal|.  
That is why I remove the |relSignalStrength|.

> To be clear, I don't insist we should keep 'relSingalStrength.' What I meant
> was, if we do need to provide 'refactored' signal strength rather than what
> we received from rild, then I vote for additional relSignalStrength.
> Otherwise, simply as comment 6 said, we just provide everything truly on
> specification.
If we just provide all RF values getting from vendor's RIL to Gaia, the values of  this API will unpredictable. Moreover, we have to handle the differences between chip vendors in Gaia. I am not sure if it is suitable to do these matters in Gaia.

Comment 26

[Ken Chang[:kenkai]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #24)
> Comment on attachment 717790 [details] [diff] [review]
> Updating all rf signal values in radiointerfacelayer
> 
> Review of attachment 717790 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Except for the coding style is mess,
> why do you do these things in RadioInterfaceLayer?
> Unless you got strong reason, otherwise we should move those code in
> ril_worker.
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +246,5 @@
> > +                                evdoECIO: null,
> > +                                evdoSNR: null,
> > +                                lteSignal: null,
> > +                                lteRSRP: null,
> > +                                lteSNR: null}},
> 
> The change here is strange to me.
> We're writing Javascript, it's not Java nor cpp, why do you want to
> initialize every member here?
> Also these members won't exist at the same time.
In unagi, ril_worker.js always sends all of those values to RadioInterfaceLayer.js.

Comment 27

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Ken Chang from comment #26)
> In unagi, ril_worker.js always sends all of those values to
> RadioInterfaceLayer.js.

So why don't you modify ril_worker to make it send only useful information?

Comment 28

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

(In reply to Ken Chang from comment #25)
> 
> > To be clear, I don't insist we should keep 'relSingalStrength.' What I meant
> > was, if we do need to provide 'refactored' signal strength rather than what
> > we received from rild, then I vote for additional relSignalStrength.
> > Otherwise, simply as comment 6 said, we just provide everything truly on
> > specification.
> If we just provide all RF values getting from vendor's RIL to Gaia, the
> values of  this API will unpredictable. Moreover, we have to handle the
> differences between chip vendors in Gaia. I am not sure if it is suitable to
> do these matters in Gaia.

I understood your concerns but I also realize that those values are definitely predictable. 
Please refer to B2G/hardware/ril/include/telephony/ril.h. [1] [2] [3]
 
ril.h declares the values and explains what we will receive from rild very clearly that we can have explicitly bounded values, so you don't have to refactor the values.

For example, according to [1], it guarantees that cdmaECIO value we receive falls between 5 and 315. More, [3] says that lteRSSNR falls between -200 and 300, not -160~240 as you assumed in your patch. Please be consistent with ril.h and ril_worker.js [4]. Thanks!
 

[1] https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/include/telephony/ril.h#L621
[2] https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/include/telephony/ril.h#L633
[3] https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/include/telephony/ril.h#L645
[4] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#4436

Comment 29

[Ken Chang[:kenkai]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #28)
Thanks, I modify it to just provide what we receive from RIL.

Comment 30

[Ken Chang[:kenkai]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #27)
> So why don't you modify ril_worker to make it send only useful information?
Thanks for your suggestion.
But I will follow the comment 28 to just provide what we received from RIL. I think it is more consistent with the purpose of this bug.

Comment 31

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 717796 [details] [diff] [review]
Updating signal level in BT for new signal architecture - WIP V1.

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

ok, looks like we won't get relSignalStrength which is normalized to 0~100 anymore but will receive a magic number indicating signal strength instead. This patch handles only signal strength of GSM but no CDMA, should we check the connection type ('gsm' or 'cdma') and call another function like GetCdmaDBM() to get signal strength of CDMA as well?

In addition, I would suggest that we should create another function such as GetSignalStrength() and move the logic into it. Also, it would be better if we can leave comments for those magic numbers. Since this part was implemented by Gina, so I'll let her review again.

Comment 32

[Ken Chang[:kenkai]]

Attachment: Part 1/3 - add signal information in nsIDOMMobileConnection.idl

Comment 33

[Ken Chang[:kenkai]]

Attachment: Part 2/3 - Get all signal information from RadioInterfaceLayer.js

Comment 34

[Ken Chang[:kenkai]]

Attachment: Part 3/3 - Update all signal information what we received from RIL in RadioInterfaceLayer.js

Comment 35

[Ken Chang[:kenkai]]

Attachment: Part 2/3 - Get all signals information from RadioInterfaceLayer.js

Comment 36

[Ken Chang[:kenkai]]

Attachment: Part 1/3 - Add signal interface in nsIDOMMobileConnection.idl

Comment 37

[Ken Chang[:kenkai]]

Attachment: Part 2/3 - Get all signals information from RadioInterfaceLayer.js

Comment 38

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

Comment on attachment 719887 [details] [diff] [review]
Part 3/3 - Update all signal information what we received from RIL in RadioInterfaceLayer.js

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

Thanks for the patch. It looks better now though it still needs improvement.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1038,5 @@
> +    let dataInfo = this.rilContext.data;
> +    let updateRFValues = false;
> +    // Just provide what we received from RIL.
> +    for (let rfItem in voiceInfo.signal) {
> +      if (voiceInfo.signal[rfItem] != message[rfItem]) {

Please note that we don't necessarily receive LTE-related values, such as lteSignalStrength or lteRSRP. See [1]. So the implementation here is likely to let us get an exception indicating that |message.lteSignalStrength| undefined.

Besides, ril_worker.js obviously needs some modification. It seems that the patch needs more tests before asking for review... A-ha, that reminds me where the test case for this patch is? 

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#4479

Comment 39

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

Comment on attachment 719894 [details] [diff] [review]
Part 2/3 - Get all signals information from RadioInterfaceLayer.js

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

The patch looks good, thanks! But I'd like to hold the review for a second until the API change passes!

Comment 40

[Ken Chang[:kenkai]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #38)
> Comment on attachment 719887 [details] [diff] [review]
> Part 3/3 - Update all signal information what we received from RIL in
> RadioInterfaceLayer.js
> 
> Review of attachment 719887 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch. It looks better now though it still needs improvement.
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1038,5 @@
> > +    let dataInfo = this.rilContext.data;
> > +    let updateRFValues = false;
> > +    // Just provide what we received from RIL.
> > +    for (let rfItem in voiceInfo.signal) {
> > +      if (voiceInfo.signal[rfItem] != message[rfItem]) {
> 
> Please note that we don't necessarily receive LTE-related values, such as
> lteSignalStrength or lteRSRP. See [1]. So the implementation here is likely
> to let us get an exception indicating that |message.lteSignalStrength|
> undefined.
Thanks for your review. 
The LTE signal values no matter valid or not always be provided in RIL V6 or later version. And RIL V5 have phased out. So I will just add backward compatibility for RIL V5 in the ril_worker.js.

Comment 41

[Ken Chang[:kenkai]]

Attachment: Part 1/3 - Add signal interface in nsIDOMMobileConnection.idl

Comment 42

[Ken Chang[:kenkai]]

Attachment: Part 4 - Add backward compacibility for RIL v5 in ril_worker.js

Comment 43

[Ken Chang[:kenkai]]

Attachment: Part 5 - Add marionette test.

Comment 44

[Ken Chang[:kenkai]]

Comment on attachment 719887 [details] [diff] [review]
Part 3/3 - Update all signal information what we received from RIL in RadioInterfaceLayer.js

After adding the part 4 patch, I think I needn't modify the part 3 patch.

Comment 45

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

Comment on attachment 720564 [details] [diff] [review]
Part 4 - Add backward compacibility for RIL v5 in ril_worker.js

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

Great!

Comment 46

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

Comment on attachment 719887 [details] [diff] [review]
Part 3/3 - Update all signal information what we received from RIL in RadioInterfaceLayer.js

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +921,5 @@
>      // Make sure we also reset the operator and signal strength information
>      // if we drop off the network.
>      if (newInfo.regState == RIL.NETWORK_CREG_STATE_UNKNOWN) {
>        voiceInfo.network = null;
> +      voiceInfo.signal = {};

Making .signal an empty object leads to problems of further |handleSignalStrengthChange|.

Except this, the patch is almost ready from my side :)

Comment 47

[Ken Chang[:kenkai]]

I will post this new DOM API to dev-webapi for further discussion.

Comment 48

[Edgar Chen [:edgar]]

According to previous discussion in dev-webapi [1] and above comments. I plan to continue this bug with below direction.

* Keep relSignalStrength in |nsIDOMMozMobileConnectionInfo| for the applications who want to know the signal strength but does not care about the details of underlying radio technology. (Normalize to 0 to 100)

* Expose all details of signal information to |nsIDOMMozMobileSignalInfo| for the applications who want to know these information.

* Put |nsIDOMMozMobileSignalInfo| in |nsIDOMMozMobileConnection|. Because |nsIDOMMozSingalInfo| includes the signal informations of both voice and data already.

Any thought?

[1] https://groups.google.com/forum/?hl=zh-TW#!searchin/mozilla.dev.webapi/MozMobileConnectionInfo$20API/mozilla.dev.webapi/IFasqbQk-DE/2VDwrjO1RZMJ

Comment 49

[Anshul]

Edgar, please see my my comment #4 in bug 842220. I also went through the link to the discussion you pointed out in your comment #48 but I still don't see a strong reason to send all the signal strength information to gaia. Can you please point me to a use case besides a possibility of an app that can provide all details of the signal strength for all radio tech? Status bar can be updated based solely on relSignalStrength?

Comment 50

[Edgar Chen [:edgar]]

(In reply to Anshul from comment #49)
> Edgar, please see my my comment #4 in bug 842220. I also went through the
> link to the discussion you pointed out in your comment #48 but I still don't
> see a strong reason to send all the signal strength information to gaia. Can
> you please point me to a use case besides a possibility of an app that can
> provide all details of the signal strength for all radio tech? Status bar
> can be updated based solely on relSignalStrength?

Anshul, there are some reasons that we expose these informations:
1. For the application who wants to know the precise information of signal quality, instead of just the 'level'. For example, an engineering tool to show the signal quality in dBm ... etc.
2. relSignalStrength is provided based on some logic, but it may not be suitable for everyone. If the application doesn't want to use the logic we provide, it can take the detailed information in |nsIDOMMozSignalInfo| to build it's own logic.
Thanks

Comment 51

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

(In reply to Edgar Chen [:edgar][:echen] from comment #50)
> (In reply to Anshul from comment #49)
> > Edgar, please see my my comment #4 in bug 842220. I also went through the
> > link to the discussion you pointed out in your comment #48 but I still don't
> > see a strong reason to send all the signal strength information to gaia. Can
> > you please point me to a use case besides a possibility of an app that can
> > provide all details of the signal strength for all radio tech? Status bar
> > can be updated based solely on relSignalStrength?
> 
> Anshul, there are some reasons that we expose these informations:
> 1. For the application who wants to know the precise information of signal
> quality, instead of just the 'level'. For example, an engineering tool to
> show the signal quality in dBm ... etc.
> 2. relSignalStrength is provided based on some logic, but it may not be
> suitable for everyone. If the application doesn't want to use the logic we
> provide, it can take the detailed information in |nsIDOMMozSignalInfo| to
> build it's own logic.
> Thanks

I am sorry if my previous comment (several months ago...) leads you to the direction, Edgar. Since we didn't have a clear user story at that time but we do now, 
for the user story we want to support, I think exposing different relSignalStrength to data and voice should be enough. 

It's helpful to expose more details but I would vote for doing so when we have a strong and explicit user story request in the future, and let this bug be focused.

Comment 52

[Edgar Chen [:edgar]]

Attachment: Part 1: Update signal strength for cdma/evdo network, v2

Update new patches which is implemented based on the conclusion of comment #51. Thanks

Comment 53

[Edgar Chen [:edgar]]

Attachment: Part 1: Update signal strength for cdma/evdo network, v3

Comment 54

[Edgar Chen [:edgar]]

Comment on attachment 783569 [details] [diff] [review]
Part 1: Update signal strength for cdma/evdo network, v3

In this patch I left interface unchanged and expose signal strength information of CDMA/EVDO to relSignalStrength. I also refer aosp's implementation and re-define the range of signal level.

Comment 55

[Edgar Chen [:edgar]]

Attachment: Part 2: Add marionette tests for signal strength, v2

Comment 56

[Edgar Chen [:edgar]]

Comment on attachment 783614 [details] [diff] [review]
Part 2: Add marionette tests for signal strength, v2

I will file a new bug for having a tests in CDMA mode. Thanks

Comment 57

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

Comment on attachment 783569 [details] [diff] [review]
Part 1: Update signal strength for cdma/evdo network, v3

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

Looks great. r=me, but please take care of my comments below. Thank you!

::: dom/system/gonk/ril_worker.js
@@ +3420,5 @@
> +        relSignalStrength: null
> +      }
> +    };
> +
> +    // TODO: Lte signal strength.

TODO is generally followed by a bug number. Do we have the corresponding bug now? If no, let's just remove the comment.

@@ +3430,5 @@
> +      if (signal.cdmaDBM && signal.cdmaDBM > 0) {
> +        let signalStrength = -1 * signal.cdmaDBM;
> +        info.voice.signalStrength = signalStrength;
> +
> +        let signalLevel = this._processSignalLevel(signalStrength, -105, -70);

Would you please point me where the values, -105 and -70, come from? Could they be constants? Or more helpful comments on the values?

@@ +3442,5 @@
> +      if (signal.evdoDBM && signal.evdoDBM > 0) {
> +        let signalStrength = -1 * signal.evdoDBM;
> +        info.data.signalStrength = signalStrength;
> +
> +        let signalLevel = this._processSignalLevel(signalStrength, -105, -70);

ditto.

@@ +4997,5 @@
> +  }
> +
> +  if (DEBUG) debug("signal strength: " + JSON.stringify(signal));
> +
> +  this._processSignalStrength(signal);

The refactoring looks clear!

Comment 58

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #57)
> Comment on attachment 783569 [details] [diff] [review]
> Part 1: Update signal strength for cdma/evdo network, v3
> 
> Review of attachment 783569 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +3430,5 @@
> > +      if (signal.cdmaDBM && signal.cdmaDBM > 0) {
> > +        let signalStrength = -1 * signal.cdmaDBM;
> > +        info.voice.signalStrength = signalStrength;
> > +
> > +        let signalLevel = this._processSignalLevel(signalStrength, -105, -70);
> 
> Would you please point me where the values, -105 and -70, come from? Could
> they be constants? Or more helpful comments on the values?

-105 and -70 are referred to AOSP's implementation. They are not constants and can be customized based on different requirement.

Comment 59

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #58)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #57)
> > Comment on attachment 783569 [details] [diff] [review]
> > Part 1: Update signal strength for cdma/evdo network, v3
> > 
> > Review of attachment 783569 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > @@ +3430,5 @@
> > > +      if (signal.cdmaDBM && signal.cdmaDBM > 0) {
> > > +        let signalStrength = -1 * signal.cdmaDBM;
> > > +        info.voice.signalStrength = signalStrength;
> > > +
> > > +        let signalLevel = this._processSignalLevel(signalStrength, -105, -70);
> > 
> > Would you please point me where the values, -105 and -70, come from? Could
> > they be constants? Or more helpful comments on the values?
> 
> -105 and -70 are referred to AOSP's implementation. They are not constants
> and can be customized based on different requirement.

I will add more comment on these values. Thanks!

Comment 60

[Edgar Chen [:edgar]]

Attachment: Part 1: Update signal strength for cdma/evdo network, v4, r=hsinyi

Address review comment #57.

Comment 61

[Edgar Chen [:edgar]]

try server: https://tbpl.mozilla.org/?tree=Try&rev=136a0da51d0f

Comment 62

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/rev/7d74434e7836
https://hg.mozilla.org/integration/b2g-inbound/rev/436aa701ca7c

Comment 63

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7d74434e7836
https://hg.mozilla.org/mozilla-central/rev/436aa701ca7c