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?

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.

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

(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

The bug depends mainly on bug 926343 and 818353 for RIL API to bluetooth. Remove webAPI bugs dependency.

WIP patch for BT HFP. Tested on single-sim device. Will test on emulator for multi-sim environment.

Changes: - implement sim selection mechanism for multi-sim - refactor BluetoothRilListener

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

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.

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.

try server link: https://tbpl.mozilla.org/?tree=Try&rev=45894fc374d8

Backed out for causing bug 937199. https://hg.mozilla.org/integration/b2g-inbound/rev/80fbd6ffab29

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.

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

Sorry, it should be sprint 5.

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.

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; ...)

* nits picked. Thanks, Blake.

try: https://tbpl.mozilla.org/?tree=Try&rev=46d8b7cad27b

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