Closed Bug 1048723 Opened 10 years ago Closed 10 years ago

B2G RIL: send data registration request one by one

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.0M+, firefox32 wontfix, firefox33 wontfix, firefox34 fixed, b2g-v2.0 wontfix, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

RESOLVED FIXED
2.1 S2 (15aug)
blocking-b2g 2.0M+
Tracking Status
firefox32 --- wontfix
firefox33 --- wontfix
firefox34 --- fixed
b2g-v2.0 --- wontfix
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: jessica, Assigned: jessica)

References

Details

Attachments

(8 files, 2 obsolete files)

In cases where we need to attach data registration on demand, e.g. on multi-sim devices, we should wait for the previous data registration request response to send the next data registration request.
Attached patch patch, v1. (obsolete) — Splinter Review
Hi Hsinyi, I have made setDataRegistration() return a promise and reorganized a little bit part of the code. May I have your review? Thanks.
Attachment #8468251 - Flags: review?(htsai)
Comment on attachment 8468251 [details] [diff] [review]
patch, v1.

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

Thank you Jessica! Please see my comments below.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +822,4 @@
>          if (RILQUIRKS_DATA_REGISTRATION_ON_DEMAND) {
>            let radioInterface = connHandler.radioInterface;
> +          radioInterface.setDataRegistration(false)
> +            .then(() => execPendingDataCallRequest());

Ideally, as the name "_pendingDataCallRequest" presents itself, it should include all the following operations, and we don't need an extra call to |radioInterface.setDataRegistration(false)|. Can we try to move |radioInterface.setDataRegistration(false)| into that pending request?

::: dom/system/gonk/ril_worker.js
@@ +6709,5 @@
> +  options.success = (options.rilRequestError === 0);
> +  if (!options.success) {
> +    options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];
> +  }
> +  this.sendChromeMessage(options);

RIL_Worker may internally trigger data attach, such as line 6688. In the case, we don't need to sendChromeMessage to the main thread. Please address that well, thanks.
Attachment #8468251 - Flags: review?(htsai)
Attached patch patch, v2. (obsolete) — Splinter Review
Hsinyi, I have made some modifications according to review comment 2, not sure if it's better this way, could you help review again? Thanks.
Attachment #8468251 - Attachment is obsolete: true
Attachment #8468996 - Flags: review?(htsai)
Comment on attachment 8468996 [details] [diff] [review]
patch, v2.

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

I like the structure! I think it's clearer :) Thank you thank you~

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2814,5 @@
> +    this.workerMessenger.send("setDataRegistration",
> +                              {attach: attach},
> +                              (function(response) {
> +      // Always resolve to proceed with the following steps.
> +      deferred.resolve(response.errorMsg ? response.errorMsg : null);

Due to the existing structure design, we can't really do something for error handling, so this looks fine for now. :)

::: dom/system/gonk/ril_worker.js
@@ +6688,5 @@
>      this.setDataRegistration({attach: true});
>    }
>  };
> +RilObject.prototype[REQUEST_SET_DATA_SUBSCRIPTION] = function REQUEST_SET_DATA_SUBSCRIPTION(length, options) {
> +  if (options.rilMessageType == null) {

nit: if (!options.rilMessageType)

@@ +6709,5 @@
>    options.retryCount = length ? this.context.Buf.readInt32List()[0] : -1;
>    this.sendChromeMessage(options);
>  };
> +RilObject.prototype[RIL_REQUEST_GPRS_ATTACH] = function RIL_REQUEST_GPRS_ATTACH(length, options) {
> +  if (options.rilMessageType == null) {

ditto.
Attachment #8468996 - Flags: review?(htsai) → review+
Attached patch patch (final).Splinter Review
Thank you, Hsinyi!

Address nits in review comment 4.
Attachment #8468996 - Attachment is obsolete: true
Attachment #8469163 - Flags: review+
[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.0?
(In reply to shawn ku [:sku] from comment #9)
> [Blocking Requested - why for this release]:

woodduck needs this solution to make data connection works on both SIM.
(In reply to shawn ku [:sku] from comment #10)
> (In reply to shawn ku [:sku] from comment #9)
> > [Blocking Requested - why for this release]:
> 
> woodduck needs this solution to make data connection works on both SIM.

Hi Shawn, is it possible for them to try the patch first to see if it works? Thanks.
What is the user impact here?

What is the risk of not taking fix in 2.0?
needinfo on comment 10
Flags: needinfo?(sku)
(In reply to Jessica Jong (OOO Aug 19 - 31) [:jjong] [:jessica] from comment #11)
> (In reply to shawn ku [:sku] from comment #10)
> > (In reply to shawn ku [:sku] from comment #9)
> > > [Blocking Requested - why for this release]:
> > 
> > woodduck needs this solution to make data connection works on both SIM.
> 
> Hi Shawn, is it possible for them to try the patch first to see if it works?
> Thanks.

Hi Jessica:
 I will ask partner first, but I am afraid it might have some difficulty for partner to apply the patch since they use p4, not git.

Keep ni, before any response back from partner.
After apply the patch to 2.0, woodduck can get data service after change default data sim.
Still wait for partner to double confirm the result.
(In reply to shawn ku [:sku] from comment #15)
> After apply the patch to 2.0, woodduck can get data service after change
> default data sim.
> Still wait for partner to double confirm the result.

This cannot land on 2.0 as there is no impact to flame and the branch is strictly open for 2.0+. We are still discussing on how to proceed for patches needed for woodduck, so you should be hearing soon on how and where to land this. For now I'll keep the nom until we have the woodduck flags in place but please do not 2.0+ it.
(In reply to bhavana bajaj [:bajaj] from comment #16)
> (In reply to shawn ku [:sku] from comment #15)
> > After apply the patch to 2.0, woodduck can get data service after change
> > default data sim.
> > Still wait for partner to double confirm the result.
> 
> This cannot land on 2.0 as there is no impact to flame and the branch is
> strictly open for 2.0+. We are still discussing on how to proceed for
> patches needed for woodduck, so you should be hearing soon on how and where
> to land this. For now I'll keep the nom until we have the woodduck flags in
> place but please do not 2.0+ it.

understood, and thanks!!
Flags: needinfo?(sku)
Triage discussed and concluded we should not risk 2.0 branch stability for woodduck. Or at least not until other 2.0 branch consumers have cut their release. For now this should go where woodduck-only fixes go, once that's decided.
blocking-b2g: 2.0? → -
nominate 2.0M? per comment 10/15.
blocking-b2g: - → 2.0M?
Blocks: 1058585
2.0M+ as Woodduck needs this solution.
blocking-b2g: 2.0M? → 2.0M+
Blocks: Woodduck
Jessica, could you make a patch for v2.0m branch? Thanks!
Flags: needinfo?(jjong)
(In reply to Kai-Zhen Li from comment #21)
> Jessica, could you make a patch for v2.0m branch? Thanks!

I will help on this. ni? me for tracking.
Flags: needinfo?(jjong)
Flags: needinfo?(echen)
Patch for b2g32_2_0m
Flags: needinfo?(echen)
(In reply to Edgar Chen [:edgar][:echen] from comment #22)
> (In reply to Kai-Zhen Li from comment #21)
> > Jessica, could you make a patch for v2.0m branch? Thanks!
> 
> I will help on this. ni? me for tracking.

Thanks Edgar for the help! :)
Depends on: 1060217
Blocks: 1076703
Hi Norry,
Please verify on Woodduck 2.0M.
Flags: needinfo?(fan.luo)
Keywords: verifyme
Verification STR:

[Initial condition]
2 SIM cards, data connection is connected at SIM1.

[Step]
1 Switch data connection to SIM2
2 Browse some website with SIM2 connection
3 Switch connection back to SIM1
4 Browse some website with SIM1 connection

[Expected result]
Data connection should be switched smoothly without suffer from disconnection.
(In reply to Josh Cheng [:josh] from comment #28)
> Verification STR:
> 
> [Initial condition]
> 2 SIM cards, data connection is connected at SIM1.
> 
> [Step]
> 1 Switch data connection to SIM2
> 2 Browse some website with SIM2 connection
> 3 Switch connection back to SIM1
> 4 Browse some website with SIM1 connection
> 
> [Expected result]
> Data connection should be switched smoothly without suffer from
> disconnection.

Hi Josh
My test steps

1. 2 SIM cards, data connection is connected at SIM1
>You can browse the website successfully, SIM 1 data connection icon(PDH icon) is displayed,see figure:SIM1.png

2.Switch data connection to SIM2,Browse some website with SIM2 connection
>You can browse the website successfully,SIM 2 data connection icon(PDH icon) is displayed,see figure:SIM2.png

3.Switch data connection to SIM1 Again
>No Data connect icon(PDH icon) displayed and can not browse any website,see figure:No PDH icon.png

Test build information:

Gaia-Rev        fdb8236d7d99061ef6bedc021fd6b482e1af3f5a
Gecko-Rev       3d95c1683ef5eb017bd15bc38ba111ddb1ad792e
Build-ID        20141024050313
Version         32.0
Device-Name     soul35
FW-Release      4.4.2
FW-Incremental  1414098327
FW-Date         Fri Oct 24 05:05:49 CST 2014
Flags: needinfo?(fan.luo) → needinfo?(jocheng)
Attached image SIM1.png
Attached image SIM2.png
Attached image No PDH icon.png
Attached file logcat
(In reply to Eric from comment #33)
> Created attachment 8510867 [details]
> logcat

Eric, your log file does not match the screenshots, the log ends at 14:29, and I only see data connection switch to SIM 1 once:

> 10-24 05:33:36.534 I/Gecko   (  160): -*- DataConnectionManager: 'ril.data.defaultServiceId' is now 1

Could you help check if the attached log is correct? Thanks.
Flags: needinfo?(yongdong.xia)
(In reply to Jessica Jong [:jjong] [:jessica] from comment #34)
> (In reply to Eric from comment #33)
> > Created attachment 8510867 [details]
> > logcat
> 
> Eric, your log file does not match the screenshots, the log ends at 14:29,
> and I only see data connection switch to SIM 1 once:
> 
> > 10-24 05:33:36.534 I/Gecko   (  160): -*- DataConnectionManager: 'ril.data.defaultServiceId' is now 1
> 
> Could you help check if the attached log is correct? Thanks.

Default data connection is SIM1.

The attached log is recorded when we reproduce it for the first time, while the screenshot is captured when we reproduce for the second time. 

I reproduced this issue again  just now and attached the newest log for your reference.  Log time: 4:23PM-4:25pm.New log:New logcat.txt
Flags: needinfo?(yongdong.xia)
Attached file New logcat
Thanks Eric for reproducing again. From the new logcat, data registration is set on SIM 0, but it did not get registered:

Set data registration to SIM 0:

> 10-24 16:24:44.077 I/Gecko   (  163): RIL Worker: [0] Received chrome message {"attach":true,"rilMessageClientId":0,"rilMessageToken":43,"rilMessageType":"setDataRegistration"}

Request success:

> 10-24 16:24:44.169 I/Gecko   (  163): -*- RadioInterface[0]: Received message from worker: {"attach":true,"rilMessageClientId":0,"rilMessageToken":43,"rilMessageType":"setDataRegistration","rilRequestType":5018,"rilRequestError":0,"success":true}

Data registration is still in UNKNOWN (4) state:

10-24 16:24:44.265 I/Gecko   (  163): -*- RadioInterface[1]: Received message from worker: {"rilMessageType":"networkinfochanged","rilMessageClientId":1,"dataRegistrationState":{"regState":4,"state":null,"connected":false,"roaming":false,"emergencyCallsOnly":true,"cell":{"gsmLocationAreaCode":-1,"gsmCellId":-1},"radioTech":0,"type":null,"rilMessageType":"dataregistrationstatechange"},"signal":{"voice":{"signalStrength":-75,"relSignalStrength":100},"data":{"signalStrength":-75,"relSignalStrength":100},"rilMessageType":"signalstrengthchange"}}

Can someone from modem check this as well? Thanks.
(In reply to Jessica Jong [:jjong] [:jessica] from comment #37)
> Thanks Eric for reproducing again. From the new logcat, data registration is
> set on SIM 0, but it did not get registered:
> 
> Set data registration to SIM 0:
> 
> > 10-24 16:24:44.077 I/Gecko   (  163): RIL Worker: [0] Received chrome message {"attach":true,"rilMessageClientId":0,"rilMessageToken":43,"rilMessageType":"setDataRegistration"}
> 
> Request success:
> 
> > 10-24 16:24:44.169 I/Gecko   (  163): -*- RadioInterface[0]: Received message from worker: {"attach":true,"rilMessageClientId":0,"rilMessageToken":43,"rilMessageType":"setDataRegistration","rilRequestType":5018,"rilRequestError":0,"success":true}
> 
> Data registration is still in UNKNOWN (4) state:
> 
> 10-24 16:24:44.265 I/Gecko   (  163): -*- RadioInterface[1]: Received
> message from worker:
> {"rilMessageType":"networkinfochanged","rilMessageClientId":1,
> "dataRegistrationState":{"regState":4,"state":null,"connected":false,
> "roaming":false,"emergencyCallsOnly":true,"cell":{"gsmLocationAreaCode":-1,
> "gsmCellId":-1},"radioTech":0,"type":null,"rilMessageType":
> "dataregistrationstatechange"},"signal":{"voice":{"signalStrength":-75,
> "relSignalStrength":100},"data":{"signalStrength":-75,"relSignalStrength":
> 100},"rilMessageType":"signalstrengthchange"}}
> 
> Can someone from modem check this as well? Thanks.

Hi Reporter,

Could you check whether the problem is fixed with your latest ROM? Thanks!
Flags: needinfo?(jocheng) → needinfo?(wudduc)
Keywords: verifyme
BTW, I can switch to SIM 1 and back to SIM 0 with my local build, woodduck v2.0m:

[ro.build.fingerprint]: [alps/full_soul35/soul35:4.4.2/KOT49H/1412238667:eng/dev-keys]
[ro.mediatek.chip_ver]: [S01]
[ro.mediatek.gemini_support]: [true]
[ro.mediatek.platform]: [MT6572]
[ro.mediatek.version.branch]: [KK.AOSP]
[ro.mediatek.version.release]: [ALPS.W10.24.p0]
Attached file adb log radio.log
(In reply to Josh Cheng [:josh] from comment #28)
> Verification STR:
> 
> [Initial condition]
> 2 SIM cards, data connection is connected at SIM1.
> 
> [Step]
> 1 Switch data connection to SIM2
> 2 Browse some website with SIM2 connection
> 3 Switch connection back to SIM1
> 4 Browse some website with SIM1 connection
> 
> [Expected result]
> Data connection should be switched smoothly without suffer from
> disconnection.

I followed above steps, but I still can see the network disconnection problem on latest mozilla build.

Attached is the adb log. Test starts: 10-27 11:54:22.349
Steps:
0. Flash a new build, the default data SIM is SIM1
1. Browse a website, website load successfully
2. Change data SIM to SIM2
   --> It took about 20s for data connection icon appears on SIM2. If user load website during this time. They will see the network disconnection problem.
3. Change data SIM to SIM1
   --> It took over 10s for data connection icon appears on SIM1. If user load website during this time. They will see the network disconnection problem.
4. Change data SIM to SIM2
   --> Same as step 2. It took over 20s for data connection change.

Mozilla build
-------------------------------------------------------------------------
Gaia-Rev        fdb8236d7d99061ef6bedc021fd6b482e1af3f5a
Gecko-Rev       717b111a9f7102a3fe76cece8f21a5c794e3bddf
Build-ID        20141027050313
Version         32.0
Device-Name     soul35
FW-Release      4.4.2
FW-Incremental  1414357512
FW-Date         Mon Oct 27 05:05:35 CST 2014
(In reply to Peipei Cheng from comment #40)
> Created attachment 8511798 [details]
> adb log radio.log
> 
> (In reply to Josh Cheng [:josh] from comment #28)
> > Verification STR:
> > 
> > [Initial condition]
> > 2 SIM cards, data connection is connected at SIM1.
> > 
> > [Step]
> > 1 Switch data connection to SIM2
> > 2 Browse some website with SIM2 connection
> > 3 Switch connection back to SIM1
> > 4 Browse some website with SIM1 connection
> > 
> > [Expected result]
> > Data connection should be switched smoothly without suffer from
> > disconnection.
> 
> I followed above steps, but I still can see the network disconnection
> problem on latest mozilla build.
> 
> Attached is the adb log. Test starts: 10-27 11:54:22.349
> Steps:
> 0. Flash a new build, the default data SIM is SIM1
> 1. Browse a website, website load successfully
> 2. Change data SIM to SIM2
>    --> It took about 20s for data connection icon appears on SIM2. If user
> load website during this time. They will see the network disconnection
> problem.
> 3. Change data SIM to SIM1
>    --> It took over 10s for data connection icon appears on SIM1. If user
> load website during this time. They will see the network disconnection
> problem.
> 4. Change data SIM to SIM2
>    --> Same as step 2. It took over 20s for data connection change.
> 
> Mozilla build
> -------------------------------------------------------------------------
> Gaia-Rev        fdb8236d7d99061ef6bedc021fd6b482e1af3f5a
> Gecko-Rev       717b111a9f7102a3fe76cece8f21a5c794e3bddf
> Build-ID        20141027050313
> Version         32.0
> Device-Name     soul35
> FW-Release      4.4.2
> FW-Incremental  1414357512
> FW-Date         Mon Oct 27 05:05:35 CST 2014

It is normal that you experience a short disconnection while you switch default SIM for data call, this is because data registration needs to get unregistered and registered and it takes time. The latency of this disconnection depends on the modem/network.
Hi Jessica,
If the test result per comment 40 is expected. I think it's resolved until partner report same problem with longer disconnect time.
Thanks!
Hi Jessica,
Right after I saved my last comment. I see ODM partner submitted exact same issue as bug 1090133. I am requesting the reporter for snapshot and console log. Do you still need anything else? 
Thanks!
Flags: needinfo?(jjong)
(In reply to Josh Cheng [:josh] from comment #43)
> Hi Jessica,
> Right after I saved my last comment. I see ODM partner submitted exact same
> issue as bug 1090133. I am requesting the reporter for snapshot and console
> log. Do you still need anything else? 
> Thanks!

Thank you, Josh, I think we can discuss on that bug.
Flags: needinfo?(jjong)
Flags: needinfo?(wudduc)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: