943215 - [DSDS] We shouldn't radio on on slot 2 when a card is absent otherwise single SIM cases are not correctly supported

Status: VERIFIED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Enpei]

Attachment: fugu_singleSIM1.txt

When only one SIM in Fugu, radio part cannot work correctly. This will block many DSDS test.

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

* Reproduce Steps
Insert one SIM into either slot 0 or slot 1.

* Expected Result
Telephony function should still work even if there is only one SIM.

* Actual Result
1. With slot 0 SIM, it cannot detect SIM.
2. With slot 1 SIM, it can receive SMS (SIM manager is not ready, so only SMS can verified.)

* Occurrence rate
100%

Comment 1

[James Zhang (Spreadtrum)]

Sam will take it.

Comment 2

[Enpei]

Still can reproduce the bug with today's build.

Gaia:     087d109c83235fc157b2efac7ace6cc54a4c5aaa
Gecko:    8fdb69114734785065ce6c76ac36f4438b90c2e6
BuildID   20131127093419
Version   28.0a1

Comment 3

[Enpei]

Attachment: from build 20131127093419

Comment 4

[sam.hua]

Hi.

Using CMCC SIM card, it works well in sim slot 0 and sim slot 1, 
Using CUCC SIM card, same as the description of this bug.

Comment 5

[James Zhang (Spreadtrum)]

(In reply to sam.hua from comment #4)
> Hi.
> 
> Using CMCC SIM card, it works well in sim slot 0 and sim slot 1, 
> Using CUCC SIM card, same as the description of this bug.

OK, we can reproduce this issue in my side.

Comment 6

[James Zhang (Spreadtrum)]

Need Bruce support.

Comment 7

[sam.hua]

I tested it today, and still can't find the network with only one SIM card.
Maybe the problem is modem can't return correct response of the "AT> AT+COPS=3,0;+COPS?;+COPS=3,1;+COPS?;+COPS=3,2;+COPS?" for operator .

Now our modem team is checking this problem.

Comment 8

[James Zhang (Spreadtrum)]

Need Bruce help.

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

blocking DSDS tests. Adding POVB and 1.3+ to make sure it's being tracked

Comment 10

[sam.hua]

Hi Enpei,

could fugu work well with single card ?

Comment 11

[Enpei]

(In reply to sam.hua from comment #10)
> Hi Enpei,
> 
> could fugu work well with single card ?

Hi Sam,

The latest build I have now is 
Gaia      fbb6ce88ce8b7bd4d2efdb7a4a9f5a3c145f3eab
Gecko     a0bb585098cc89c454fac0297b5ef748d5cab82c
BuildID   20131210063021
Version   28.0a1

And it still has bug 943210. When only SIM 1 in device, it cannot MT/MO voice call. With only SIM 2, it can only MT voice call.

Does single card case in your repository work now?

Comment 12

[James Zhang (Spreadtrum)]

Please see spreadtrum frameworks DSDS patch.
GsmServiceStateTracker.java---> setPowerStateToDesired(boolean force)

Comment 13

[sam.hua]

Fugu can work with single SIM1 or SIM2,when i added some control in the RILC.(don't send RADIO_POWER on to Modem if NO_SIM).

maybe u can use this new lib to test the the single card temporary.

Comment 14

[sam.hua]

Attachment: libreference-ril_sp.so

temporary lib.(Don't power on radio if no sim).

Comment 15

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

(In reply to sam.hua from comment #13)
> Fugu can work with single SIM1 or SIM2,when i added some control in the
> RILC.(don't send RADIO_POWER on to Modem if NO_SIM).
> 
> maybe u can use this new lib to test the the single card temporary.

Sam, thanks for the .so file. So, are you going to handle this on RILC? Or should we still need to take care of this on gecko as well? Thanks.

Comment 16

[sam.hua]

Hi Hsin,

Now I simply avoid to send radio power on to modem in the RILC when SIM is absent,but I think it should be better to do this control mainly in the Gecko, like the PS control in DSDS.
Maybe gecko could give us some new interfaces to make it more general.

Comment 17

[sam.hua]

What is going on now?

Comment 18

[Szu-Yu Chen [:aknow] (inactive)]

Hi Sam,
I'm working on this. WIP patch is ready. Now I'm doing some testing before requesting review.
I know that if we turn on the radio for a slot that do not have a sim card, it might cause some problem. Any specific problem that I could reference. I would like to make sure that all of those issue are not happened if we have followed the correct rule when turning on the radio.

Comment 19

[sam.hua]

sorry. I don't have any document about it.

It is experience from our Android team, I think it is the modem problem,but they won't change it because our Android team has added the control for radio.

Comment 20

[Szu-Yu Chen [:aknow] (inactive)]

Yeah, I found one.

If we only use sim1 in fugu,
before fixing this bug, it will show searching... and cannot find the network in the end.
However, if we fix this bug (just test my patch), it becomes OK!!

Comment 21

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (WIP) Control radio according to cardState

TODO:
1. Need introduce a new flag to wrap those special handling
2. ...

Comment 22

[Ken Chang[:kenkai]]

(In reply to Szu-Yu Chen [:aknow] from comment #18)
> Hi Sam,
> I'm working on this. WIP patch is ready. Now I'm doing some testing before
> requesting review.
Thanks, aknow.

Comment 23

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

Comment on attachment 8358259 [details] [diff] [review]
(WIP) Control radio according to cardState

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

The patch is towards the right direction, thank you!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +562,5 @@
> +        // We could only turn on the radio for clientId if
> +        // 1. the sim card is installed or
> +        // 2. it is the default clientId(0) and there is no any sim card.
> +
> +        let cardPresented = function(cardState) {

I don't think we benefit from this new function. Please discard this.

@@ +567,5 @@
> +          return cardState !== RIL.GECKO_CARDSTATE_UNDETECTED &&
> +              cardState !== RIL.GECKO_CARDSTATE_UNKNOWN;
> +        };
> +
> +        let totalCards = 0;

s/totalCards/numCards/

@@ +576,5 @@
> +          }
> +        }
> +
> +        let cardState = radioInterface.rilContext.cardState;
> +        if (cardPresented(cardState) || (clientId === 0 && totalCards === 0)) {

1) Let us use a constant "HW_DEFAULT_CLIENT_ID" to indicate clientId 0.
2) Please move the comments #562~#564 to here.

@@ +2627,5 @@
>  
>    observe: function observe(subject, topic, data) {
>      switch (topic) {
>        case kSysMsgListenerReadyObserverTopic:
> +        //this.setRadioEnabledInternal({enabled: true}, null);

I am guessing, the reason you marked this line is for self testing, right?

However, in the final version, we will still need to remove this completely from here, but let gRadioEnabledController to observe |kSysMsgListenerReadyObserverTopic| and to react correctly.

Comment 24

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #23)
> Comment on attachment 8358259 [details] [diff] [review]
> (WIP) Control radio according to cardState
> 
> Review of attachment 8358259 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch is towards the right direction, thank you!
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +562,5 @@
> > +        // We could only turn on the radio for clientId if
> > +        // 1. the sim card is installed or
> > +        // 2. it is the default clientId(0) and there is no any sim card.
> > +
> > +        let cardPresented = function(cardState) {
> 
> I don't think we benefit from this new function. Please discard this.

Without this function, I have to repeat the logic twice in line 574 and line 580... well I am not really like this. So, instead, how about make this utility more easier to use.

boolean isCardPresentedForClient(i)

by this way, I could also remove the variable |cardState|

> @@ +567,5 @@
> > +          return cardState !== RIL.GECKO_CARDSTATE_UNDETECTED &&
> > +              cardState !== RIL.GECKO_CARDSTATE_UNKNOWN;
> > +        };
> > +
> > +        let totalCards = 0;
> 
> s/totalCards/numCards/
> 
> @@ +576,5 @@
> > +          }
> > +        }
> > +
> > +        let cardState = radioInterface.rilContext.cardState;
> > +        if (cardPresented(cardState) || (clientId === 0 && totalCards === 0)) {
> 
> 1) Let us use a constant "HW_DEFAULT_CLIENT_ID" to indicate clientId 0.
> 2) Please move the comments #562~#564 to here.

Do we have to store "HW_DEFAULT_CLIENT_ID" in preference?
Or just just let it be a constant in this file?

> @@ +2627,5 @@
> >  
> >    observe: function observe(subject, topic, data) {
> >      switch (topic) {
> >        case kSysMsgListenerReadyObserverTopic:
> > +        //this.setRadioEnabledInternal({enabled: true}, null);
> 
> I am guessing, the reason you marked this line is for self testing, right?
> 
> However, in the final version, we will still need to remove this completely
> from here, but let gRadioEnabledController to observe
> |kSysMsgListenerReadyObserverTopic| and to react correctly.

Yes, you are right.
We have to turn on the radio when power on in gRadioEnabledController. So it could also be checked by our card presented logic. I will do this in next revision.

Comment 25

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (WIP) Control radio according to cardState #2

Comment 26

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (WIP) Part 2: add quirk

Code after adding the quirk

Comment 27

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

Comment on attachment 8359035 [details] [diff] [review]
(WIP) Control radio according to cardState #2

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +546,5 @@
>  
> +    _getNumCards: function() {
> +      let numCards = 0;
> +      for (let i = 0, N = this.ril.numRadioInterfaces; i < N; ++i) {
> +        if (this._isClientCardPresented(i)) {

s/_isClientCardPresented/_isCardPresentAtClient

@@ +553,5 @@
> +      }
> +      return numCards;
> +    },
> +
> +    _isClientCardPresented: function(clientId) {

s/_isClientCardPresented/_isCardPresentAtClient

@@ +559,5 @@
> +      return cardState !== RIL.GECKO_CARDSTATE_UNDETECTED &&
> +        cardState !== RIL.GECKO_CARDSTATE_UNKNOWN;
> +    },
> +
> +    _isClientAbleToEnableRadio: function(clientId, numCards) {

s/_isClientAbleToEnableRadio/_isRadioAbleToEnableAtClient

@@ +561,5 @@
> +    },
> +
> +    _isClientAbleToEnableRadio: function(clientId, numCards) {
> +      // We could only turn on the radio for clientId if
> +      // 1. the sim card is presented or

"a SIM card is present or"

@@ +562,5 @@
> +
> +    _isClientAbleToEnableRadio: function(clientId, numCards) {
> +      // We could only turn on the radio for clientId if
> +      // 1. the sim card is presented or
> +      // 2. it is the default clientId and there is no any sim card.

... no any SIM card at any client.

@@ +569,5 @@
> +        numCards = this._getNumCards();
> +      }
> +
> +      return this._isClientCardPresented(clientId) ||
> +        (clientId === HW_DEFAULT_CLIENT_ID && numCards === 0);

This function is logically right. However, could you please revise it a little bit to avoid an unnecessary call to this._getNumCards()? That said, if |_isClientCardPresented(clientId)| is true, then we don't care about numCards.

Comment 28

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

Comment on attachment 8359589 [details] [diff] [review]
(WIP) Part 2: add quirk

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +47,5 @@
>    libcutils.property_get("ro.moz.ril.data_reg_on_demand", "false") == "true";
>  
> +// Ril quirk to forbit turning on radio without card except for hw default client.
> +let RILQUIRKS_DSDS_RADIO_FORBIDDEN_WO_CARD =
> +  libcutils.property_get("ro.moz.ril.dsds_radio_forbidden_wo_card", "false") == "true";

Unfortunately, the maximum length of the property name is 32 :(
The rest looks good though f- for this.

Comment 29

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Control radio according to cardState

Merge two WIP patches into one.

Comment 30

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

Comment on attachment 8359685 [details] [diff] [review]
Control radio according to cardState

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

r=me with the comment addressed. Thank you thank you.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +578,5 @@
> +        return true;
> +      }
> +
> +      if (numCards == null) {
> +        numCards = this._getNumCards();

numCards = numCards == null ? this._getNumCards() : numCards;

Comment 31

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Control radio according to cardState. r=hsinyi

Comment 32

[Szu-Yu Chen [:aknow] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=8a4176a2ab33

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/469a0feafe5f

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/469a0feafe5f

Comment 35

[Enpei]

Hi Ryan,

Can this be uplifted to 1.3?

Comment 36

[Enpei]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #34)
> https://hg.mozilla.org/mozilla-central/rev/469a0feafe5f

Hi Ryan,

Can you help to uplift this bug to 1.3 branch?

Comment 37

[Ryan VanderMeulen [:RyanVM]]

POVB, NPOTB, or NO_UPLIFT on the whiteboard = I'll never see it as something needing uplift. It's a US holiday tomorrow, but I will get it on Tuesday if nobody else gets to it first.

Comment 38

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #37)
> POVB, NPOTB, or NO_UPLIFT on the whiteboard = I'll never see it as something
> needing uplift. It's a US holiday tomorrow, but I will get it on Tuesday if
> nobody else gets to it first.

Thanks, Ryan. We will check we have correct keywords on whiteboard for other bugs. And I will help uplift this to mozilla-aurora later.

Comment 39

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

Hi Aknow,

Your final patch isn't cleanly applied to mozilla-aurora. Could you please provide an updated patch for aurora so that I could help uplift? Thanks!

Comment 40

[sam.hua]

Hi,could u give me a total patch to verify it?

Comment 41

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final/aurora] Control radio according to cardState. r=hsinyi

Comment 42

[sam.hua]

SIM1 card,
make mo call,it shows "to make a call u need to disable the airplane"

Comment 43

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

(In reply to sam.hua from comment #42)
> SIM1 card,
> make mo call,it shows "to make a call u need to disable the airplane"

Hi Sam,

Could you clarify your comment? I don't quite follow. And the fix has been in m-c already.

Comment 44

[sam.hua]

Sorry,
I applied the patch and download the new img.

With only SIM1, it can find the network, but can't make mo call with the prompt "to make a call u need to disable the airplane".

Comment 45

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/757961112eea

Comment 46

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

(In reply to sam.hua from comment #44)
> Sorry,
> I applied the patch and download the new img.
> 
> With only SIM1, it can find the network, but can't make mo call with the
> prompt "to make a call u need to disable the airplane".

Sam,

Could you please file a new bug for your problem? And please describe detailed STR. It would be even helpful if you attach logcat with ril debug messages. Thanks!

Comment 47

[Enpei]

Verified on fugu
Gaia      ee25b0e45649d2955f340ce4f71ad55712dd0fab
Gecko     913cf2b92845441c9578787362ddad6f2ac15df6
BuildID   20140121095108
Version   28.0a2

RSSI for single SIM(either slot 0 or 1) is correct now. Radio function (MT/MO voice) on slot 0 only case is fixed. For slot 2, uses bug 949225 for tracking.

Comment 48

[Szu-Yu Chen [:aknow] (inactive)]

Hi Sam,

Could you try the following steps
1. Enter setting > sim manager. Do nothing and then go out.
2. Make the call again

If it is workable, it means that when you dialing the call, the default outgoing sim is not set to sim1. The phone should automatically switch the default sim if there is only one sim installed. However, currently, there is a limitation on this part. The logic is handled in sim manager. So, it could only be triggered when you launch the sim manager. Gaia will later move the switching handling code to system app and resolve the problem.

Comment 49

[sam.hua]

yes.i works after i enter setting->sim manager

I report it as a new bug or just trace 949225?

Comment 50

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to sam.hua from comment #49)
> yes.i works after i enter setting->sim manager
> 
> I report it as a new bug or just trace 949225?

I already filed one in Gaia here : https://bugzilla.mozilla.org/show_bug.cgi?id=958412 

Thanks :)

Comment 51

[sam.hua]

Hi,

The SIM2 can't find the network after this update. Gecko won't send setRadioEnabled to RILC for SIM2.

Comment 52

[Szu-Yu Chen [:aknow] (inactive)]

Hi Sam,
We have verified the patch in Comment 47. RSSI for single SIM works well. I also test the dual SIM case locally and it's good. Could you give more details about the issue?

Comment 53

[sam.hua]

Hi szu,

Our newest build can't find the network of SIM2, and after I disable the ro.moz.ril.radio_off_wo_card, then SIM2 can work.
I checked the log,it looks like that gRadioEnabledController doesn't send RADIO_POWER(on) to ril_worker[1].

Comment 54

[Szu-Yu Chen [:aknow] (inactive)]

Hi Sam,
To clarify it, did you insert the SIM card in SIM2 slot?

Comment 55

[sam.hua]

Attachment: 211.txt

yes,of course.the SIM is detected.

please see the log in the attachment

Comment 56

[sam.hua]

Hi,szu
do u configure them in the base.mk?
    ro.moz.ril.0.network_types = gsm,wcdma\
    ro.moz.ril.1.network_types = gsm

Comment 57

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

I filed a new bug 963054 as a follow-up bug.