Closed Bug 943191 Opened 11 years ago Closed 11 years ago

[fugu] [B2G] [DSDS] On 1st sim slot, it's hard to establish data call connection unless we manually select preferred network type as WCDMA only

Categories

(Firefox OS Graveyard :: RIL, defect)

Product:
Firefox OS Graveyard
Component:
RIL
Platform:
x86_64
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed)

Status:
VERIFIED FIXED
Milestone:
1.3 C1/1.4 S1(20dec)
Project Flags:
blocking-b2g 1.3+
Tracking Flags:
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed

People

(Reporter: echu, Assigned: jessica)

References

Details

(Whiteboard: [dsds_US_test])

Attachments

(6 files, 6 obsolete files)

fugu_data1.txt
2.01 MB, text/plain
Details
fugu_airplane.txt
1.01 MB, text/plain
Details
libreference-ril_sp.so
72.06 KB, application/x-sharedlib
Details
1206_fugu_auto_attach.log
539.31 KB, text/x-log
Details
[final] Part 1: move dsds data setup handling to RadioInterfaceLayer.
14.69 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
[final] Part 2: attach/detach data registration on demand (for fugu).
9.77 KB, patch
jessica
: review+
Details | Diff | Splinter Review
Attached file fugu_data1.txt — 
Found data call is hard to be established after flash build. Recovered by reboot and change network type. Please refer to reproduce steps.

* Build Number                
Gaia:     671a7c1fae1c32a77b2675636c7efdf089fd6443
Gecko:    b79ea773754854e17566f302e80e2654d9c155c9
BuildID   20131126092608
Version   28.0a1

* Reproduce Steps
Marked with time stamp of fugu_data1.txt
1. Flash latest code.
2. Enable data call during FTE (12:03, 11/26)
3. After enter home screen, data call is not seen.
4. Go to Settings, change network mode from WCDMA preferred to WCDMA, data call is established. (12:04)
5. Browser website, data call icon disappears(12:05)
6. Switch network mode and can't get data call still (12:06)
Marked with time stamp of fugu_airplane.txt
7. Reboot device and change network type to WCDMA preferred again. Data call is established. (12:11, 11/26)

* Expected Result
Data call should be established no matter in WCDMA or WCDMA preferred mode.

* Occurrence rate
2/2
blocking-b2g: --- → 1.3?
Whiteboard: [FT:RIL]
Attached file fugu_airplane.txt — 

    
    

    
  
Jessica, need your help.
Assignee: nobody → jjong

    
  
Whiteboard: [FT:RIL] → [dsds_US_test]

    
    

    
  
This should be a known issue, PS is in "searching" state until user selects the preferred network type in Settings. I've talked to Shawn, and it should be a modem issue related to preferred network type.

    
  
Summary: [fugu] Hard to establish data call connection. → [fugu][DSDS] Hard to establish data call connection.

    
    

    
  
RIL Triage: 1.3+
blocking-b2g: 1.3? → 1.3+

    
  
Summary: [fugu][DSDS] Hard to establish data call connection. → [DSDS] Hard to establish data call connection.

    
    

    
  
Please help to check:
"I/Gecko   (  105): -*- RadioInterface[0]: 'ril.radio.preferredNetworkType' is now 
I/Gecko   (  105): -*- RadioInterface[0]: 'ril.data.roaming_enabled' is now false
I/Gecko   (  105): -*- RadioInterface[0]: We haven't gotten completely the APN data.
I/Gecko   (  105): -*- RadioInterface[0]: 'ril.data.enabled' is now false
I/Gecko   (  105): -*- RadioInterface[0]: 'ril.data.apnSettings' is now ""
I/Gecko   (  105): -*- RadioInterface[0]: 'ril.data.defaultServiceId' is now 0
I/Gecko   (  105): -*- RadioInterface[0]: 'ril.cellbroadcast.searchlist' is now 
"
This values should be get from setting DB, any problem when reading from or writing to DB?

Thanks!

    
    

    
  
I would recommend you to take a look at bug 928297 too. That is the Gaia side we are working on to support data calls on multi ICC devices.
See Also:  → 928297

    
    

    
  
