By supporting LTE signal strength, ril_work.js need to handle rxlev/rsrp/rssnr properly. (see [1]) GSM/UMTS environment: rssi/ber are used to map signal bar. rxlev/rsrp/rssnr are all invalid values. LTE environment: rxlev/rsrp/rssnr are used to map signal bar, rssi/ber will be 99/99 which denote invalid values. Note: Bug 850996 is for LTE signal strength simulating, "gsm lte_signal <rxlev> <rsrp> <rssnr>" can be used to perform the manual/marionette tests. [1] AOSP reference code - https://github.com/android/platform_frameworks_base/blob/master/telephony/java/android/telephony/SignalStrength.java#L740

Shawn, I wonder if you could take this bug. Thanks.

This bug blocks 960206.

Sure Ken, I can take this bug, I will try to finsh things (including asking for review) by 3/14. Thanks!! Shawn

Comment on attachment 8386587 [details] [diff] [review] Bug 977433 Part 1: RIL patch - B2G RIL: Handling LTE signal strength. Review of attachment 8386587 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch. Please see my comments below. ::: dom/system/gonk/ril_worker.js @@ +3398,5 @@ > > return Math.floor((signal - min) * 100 / (max - min)); > }, > > + /** nit: the format is /** * Comment ... * ... */ @@ +3409,5 @@ > + * > + * @return level > + * The signal level from 0 to 100. > + */ > + _processLteLevel: function(signal, info) { I don't think it's appropriate to take "info" as an "input" parameter because "info" is actually a object holding "processed" value. The function should be rewritten as _processLteSignal: function(signal) { // ... ... ... return info; } If lte signal is not valid, simply "return;" Also note, I rename it "_proccessLteSignal" @@ +3413,5 @@ > + _processLteLevel: function(signal, info) { > + let signalLevel = 0, rsrpLevel = -1, rssnrLevel = -1; > + // The current Reference Signal Receive Power in dBm multipled by -1. > + // Range: 44 to 140 dBm. > + // Reference: 3GPP TS 36.133 9.1.4. Note that in our ril code, we usually wrap comments at 80th character. I'd suggest rewriting and reorganizing as // LTE RSRP (reference signal receive power) // This value is the actual RSRP (reference signal receive power) value // multiplied by -1. Valid values are 44 - 140. @@ +3416,5 @@ > + // Range: 44 to 140 dBm. > + // Reference: 3GPP TS 36.133 9.1.4. > + if (signal.lteRSRP && > + signal.lteRSRP >= 44 && > + signal.lteRSRP <= 140) { nit: wrap at 80 @@ +3423,5 @@ > + } > + > + // The current reference signal signal-to-noise ratio in 0.1 dB units. > + // Range: -200 to +300 (-200 = -20.0 dB, +300 = 30dB). > + // Reference: 3GPP TS 36.101 8.1.1. ditto @@ +3431,5 @@ > + // -30 and -130 are referred to AOSP's implementation. > + rssnrLevel = this._processSignalLevel(signal.lteRSSNR, -30, 130); > + } > + > + let validLevel = true; Bail-out early: if (rsrpLevel === -1 || rssnrLevel === -1) { return; } @@ +3446,5 @@ > + } > + > + if (validLevel) { > + info.voice.relSignalStrength = info.data.relSignalStrength = signalLevel; > + return signalLevel; how about info.voice.signalStrength and info.data.signalStrength? What would they be in this validLevel case? @@ +3449,5 @@ > + info.voice.relSignalStrength = info.data.relSignalStrength = signalLevel; > + return signalLevel; > + } > + > + // Valid values are (0-63, 99) as defined in TS 27.007 clause 8.6.9. I cannot find clause 8.6.9 in TS 27.007. And the valid values defined in ril.h are (0-31, 99). @@ +3453,5 @@ > + // Valid values are (0-63, 99) as defined in TS 27.007 clause 8.6.9. > + if (signal.lteSignalStrength && > + signal.lteSignalStrength >= 0 && > + signal.lteSignalStrength <= 63) { > + let signalStrength = -111 + signal.lteSignalStrength; Could you explain this line? Where does the calculation come from? @@ +3504,5 @@ > info.data.relSignalStrength = signalLevel; > } > } else { > + // If LTE signal level is not valid, go check GSM/UMTS level next. > + if (this._processLteLevel(signal, info) === 0) { Based on my suggestion above, this becomes |if (!this._processLteLeve(signal))| ...

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #6) > Comment on attachment 8386587 [details] [diff] [review] > Bug 977433 Part 1: RIL patch - B2G RIL: Handling LTE signal strength. > > Review of attachment 8386587 [details] [diff] [review]: > ----------------------------------------------------------------- > > @@ +3431,5 @@ > > + // -30 and -130 are referred to AOSP's implementation. > > + rssnrLevel = this._processSignalLevel(signal.lteRSSNR, -30, 130); > > + } > > + > > + let validLevel = true; > > Bail-out early: > > if (rsrpLevel === -1 || rssnrLevel === -1) { > return; > } > Oops... it should be if (rsrpLevel === -1 && rssnrLevel === -1) { return; }

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #7) > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #6) > > Comment on attachment 8386587 [details] [diff] [review] > > Bug 977433 Part 1: RIL patch - B2G RIL: Handling LTE signal strength. > > > > Review of attachment 8386587 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > > @@ +3431,5 @@ > > > + // -30 and -130 are referred to AOSP's implementation. > > > + rssnrLevel = this._processSignalLevel(signal.lteRSSNR, -30, 130); > > > + } > > > + > > > + let validLevel = true; > > > > Bail-out early: > > > > if (rsrpLevel === -1 || rssnrLevel === -1) { > > return; > > } > > > > Oops... it should be > > if (rsrpLevel === -1 && rssnrLevel === -1) { > return; > } My bad. Please ignore this comment :)

Thanks HsinYi. Please check my in-line reply. (In reply to Hsin-Yi Tsai [:hsinyi] from comment #6) > Comment on attachment 8386587 [details] [diff] [review] > Bug 977433 Part 1: RIL patch - B2G RIL: Handling LTE signal strength. > > Review of attachment 8386587 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks for the patch. Please see my comments below. > > ::: dom/system/gonk/ril_worker.js > @@ +3398,5 @@ > > > > return Math.floor((signal - min) * 100 / (max - min)); > > }, > > > > + /** > > nit: the format is > > /** > * Comment ... > * ... > */ > My Bad, Will address it in next patch!! > @@ +3409,5 @@ > > + * > > + * @return level > > + * The signal level from 0 to 100. > > + */ > > + _processLteLevel: function(signal, info) { > > I don't think it's appropriate to take "info" as an "input" parameter > because "info" is actually a object holding "processed" value. > > The function should be rewritten as > > _processLteSignal: function(signal) { > // ... ... ... > return info; > } > > If lte signal is not valid, simply "return;" > Also note, I rename it "_proccessLteSignal" > info is an local object created at _processSignalStrength. If we implement the way as you mention above, we have to create a new object in _processLteSignal, fill up with results, and return it to _processSignalStrength. Finally, We have to make an extra copy from the return value of _processLteSignal to the object at _processSignalStrength. Is this we want? If yes, I will address this in next patch too. :) > @@ +3413,5 @@ > > + _processLteLevel: function(signal, info) { > > + let signalLevel = 0, rsrpLevel = -1, rssnrLevel = -1; > > + // The current Reference Signal Receive Power in dBm multipled by -1. > > + // Range: 44 to 140 dBm. > > + // Reference: 3GPP TS 36.133 9.1.4. > > Note that in our ril code, we usually wrap comments at 80th character. > > I'd suggest rewriting and reorganizing as > > // LTE RSRP (reference signal receive power) > // This value is the actual RSRP (reference signal receive power) value > // multiplied by -1. Valid values are 44 - 140. > Roger that, will address it in next patch. > @@ +3416,5 @@ > > + // Range: 44 to 140 dBm. > > + // Reference: 3GPP TS 36.133 9.1.4. > > + if (signal.lteRSRP && > > + signal.lteRSRP >= 44 && > > + signal.lteRSRP <= 140) { > > nit: wrap at 80 > Roger that, will address this in next patch. > @@ +3423,5 @@ > > + } > > + > > + // The current reference signal signal-to-noise ratio in 0.1 dB units. > > + // Range: -200 to +300 (-200 = -20.0 dB, +300 = 30dB). > > + // Reference: 3GPP TS 36.101 8.1.1. > > ditto > Roger that, will address this in next patch. > @@ +3431,5 @@ > > + // -30 and -130 are referred to AOSP's implementation. > > + rssnrLevel = this._processSignalLevel(signal.lteRSSNR, -30, 130); > > + } > > + > > + let validLevel = true; > > Bail-out early: > > if (rsrpLevel === -1 || rssnrLevel === -1) { > return; > } > I am afraid it is not proper to do the early bail-out here. The reason is: For LTE signal strength algorithm, 1. check/return the min of rsrp and rssnr if both are valid. 2. return rssnr level if only rssnr is valid. 3. return rsrp level if only rsrp is valid. 4. return lteSignalStrength level if both rsrp and rssnr are invalid and lteSignalStrength is valid. Early bail-out will let step 4 hard to be handled. > @@ +3446,5 @@ > > + } > > + > > + if (validLevel) { > > + info.voice.relSignalStrength = info.data.relSignalStrength = signalLevel; > > + return signalLevel; > > how about info.voice.signalStrength and info.data.signalStrength? What would > they be in this validLevel case? > rsrp is with dBm unit. rssnr is with dB unit. and signalStrength is a converted variable with dBm unit. Which means there might be no proper value for both info.voice.signalStrength and info.data.signalStrength by following the LTE signal strength algorithm. That's why I let info.voice.signalStrength and info.data.signalStrength as null if we can not determine the result. > @@ +3449,5 @@ > > + info.voice.relSignalStrength = info.data.relSignalStrength = signalLevel; > > + return signalLevel; > > + } > > + > > + // Valid values are (0-63, 99) as defined in TS 27.007 clause 8.6.9. > > I cannot find clause 8.6.9 in TS 27.007. And the valid values defined in > ril.h are (0-31, 99). > Sorry, there was a typo. it should be Clause 8.69. (not 8.6.9). Besides, I also believe there was a typo in ril.h. (see https://github.com/android/platform_frameworks_base/blob/master/telephony/java/android/telephony/SignalStrength.java#L740) > @@ +3453,5 @@ > > + // Valid values are (0-63, 99) as defined in TS 27.007 clause 8.6.9. > > + if (signal.lteSignalStrength && > > + signal.lteSignalStrength >= 0 && > > + signal.lteSignalStrength <= 63) { > > + let signalStrength = -111 + signal.lteSignalStrength; > > Could you explain this line? Where does the calculation come from? > Please see <rxlev> @ TS 27.007 8.69 Extended signal quality +CESQ > @@ +3504,5 @@ > > info.data.relSignalStrength = signalLevel; > > } > > } else { > > + // If LTE signal level is not valid, go check GSM/UMTS level next. > > + if (this._processLteLevel(signal, info) === 0) { > > Based on my suggestion above, > this becomes |if (!this._processLteLeve(signal))| ... Thanks again!!! Shawn

> Sorry, there was a typo. it should be Clause 8.69. (not 8.6.9). > > Besides, I also believe there was a typo in ril.h. > (see > https://github.com/android/platform_frameworks_base/blob/master/telephony/ > java/android/telephony/SignalStrength.java#L740) Should be: https://github.com/android/platform_frameworks_base/blob/master/telephony/java/android/telephony/SignalStrength.java#L788

Per offline discussion with Shawn, let's focus on reading "lteSignalStrength" and mapping the signal to a relative number in this bug. However, we both agree that our API definition isn't appropriate to CDMA/WCDMA networks. "relSignalStrength" should NOT be a linearly represented number of "signalStrength." It is rather a relative signal strength which could consider SNR as well. So I'd suggest filing another followup bug to address the API meaning change and handle both CDMA and LTE cases more properly there. And once in LTE rssnr is used as the indication of "relSignalStrength" then "rsrp" would be "signalStrength." Thank you, Shawn :)

Comment on attachment 8388989 [details] [diff] [review] Bug 977433 Part 1: RIL patch - B2G RIL: Handling LTE signal strength. v2. Review of attachment 8388989 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +3407,5 @@ > + * > + * @return The object of signal strength info. > + * Or null if invalid signal input. > + */ > + _processLteLevel: function(signal) { 1) s/_processLteLevel/_processLteSignal/ 2) let's bail-out early, so it becomes: if (!signal.lteSignalStrength || signal.lteSignalStrength < 0 || signal.lteSignalStrength > 63) { return; } @@ +3420,5 @@ > + } > + }; > + > + // TODO: reconsider signalStrength/relSignalStrength API for GSM/CDMA/LTE, > + // and take rsrp/rssnr into account for LTE case then. TODO is typically followed by a opened bug. Please file a bug and provide the bug number here.

Comment on attachment 8388990 [details] [diff] [review] Bug 977433 Part 2 : Test cases - B2G RIL: Handling LTE signal strength. Review of attachment 8388990 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobileconnection/tests/marionette/test_mobile_signal_strength.js @@ +62,5 @@ > + // All invalid case. > + {input: { > + rxlev: 99, > + rsrp: 65535, > + rssnr: 65535,}, nit: no , right after 65535. Same issue all over the file, kindly correct them please :) @@ +65,5 @@ > + rsrp: 65535, > + rssnr: 65535,}, > + expect: { > + signalStrength: null, > + relSignalStrength: null,} nit: no , right after null. Same issue all over the file, kindly correct them please :) @@ +110,5 @@ > + > +/* Reset Signal Strength Info to default, and finsih the test */ > +taskHelper.push(function testResetSignalStrengthInfo() { > + // Reset emulator's signal strength and wait for 'onvoicechange' event. > + function doResetSignalStrength(rssi, callback) { Looks we don't really need callback. Please remove it. @@ +121,5 @@ > + callback(); > + } > + > + if (emulatorHelper.pendingCommandCount) { > + window.setTimeout(cleanUp, 100); Let's not use a random timeout here. We could and should use a smarter way. We should have a general and overall handling in cleanUp function in mobile_head.js as below: let cleanUp = function() { waitFor(function() { SpecialPowers.removePermission("mobileconnection", document); finish(); }, function() { return _pendingEmulatorCmdCount === 0; }); }; Once we have expended cleanUp this way, we don't care about pendingCommandCount in each test case.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #18) > Comment on attachment 8388990 [details] [diff] [review] > Bug 977433 Part 2 : Test cases - B2G RIL: Handling LTE signal strength. > > Review of attachment 8388990 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/mobileconnection/tests/marionette/test_mobile_signal_strength.js > @@ +62,5 @@ > > + // All invalid case. > > + {input: { > > + rxlev: 99, > > + rsrp: 65535, > > + rssnr: 65535,}, > > nit: no , right after 65535. Same issue all over the file, kindly correct > them please :) > > @@ +65,5 @@ > > + rsrp: 65535, > > + rssnr: 65535,}, > > + expect: { > > + signalStrength: null, > > + relSignalStrength: null,} > > nit: no , right after null. Same issue all over the file, kindly correct > them please :) > > @@ +110,5 @@ > > + > > +/* Reset Signal Strength Info to default, and finsih the test */ > > +taskHelper.push(function testResetSignalStrengthInfo() { > > + // Reset emulator's signal strength and wait for 'onvoicechange' event. > > + function doResetSignalStrength(rssi, callback) { > > Looks we don't really need callback. Please remove it. > > @@ +121,5 @@ > > + callback(); > > + } > > + > > + if (emulatorHelper.pendingCommandCount) { > > + window.setTimeout(cleanUp, 100); > > Let's not use a random timeout here. We could and should use a smarter way. > > We should have a general and overall handling in cleanUp function in > mobile_head.js as below: > > let cleanUp = function() { > waitFor(function() { > SpecialPowers.removePermission("mobileconnection", document); > finish(); > }, function() { > return _pendingEmulatorCmdCount === 0; > }); > }; > Oh, forgot to say: above is an example I had in mind, please revise it correctly to make it really work. :p Looks like we need to have a global _pendingEmulatorCmdCount which replaces "emulatorHelper.pendingCommandCount." > Once we have expended cleanUp this way, we don't care about > pendingCommandCount in each test case.

Comment on attachment 8389067 [details] [diff] [review] Bug 977433 Part 1: RIL patch - B2G RIL: Handling LTE signal strength. v3. Review of attachment 8389067 [details] [diff] [review]: ----------------------------------------------------------------- Looks great. Thank you. I'll try to test this with the real LTE network today. Let us land it if the test goes well.

Comment on attachment 8389069 [details] [diff] [review] Bug 977433 Part 2 : Test cases - B2G RIL: Handling LTE signal strength. v2. Review of attachment 8389069 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comment addressed, thank you~ ::: dom/mobileconnection/tests/marionette/mobile_header.js @@ +8,5 @@ > let mobileConnection = window.navigator.mozMobileConnections[0]; > ok(mobileConnection instanceof MozMobileConnection, > "mobileConnection is instanceof " + mobileConnection.constructor); > > +let pendingCommandCount = 0; As this attribute is moved out of "EmulatorHelper" I'd like to make the name clearer. Please change it to "_pendingEmulatorCmdCount." Note: in RIL code, we usually use "_" prefix to show it's a private member. ::: dom/mobileconnection/tests/marionette/test_mobile_signal_strength.js @@ +122,5 @@ > + > + setEmulatorGsmSignalStrength(rssi); > + } > + > + // Emulator use rssi = 7 as default value, and we need to reset it after nit: s/use/uses/

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #23) > Comment on attachment 8389067 [details] [diff] [review] > Bug 977433 Part 1: RIL patch - B2G RIL: Handling LTE signal strength. v3. > > Review of attachment 8389067 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks great. Thank you. > > I'll try to test this with the real LTE network today. Let us land it if the > test goes well. Hi Hsinyi: Thanks for your review and support. There is one thing need to be noted while running live test, please remember to set preferreed network type to 8 for CDMA+LTE [1], 9 for UMTS+LTE to get LTE service. Or we might stay at EVDO/HSPA only. int NETWORK_MODE_LTE_CDMA_EVDO = 8; /* LTE, CDMA and EvDo */ int NETWORK_MODE_LTE_GSM_WCDMA = 9; /* LTE, GSM/WCDMA */ int NETWORK_MODE_LTE_CMDA_EVDO_GSM_WCDMA = 10; /* LTE, CDMA, EvDo, GSM/WCDMA */ int NETWORK_MODE_LTE_ONLY = 11; /* LTE Only mode. */ int NETWORK_MODE_LTE_WCDMA = 12; /* LTE/WCDMA */ [1] https://code.google.com/p/android-source-browsing/source/browse/telephony/java/com/android/internal/telephony/RILConstants.java?repo=platform--frameworks--base&name=jb-dev&r=4281817f6b624cb51926eb24fa78c68cd9431dce#71

try result - https://tbpl.mozilla.org/?tree=Try&rev=5032130b7a42

Signal bar is 0 (signalStrength and relSignalStrength are null in RILContentHelper) when I tested this patch in a test lab with very strong LTE signal.

So, having a looooong discussion with Shawn, I think we should process signal based on voiceregistrationstate.radioTech instead of the radio technology we get from REQUEST_VOICE_RADIO_TECH on boot-up.

Comment on attachment 8389569 [details] [diff] [review] Bug 977433 Part 1: RIL patch - B2G RIL: Handling LTE signal strength. v3. r=HsinYi. Review of attachment 8389569 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +3408,5 @@ > + * @return The object of signal strength info. > + * Or null if invalid signal input. > + */ > + _processLteSignal: function(signal) { > + let info = { Sorry that I should have pointed this out in the review. Please move this to line 3432 because we don't need this if the signal strength is invalid.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #31) > Comment on attachment 8389569 [details] [diff] [review] > Bug 977433 Part 1: RIL patch - B2G RIL: Handling LTE signal strength. v3. > r=HsinYi. > > Review of attachment 8389569 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/ril_worker.js > @@ +3408,5 @@ > > + * @return The object of signal strength info. > > + * Or null if invalid signal input. > > + */ > > + _processLteSignal: function(signal) { > > + let info = { > > Sorry that I should have pointed this out in the review. Please move this to > line 3432 because we don't need this if the signal strength is invalid. Thanks!! Will provide a new patch this afternoon, and look forward to your testing result:)

(In reply to shawn ku [:sku] from comment #34) > Created attachment 8390337 [details] [diff] [review] > Bug 977433 Part 1: RIL patch - B2G RIL: Handling LTE signal strength. v4. This works well in the test lab!

Comment on attachment 8390944 [details] [diff] [review] Bug 977433 Part 2 : Test cases - B2G RIL: Handling LTE signal strength. v2. Review of attachment 8390944 [details] [diff] [review]: ----------------------------------------------------------------- Since new ril patch works well in the test lab, re-submit both ril/test patch for review. Please help double check if any revise needed. Thanks!! Shawn

Comment on attachment 8390337 [details] [diff] [review] Bug 977433 Part 1: RIL patch - B2G RIL: Handling LTE signal strength. v4. Review of attachment 8390337 [details] [diff] [review]: ----------------------------------------------------------------- Since new ril patch works well in the test lab, re-submit both ril/test patch for review. Please help double check if any revise needed. Thanks!! Shawn

Comment on attachment 8390944 [details] [diff] [review] Bug 977433 Part 2 : Test cases - B2G RIL: Handling LTE signal strength. v2. Review of attachment 8390944 [details] [diff] [review]: ----------------------------------------------------------------- Since new ril patch works well in the test lab, re-submit both ril/test patch for review. Please help double check if any revise needed. Thanks!! Shawn

Comment on attachment 8390337 [details] [diff] [review] Bug 977433 Part 1: RIL patch - B2G RIL: Handling LTE signal strength. v4. Review of attachment 8390337 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. r=me with comment addressed. Please help modify the comment of "_isCdma" [1] to make the description more accurate and explicit. I.e."True if we are on a CDMA phone." [1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#295 ::: dom/system/gonk/ril_worker.js @@ +3412,5 @@ > + // Valid values are 0-63 as defined in TS 27.007 clause 8.69. > + if (signal.lteSignalStrength === undefined || > + signal.lteSignalStrength < 0 || > + signal.lteSignalStrength > 63) { > + return null; s/return null/return;/

Comment on attachment 8390944 [details] [diff] [review] Bug 977433 Part 2 : Test cases - B2G RIL: Handling LTE signal strength. v2. Review of attachment 8390944 [details] [diff] [review]: ----------------------------------------------------------------- Great, thank you :) Please remember to file a bug for addressing emulator voice registration state on lte network.

try result - https://tbpl.mozilla.org/?tree=Try&rev=86bceae9ccd2

https://hg.mozilla.org/integration/b2g-inbound/rev/375fef5b004c https://hg.mozilla.org/integration/b2g-inbound/rev/a580e8f3ee06

https://hg.mozilla.org/mozilla-central/rev/375fef5b004c https://hg.mozilla.org/mozilla-central/rev/a580e8f3ee06

This patch should be uplifted to 1.4. thanks.

https://hg.mozilla.org/releases/mozilla-aurora/rev/f0738f721655 https://hg.mozilla.org/releases/mozilla-aurora/rev/5be7fd72c1b1