Closed
Bug 977433
Opened 11 years ago
Closed 11 years ago
B2G RIL: Handling LTE signal strength.
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(blocking-b2g:1.4+, firefox29 wontfix, firefox30 fixed, firefox31 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)
People
(Reporter: sku, Assigned: sku)
References
Details
Attachments
(2 files, 16 obsolete files)
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
Assignee | ||
Updated•11 years ago
|
Hardware: x86_64 → ARM
Updated•11 years ago
|
Flags: needinfo?(sku)
Comment 2•11 years ago
|
||
This bug blocks 960206.
Blocks: b2g-LTE-1.4
Target Milestone: --- → 1.4 S3 (14mar)
Assignee | ||
Comment 3•11 years ago
|
||
Sure Ken, I can take this bug, I will try to finsh things (including asking for review) by 3/14. Thanks!! Shawn
Flags: needinfo?(sku)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → sku
Assignee | ||
Comment 4•11 years ago
|
||
Assignee | ||
Comment 5•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8386587 -
Flags: review?(htsai)
Assignee | ||
Updated•11 years ago
|
Attachment #8386588 -
Flags: review?(htsai)
Comment 6•11 years ago
|
||
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))| ...
Attachment #8386587 -
Flags: review?(htsai)
Comment 7•11 years ago
|
||
(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; }
Comment 8•11 years ago
|
||
(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 :)
Assignee | ||
Comment 9•11 years ago
|
||
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
Assignee | ||
Comment 10•11 years ago
|
||
> 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
Comment 11•11 years ago
|
||
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 :)
Assignee | ||
Comment 15•11 years ago
|
||
Attachment #8386588 -
Attachment is obsolete: true
Attachment #8388986 -
Attachment is obsolete: true
Attachment #8386588 -
Flags: review?(htsai)
Assignee | ||
Comment 16•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8388989 -
Flags: review?(htsai)
Assignee | ||
Updated•11 years ago
|
Attachment #8388990 -
Flags: review?(htsai)
Comment 17•11 years ago
|
||
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.
Attachment #8388989 -
Flags: review?(htsai)
Comment 18•11 years ago
|
||
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.
Attachment #8388990 -
Flags: review?(htsai)
Comment 19•11 years ago
|
||
(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.
Assignee | ||
Comment 20•11 years ago
|
||
Attachment #8388989 -
Attachment is obsolete: true
Attachment #8388990 -
Attachment is obsolete: true
Assignee | ||
Comment 21•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8389067 -
Flags: review?(htsai)
Assignee | ||
Updated•11 years ago
|
Attachment #8389069 -
Flags: review?(htsai)
Comment 23•11 years ago
|
||
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.
Attachment #8389067 -
Flags: review?(htsai) → review+
Comment 24•11 years ago
|
||
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/
Attachment #8389069 -
Flags: review?(htsai) → review+
Assignee | ||
Comment 25•11 years ago
|
||
(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
Assignee | ||
Comment 26•11 years ago
|
||
Attachment #8389067 -
Attachment is obsolete: true
Attachment #8389069 -
Attachment is obsolete: true
Attachment #8389569 -
Flags: review+
Comment 29•11 years ago
|
||
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.
Comment 30•11 years ago
|
||
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 31•11 years ago
|
||
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.
Assignee | ||
Comment 32•11 years ago
|
||
(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:)
Comment 35•11 years ago
|
||
(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!
Assignee | ||
Comment 37•11 years ago
|
||
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
Attachment #8390944 -
Flags: review?(htsai)
Assignee | ||
Comment 38•11 years ago
|
||
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
Attachment #8390337 -
Flags: review?(htsai)
Assignee | ||
Comment 39•11 years ago
|
||
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 40•11 years ago
|
||
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;/
Attachment #8390337 -
Flags: review?(htsai) → review+
Comment 41•11 years ago
|
||
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.
Attachment #8390944 -
Flags: review?(htsai) → review+
Assignee | ||
Comment 42•11 years ago
|
||
Attachment #8390337 -
Attachment is obsolete: true
Attachment #8390944 -
Attachment is obsolete: true
Assignee | ||
Comment 43•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8390215 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 45•11 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/375fef5b004c https://hg.mozilla.org/integration/b2g-inbound/rev/a580e8f3ee06
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/375fef5b004c https://hg.mozilla.org/mozilla-central/rev/a580e8f3ee06
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 47•11 years ago
|
||
This patch should be uplifted to 1.4. thanks.
blocking-b2g: --- → 1.4+
status-b2g-v1.4:
--- → affected
Comment 48•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/f0738f721655 https://hg.mozilla.org/releases/mozilla-aurora/rev/5be7fd72c1b1
Updated•11 years ago
|
status-b2g-v2.0:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•