Created an attachment (id=385957) HSP cases +++ This bug was initially created as a clone of Bug #437511 +++ Created an attachment (id=385878) log DEFECT DESCRIPTION: Can not connected to PTS-HSP-VAL-yuanying for HSP/HFP/OPP profile cases test REPRODUCING PROCEDURES: 1.Open PTS,run case,find can not connected to PTS-HSP-VAL-yuanying EXPECTED BEHAVIOUR: For KO,should can connected to PTS-HSP-VAL-yuanying,then run HFP cases ASSOCIATE SPECIFICATION: TEST PLAN REFERENCE: TOOLS AND PLATFORMS USED: USER IMPACT: REPRODUCING RATE: 5/5 For FT PR, Please list reference mobile's behavior: ++++++++++ end of initial bug #437511 description ++++++++++

Hi Eric, can you confirm HSP socket can be opened for v1.0.1?

Please help to clarify that HFP/OPP cannot get connected, it works for me.

(In reply to Shawn Huang from comment #1) > Hi Eric, can you confirm HSP socket can be opened for v1.0.1? Currently we don't create socket listening to HSP, so PTS tests of HSP must fail. The reason why we didn't have a HSP listening socket is that BluetoothHfpManager could listen to more than one socket at a time. After bug 851046 landed, we can start fixing this. I'll take this and nominate as a tef+ just like bug 823803.

Created an attachment (id=385968) PTS guide

Created an attachment (id=385968) PTS guide

Sync from brother

Sync from brother

Sync from brother

Sync from brother

Sync from brother

Sync from brother

Created an attachment (id=390202) HSP cases

Created an attachment (id=390202) HSP cases

Eric, can you provide an ETA here? Thanks!

(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #14) > Eric, can you provide an ETA here? Thanks! The patch is almost ready.

This patch is quite similar as the patch of bug 823803. Because we have implemented the logic of Headset profile (e.g. dealing with command AT+CKPD=200) in BluetoothHfpManager.cpp, all we have to do is to maintain two server sockets for listening to HSP and HFP at the same time.

Comment on attachment 738419 [details] [diff] [review] patch 1: v1: Use two server sockets to listen to both HFP and HSP services Review of attachment 738419 [details] [diff] [review]: ----------------------------------------------------------------- r+ with nit addressed. ::: dom/bluetooth/BluetoothHfpManager.cpp @@ +1412,2 @@ > CloseScoSocket(); > + nit: Please move the above comments to here. Would that be clearer?

* Revised HSP related code based on Bluetooth HSP 1.2 spec

Comment on attachment 738979 [details] [diff] [review] patch 2: v1: Revise AT-command handler (AT+CKPD) Review of attachment 738979 [details] [diff] [review]: ----------------------------------------------------------------- The overall is good. Would be happy to see a new patch with handling both ATA and CHUP again. ::: dom/bluetooth/BluetoothHfpManager.cpp @@ +894,5 @@ > + } else { > + // Bluetooth HSP spec 4.4 ~ 4.6 > + // AG can decide the way dealing with AT+CKPD=200. Currently we don't > + // do anything but switching SCO. > + BluetoothScoManager* sco = BluetoothScoManager::Get(); nit: Suggest to use "NS_ENSURE_TRUE_VOID(sco);" here, then we don't need to indent the following lines. Or, you may want to add some warning message when we fail to get BluetoothScoManager. @@ +904,5 @@ > + mSocket->GetAddress(address); > + > + sco->Connect(address); > + } > + } I would recommend to support both answer and hangup a phone call, and a Sco link will be closed after hanging up, (like what we did for answering). For example: // If we have at least a call, then send ATA/CHUP // Otherwise, we switch sco if (mCurrentCallArray.Length() > 1) { if (!sStopSendingRingFlag) NotifyDialer(NS_LITERAL_STRING("ATA")); else NotifyDialer(NS_LITERAL_STRING("CHUP")); } else { BluetoothScoManager* sco = BluetoothScoManager::Get(); NS_ENSURE_TRUE_VOID(sco); if (sco->IsConnected()) { sco->Disconnect(); } else { nsAutoString address; mSocket->getAddress(address); sco->Connect(address); } } Well, it's just for your reference, any way to handle both ATA and CHUP are welcomed. And please correct me if it doesn't work. :( ::: dom/bluetooth/BluetoothScoManager.cpp @@ +205,1 @@ > NS_WARNING("Sco socket has been connected"); nit: Since the return value of IsConnected() is true whenever socket connection status is CONNECTED or CONNECTING, modify the warning message here would be better. @@ +294,5 @@ > +BluetoothScoManager::IsConnected() > +{ > + if (mSocket) { > + if (mSocket->GetConnectionStatus() == SocketConnectionStatus::SOCKET_CONNECTED || > + mSocket->GetConnectionStatus() == SocketConnectionStatus::SOCKET_CONNECTING) { Question 1: How about make a varialbe for |mSocket->GetConnectionStatus| and reuse it in if condition? Question 2: I don't think it would be a good idea for the case of returning true whenever the connection status is CONNECTING. nit: trailing-space.

> > The overall is good. Would be happy to see a new patch with handling both > ATA and CHUP again. > > ::: dom/bluetooth/BluetoothHfpManager.cpp > @@ +894,5 @@ > > + } else { > > + // Bluetooth HSP spec 4.4 ~ 4.6 > > + // AG can decide the way dealing with AT+CKPD=200. Currently we don't > > + // do anything but switching SCO. > > + BluetoothScoManager* sco = BluetoothScoManager::Get(); > > nit: Suggest to use "NS_ENSURE_TRUE_VOID(sco);" here, then we don't need to > indent the following lines. Or, you may want to add some warning message > when we fail to get BluetoothScoManager. > I'll add warning messages, but I rather use if(sco) instead of NS_ENSURE_TRUE_VOID(sco). If NS_ENSURE_TRUE_VOID(sco) is used, we won't respond with OK/ERROR to the remote device. > @@ +904,5 @@ > > + mSocket->GetAddress(address); > > + > > + sco->Connect(address); > > + } > > + } > > I would recommend to support both answer and hangup a phone call, and a Sco > link will be closed after hanging up, (like what we did for answering). > > For example: > // If we have at least a call, then send ATA/CHUP > // Otherwise, we switch sco > if (mCurrentCallArray.Length() > 1) { > if (!sStopSendingRingFlag) > NotifyDialer(NS_LITERAL_STRING("ATA")); > else > NotifyDialer(NS_LITERAL_STRING("CHUP")); > } else { > BluetoothScoManager* sco = BluetoothScoManager::Get(); > NS_ENSURE_TRUE_VOID(sco); > if (sco->IsConnected()) { > sco->Disconnect(); > } else { > nsAutoString address; > mSocket->getAddress(address); > sco->Connect(address); > } > } > > > > Well, it's just for your reference, any way to handle both ATA and CHUP are > welcomed. And please correct me if it doesn't work. :( > The behavior after received AT+CKPD is up to the AG, so I guess it should be fine. We'll see.

Comment on attachment 738419 [details] [diff] [review] patch 1: v1: Use two server sockets to listen to both HFP and HSP services Review of attachment 738419 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothHfpManager.h @@ +128,5 @@ > + // Server sockets. Once an inbound connection is established, it will hand > + // over the ownership to mSocket, and get a new server socket while Listen() > + // is called. > + nsRefPtr<BluetoothSocket> mHfpSocket; > + nsRefPtr<BluetoothSocket> mHspSocket; Even knowing what Hfp and Hsp stand for, I find these two variable names way too close to each other. Could you rename them to something like: mHandsFreeSocket and mHeadSetSocket so there's less of a chance of confusing them?

* nits picked

* all nits picked and problems fixed, except: >I would recommend to support both answer and hangup a phone call, and a Sco >link will be closed after hanging up, (like what we did for answering). >For example: >// If we have at least a call, then send ATA/CHUP >// Otherwise, we switch sco >if (mCurrentCallArray.Length() > 1) { > if (!sStopSendingRingFlag) > NotifyDialer(NS_LITERAL_STRING("ATA")); > else > NotifyDialer(NS_LITERAL_STRING("CHUP")); >} else { > BluetoothScoManager* sco = BluetoothScoManager::Get(); > NS_ENSURE_TRUE_VOID(sco); > if (sco->IsConnected()) { > sco->Disconnect(); > } else { > nsAutoString address; > mSocket->getAddress(address); > sco->Connect(address); > } >} It seems to work at first glance, however, imagine this scenario: (1) No Bluetooth connections (2) Make a call, so there is an ongoing phone call. (3) HS connects with AG, and AG receives AT+CKPD (Section 4.3 in Bluetooth HSP spec) In this case, CHUP would be sent to Dialer app and the ongoing phone call would be terminated by Dialer. Of course it's not what we expected, but we do want to end a call when we receive AT+CKPD=200 at any other time. To solve this, I add a flag mFirstCKPD to identify if this CKPD is the very first AT+CKPD=200. If it is, then don't notify dialer but just ignore this command.

Comment on attachment 739433 [details] [diff] [review] patch 2: v2: Revise AT-command handler (AT+CKPD) Review of attachment 739433 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for correcting me. This patch looks good to me. r+ with nits addressed. ::: dom/bluetooth/BluetoothHfpManager.cpp @@ +918,5 @@ > + // Three conditions have to be matched to come in here: > + // (1) Not sending RING indicator > + // (2) A SCO link exists > + // (3) This is the very first AT+CKPD=200 of this session > + // It is the case of Figure 4.3, Bluetooth HSP spec. Do nothing. Since the CKPD is ignored here, please add a warning message. It would be easier for us to debug. ::: dom/bluetooth/BluetoothScoManager.cpp @@ +202,5 @@ > } > > + if (mSocket->GetConnectionStatus() == SocketConnectionStatus::SOCKET_CONNECTED || > + mSocket->GetConnectionStatus() == SocketConnectionStatus::SOCKET_CONNECTING) { > + NS_WARNING("SCO connection exists or is being established"); Please see comment in the last review.

birch: https://hg.mozilla.org/projects/birch/rev/dc68cf5c11c2 https://hg.mozilla.org/projects/birch/rev/e57d3f2e3284

Hi sheriff, please use this patch for b2g18. Thank you.

Hi sheriff, please use this patch for b2g18. Thank you.

As for BluetoothHfpManager.cpp on branch v1_0_1, it's too different from b2g18, and we're still waiting for the triage result to see if we can merge following bugs into v1_0_1: 01. bug827204 02. bug827255 03. bug823346 04. bug827230 05. bug828798 06. bug827212, patch 1 06. bug827212, patch 2 07. bug827266 08. bug828175 09. bug846647 10. bug825861 11. bug825851 12. bug835740 Once these bugs are allowed to be merged, we should be able to use the patch for b2g18. Otherwise, I'll provide a v1_0_1-specific patch.

https://hg.mozilla.org/mozilla-central/rev/dc68cf5c11c2 https://hg.mozilla.org/mozilla-central/rev/e57d3f2e3284

https://hg.mozilla.org/releases/mozilla-b2g18/rev/c3761490ffac https://hg.mozilla.org/releases/mozilla-b2g18/rev/19b5590e161c

v1.0.1-specific patch

v1.0.1-specific patch

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/f33f0472001e https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/3a67efe71e11

Created an attachment (id=401242) all log KO ON 126+DT. Please refer to log_2013_05_02

Created an attachment (id=401242) all log KO ON 126+DT. Please refer to log_2013_05_02

(In reply to sync-1 from comment #36) > Created attachment 744430 [details] > all log > > Created an attachment (id=401242) > all log > > KO ON 126+DT. > Please refer to log_2013_05_02 I couldn't extract the .rar correctly. In addition, please describe more detail of the problem you hit, such as which test case is failed.

Created an attachment (id=401316) sniffer log

Created an attachment (id=401316) sniffer log

Created an attachment (id=401318) kernel log

Created an attachment (id=401318) kernel log

All HSP and HFP cases can not run pass. Because test phone can not connected to PTS!

Created an attachment (id=401320) logcat

Created an attachment (id=401320) logcat

I don't think current code can always cause connection failure. But it's better to get PTS logs for failure case. After patch applied, HSP shall be able to connect. Please confirm PIXIT configuration is correct. Please confirm (1). BD address (TSPX_bd_addr_iut) in pixit config is correct (2). Sometimes PTS cannot connect due to link key corrupted, please try to change TSPX_delete_link_key to TRUE, this can remove PTS local cached link key.

Created an attachment (id=401531) HFPlog

Created an attachment (id=401531) HFPlog

(In reply to comment #35) > Comment from Mozilla:I don't think current code can always cause connection > failure. > But it's better to get PTS logs for failure case. > After patch applied, HSP shall be able to connect. Please confirm PIXIT > configuration is correct. > Please confirm > (1). BD address (TSPX_bd_addr_iut) in pixit config is correct > (2). Sometimes PTS cannot connect due to link key corrupted, please try to > change TSPX_delete_link_key to TRUE, this can remove PTS local cached link key. (1)BT address is correct (2)I choose TSPX_delete_link_key to TRUE in HSP and HFP pixit And also can not connected to PTS,I have upload HSP and HFP log ,If any question,please reply me. Thanks!

Created an attachment (id=401532) HSPlog

Created an attachment (id=401532) HSPlog

xml log can only tell VERDICT is "INCONC". Can you upload your HSP logfile.log?

(In reply to comment #41) > Comment from Mozilla:xml log can only tell VERDICT is "INCONC". Can you upload > your HSP logfile.log? My loggile.log all show blank,how should I catch this log?

Created an attachment (id=401701) HSP log

Created an attachment (id=401701) HSP log

Marking as QARegressExclude. Unable to test due to not having the correct test tools to test this.

(In reply to comment #44) > Comment from Mozilla:Marking as QARegressExclude. Unable to test due to not > having the correct test tools to test this. which test tool?we all use PTS tool to test.

Just found out the root cause. With current implementation, we would not call Listen() again when GetDeviceServiceChannel() failed. So, if you try to test HSP before HFP, connection request would be rejected by PTS because only HSP is supported. After that, both HFP and HSP would not be able to be connected with. However, Bluetooth still works when users tend to connect to a device without correct services, such as trying to send a file to a headset. We need to fix this in all branches. I'm going to reopen this bug, I'll make a patch, and try to solve it by next Monday.

typo: > However, Bluetooth still works when users tend to connect to a device s/tend/not tend > without correct services, such as trying to send a file to a headset.

Re-milestoning for completion by 5/10, now that this has reopened.

This patch can fix the problem mentioned in comment 57. Bluetooth now could recover from a connection failure.

BluetoothProfileManagerBase is a base class of Bluetooth profile managers. It will be a generic interface declaring basic functions that all profile managers should implement, such as Connect(), Disconnect() and IsConnected(). Currently there is only one callback function OnGetServiceChannel() declared, which will be called once BluetoothService::GetServiceChannel() is done.

Things we have done in this patch: 1. Make BluetoothHfpManager/BluetoothOppManager implement BluetoothProfileManagerBase 2. Add function BluetoothService::GetServiceChannel() 3. Once GetServiceChannel() is done, callback Bluetooth*Manager::OnGetServiceChannel() will be invoked 4. Remove unused function BluetoothService::GetSocketViaService()

Comment on attachment 746256 [details] [diff] [review] patch 3: v2: create interface BluetoothProfileManagerBase Hi Blake, could you help with reviewing this patch? Thanks.

Comment on attachment 746257 [details] [diff] [review] patch 4: v1: New mechanism for establishing outboung Bluetooth connections Hi Blake, could you help with reviewing this patch? Thanks.gyeh@mozilla.com

Comment on attachment 746257 [details] [diff] [review] patch 4: v1: New mechanism for establishing outboung Bluetooth connections Review of attachment 746257 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothHfpManager.cpp @@ +974,5 @@ > > + if (NS_FAILED(bs->GetServiceChannel(aDevicePath, uuid, this))) { > + BluetoothValue v; > + nsString replyError; > + replyError.AssignLiteral("GetServiceChannel failed"); Please do this as: NS_NAMED_LITERAL_STRING(replyError, "GetServiceChannel failed"); @@ +1448,5 @@ > + nsString replyError; > + > + if (aChannel < 0) { > + replyError.AssignLiteral("DeviceChannelRetrievalError"); > + DispatchBluetoothReply(mRunnable, v, replyError); Instead of using an explicit nsString, just use NS_LITERAL_STRING("DeviceChannelRetrievalError"). That way you can get rid of replyError. @@ +1457,5 @@ > + > + nsCString address; > + address.Append(NS_ConvertUTF16toUTF8(aDeviceAddress)); > + > + if (!mSocket->Connect(address, aChannel)) { Just pass the result of NS_ConvertUTF16toUTF8(aDeviceAddress) directly to mSocket->Connect. ::: dom/bluetooth/BluetoothOppManager.cpp @@ +1487,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + MOZ_ASSERT(mRunnable); > + > + BluetoothValue v; > + nsString replyError; The comments in the Hfp manager apply equally to this file as well. ::: dom/bluetooth/linux/BluetoothDBusService.cpp @@ +2546,2 @@ > errorStr.AssignLiteral("Unknown profile"); > DispatchBluetoothReply(aRunnable, v, errorStr); Instead of the explicit string variable, just pass NS_LITERAL_STRING("Unknown profile") through. @@ +2704,4 @@ > return NS_ERROR_FAILURE; > } > > + r.forget(); This leaks, don't forget(). @@ +2731,4 @@ > return NS_ERROR_FAILURE; > } > > + r.forget(); This leaks, don't forget().

* problems solved

When run HFP/HSP PTS cases,Still failed to connected to PTS,use PTS 4.5.2 because can not install 4.5.3. You can refer to log(log_0509). AU_LINUX_GECKO_ICS_STRAWBERRY_V1.01.00.01.019.094 Firefox os v1.0.1 Mozilla build ID:20130502070201

When run HFP/HSP PTS cases,find also can not connected to PTS,use PTS 4.5.2 because can not install 4.5.3.You can refer to log(log_0509). Thanks!

Based Comment 69, is this result based on applied patch 3, patch 4?

Hello Eric, is there any additional work need to do except patch 3, patch 4? Like gaia?

= birch = https://hg.mozilla.org/projects/birch/rev/22cfc706d287 https://hg.mozilla.org/projects/birch/rev/faf47bc87fff

* patch 4 for b2g18

* patch 4 for b2g18_v1_0_1

* patch 4 for b2g18 (updated. The commit summary of the former patch is incorrect)

* patch 4 for b2g18-v1_0_1 (updated. The commit summary of the former patch is incorrect)

https://hg.mozilla.org/mozilla-central/rev/faf47bc87fff https://hg.mozilla.org/mozilla-central/rev/22cfc706d287

https://hg.mozilla.org/releases/mozilla-b2g18/rev/18ecbe8d086e https://hg.mozilla.org/releases/mozilla-b2g18/rev/a157791b86ce https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/0f66b12ef3df https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/667e41dd15a2

Gaia shall be Bug 870689.