934891 - [fugu][Bluetooth]Can not disconnect bluetooth headset

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Xinhe Yan]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/535.19 (KHTML, like Gecko) Ubuntu/11.04 Chromium/18.0.1025.151 Chrome/18.0.1025.151 Safari/535.19

Steps to reproduce:

1 Connect A2DP bluetooth headset(N9)
2 Make a call out
3 Press home button
4 Enter setting app and choose blueooth
5 Click N9 in Paired devices and Choose Disconnect


Actual results:

Can not disconnect heaset


Expected results:

headset will be disconnect and phone sound turn to handset

Comment 1

[Xinhe Yan]

I found BluetoothHfpManager::Disconnect is not called. The root cause is in BluetoothA2dpManager.cpp BluetoothA2dpManager::HandleSinkPropertyChanged.

When got SINK_DISCONNECTED signal,
      // case 7 only
      if (prevState == SinkState::SINK_DISCONNECTING) {
        OnDisconnect(EmptyString());
               }
 
In external/bluetooth/bluez/audio/sink.c
static char *str_state[] = { 
    "SINK_STATE_DISCONNECTED",
    "SINK_STATE_CONNECTING",
    "SINK_STATE_CONNECTED",
    "SINK_STATE_PLAYING",
};
But qualcomm add a new state SINK_STATE_DISCONNECTING. 

So this SINK_STATE_DISCONNECTING state cause OnDisconnect can not be called.
I remove this state check, just call OnDisconnect(EmptyString()), fugu can also disconnect hfp.

Comment 2

[Gina Yeh [:gyeh] [:ginayeh]]

Could you give me some more information about the BlueZ you're using? Which repo and the version number is? Thanks.

Comment 3

[Xinhe Yan]

For qualcomm bluez, I use hamachi code which download form https://github.com/mozilla-b2g/b2g-manifest/blob/v1.2/hamachi.xml. git commit is 4586e7d69d07e96704b506a91d90d5a0b8495afe

For sprd bluez, fugu is not push to mozilla github now. But the bluez code can be download from our server, you can ask steven yang for manifest. Our git commit in bluez is 2e14f94021d37685f17644fb171cae403cbea45d.

Comment 4

[Gina Yeh [:gyeh] [:ginayeh]]

Thanks for your quick reply.

The solutions seems great to me. When disconnecting from remote devices, OnDisconnect() is invoked and returns immediately since mController is null in this case.

void
BluetoothA2dpManager::OnDisconnect(const nsAString& aErrorStr)
{
  MOZ_ASSERT(NS_IsMainThread());

  /**
   * On the one hand, notify the controller that we've done for outbound
   * connections. On the other hand, we do nothing for inbound connections.
   */
  NS_ENSURE_TRUE_VOID(mController);
  ...
}

Let me refine comments and create a patch. I'll also ask some feedback from Xinhe Yan. :)

Comment 5

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v1): Remove state DISCONNECTING

Comment 6

[Xinhe Yan]

Hi Gina,

I test this patch on v1.2 and master branch, this patch resolved our problem.

Thanks.

Comment 7

[Gina Yeh [:gyeh] [:ginayeh]]

Great! Would you mind setting flag feedback to '+'? Thanks.

(Click "Review" in the attachments section, then you can set the feedback from '?' to '+'.)

Comment 8

[Xinhe Yan]

Comment on attachment 827838 [details] [diff] [review]
Patch 1(v1): Remove state DISCONNECTING

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

This patch fix fugu problem.

Comment 9

[Steven Yang [:styang]]

There is no option for Xinhe to change the flag, so I changed it based on his comments.

Comment 10

[Gina Yeh [:gyeh] [:ginayeh]]

I see. Thanks, Steven.

Comment 11

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 827838 [details] [diff] [review]
Patch 1(v1): Remove state DISCONNECTING

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

r=me with nits addressed and question answered.

::: dom/bluetooth/BluetoothA2dpManager.cpp
@@ +269,5 @@
>    const BluetoothValue& value = arr[0].value();
>    MOZ_ASSERT(value.type() == BluetoothValue::TnsString);
> +  SinkState newState = StatusStringToSinkState(value.get_nsString());
> +  NS_ENSURE_TRUE_VOID((newState != SinkState::SINK_UNKNOWN) &&
> +                      (newState != mSinkState));

Question: is it possible that

  newState == SinkState::SINK_UNKNOWN  and  current mSinkState != SinkState::SINK_UNKNOWN ?

