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

RESOLVED FIXED

Status

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: kchang, Assigned: edgar)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:koi+, firefox26 fixed)

Details

(Whiteboard: [ETA:8/9], [FT:RIL], [Sprint:2])

Attachments

(2 attachments, 18 obsolete attachments)

1.44 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
9.38 KB, patch
edgar
: review+
Details | Diff | Splinter Review
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.
Blocks: b2g-ril-cdma
In Bug 729173, there are already some discussions about signal strength for GSM and CDMA.
Posted patch Add nsIDOMMozMobileSignalInfo (obsolete) — Splinter Review
Attachment #709321 - Flags: review?(vyang)
Getting all rf signal values from radiointerfacelayer
Attachment #709322 - Flags: review?(vyang)
Setting all rf signal values in radiointerfacelayer
Attachment #709323 - Flags: review?(vyang)
(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.
Attachment #709321 - Flags: review?(vyang) → review?(mounir)
(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 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.
Include all people joined signalStrength discuss on bug 729173.
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?
(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.
Assignee: nobody → kchang
(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 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.
Attachment #709321 - Flags: review?(mounir)
Depends on: 842220
(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.
No longer depends on: 842220
Depends on: 842220
Attachment #709321 - Attachment is obsolete: true
Attachment #709322 - Attachment is obsolete: true
Attachment #709322 - Flags: review?(vyang)
Attachment #709323 - Attachment is obsolete: true
Attachment #709323 - Flags: review?(vyang)
Add |signal| and remove |signalStrength| and |relSignalStrength|.
Attachment #717786 - Flags: review?(mounir)
Attachment #717788 - Flags: review?(swu)
Attachment #717788 - Flags: review?(htsai)
Attachment #717790 - Flags: review?(swu)
Attachment #717790 - Flags: review?(htsai)
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 =.
Attachment #717790 - Flags: review?(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.
Attachment #717788 - Flags: review?(swu)
(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 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.
Attachment #717788 - Flags: review?(htsai)
Attachment #717786 - Attachment is obsolete: true
Attachment #717786 - Flags: review?(mounir)
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?
Attachment #717790 - Flags: review?(htsai) → review-
(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 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 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.
(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.
(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?
(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
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #28)
Thanks, I modify it to just provide what we receive from RIL.
(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 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.
Attachment #717796 - Flags: review?(echou) → review?(gyeh)
Attachment #717796 - Attachment is obsolete: true
Attachment #717796 - Flags: review?(gyeh)
Attachment #717788 - Attachment is obsolete: true
Attachment #717790 - Attachment is obsolete: true
Attachment #719885 - Attachment is obsolete: true
Attachment #719884 - Attachment is obsolete: true
Attachment #719891 - Attachment is obsolete: true
Attachment #719887 - Flags: review?(shianyow)
Attachment #719887 - Flags: review?(htsai)
Attachment #719894 - Flags: review?(shianyow)
Attachment #719894 - Flags: review?(htsai)
Attachment #719893 - Flags: review?(mounir)
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
Attachment #719887 - Flags: review?(htsai) → review-
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!
Attachment #719894 - Flags: review?(htsai)
(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.
Attachment #719893 - Attachment is obsolete: true
Attachment #719893 - Flags: review?(mounir)
Blocks: 842220
No longer depends on: 842220
Posted patch Part 5 - Add marionette test. (obsolete) — Splinter Review
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.
Attachment #719887 - Flags: review- → review?(htsai)
Attachment #720564 - Flags: review?(htsai)
Attachment #721568 - Flags: review?(htsai)
Attachment #720563 - Flags: review?(mounir)
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!
Attachment #720564 - Flags: review?(htsai) → review+
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 :)
Attachment #719887 - Flags: review?(htsai)
Attachment #719887 - Flags: review?(shianyow)
Attachment #720563 - Flags: review?(mounir)
Attachment #719894 - Flags: review?(shianyow)
Attachment #720564 - Flags: review+
Attachment #721568 - Flags: review?(htsai)
I will post this new DOM API to dev-webapi for further discussion.
Blocks: 890325
blocking-b2g: --- → koi+
Assignee: kchang → echen
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
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?
(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
(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.
Whiteboard: [ETA:8/9]
Update new patches which is implemented based on the conclusion of comment #51. Thanks
Attachment #719887 - Attachment is obsolete: true
Attachment #719894 - Attachment is obsolete: true
Attachment #720563 - Attachment is obsolete: true
Attachment #720564 - Attachment is obsolete: true
Attachment #721568 - Attachment is obsolete: true
Attachment #782484 - Attachment is obsolete: true
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.
Attachment #783569 - Flags: review?(htsai)
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
Attachment #783614 - Flags: review?(htsai)
Blocks: 899921
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!
Attachment #783569 - Flags: review?(htsai) → review+
Attachment #783614 - Flags: review?(htsai) → review+
(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.
(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!
Address review comment #57.
Attachment #783569 - Attachment is obsolete: true
Attachment #784221 - Flags: review+
Whiteboard: [ETA:8/9] → [ETA:8/9], [FT:RIL], [Sprint:2]
https://hg.mozilla.org/mozilla-central/rev/7d74434e7836
https://hg.mozilla.org/mozilla-central/rev/436aa701ca7c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
Target Milestone: mozilla25 → ---
You need to log in before you can comment on or make changes to this bug.