(In reply to sam.hua from comment #5)
> Please help to check:
> "I/Gecko   (  105): -*- RadioInterface[0]: 'ril.radio.preferredNetworkType'
> is now 
> I/Gecko   (  105): -*- RadioInterface[0]: 'ril.data.roaming_enabled' is now
> false
> I/Gecko   (  105): -*- RadioInterface[0]: We haven't gotten completely the
> APN data.
> I/Gecko   (  105): -*- RadioInterface[0]: 'ril.data.enabled' is now false
> I/Gecko   (  105): -*- RadioInterface[0]: 'ril.data.apnSettings' is now ""
> I/Gecko   (  105): -*- RadioInterface[0]: 'ril.data.defaultServiceId' is now
> 0
> I/Gecko   (  105): -*- RadioInterface[0]: 'ril.cellbroadcast.searchlist' is
> now 
> "
> This values should be get from setting DB, any problem when reading from or
> writing to DB?
> 
> Thanks!

Hi Sam,

Where did you find these logs? I can not find them in the attached log files..

I think the problem here is:
11-26 12:02:56.426 I/Gecko   (  105): -*- RadioInterface[0]: 'ril.data.enabled' is now true
11-26 12:02:56.426 I/Gecko   (  105): -*- RadioInterface[0]: RIL is not ready for data connection: Phone's not registered or doesn't have data connection.

this is due to data registration not registered while voice registration seems okay:
11-26 12:02:54.376 I/Gecko   (  294): -*- RILContentHelper: Received message 'RIL:VoiceInfoChanged': {"clientId":0,"data":{"connected":true,"emergencyCallsOnly":false,"roaming":false,"network":{"longName":"Taiwan Cellular Corporation","shortName":"TCC","mcc":"466","mnc":"97"},"cell":{"gsmLocationAreaCode":11115,"gsmCellId":9111196},"type":"umts","signalStrength":-73,"relSignalStrength":100,"state":"registered"}}
11-26 12:02:54.376 I/Gecko   (  294): -*- RILContentHelper: Received message 'RIL:DataInfoChanged': {"clientId":0,"data":{"connected":false,"emergencyCallsOnly":false,"roaming":false,"network":null,"cell":null,"type":null,"signalStrength":null,"relSignalStrength":null,"state":"searching"}}

This happens in lots of devices here, we need to manually select preferred network type in order to get data registration registered. Doesn't this happen on your side?

Thank you.
Flags: needinfo?(sam.hua)

    
    

    
  
Hi jessica,

1. I cat these logs by I changed ril_const.js and set debug flag to true. 
2. data registration process in dual SIM mode is different with single SIM card mode.
Flags: needinfo?(sam.hua)

    
    

    
  
(In reply to sam.hua from comment #8)
> Hi jessica,
> 
> 1. I cat these logs by I changed ril_const.js and set debug flag to true. 
> 2. data registration process in dual SIM mode is different with single SIM
> card mode.

Thanks for your response.
Just wanted to make things clearer. This bug is about getting data connection in SIM1 and bug 944231 is about getting data connection in SIM2.

Currently for us, after boot up, data registration for both SIMs are in searching state while voice registrations are in registered state. After selecting manually the preferred network type, we can get data registration of SIM1 registered, at that point data connection can be established successfully on SIM1 (this is what this bug is reporting). However, I can not get data registration registered on SIM2 no matter how I select the preferred network type, this is reported in bug 944231.

You said that data registration process in dual SIM mode is different, what exactly is the difference? Are there extra steps that we need to take care of?

Thank you.

    
    

    
  
It is impossible to let two sim cards keep data connection at the same time.So our modem just keep data connection when it is necessary. And,SIM 2 will always work in GSM only.so it is useless to set preferred network to SIM 2.
Maybe u can force the perferredNetworkType to GSM Only in the Gecko and check what happened.

    
    

    
  
(In reply to sam.hua from comment #10)
> It is impossible to let two sim cards keep data connection at the same
> time.So our modem just keep data connection when it is necessary. And,SIM 2
> will always work in GSM only.so it is useless to set preferred network to
> SIM 2.
> Maybe u can force the perferredNetworkType to GSM Only in the Gecko and
> check what happened.

Sam, we are not trying to "let two sim cards keep data connection at the same time", we just want to be able to use data connection in SIM1 and then be able to "switch to" SIM2. Data connection from only ONE sim will be established at the same time.
But we need to have "data registration" registered before setting up data connection [1], so here are the two issues found:

1. we can get "data registration" registered on SIM1 only after manually selecting preferred network type (this bug)

2. we can't get "data registration" registered on SIM2 even after selecting GSM only for preferred network type (bug 944234)

I think Shawn will explain to you on your conference call today. Thank you.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#1781
Summary: [DSDS] Hard to establish data call connection. → [fugu] [B2G] [DSDS] On 1st sim slot, it's hard to establish data call connection unless we manually select preferred network type as WCDMA only

    
    

    
  
(In reply to Jessica Jong [:jjong] [:jessica] from comment #11)
> (In reply to sam.hua from comment #10)
> > It is impossible to let two sim cards keep data connection at the same
> > time.So our modem just keep data connection when it is necessary. And,SIM 2
> > will always work in GSM only.so it is useless to set preferred network to
> > SIM 2.
> > Maybe u can force the perferredNetworkType to GSM Only in the Gecko and
> > check what happened.
> 
> Sam, we are not trying to "let two sim cards keep data connection at the
> same time", we just want to be able to use data connection in SIM1 and then
> be able to "switch to" SIM2. Data connection from only ONE sim will be
> established at the same time.
> But we need to have "data registration" registered before setting up data
> connection [1], so here are the two issues found:
> 
> 1. we can get "data registration" registered on SIM1 only after manually
> selecting preferred network type (this bug)
> 
> 2. we can't get "data registration" registered on SIM2 even after selecting
> GSM only for preferred network type (bug 944234)
> 
> I think Shawn will explain to you on your conference call today. Thank you.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/
> RadioInterfaceLayer.js#1781

Correct link:
[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#1810

    
    

    
  
Hi
We should get the right APN settings for sim2, then we can setup the data call.But i can't find the correct settings for SIM2 in the log.
Could u do a fake apn settings in the RadioInterfaceLayer for SIM2?

    
    

    
  
I can't find the way to set the apn and ril.data.enabled of SIM2 by UI operation.
how to do it?

    
    

    
  
Sorry, I think i mistake the bugs..
It is for SIM1 to establish data link.

    
    

    
  
(In reply to sam.hua from comment #13)
> Hi
> We should get the right APN settings for sim2, then we can setup the data
> call.But i can't find the correct settings for SIM2 in the log.
> Could u do a fake apn settings in the RadioInterfaceLayer for SIM2?

Please refer to comment 6, the gaia part is in Bug 928297.
And to enable data connection on SIM2, first go to Settings -> SIM Manager, and choose SIM 2 for DATA, then go to Settings -> Cellular & Data -> SIM 2 to turn on data connection on SIM 2. The SIM Manager patch has landed already, see bug 932731.

    
    

    
  
FYI I am able to setup data calls for ICC card 1 without problems. By applying the patch from bug 933203 once the device boot the APN settings for the two ICC cards should get configured.

    
    

    
  
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #17)
> FYI I am able to setup data calls for ICC card 1 without problems. By
> applying the patch from bug 933203 once the device boot the APN settings for
> the two ICC cards should get configured.

Jose, you mean you don't need to manually select preferred network type to make ICC card 1 data connection work? That's strange... does this happen only in Taipei?

    
    

    
  
It is same in Shanghai,i have to change preferred network to make sim1 setup the data call.

    
    

    
  
(In reply to Jessica Jong [:jjong] [:jessica] from comment #18)
> (In reply to José Antonio Olivera Ortega [:jaoo] from comment #17)

There is no need in my case to change the preferred network type. Data calls for the ICC card 1 work correctly with Movistar ICC card in Spain.

    
    

    
  
Thanks José and sam for the feedback.
sam, were you able to find the cause of this?

    
    

    
  
Hi Jessic,

I updated my code today, and now I can use data network just by open the data connection.

gecko:0e3362fb5625eb6d98c7617b1b3019a2cc553d47
gaia: f64d282b6b44eaaf8cddc8be8f3250cb6fbacd95

    
    

    
  
I get log:
I/Gecko   (  105): -*- RadioInterface[0]: 'ril.data.enabled' is now true
I/Gecko   (  105): -*- RadioInterface[0]: Data call settings: connect data call.
I/Gecko   (  105): -*- RadioInterface[0]: Registering callback: [object Object]
I/Gecko   (  105): -*- RILNetworkInterface[0:1]: Going to set up data connection with APN 3gnet
I/Gecko   (  105): -*- RILNetworkInterface[0:1]: Invalid authType undefined
I/Gecko   (  105): RIL Worker[0]: Received chrome message {"radioTech":11,"apn":"3gnet","chappap":3,"pdptype":"IP","rilMessageToken":11,"rilMessageType":"setupDataCall"}

I think that's reason.

    
    

    
  
(In reply to sam.hua from comment #23)
> I get log:
> I/Gecko   (  105): -*- RadioInterface[0]: 'ril.data.enabled' is now true
> I/Gecko   (  105): -*- RadioInterface[0]: Data call settings: connect data
> call.
> I/Gecko   (  105): -*- RadioInterface[0]: Registering callback: [object
> Object]
> I/Gecko   (  105): -*- RILNetworkInterface[0:1]: Going to set up data
> connection with APN 3gnet
> I/Gecko   (  105): -*- RILNetworkInterface[0:1]: Invalid authType undefined
> I/Gecko   (  105): RIL Worker[0]: Received chrome message
> {"radioTech":11,"apn":"3gnet","chappap":3,"pdptype":"IP","rilMessageToken":
> 11,"rilMessageType":"setupDataCall"}
> 
> I think that's reason.

I'm sorry I don't understand, what's the reason?
Note that if data registration state is not registered, you will not reach these lines in the logs, you'll get:
12-02 18:48:24.680 I/Gecko   (  105): -*- RadioInterface[0]: 'ril.data.enabled' is now true
12-02 18:48:24.680 I/Gecko   (  105): -*- RadioInterface[0]: RIL is not ready for data connection: Phone's not registered or doesn't have data connection.
Flags: needinfo?(sam.hua)

    
    

    
  
Hi Jessica,
I means if we get the right APN settings and setup the data call, we will have the data connection.
So if u fail, u could find out 
1. have u get the right APN settings ?
2. have u setup the data call? if have not, maybe u should find what cause it.
Flags: needinfo?(sam.hua)

    
    

    
  
(In reply to sam.hua from comment #25)
> Hi Jessica,
> I means if we get the right APN settings and setup the data call, we will
> have the data connection.
> So if u fail, u could find out 
> 1. have u get the right APN settings ?
> 2. have u setup the data call? if have not, maybe u should find what cause
> it.

What I have been trying to say is that the PRECONDITION for setting up data call is to have data registration registered. If data registration is not registered, RIL will not try to setup data call.
So, to answer your questions...
1. Yes
2. No, it is because data registration is in searching state, which is the problem reported in this bug.

    
    

    
  
In our design of dual cards, we can setup data call even if data isn't registed

    
    

    
  
(In reply to sam.hua from comment #27)
> In our design of dual cards, we can setup data call even if data isn't
> registed

Then you are not following AOSP design. Could you solve this on modem side?
Thank you.

    
    

    
  
It is modified to support data service of two cards.

    
    

    
  
for example.
In dsds mode,if u are using data service of SIM1 and want to send MMS by SIM2,you will have to deactivate the SIM1 data connection first then setup data connection of SIM2.

    
    

    
  
(In reply to sam.hua from comment #30)
> for example.
> In dsds mode,if u are using data service of SIM1 and want to send MMS by
> SIM2,you will have to deactivate the SIM1 data connection first then setup
> data connection of SIM2.

Yes, we have taken care of that too, see bug 926372. But the precondition for setting data call is still the same: to have data registration registered.

    
    

    
  
In ril_worker.js
  setRadioEnabled: function setRadioEnabled(options) {
    Buf.newParcel(REQUEST_RADIO_POWER, options);
    Buf.writeInt32(1);
    Buf.writeInt32(options.enabled ? 1 : 0);
    Buf.sendParcel();

changed to 
  setRadioEnabled: function setRadioEnabled(options) {
    Buf.newParcel(REQUEST_RADIO_POWER, options);
    Buf.writeInt32(2);
    Buf.writeInt32(options.enabled ? 1 : 0);
    Buf.writeInt32(CLIENT_ID?0:1);  //SIM1 1;SIM2 0;
    Buf.sendParcel();

this can set SIM1 to auto attach the data network,u can try to get the precondition.

    
    

    
  
Hi Sam:
 Why add extra parameter in ril_worker.js to make auto attach work?
If this is a place to make things work, is it possible to do the stuffs in RILD instead?

For pure AOSP, this is not done by telephony framework side, IMO, it is not proper to do such specific things in gecko either.

Please let me know any comments you have.
Thanks!!
sku

    
    

    
  
(In reply to shawn ku [:sku] from comment #33)
> Hi Sam:
>  Why add extra parameter in ril_worker.js to make auto attach work?
> If this is a place to make things work, is it possible to do the stuffs in
> RILD instead?
> 
> For pure AOSP, this is not done by telephony framework side, IMO, it is not
> proper to do such specific things in gecko either.
> 
> Please let me know any comments you have.
> Thanks!!
> sku

AOSP don't support DSDS, it's sprd DSDS implement, just like AOSP has no FM implement. You can add sprd or DSDS flag.

    
    

    
  
Hi Jessica,

"autoAttach" is defined in the Settings, if we choose SIM1 as default data sim card, it should be set to "1", and the other SIM card won't try to register data network at the beginning.I think gecko should add this control to support this setting.

and, sprd extend the interface of rilc, REQUEST_GPRS_ATTACH,REQUEST_GPRS_DETACH,to control the data network.it could be found in sprd_ril.h.
Gecko can use them to attach/detach when switch the simcard for data service.

    
    

    
  
(In reply to James Zhang from comment #34)
> (In reply to shawn ku [:sku] from comment #33)
> > Hi Sam:
> >  Why add extra parameter in ril_worker.js to make auto attach work?
> > If this is a place to make things work, is it possible to do the stuffs in
> > RILD instead?
> > 
> > For pure AOSP, this is not done by telephony framework side, IMO, it is not
> > proper to do such specific things in gecko either.
> > 
> > Please let me know any comments you have.
> > Thanks!!
> > sku
> 
> AOSP don't support DSDS, it's sprd DSDS implement, just like AOSP has no FM
> implement. You can add sprd or DSDS flag.

Hi James/Sam:
 Thanks for lecture me the fugu behavior.

My original though is:
 1. RILD can use +CGATT=1 to auto attach PS domain during boot-up, then we got both SIM with PS auto attached.
 2. radioInterfaceLayer will do the control of which SIM is used to setupData by user's desire. (there is a setting to control this part as you said)

BTW, I said pure AOSP behavior is for single SIM w/o any modification from Google definitely.

Please correct me if anything is wrong.

Thanks!!
shawn

    
    

    
  
Hi jessica,
Only one sim card could be auto attached.
if u switch the card for PS, the active one should disconnect data link and DEATTCH GRPS, 
then the other could do attach and setup data call.

    
    

    
  
could u give the AOSP link to me?
Thanks!

    
    

    
  
You can get the AOSP from the instructions of below link:

http://source.android.com/source/downloading.html

And,
The latest version is "android-4.3_r2.1"

    
    

    
  
The patch in comment 32 doesn't work. Using REQUEST_GPRS_ATTACH/REQUEST_GPRS_DETACH mentioned in comment 35 does work, but we will need to add these extra quirks for fugu devices. :(

What is more, do we need to send GPRS_ATTACH request after airplanemode off/on? What if we lost PS registration temporary, do we need to send GPRS_ATTACH again?
Flags: needinfo?(sam.hua)

    
    

    
  
I think we should add the control in the gecko. 

Don't need it in the cases u mentioned.
Flags: needinfo?(sam.hua)

    
    

    
  
I will try to make the "AutoAttach" flag to work, it's a important flag to modem.I will info u if I succeed.

    
    

    
  
Hi jessica,
I updated libreference-ril_sp.so, and please use comment 32. 

can u try it?

if it is ok,i will update it in the github.

    
    

    
  

      
      
      
      

        Attached file
          
        libreference-ril_sp.so
        — 
      

    


  

    
    

    
  
(In reply to sam.hua from comment #43)
> Hi jessica,
> I updated libreference-ril_sp.so, and please use comment 32. 
> 
> can u try it?
> 
> if it is ok,i will update it in the github.

I used the new libreference-ril_sp.so but it does not work. From the logs, I see libreference-ril_sp sending 'AT+SAUTOATT=0' to modem, but the PS are still always in searching state.

    
    

    
  
It should send "AT+SAUTOATT=1" for SIM card1 to auto attach gprs.
could u give me the log?

    
    

    
  


  
Log for auto-attach.

    
    

    
  
(In reply to sam.hua from comment #46)
> It should send "AT+SAUTOATT=1" for SIM card1 to auto attach gprs.
> could u give me the log?

I have attached the logs.
From the SENDTRUE spec I googled, 0 is auto-attach?
description
0   Set to auto attach
1   Set to manual attach(cancel auto attach)
Note: Command AT+SAUTOATT is SENDTRUE’s specific.

    
    

    
  
Hi jessica,

You can ask Ken to get the spec for at commands of our modem.

Actually I can't find the description of the SAUTOATT command in the document, but i think it should be AT+SAUTOATT=1 to set auto-attach from the code in Android Framework.

Do u send it with 1 for SIM1? I find that u sent AT+AUTOATT=0 for SIM1 in the log.

    
    

    
  
(In reply to sam.hua from comment #49)
> Hi jessica,
> 
> You can ask Ken to get the spec for at commands of our modem.
> 
> Actually I can't find the description of the SAUTOATT command in the
> document, but i think it should be AT+SAUTOATT=1 to set auto-attach from the
> code in Android Framework.
> 
> Do u send it with 1 for SIM1? I find that u sent AT+AUTOATT=0 for SIM1 in
> the log.

I just applied the patch in comment 32. So, for SIM1 (CLIENT_ID 0), it would send 1 and for SIM2 (CLIENT_ID 1), it would send 0.

    
    

    
  
Hi sam,

Sorry, I think I missed something before. Now I can see SIM1's data registration state registered.
But how do we switch to SIM2? We can not call setRadioEnabled() each time we want to switch the default SIM for data call...
Flags: needinfo?(sam.hua)

    
    

    
  
Hi Jessica,
if u switch the card for PS, the active one should disconnect data link and DEATTCH GRPS, then the other could do attach and setup data call.

maybe u should add a new process for DSDS data control in gecko instead of reusing the setRadioEnabled().
Flags: needinfo?(sam.hua)

    
    

    
  
(In reply to sam.hua from comment #52)
> Hi Jessica,
> if u switch the card for PS, the active one should disconnect data link and
> DEATTCH GRPS, then the other could do attach and setup data call.
> 
> maybe u should add a new process for DSDS data control in gecko instead of
> reusing the setRadioEnabled().

So we still need to use REQUEST_GPRS_ATTACH/REQUEST_GPRS_DETACH as mentioned in comment 40, right? Then, what is the auto-attach flag in setRadioEnabled() for?
And, does gecko need to send REQUEST_GPRS_ATTACH each time we leave airplane mode? Will modem auto-attach after a temporary lost of data registration state?

Thanks.
Flags: needinfo?(sam.hua)

    
    

    
  
Yes, we need REQUEST_GPRS_ATTACH/DETACH action in the switch process.
When we want to use SIM2 in the case of default PS is SIM1,
we should:
1. deactive SIM1 and detach SIM1
2. attach SIM2 and setup data call
3. deactive SIM2 and detach SIM2 when ps completed
4. attach SIM1 and setup data call SIM1.

or should be :
1. Switching to SIM2 for PS
2. PS transaction
3. Switching back to SIM1.

The auto flag:
It is to indicate the default sim card for PS.So we don't need to send REQUEST_GPRS_ATTACH when we leave airplane mode, the modem should know which sim card will auto attach the PS and which one won't.
The auto flag is changed only in the case of the change for default SIM card of PS.
Flags: needinfo?(sam.hua)

    
    

    
  
(In reply to sam.hua from comment #54)
> Yes, we need REQUEST_GPRS_ATTACH/DETACH action in the switch process.
> When we want to use SIM2 in the case of default PS is SIM1,
> we should:
> 1. deactive SIM1 and detach SIM1
> 2. attach SIM2 and setup data call
> 3. deactive SIM2 and detach SIM2 when ps completed
> 4. attach SIM1 and setup data call SIM1.
> 
> or should be :
> 1. Switching to SIM2 for PS
> 2. PS transaction
> 3. Switching back to SIM1.
> 
> The auto flag:
> It is to indicate the default sim card for PS.So we don't need to send
> REQUEST_GPRS_ATTACH when we leave airplane mode, the modem should know which
> sim card will auto attach the PS and which one won't.
> The auto flag is changed only in the case of the change for default SIM card
> of PS.

But it seems that the auto-attach flag can be sent only in setRadioEnable(), how can we set the auto-attach flag if we are only switching the default SIM for PS?

    
    

    
  
Hi Jessica,
yes,but the flag seems to do work after setRadioEnable().

It is a bit complex in data control of DSDS in our Android framework.

So ,could u give me design documents about DSDS?

    
    

    
  
(In reply to sam.hua from comment #56)
> Hi Jessica,
> yes,but the flag seems to do work after setRadioEnable().
> 
> It is a bit complex in data control of DSDS in our Android framework.
> 
> So ,could u give me design documents about DSDS?

PLease refer to: https://bug917705.bugzilla.mozilla.org/attachment.cgi?id=8342704

    
    

    
  
Hi jessica,

You can get the manifest.xml from Ken of our android framework.
The two new files,MsmsGsmDataConnectionTracker.java and MsmsGsmDataConnectionTrackerProxy.java, are used to control the PS in DSDS.
Maybe u can reference it.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        (WIP) proposed patch. (obsolete)
        — 
          — Splinter Review
      

    


  
Attach/detach data registration on demand. This is needed for fugu devices only.

    
  

        Attachment #8345145 -
        Flags: feedback?(htsai)

    
    

    
  
Comment on attachment 8345145 [details] [diff] [review]
(WIP) proposed patch.

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

The whole concept looks right though I am still curious about how our partner's android framework handles this. Would you please share some information such as when they manually attach/detach data registration, Jessica? Thanks!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1828,5 @@
> +      if (this._dataEnabled) {
> +        this.dataCallSettings.oldEnabled = this.dataCallSettings.enabled;
> +        this.dataCallSettings.enabled = true;
> +        if (gNetworkManager.active &&
> +            gNetworkManager.active.type > Ci.nsINetworkInterface.NETWORK_TYPE_WIFI) {

Since the flag 'this._dataEnabled' is used to indicate the user preference for only default type data setting, looks Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE is the only type we care here, no?

@@ +1832,5 @@
> +            gNetworkManager.active.type > Ci.nsINetworkInterface.NETWORK_TYPE_WIFI) {
> +          if (DEBUG) this.debug("Another data active, wait for it to get disconnected.");
> +          this._dataCallSetupPending = true;
> +          return;
> +        }

nit: add an empty line

@@ +1841,2 @@
>          return;
>        }

nit: add an empty line

@@ +1852,5 @@
> +        this.dataCallSettings.oldEnabled = this.dataCallSettings.enabled;
> +        this.dataCallSettings.enabled = false;
> +        this.updateRILNetworkInterface();
> +        return;
> +      }

nit: add an empty line

@@ +2233,5 @@
> +    // Detach data registration after data call is deactivated.
> +    if (RILQUIRKS_ATTACH_DATA_REG_ON_DEMAND &&
> +        datacall.state == RIL.GECKO_NETWORK_STATE_UNKNOWN &&
> +        this.defaultServiceId != this.clientId) {
> +      this.workerMessenger.send("setDataRegistration", {attach: false});

Oh, do we really need to detach data registration? What if we don't?

::: dom/system/gonk/ril_worker.js
@@ +1893,5 @@
> +  setDataRegistration: function setDataRegistration(options) {
> +    let request = options.attach ? RIL_REQUEST_GPRS_ATTACH :
> +                                   RIL_REQUEST_GPRS_DETACH;
> +    Buf.simpleRequest(request);
> +  },

Please don't forget to add request handlers, i.e. 
RIL[RIL_REQUEST_GPRS_ATTACH] = null;
RIL[RIL_REQUEST_GPRS_DETACH] = null;

        Attachment #8345145 -
        Flags: feedback?(htsai)

    
    

    
  
(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #60)
> Comment on attachment 8345145 [details] [diff] [review]
> (WIP) proposed patch.
> 
> Review of attachment 8345145 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The whole concept looks right though I am still curious about how our
> partner's android framework handles this. Would you please share some
> information such as when they manually attach/detach data registration,
> Jessica? Thanks!

No problem! downloading partner's android code...

> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1828,5 @@
> > +      if (this._dataEnabled) {
> > +        this.dataCallSettings.oldEnabled = this.dataCallSettings.enabled;
> > +        this.dataCallSettings.enabled = true;
> > +        if (gNetworkManager.active &&
> > +            gNetworkManager.active.type > Ci.nsINetworkInterface.NETWORK_TYPE_WIFI) {
> 
> Since the flag 'this._dataEnabled' is used to indicate the user preference
> for only default type data setting, looks
> Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE is the only type we care here, no?

Actually, this is a workaround for a timing issue that I met: if the RadioInterface, that is becoming non-default for data, observes the 'ril.data.defaultServiceId' first, it will disconnect its data call and remove the data type from RILNetworkInterface.connectedTypes; later when the other RadioInterface, that is becoming default for data, observes the change and check for gNetworkManager.active.type, it will return 'NETWORK_TYPE_MOBILE_OTHERS' since it is no longer in RILNetworkInterface.connectedTypes.

By the way, we are not handling data default service id change for non-default data calls, i.e. switching default sim for data while a mms send is in progress. I think lot of code will need to be changed to handle this case, should we fix it for 1.3?

> 
> @@ +1832,5 @@
> > +            gNetworkManager.active.type > Ci.nsINetworkInterface.NETWORK_TYPE_WIFI) {
> > +          if (DEBUG) this.debug("Another data active, wait for it to get disconnected.");
> > +          this._dataCallSetupPending = true;
> > +          return;
> > +        }
> 
> nit: add an empty line

will do.

> 
> @@ +1841,2 @@
> >          return;
> >        }
> 
> nit: add an empty line

will do.

> 
> @@ +1852,5 @@
> > +        this.dataCallSettings.oldEnabled = this.dataCallSettings.enabled;
> > +        this.dataCallSettings.enabled = false;
> > +        this.updateRILNetworkInterface();
> > +        return;
> > +      }
> 
> nit: add an empty line

will do.

> 
> @@ +2233,5 @@
> > +    // Detach data registration after data call is deactivated.
> > +    if (RILQUIRKS_ATTACH_DATA_REG_ON_DEMAND &&
> > +        datacall.state == RIL.GECKO_NETWORK_STATE_UNKNOWN &&
> > +        this.defaultServiceId != this.clientId) {
> > +      this.workerMessenger.send("setDataRegistration", {attach: false});
> 
> Oh, do we really need to detach data registration? What if we don't?

From my tests, if we don't detach here, the other SIM's data registration can still be attached, but when setting up data connection, modem always return failure in this case. It seems that only one SIM's data registration can be attached at the same time.

> 
> ::: dom/system/gonk/ril_worker.js
> @@ +1893,5 @@
> > +  setDataRegistration: function setDataRegistration(options) {
> > +    let request = options.attach ? RIL_REQUEST_GPRS_ATTACH :
> > +                                   RIL_REQUEST_GPRS_DETACH;
> > +    Buf.simpleRequest(request);
> > +  },
> 
> Please don't forget to add request handlers, i.e. 
> RIL[RIL_REQUEST_GPRS_ATTACH] = null;
> RIL[RIL_REQUEST_GPRS_DETACH] = null;

Thanks for reminding me this. :)

    
    

    
  
Hsinyi, after a quick scan, the following are what I found in partner's android code:

On radio power, RIL will set the auto-attach flag on the default sim for data.
(We are not doing this, instead we directly attach data registration on the default data sim for data.)

When switching default sim for data (and setting up data connection), their RIL will check the 'mAutoAttachOnCreation' flag, if this flag is set means they don't need data registarion to setup data connection.
Besides from that, their procedure is similar to what we have done:
1. disconnect all data calls on previous default sim for data.
2. detach data registration on previous default sim for data after data calls are disconnected.
3. If 'mAutoAttachOnCreation' is false, attach data registration on current default sim for data, otherwise setup data connection directly on current default sim for data.

You can check out the checkAndSwitchPhone() and onDisconnectDone() in MsmsGsmDataConnectionTrackerProxy.java

    
    

    
  


  
We are moving dsds data setup handling to RadioInterfaceLayer, so that we can monitor data calls from every client correctly. We should move these to 'DataConnectionManager' when it is ready, see bug 905568.

    
    

    
  


  

        Attachment #8346336 -
        Attachment is obsolete: true

        Attachment #8346368 -
        Flags: review?(htsai)

    
  

        Attachment #8346338 -
        Flags: review?(htsai)

    
    

    
  
Comment on attachment 8346368 [details] [diff] [review]
Part 1: move dsds data setup handling to RadioInterfaceLayer

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

Hi Jessica, thanks for the patch. I like the idea of move the data setup dandling to RadioInterfaceLayer, and I believe it's the right way to go. Please see my other questions and comments below. Thank you!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +718,5 @@
> +  _dataEnabled: null,
> +
> +  // Flag to record the default client id for data call.  It
> +  // corresponds to the 'ril.data.defaultServiceId' setting from the UI.
> +  _dataDefaultClientId: null,

Just realized that we should set this value to default '0' in constructor in case gaia doesn't have this settings key in single sim scenario.

@@ +727,5 @@
> +  _currentDataClientId: null,
> +
> +  // Flag to determine if we need to setup data call when we are notified
> +  // that another data call has been disconnected.
> +  _dataCallSetupPending: null,

I'd like to have _pendingDataCallRequest instead so that once we have _pendingDataCallRequest, we simply call |this._pendingDataCallRequest()| to execute it. More ideas on at line #839.

@@ +743,5 @@
> +      case kNetworkInterfaceStateChangedTopic:
> +        let network = subject.QueryInterface(Ci.nsINetworkInterface);
> +        // DSDS: setup pending data connection when switching the default id
> +        // for data call.
> +        // We can not use network.type to tell if it's NETWORK_TYPE_MOBILE,

nit: move line #747 to the end of line #746, and wrap at 80th character.

@@ +747,5 @@
> +        // We can not use network.type to tell if it's NETWORK_TYPE_MOBILE,
> +        // since the type is removed from RILNetworkInterface.connectedTypes
> +        // on disconnect().
> +        if (network.state == Ci.nsINetworkInterface.NETWORK_STATE_UNKNOWN) {
> +          let oldRadioInterface = this.radioInterfaces[this._currentDataClientId];

nit: warp at 80th, so rewrite the line as

let oldRadioInterface = 
    this.radioInterfaces[this._currentDataClientId];

@@ +748,5 @@
> +        // since the type is removed from RILNetworkInterface.connectedTypes
> +        // on disconnect().
> +        if (network.state == Ci.nsINetworkInterface.NETWORK_STATE_UNKNOWN) {
> +          let oldRadioInterface = this.radioInterfaces[this._currentDataClientId];
> +          let newRadioInterface = this.radioInterfaces[this._dataDefaultClientId];

ditto.

@@ +782,5 @@
> +        }
> +        this._dataEnabled = result;
> +
> +        if (DEBUG) debug("Default id for data call: " + this._dataDefaultClientId);
> +        if (!this._dataDefaultClientId) {

We shouldn't have the case of null dataDefaultClientId, right?

@@ +810,5 @@
> +  },
> +
> +  handleDataClientIdChange: function handleDataClientIdChange() {
> +    // This is to handle boot up stage.
> +    if (!this._currentDataClientId) {

I don't see |this._currentDataClientId| being updated when we setup a data call first time after booting up. That said, for example, ril.data.enabled == true, _dataDefaultClientId == 0, and we have a data connection already. However, at this moment, _currentDataClientId is still null.
 
Then when ril.data.defaultServiceId changes, we jump to this condition and we fail to switch data call to the new Id because we don't deactivate the old/previous data call.

@@ +824,5 @@
> +
> +    let oldRadioInterface = this.radioInterfaces[this._currentDataClientId];
> +    let newRadioInterface = this.radioInterfaces[this._dataDefaultClientId];
> +
> +    if (this._dataEnabled) {

Once there's a non-null _currentDataClientId, why do we still check _dataEnabled here?

@@ +825,5 @@
> +    let oldRadioInterface = this.radioInterfaces[this._currentDataClientId];
> +    let newRadioInterface = this.radioInterfaces[this._dataDefaultClientId];
> +
> +    if (this._dataEnabled) {
> +        oldRadioInterface.dataCallSettings.oldEnabled = oldRadioInterface.dataCallSettings.enabled;

nit: indention, here and below

@@ +835,5 @@
> +
> +    if (oldRadioInterface.anyDataConnected()) {
> +      if (DEBUG) debug("Existing data call(s) active, wait for them to get disconnected.");
> +      oldRadioInterface.deactivateDataCalls();
> +      this._dataCallSetupPending = true;

The concept of _pendingDataCallRequest() I imagine looks like below though fine modification is still needed. Please see if it works.

this._pendingDataCallRequest = function() {
    newRadioInterface.dataCallSettings.oldEnabled = newRadioInterface.dataCallSettings.enabled;
    newRadioInterface.dataCallSettings.enabled = this._dataEnabled;
    this._currentDataClientId = this._dataDefaultClientId;
    newRadioInterface.updateRILNetworkInterface();
};

@@ +840,5 @@
> +      return;
> +    }
> +
> +    this._currentDataClientId = this._dataDefaultClientId;
> +    newRadioInterface.updateRILNetworkInterface();

I don't think I get the idea. Could you explain why line #844 is needed?

        Attachment #8346368 -
        Flags: review?(htsai)

    
    

    
  
(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #67)
> 
> @@ +747,5 @@
> > +        // We can not use network.type to tell if it's NETWORK_TYPE_MOBILE,
> > +        // since the type is removed from RILNetworkInterface.connectedTypes
> > +        // on disconnect().
> > +        if (network.state == Ci.nsINetworkInterface.NETWORK_STATE_UNKNOWN) {
> > +          let oldRadioInterface = this.radioInterfaces[this._currentDataClientId];
> 
> nit: warp at 80th, so rewrite the line as
> 
> let oldRadioInterface = 
>     this.radioInterfaces[this._currentDataClientId];
> 

Oops, wrong format. Should be:

let oldRadioInterface = 
  this.radioInterfaces[this._currentDataClientId];

    
    

    
  
Comment on attachment 8346338 [details] [diff] [review]
Part 2: attach/detach data registration on demand (for fugu)

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

I'd like to hold the review for this part until we both agree with what part1 should look like.

        Attachment #8346338 -
        Flags: review?(htsai)

    
    

    
  
(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #67)
> Comment on attachment 8346368 [details] [diff] [review]
> Part 1: move dsds data setup handling to RadioInterfaceLayer
> 
> Review of attachment 8346368 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Jessica, thanks for the patch. I like the idea of move the data setup
> dandling to RadioInterfaceLayer, and I believe it's the right way to go.
> Please see my other questions and comments below. Thank you!
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +718,5 @@
> > +  _dataEnabled: null,
> > +
> > +  // Flag to record the default client id for data call.  It
> > +  // corresponds to the 'ril.data.defaultServiceId' setting from the UI.
> > +  _dataDefaultClientId: null,
> 
> Just realized that we should set this value to default '0' in constructor in
> case gaia doesn't have this settings key in single sim scenario.

The idea was to set _dataDefaultClientId to null to differentiate it from 0 (SIM1), then null would mean that we haven't read successfully from the db. If the result from the database returns and it's null, then we will set it to 0, see line #795.

> 
> @@ +727,5 @@
> > +  _currentDataClientId: null,
> > +
> > +  // Flag to determine if we need to setup data call when we are notified
> > +  // that another data call has been disconnected.
> > +  _dataCallSetupPending: null,
> 
> I'd like to have _pendingDataCallRequest instead so that once we have
> _pendingDataCallRequest, we simply call |this._pendingDataCallRequest()| to
> execute it. More ideas on at line #839.

I will give it a try, thanks.

> 
> @@ +743,5 @@
> > +      case kNetworkInterfaceStateChangedTopic:
> > +        let network = subject.QueryInterface(Ci.nsINetworkInterface);
> > +        // DSDS: setup pending data connection when switching the default id
> > +        // for data call.
> > +        // We can not use network.type to tell if it's NETWORK_TYPE_MOBILE,
> 
> nit: move line #747 to the end of line #746, and wrap at 80th character.

will do.

> 
> @@ +747,5 @@
> > +        // We can not use network.type to tell if it's NETWORK_TYPE_MOBILE,
> > +        // since the type is removed from RILNetworkInterface.connectedTypes
> > +        // on disconnect().
> > +        if (network.state == Ci.nsINetworkInterface.NETWORK_STATE_UNKNOWN) {
> > +          let oldRadioInterface = this.radioInterfaces[this._currentDataClientId];
> 
> nit: warp at 80th, so rewrite the line as
> 
> let oldRadioInterface = 
>     this.radioInterfaces[this._currentDataClientId];

will do.

> 
> @@ +748,5 @@
> > +        // since the type is removed from RILNetworkInterface.connectedTypes
> > +        // on disconnect().
> > +        if (network.state == Ci.nsINetworkInterface.NETWORK_STATE_UNKNOWN) {
> > +          let oldRadioInterface = this.radioInterfaces[this._currentDataClientId];
> > +          let newRadioInterface = this.radioInterfaces[this._dataDefaultClientId];
> 
> ditto.

will do.

> 
> @@ +782,5 @@
> > +        }
> > +        this._dataEnabled = result;
> > +
> > +        if (DEBUG) debug("Default id for data call: " + this._dataDefaultClientId);
> > +        if (!this._dataDefaultClientId) {
> 
> We shouldn't have the case of null dataDefaultClientId, right?

This is for the case where we haven't read successfully from the database, so we would update _dataEnabled first and leave the rest when 'ril.data.defaultClientId' returns.

> 
> @@ +810,5 @@
> > +  },
> > +
> > +  handleDataClientIdChange: function handleDataClientIdChange() {
> > +    // This is to handle boot up stage.
> > +    if (!this._currentDataClientId) {
> 
> I don't see |this._currentDataClientId| being updated when we setup a data
> call first time after booting up. That said, for example, ril.data.enabled
> == true, _dataDefaultClientId == 0, and we have a data connection already.
> However, at this moment, _currentDataClientId is still null.
>  
> Then when ril.data.defaultServiceId changes, we jump to this condition and
> we fail to switch data call to the new Id because we don't deactivate the
> old/previous data call.

Just to make sure, does this result from setting _dataDefaultClientId to 0? If this is the case and the default id for data is 0 at bootup, handleDataClientIdChange() won't be called on boot stage due to line #797.

> 
> @@ +824,5 @@
> > +
> > +    let oldRadioInterface = this.radioInterfaces[this._currentDataClientId];
> > +    let newRadioInterface = this.radioInterfaces[this._dataDefaultClientId];
> > +
> > +    if (this._dataEnabled) {
> 
> Once there's a non-null _currentDataClientId, why do we still check
> _dataEnabled here?

_currentDataClientId it's just the selected client id for data. If data is not enabled, we just need to update _currentDataClientId and do nothing, but if data is enabled, we need to switch each radio interface's dataCallSettings.
I'd like to move the 'if (oldRadioInterface.anyDataConnected())' clause under 'if (this._dataEnabled)', do you think it's okay?

> 
> @@ +825,5 @@
> > +    let oldRadioInterface = this.radioInterfaces[this._currentDataClientId];
> > +    let newRadioInterface = this.radioInterfaces[this._dataDefaultClientId];
> > +
> > +    if (this._dataEnabled) {
> > +        oldRadioInterface.dataCallSettings.oldEnabled = oldRadioInterface.dataCallSettings.enabled;
> 
> nit: indention, here and below

will do.

> 
> @@ +835,5 @@
> > +
> > +    if (oldRadioInterface.anyDataConnected()) {
> > +      if (DEBUG) debug("Existing data call(s) active, wait for them to get disconnected.");
> > +      oldRadioInterface.deactivateDataCalls();
> > +      this._dataCallSetupPending = true;
> 
> The concept of _pendingDataCallRequest() I imagine looks like below though
> fine modification is still needed. Please see if it works.
> 
> this._pendingDataCallRequest = function() {
>     newRadioInterface.dataCallSettings.oldEnabled =
> newRadioInterface.dataCallSettings.enabled;
>     newRadioInterface.dataCallSettings.enabled = this._dataEnabled;
>     this._currentDataClientId = this._dataDefaultClientId;
>     newRadioInterface.updateRILNetworkInterface();
> };

I will give it a try, thanks for this.

> 
> @@ +840,5 @@
> > +      return;
> > +    }
> > +
> > +    this._currentDataClientId = this._dataDefaultClientId;
> > +    newRadioInterface.updateRILNetworkInterface();
> 
> I don't think I get the idea. Could you explain why line #844 is needed?

This is for the case where data is enabled, but there is no active data calls, then we need to call updateRILNetworkInterface() to trigger data call setup for the new selected client/sim.


Thank you Hsinyi for the review, please let me know if there is anything unclear or we can discuss f2f.

    
    

    
  
(In reply to Jessica Jong [:jjong] [:jessica] from comment #70)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #67)
> > Comment on attachment 8346368 [details] [diff] [review]
> > Part 1: move dsds data setup handling to RadioInterfaceLayer
> > 
> > Review of attachment 8346368 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Hi Jessica, thanks for the patch. I like the idea of move the data setup
> > dandling to RadioInterfaceLayer, and I believe it's the right way to go.
> > Please see my other questions and comments below. Thank you!
> > 
> > ::: dom/system/gonk/RadioInterfaceLayer.js
> > @@ +718,5 @@
> > > +  _dataEnabled: null,
> > > +
> > > +  // Flag to record the default client id for data call.  It
> > > +  // corresponds to the 'ril.data.defaultServiceId' setting from the UI.
> > > +  _dataDefaultClientId: null,
> > 
> > Just realized that we should set this value to default '0' in constructor in
> > case gaia doesn't have this settings key in single sim scenario.
> 
> The idea was to set _dataDefaultClientId to null to differentiate it from 0
> (SIM1), then null would mean that we haven't read successfully from the db.
> If the result from the database returns and it's null, then we will set it
> to 0, see line #795.
> 

Got the idea. Sounds good to me then!

> > 
> > @@ +810,5 @@
> > > +  },
> > > +
> > > +  handleDataClientIdChange: function handleDataClientIdChange() {
> > > +    // This is to handle boot up stage.
> > > +    if (!this._currentDataClientId) {
> > 
> > I don't see |this._currentDataClientId| being updated when we setup a data
> > call first time after booting up. That said, for example, ril.data.enabled
> > == true, _dataDefaultClientId == 0, and we have a data connection already.
> > However, at this moment, _currentDataClientId is still null.
> >  
> > Then when ril.data.defaultServiceId changes, we jump to this condition and
> > we fail to switch data call to the new Id because we don't deactivate the
> > old/previous data call.
> 
> Just to make sure, does this result from setting _dataDefaultClientId to 0?
> If this is the case and the default id for data is 0 at bootup,
> handleDataClientIdChange() won't be called on boot stage due to line #797.
> 

Understood now, thanks.

> > 
> > @@ +824,5 @@
> > > +
> > > +    let oldRadioInterface = this.radioInterfaces[this._currentDataClientId];
> > > +    let newRadioInterface = this.radioInterfaces[this._dataDefaultClientId];
> > > +
> > > +    if (this._dataEnabled) {
> > 
> > Once there's a non-null _currentDataClientId, why do we still check
> > _dataEnabled here?
> 
> _currentDataClientId it's just the selected client id for data. If data is
> not enabled, we just need to update _currentDataClientId and do nothing, but
> if data is enabled, we need to switch each radio interface's
> dataCallSettings.
> I'd like to move the 'if (oldRadioInterface.anyDataConnected())' clause
> under 'if (this._dataEnabled)', do you think it's okay?
> 

I think that would be nicer.

I reviewed again and had an idea:

handleDataClientIdChange: function() {
  if (!this._currentDataClientId) {
    // Do what you are trying to do now.
    return;
  }

  if (!this._dataEnabled) {
    // This line is the only thing we need, right?
    this._currentDataClientId = this._dataDefaultClientId;
    return;
  }

  let oldRadioInterface = ... ...
  if (oldRadioInterface.anyDataConnected()) {
    this._pendingDataCallRequest = function () {
      newRadioInterface.dataCallSettings.oldEnabled =
 newRadioInterface.dataCallSettings.enabled;
     newRadioInterface.dataCallSettings.enabled = this._dataEnabled;
     this._currentDataClientId = this._dataDefaultClientId;
     newRadioInterface.updateRILNetworkInterface();
    };
    oldRadioInterface.deactivateDataCalls();
    return;
  }

  let newRadioInterface = ... ... 
  newRadioInterface.dataCallSettings.oldEnabled ... ... 
  ... 
  newRadioInterface.updateRILNetworkInterface();
}

> > 

> > 
> > @@ +840,5 @@
> > > +      return;
> > > +    }
> > > +
> > > +    this._currentDataClientId = this._dataDefaultClientId;
> > > +    newRadioInterface.updateRILNetworkInterface();
> > 
> > I don't think I get the idea. Could you explain why line #844 is needed?
> 
> This is for the case where data is enabled, but there is no active data
> calls, then we need to call updateRILNetworkInterface() to trigger data call
> setup for the new selected client/sim.
> 
Oh, yeah, I missed that.

    
    

    
  
Comment on attachment 8346368 [details] [diff] [review]
Part 1: move dsds data setup handling to RadioInterfaceLayer

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +812,5 @@
> +  handleDataClientIdChange: function handleDataClientIdChange() {
> +    // This is to handle boot up stage.
> +    if (!this._currentDataClientId) {
> +      this._currentDataClientId = this._dataDefaultClientId;
> +      let radioInterface = this.getRadioInterface(this._currentDataClientId);

Move this into the clause of |if (this._dataEnabled)| as we need this only when the condition meets.

    
    

    
  


  
Addressed comment 67, 71 and 72.
Some other changes:
- change the default values to -1 for id variables, I think it's clearer now.
- use the _pendingDataCallRequest as requested.

        Attachment #8346368 -
        Attachment is obsolete: true

        Attachment #8349219 -
        Flags: review?(htsai)

    
    

    
  


  
Rebased based on Part 1, v2.
After adjusting the logic in handleDataClientIdChange() in Part 1 v2, some extra code are introduced.

        Attachment #8346338 -
        Attachment is obsolete: true

        Attachment #8349221 -
        Flags: review?(htsai)

    
    

    
  
Comment on attachment 8349219 [details] [diff] [review]
[final] Part 1: move dsds data setup handling to RadioInterfaceLayer.

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

Looks good! :)

        Attachment #8349219 -
        Flags: review?(htsai) → review+

    
    

    
  
Comment on attachment 8349221 [details] [diff] [review]
Part 2: attach/detach data registration on demand (for fugu), v2.

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

General question:
After calling setDataRegistration(), do we need to wait until the data registration state changes then calling this._pendingDataCallRequest()? How does partner's android code do?

Sorry for not having asked this question before.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +44,5 @@
>  
> +// Ril quirk to attach data registration on demand.
> +// TODO: temporary setting default to true for testing
> +let RILQUIRKS_DATA_REGISTRATION_ON_DEMAND =
> +  libcutils.property_get("ro.moz.ril.data_reg_on_demand", "true") == "true";

Please remember to modify this while landing.

::: dom/system/gonk/ril_worker.js
@@ +90,5 @@
>  
> +// Ril quirk to attach data registration on demand.
> +// TODO: temporary setting default to true for testing
> +let RILQUIRKS_DATA_REGISTRATION_ON_DEMAND =
> +  libcutils.property_get("ro.moz.ril.data_reg_on_demand", "true") == "true";

Please remember to modify this before landing.

@@ +1907,5 @@
> +   *
> +   * @param attach
> +   *        Boolean value indicating attach or detach.
> +   */
> +  _attachDataRegistration: false,

nit: move this to line 1905

@@ +6141,5 @@
>      this.getBasebandVersion();
>      this.updateCellBroadcastConfig();
>      this.setPreferredNetworkType();
>      this.setCLIR();
> +    if (RILQUIRKS_DATA_REGISTRATION_ON_DEMAND && this._attachDataRegistration) {

This is used for tracking the previous preference before radio off, right?

    
    

    
  
(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #76)
> Comment on attachment 8349221 [details] [diff] [review]
> Part 2: attach/detach data registration on demand (for fugu), v2.
> 
> Review of attachment 8349221 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> General question:
> After calling setDataRegistration(), do we need to wait until the data
> registration state changes then calling this._pendingDataCallRequest()? How
> does partner's android code do?

Partner's android code will wait for data state change to setup the pending data call. In our case, RIL will setup data call directly and will fail cause data registration is not registered, and once data registration becomes registered, we will try to setup data call again.
We can also wait for data state change to setup the pending data call, but then the flow would be quite different with and without RILQUIRKS_DATA_REGISTRATION_ON_DEMAND. What do you think?

> 
> Sorry for not having asked this question before.
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +44,5 @@
> >  
> > +// Ril quirk to attach data registration on demand.
> > +// TODO: temporary setting default to true for testing
> > +let RILQUIRKS_DATA_REGISTRATION_ON_DEMAND =
> > +  libcutils.property_get("ro.moz.ril.data_reg_on_demand", "true") == "true";
> 
> Please remember to modify this while landing.

will do, thanks!

> 
> ::: dom/system/gonk/ril_worker.js
> @@ +90,5 @@
> >  
> > +// Ril quirk to attach data registration on demand.
> > +// TODO: temporary setting default to true for testing
> > +let RILQUIRKS_DATA_REGISTRATION_ON_DEMAND =
> > +  libcutils.property_get("ro.moz.ril.data_reg_on_demand", "true") == "true";
> 
> Please remember to modify this before landing.

will do, thanks!

> 
> @@ +1907,5 @@
> > +   *
> > +   * @param attach
> > +   *        Boolean value indicating attach or detach.
> > +   */
> > +  _attachDataRegistration: false,
> 
> nit: move this to line 1905

will do.

> 
> @@ +6141,5 @@
> >      this.getBasebandVersion();
> >      this.updateCellBroadcastConfig();
> >      this.setPreferredNetworkType();
> >      this.setCLIR();
> > +    if (RILQUIRKS_DATA_REGISTRATION_ON_DEMAND && this._attachDataRegistration) {
> 
> This is used for tracking the previous preference before radio off, right?

Yes, and also on bootup, we will try to attach data registration on the default sim for data, but it would fail due to 'radio not available', so we need to set it again on radio on.

    
    

    
  
Comment on attachment 8349221 [details] [diff] [review]
Part 2: attach/detach data registration on demand (for fugu), v2.

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

Thanks for the explanation on comment 77 that makes sense to me.
r+ and don't forget my comment 76. :)

        Attachment #8349221 -
        Flags: review?(htsai) → review+

    
  

        Attachment #8349219 -
        Attachment description: Part 1: move dsds data setup handling to RadioInterfaceLayer, v2. → [final] Part 1: move dsds data setup handling to RadioInterfaceLayer.

    
    

    
  


  
Thank you, Hsinyi!

Addressed review comment 76 and set r+.

        Attachment #8349221 -
        Attachment is obsolete: true

        Attachment #8349993 -
        Flags: review+

    
    

    
  
Try run: https://tbpl.mozilla.org/?tree=Try&rev=e57c06c9b5c5

It seems that Gi tests are always failing in try tbpl, so I think it's safe to check-in..?

    
  
Keywords: checkin-needed
Target Milestone: --- → 1.3 C1/1.4 S1(20dec)

    
    

    
  
Verified on Fugu, passed.

Gaia      a119a0692c24c5ed7c55bab838bae3ecdb9dbec9
Gecko     15ee4e78431b45922b41dea882464b0ccb6b4fac
BuildID   20140110174141
Version   28.0a2
Status: RESOLVED → VERIFIED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: