1048723 - B2G RIL: send data registration request one by one

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Jessica Jong [:jessica] (inactive)]

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.

Comment 1

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

Hi Hsinyi, I have made setDataRegistration() return a promise and reorganized a little bit part of the code. May I have your review? Thanks.

Comment 2

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

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.

Comment 3

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v2.

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.

Comment 4

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

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.

Comment 5

[Jessica Jong [:jessica] (inactive)]

Attachment: patch (final).

Thank you, Hsinyi!

Address nits in review comment 4.

Comment 6

[Jessica Jong [:jessica] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=0f544f8f0b69

try green!

Comment 7

[Jessica Jong [:jessica] (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/0c0a98b643b5

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0c0a98b643b5

Comment 9

[shawn ku [:sku]]

[Blocking Requested - why for this release]:

Comment 10

[shawn ku [:sku]]

(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.

Comment 11

[Jessica Jong [:jessica] (inactive)]

(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.

Comment 12

[Preeti Raghunath(:Preeti)]

What is the user impact here?

What is the risk of not taking fix in 2.0?

Comment 13

[Jason Smith [:jsmith]]

needinfo on comment 10

Comment 14

[shawn ku [: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.

Comment 15

[shawn ku [:sku]]

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.

Comment 16

[bhavana bajaj [:bajaj]]

(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.

Comment 17

[shawn ku [:sku]]

(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!!

Comment 18

[Dietrich Ayala (:dietrich)]

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.

Comment 19

[shawn ku [:sku]]

nominate 2.0M? per comment 10/15.

Comment 20

[Francis Lee [:frlee]]

2.0M+ as Woodduck needs this solution.

Comment 21

[Kai-Zhen Li [:seinlin][:kli]]

Jessica, could you make a patch for v2.0m branch? Thanks!

Comment 22

[Edgar Chen [:edgar]]

(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.

Comment 23

[Edgar Chen [:edgar]]

Try result for 2.0m: https://tbpl.mozilla.org/?tree=Try&rev=6ff3c7493cc1

Comment 24

[Edgar Chen [:edgar]]

Attachment: [b2g_32_2_0m] patch, r=hsinyi

Patch for b2g32_2_0m

Comment 25

[Kai-Zhen Li [:seinlin][:kli]]

v2.0m: http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/b760e37130cd

Comment 26

[Jessica Jong [:jessica] (inactive)]

(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! :)

Comment 27

[Josh Cheng [:josh]]

Hi Norry,
Please verify on Woodduck 2.0M.

Comment 28

[Josh Cheng [:josh]]

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.

Comment 29

[Eric]

(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

Comment 30

[Eric]

Attachment: SIM1.png

Comment 31

[Eric]

Attachment: SIM2.png

Comment 32

[Eric]

Attachment: No PDH icon.png

Comment 33

[Eric]

Attachment: logcat

Comment 34

[Jessica Jong [:jessica] (inactive)]

(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.

Comment 35

[Eric]

(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

Comment 36

[Eric]

Attachment: New logcat

Comment 37

[Jessica Jong [:jessica] (inactive)]

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.

Comment 38

[Josh Cheng [:josh]]

(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!

Comment 39

[Jessica Jong [:jessica] (inactive)]

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]

Comment 40

[Peipei Cheng (needinfo if you need my action)]

Attachment: 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

Comment 41

[Jessica Jong [:jessica] (inactive)]

(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.

Comment 42

[Josh Cheng [:josh]]

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!

Comment 43

[Josh Cheng [:josh]]

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!

Comment 44

[Jessica Jong [:jessica] (inactive)]

(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.