Closed Bug 921991 Opened 6 years ago Closed 6 years ago

B2G BT: support multiple sim cards

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+, firefox28 verified)

VERIFIED FIXED
1.3 Sprint 6 - 12/6
blocking-b2g 1.3+
Tracking Status
firefox28 --- verified

People

(Reporter: hsinyi, Assigned: ben.tian)

References

()

Details

Attachments

(1 file, 4 obsolete files)

No description provided.
In multisim scenario, there are several telephony, mobileconnection and icc objects. BT would need  to register specific listeners, and do corresponding tasks.
Hi Marco, can your team help this bug?
Blocks: 921394
blocking-b2g: --- → 1.3+
Flags: needinfo?(mchen)
If we need to support Multiple SIM, we need UX to comment HFP behaviors.
To assign myself first then after there is a repo or patch for integrating with BT I will assign to BT members. 
And thanks for reminding this change and effort.

Hi Ken,

By the way, does UX job already start? and consider this case?

Thanks.
Assignee: nobody → mchen
Flags: needinfo?(mchen)
(In reply to Marco Chen [:mchen] (PTO, 09/16, 09/18~09/22) from comment #4)
> By the way, does UX job already start? and consider this case?
I don't think there is a different UX in this bug.
Based on Hsinyi, 1.3 targets only dual "GSM" sim cards and in Dual Sim Dual Stand-by (DSDS) only one sim card is active for calls at a time. So BT handles call state changes as single sim card case, but listens to network status changes from multiple sim cards.

One approach to handle multiple sim cards' changes is to focus on certain sim card ('target sim') and filter out the others. For example we can always choose the first sim card as target sim when multiple sim cards exist. Still there are some special cases regarding sim card insertion/removal:
  - When target sim is removed, switch to next remaining sim card. No switch when there's no remaining sim card.
  - When a sim card is newly inserted:
    > if current target sim exists, stay on it.
    > if current target sim doesn't exist, switch to the new sim card.
In order to handle these cases, BT requires to know which sim cards exist to switch target sim accordingly.

Hsinyi, please suggest RIL API and call sequence to query which sim cards currently exist. Thanks.
Assignee: mchen → btian
Flags: needinfo?(htsai)
(In reply to Ben Tian [:btian] from comment #6)
> Based on Hsinyi, 1.3 targets only dual "GSM" sim cards and in Dual Sim Dual
> Stand-by (DSDS) only one sim card is active for calls at a time. So BT
> handles call state changes as single sim card case, but listens to network
> status changes from multiple sim cards.
> 
> One approach to handle multiple sim cards' changes is to focus on certain
> sim card ('target sim') and filter out the others. For example we can always
> choose the first sim card as target sim when multiple sim cards exist. Still
> there are some special cases regarding sim card insertion/removal:
>   - When target sim is removed, switch to next remaining sim card. No switch
> when there's no remaining sim card.
>   - When a sim card is newly inserted:
>     > if current target sim exists, stay on it.
>     > if current target sim doesn't exist, switch to the new sim card.
> In order to handle these cases, BT requires to know which sim cards exist to
> switch target sim accordingly.
> 
> Hsinyi, please suggest RIL API and call sequence to query which sim cards
> currently exist. Thanks.

Hi Ben,

We are trying to simplify the usage of internal RIL APIs. Once we have a satisfying solution I'll keep you updated. But in general, if you want to know the voice and signal information, you would use 'nsIMobileConnectionProvider.' For acquiring details of a sim card, refer to 'nsIIccProvider.'

Current status (not the final one):
[1] https://wiki.mozilla.org/WebAPI/WebMobileConnection/Multi-SIM#Proposal:_Architecture
[2] https://wiki.mozilla.org/WebAPI/WebIccManager/Multi-SIM#Implementation
Flags: needinfo?(htsai)
Depends on: 814637, 814629, 814625
Depends on: 818353
Depends on: 926343
Blocks: 927764
Target Milestone: --- → 1.3 Sprint 5 - 11/22
The bug depends mainly on bug 926343 and 818353 for RIL API to bluetooth. Remove webAPI bugs dependency.
No longer depends on: 814629, 814637
Attached patch [WIP] BT HFP multisim (obsolete) — Splinter Review
WIP patch for BT HFP. Tested on single-sim device. Will test on emulator for multi-sim environment.
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Changes:
- implement sim selection mechanism for multi-sim
- refactor BluetoothRilListener
Attachment #827218 - Attachment is obsolete: true
Attachment #827335 - Flags: review?(echou)
Attachment #827335 - Flags: feedback?(echen)
Comment on attachment 827335 [details] [diff] [review]
Patch 1 (v1): HFP sim selection mechanism for multi-sim

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

Thanks for your help to add a define in nsRadioInterface.h :)
Attachment #827335 - Flags: feedback?(echen) → feedback+
Comment on attachment 827335 [details] [diff] [review]
Patch 1 (v1): HFP sim selection mechanism for multi-sim

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

r=me with nits addressed. Thanks!

::: dom/bluetooth/BluetoothRilListener.cpp
@@ +266,5 @@
> +  }
> +
> +  return NS_SUCCEEDED(rv);
> +}
> +  

nit: extra whitespaces

@@ +273,5 @@
>   */
>  BluetoothRilListener::BluetoothRilListener()
>  {
> +  // Query number of total clients (sim slots)
> +  uint32_t numOfClient;

nit: numOfClient's'?

@@ +274,5 @@
>  BluetoothRilListener::BluetoothRilListener()
>  {
> +  // Query number of total clients (sim slots)
> +  uint32_t numOfClient;
> +  nsCOMPtr <nsIRadioInterfaceLayer> radioInterfaceLayer =

not-even-a-nit: extra whitespace in front of '<'

@@ +306,4 @@
>  
> +  for (i = 0; i < mMobileConnListeners.Length(); i++) {
> +    nsCOMPtr<nsIMobileConnectionProvider> connection =
> +      do_GetService(NS_RILCONTENTHELPER_CONTRACTID);

Question: do we actually need to get service for [mMobileConnListeners.Length()] times? I think maybe we could do this outside the for-loop. Make sense?

@@ +372,2 @@
>  {
> +  if (mClientId < mMobileConnListeners.Length()) {

not-even-a-nit: you may want to declare a new variable to keep mMobileConnListeners.Length() since it's used here and below.
Attachment #827335 - Flags: review?(echou) → review+
Changes:
- revise SelectClient()
  > get mobile connection provider only once for all mobile connections.
  > reset mClientId at the beginning in case getting mobile connection provider fails.
- add nullptr check of radioInterfaceLayer in BluetoothRilListener constructor.
- fix nits.
Attachment #827335 - Attachment is obsolete: true
Blocks: 935573
https://hg.mozilla.org/mozilla-central/rev/69acb8830b8c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Backed out for causing bug 937199.
https://hg.mozilla.org/integration/b2g-inbound/rev/80fbd6ffab29
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 937199
Comment on attachment 829090 [details] [diff] [review]
[final] Patch 1: HFP sim selection mechanism for multi-sim, r=echou

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

Hi Ben,

I looked a little into bug 937199, which brought me here. I noticed a couple of things that need fixing in this patch before we can check it in again.

::: dom/bluetooth/BluetoothRilListener.h
@@ +117,3 @@
>  
> +  IccListener mIccListener;
> +  TelephonyListener mTelephonyListener;

MobileConnectionListener, IccListener, and TelephonyListener are all XPCOM classes that are refcounted and must be allocated on the heap and must not be embedded in another object.

Given that, the array should be an array of nsRefPtr<MobileConnectionListener> and the two other members should be nsRefPtr<>s to the proper classes.

Another slightly worrying thing is that it appears that an IccListener can be held onto separately from its owner BluetoothRilListener. If the owner dies, it appears that we don't update the IccListener and might then touch deleted memory.
No longer blocks: 935573
(In reply to Blake Kaplan (:mrbkap) from comment #18)
> Comment on attachment 829090 [details] [diff] [review]
> [final] Patch 1: HFP sim selection mechanism for multi-sim, r=echou
> 
> Review of attachment 829090 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Ben,
> 
> I looked a little into bug 937199, which brought me here. I noticed a couple
> of things that need fixing in this patch before we can check it in again.
> 
> ::: dom/bluetooth/BluetoothRilListener.h
> @@ +117,3 @@
> >  
> > +  IccListener mIccListener;
> > +  TelephonyListener mTelephonyListener;
> 
> MobileConnectionListener, IccListener, and TelephonyListener are all XPCOM
> classes that are refcounted and must be allocated on the heap and must not
> be embedded in another object.
> 
> Given that, the array should be an array of
> nsRefPtr<MobileConnectionListener> and the two other members should be
> nsRefPtr<>s to the proper classes.
> 
> Another slightly worrying thing is that it appears that an IccListener can
> be held onto separately from its owner BluetoothRilListener. If the owner
> dies, it appears that we don't update the IccListener and might then touch
> deleted memory.

The explanation is so clear. Thank you, Blake. :)
add moztrap link.
Talked with Eric Chou and this bug will be resolved and closed before the end of this week. Thanks, Eric!!!
Target Milestone: 1.3 Sprint 5 - 11/22 → 1.3 Sprint 6 - 12/6
Sorry, it should be sprint 5.
Target Milestone: 1.3 Sprint 6 - 12/6 → 1.3 Sprint 5 - 11/22
Target Milestone: 1.3 Sprint 5 - 11/22 → 1.3 Sprint 6 - 12/6
Hi Blake,

Since Ben is working on other stuff, I'd like to take over the work of revising patch. Would you minding taking a look?

What I've done in this patch:

* Keep the logic in the original patch
* Use nsRefPtr to hold the memory block of an IccListener, a TelephonyListener and a set of MobileConnectionListener(nsTArray<nsRefPtf>) in BluetoothRilListener.h
* To avoid IccListener using invalid mOwner, call SetOwner(nullptr) in the dtor of BluetoothRilListener and do nullptr check whenever mOwner is going to be used.
Attachment #8336617 - Flags: review?(mrbkap)
Comment on attachment 8336617 [details] [diff] [review]
patch 1: v2: HFP sim selection mechanism for multi-sim

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

Looks much better. Thanks!

::: dom/bluetooth/BluetoothRilListener.cpp
@@ +305,5 @@
> +  // Init MobileConnectionListener array and IccInfoListener
> +  for (uint32_t i = 0; i < numOfClients; i++) {
> +    nsRefPtr<MobileConnectionListener> listener =
> +      new MobileConnectionListener(i);
> +    mMobileConnListeners.AppendElement(listener);

I'd do this on one line:

mMobileConnListeners.AppendElement(new MobileConnectionListener(i));

which saves us a refcount.

@@ +341,5 @@
>      do_GetService(NS_RILCONTENTHELPER_CONTRACTID);
> +  NS_ENSURE_TRUE_VOID(connection);
> +
> +  uint32_t i;
> +  for (i = 0; i < mMobileConnListeners.Length(); i++) {

Nit:

for (uint32_t i = 0; ...)
Attachment #8336617 - Flags: review?(mrbkap) → review+
* nits picked. Thanks, Blake.
Attachment #829090 - Attachment is obsolete: true
Attachment #8336617 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/d293b47f5879
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Verified on 

Fugu
Gaia      ee25b0e45649d2955f340ce4f71ad55712dd0fab
Gecko     913cf2b92845441c9578787362ddad6f2ac15df6
BuildID   20140121095108
Version   28.0a2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.