Closed Bug 943215 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Gonk (Firefox OS)
defect
Not set

Tracking

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

VERIFIED FIXED
1.3 C2/1.4 S2(17jan)
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- verified
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: echu, Assigned: aknow)

References

Details

(Whiteboard: [Blocked by devices,dsds_US_test])

Attachments

(6 files, 4 obsolete files)

Attached file 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%
Whiteboard: [POVB]
Whiteboard: [POVB] → [POVB,dsds_US_test]
Sam will take it.
Summary: [fugu] Single SIM cases are not correctly supported. → [fugu][DSDS] Single SIM cases are not correctly supported.
Whiteboard: [POVB,dsds_US_test] → [Blocked by devices,dsds_US_test]
Still can reproduce the bug with today's build.

Gaia:     087d109c83235fc157b2efac7ace6cc54a4c5aaa
Gecko:    8fdb69114734785065ce6c76ac36f4438b90c2e6
BuildID   20131127093419
Version   28.0a1
Summary: [fugu][DSDS] Single SIM cases are not correctly supported. → [DSDS] Single SIM cases are not correctly supported.
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.
(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.
Need Bruce support.
Blocks: 938440
Blocks: 927764
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.
blocking-b2g: --- → 1.3?
Need Bruce help.
Flags: needinfo?(bruce.jiang)
blocking DSDS tests. Adding POVB and 1.3+ to make sure it's being tracked
blocking-b2g: 1.3? → 1.3+
Whiteboard: [Blocked by devices,dsds_US_test] → [Blocked by devices,dsds_US_test] [POVB]
No longer blocks: 938440
See Also: → 938440
No longer blocks: 927764
See Also: → 927764
Hi Enpei,

could fugu work well with single card ?
(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?
Flags: needinfo?(sam.hua)
Please see spreadtrum frameworks DSDS patch.
GsmServiceStateTracker.java---> setPowerStateToDesired(boolean force)
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.
Flags: needinfo?(sam.hua)
Attached file libreference-ril_sp.so
temporary lib.(Don't power on radio if no sim).
(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.
Flags: needinfo?(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.
Flags: needinfo?(sam.hua)
Summary: [DSDS] Single SIM cases are not correctly supported. → [DSDS] We shouldn't radio on on slot 2 when a card is absent otherwise single SIM cases are not correctly supported
See Also: → 946593
Component: Vendcom → RIL
Assignee: nobody → szchen
What is going on now?
Flags: needinfo?(kchang)
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.
Flags: needinfo?(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.
Flags: needinfo?(sam.hua)
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!!
TODO:
1. Need introduce a new flag to wrap those special handling
2. ...
Attachment #8358259 - Flags: feedback?(htsai)
(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.
Flags: needinfo?(kchang)
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.
Attachment #8358259 - Flags: feedback?(htsai) → feedback+
(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.
Attachment #8358259 - Attachment is obsolete: true
Attachment #8359035 - Flags: feedback?(htsai)
Attached patch (WIP) Part 2: add quirk (obsolete) — Splinter Review
Code after adding the quirk
Attachment #8359589 - Flags: feedback?(htsai)
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.
Attachment #8359035 - Flags: feedback?(htsai) → feedback+
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.
Attachment #8359589 - Flags: feedback?(htsai) → feedback-
Merge two WIP patches into one.
Attachment #8359035 - Attachment is obsolete: true
Attachment #8359589 - Attachment is obsolete: true
Attachment #8359685 - Flags: review?(htsai)
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;
Attachment #8359685 - Flags: review?(htsai) → review+
Depends on: 959920
https://hg.mozilla.org/mozilla-central/rev/469a0feafe5f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
Hi Ryan,

Can this be uplifted to 1.3?
(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?
Flags: needinfo?(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.
Flags: needinfo?(ryanvm)
Whiteboard: [Blocked by devices,dsds_US_test] [POVB] → [Blocked by devices,dsds_US_test]
(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.
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!
Flags: needinfo?(szchen)
Hi,could u give me a total patch to verify it?
SIM1 card,
make mo call,it shows "to make a call u need to disable the airplane"
(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.
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".
(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!
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.
Status: RESOLVED → VERIFIED
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.
yes.i works after i enter setting->sim manager

I report it as a new bug or just trace 949225?
(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 :)
Hi,

The SIM2 can't find the network after this update. Gecko won't send setRadioEnabled to RILC for SIM2.
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?
Flags: needinfo?(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].
Flags: needinfo?(sam.hua)
Hi Sam,
To clarify it, did you insert the SIM card in SIM2 slot?
Flags: needinfo?(sam.hua)
Attached file 211.txt
yes,of course.the SIM is detected.

please see the log in the attachment
Flags: needinfo?(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
I filed a new bug 963054 as a follow-up bug.
Flags: needinfo?(bruce.jiang)
Depends on: 963054
Depends on: 976897
You need to log in before you can comment on or make changes to this bug.