If it is, then should we save SinkState::SINK_UNKNOWN to mSinkState?

On the other hand, if newState is impossible to be SinkState::SINK_UNKNOWN, how about adding an assertion check?

@@ +303,2 @@
>        // case 2: Connection attempt failed
>        if (prevState == SinkState::SINK_CONNECTING) {  

nit: trailing whitespaces

@@ +304,5 @@
>        if (prevState == SinkState::SINK_CONNECTING) {  
>          OnConnect(NS_LITERAL_STRING("A2dpConnectionError"));
>          break;
>        }
>        

nit: trailing whitespaces

@@ +312,1 @@
>    

nit: trailing whitespaces

Comment 12

[Andrew Overholt [:overholt]]

Apologies if this shouldn't be marked regression (it seemed like it to me).

Comment 13

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #11)
> @@ +269,5 @@
> >    const BluetoothValue& value = arr[0].value();
> >    MOZ_ASSERT(value.type() == BluetoothValue::TnsString);
> > +  SinkState newState = StatusStringToSinkState(value.get_nsString());
> > +  NS_ENSURE_TRUE_VOID((newState != SinkState::SINK_UNKNOWN) &&
> > +                      (newState != mSinkState));
> 
> Question: is it possible that
> 
>   newState == SinkState::SINK_UNKNOWN  and  current mSinkState !=
> SinkState::SINK_UNKNOWN ?
> 
> If it is, then should we save SinkState::SINK_UNKNOWN to mSinkState?
> 
> On the other hand, if newState is impossible to be SinkState::SINK_UNKNOWN,
> how about adding an assertion check?
> 

Oops! It should be OR rather than AND here. That is to say, two cases should be ignored and returned earlier:

1. The state equals to the previous one. Normally we shouldn't receive state change, however, sometimes we might got the same state twice. I think it might be a bug of our stack.

2. UNKNOWN state should be skipped.

Does it make sense to you?

Comment 14

[Eric Chou [:ericchou] [:echou]]

(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #13)
> (In reply to Eric Chou [:ericchou] [:echou] from comment #11)
> > @@ +269,5 @@
> > >    const BluetoothValue& value = arr[0].value();
> > >    MOZ_ASSERT(value.type() == BluetoothValue::TnsString);
> > > +  SinkState newState = StatusStringToSinkState(value.get_nsString());
> > > +  NS_ENSURE_TRUE_VOID((newState != SinkState::SINK_UNKNOWN) &&
> > > +                      (newState != mSinkState));
> > 
> > Question: is it possible that
> > 
> >   newState == SinkState::SINK_UNKNOWN  and  current mSinkState !=
> > SinkState::SINK_UNKNOWN ?
> > 
> > If it is, then should we save SinkState::SINK_UNKNOWN to mSinkState?
> > 
> > On the other hand, if newState is impossible to be SinkState::SINK_UNKNOWN,
> > how about adding an assertion check?
> > 
> 
> Oops! It should be OR rather than AND here. That is to say, two cases should
> be ignored and returned earlier:
> 
> 1. The state equals to the previous one. Normally we shouldn't receive state
> change, however, sometimes we might got the same state twice. I think it
> might be a bug of our stack.
> 
> 2. UNKNOWN state should be skipped.
> 
> Does it make sense to you?

Yes, it's fair enough. You have answered my questions. Thanks.

However, based on your answer, I think using AND is right and you don't need to change. Is that correct?

Comment 15

[Gina Yeh [:gyeh] [:ginayeh]]

AND is right... I need more coffee :(

Comment 16

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Andrew Overholt [:overholt] from comment #12)
> Apologies if this shouldn't be marked regression (it seemed like it to me).

It could be a regression from the users' point of view, and I think that it would be great if we have this fix in v1.2    :)

Comment 17

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: [Final] Remove DISCONNECTING state from A2DP, r=echou

https://tbpl.mozilla.org/?tree=Try&rev=87a3d52646cb

Comment 18

[Gina Yeh [:gyeh] [:ginayeh]]

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

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b6376e52dd5e

Comment 20

[Ryan VanderMeulen [:RyanVM]]

This push was empty.

Comment 21

[Gina Yeh [:gyeh] [:ginayeh]]

Sorry about that. Later I'll push it again. I'm waiting for the result of try server.

https://tbpl.mozilla.org/?tree=Try&rev=1f965b21ee12

Comment 22

[Gina Yeh [:gyeh] [:ginayeh]]

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

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ba4cb364ad6d

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/24aea16490ba