938524 - [Bluetooth] Bluedroid BluetoothHfpManager prototype

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

Implement new BluetoothHfpManager for bluedroid.

Comment 1

[Ben Tian (inactive)]

Attachment: Patch 1 (v1): Implement HFP manager for bluedroid

Comment 2

[Ben Tian (inactive)]

TODO (with 'FIXME' comment in patch 1):
- solve call enumeration crashes
- solve bug that volume control observer is not added
- postpone sco close to play busytone
- handle CDMA
- run PTS
- check bug that ril doesn't notify icc info

Comment 3

[Ben Tian (inactive)]

Attachment: Patch 1 (v2): Implement HFP manager for bluedroid

Solve call enumeration crash by dispatching it to main thread.

Comment 4

[Ben Tian (inactive)]

Attachment: Patch to enable connect/disconnect

Patch to remove Connect/Disconnect error reply on current codebase for testing.

Comment 5

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

Comment on attachment 832743 [details] [diff] [review]
Patch 1 (v2): Implement HFP manager for bluedroid

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

ok, almost there. Most of my comments are nits. Please fix them and I'll review again.

By the way, in current BluetoothHfpManager.cpp (for BlueZ), there are many segments wrapped by build flag MOZ_B2G_RIL. I didn't see any in your BluetoothHfpManager.cpp (for Bluedroid), and it could be a problem. Please be careful.

::: dom/bluetooth/bluedroid/BluetoothHfpManager.cpp
@@ +34,5 @@
> +/**
> + * Dispatch task with arguments to main thread.
> + */
> +#define BT_HF_DISPATCH_MAIN(args...) \
> +    NS_DispatchToMainThread(new MainThreadTask(args))

super-nit: two-space indentation, please.

@@ +37,5 @@
> +#define BT_HF_DISPATCH_MAIN(args...) \
> +    NS_DispatchToMainThread(new MainThreadTask(args))
> +
> +/**
> + * Process bluedroid callbacks with correspondent handlers.

super-nit: should be "corresponding"

@@ +173,5 @@
> +}
> +
> +static bthf_callbacks_t sBluetoothHfpCallbacks = {
> +  sizeof(sBluetoothHfpCallbacks),
> +  connection_state_callback,

We don't use C style naming in our codebase (at least under dom/bluetooth). Please name functions like ConnectionStateCallback. Here and below.

@@ +270,5 @@
>      MOZ_ASSERT(NS_IsMainThread());
>  
> +    switch (mCommand) {
> +      case MainThreadTaskCmd::ENUMERATE_CALLS:
> +        MOZ_ASSERT(sBluetoothHfpManager);

It's a little bit annoying calling this assertion for each case. Since sBluetoothHfpManager should also have value for the last two cases, putting it before the switch-statement makes sense to me.

@@ +593,5 @@
> +void
> +BluetoothHfpManager::ProcessAtChld(bthf_chld_type_t chld)
> +{
> +  nsAutoCString message("CHLD=");
> +  message.AppendInt((int) chld);

not-even-a-nit: no extra blank for casting, please.

@@ +843,5 @@
>    if (!value.isNumber()) {
>      BT_WARNING("Failed to get relSignalStrength in BluetoothHfpManager");
>      return;
>    }
> +  mSignal = (int) ceil(value.toNumber() / 20.0);

Ditto.

@@ +905,2 @@
>  
> +  if (mPhoneType == PhoneType::CDMA && aIndex == 1 && aCall.  IsActive()) {

nit: aCall.IsActive(), no whitespaces please.

@@ +1125,5 @@
>  
>    mCdmaSecondCall.mNumber = aNumber;
> +  if (aNumber[0] == '+') {
> +    mCdmaSecondCall.mType = BTHF_CALL_ADDRTYPE_INTERNATIONAL;
> +  }

Question: do we need to handle the case aNumber[0] != '+'? The value of mCdmaSecondCall.mType would be set to TOA_UNKNOWN if aNumber[0] != '+'.

@@ +1209,5 @@
> +
> +bool
> +BluetoothHfpManager::IsConnected()
> +{
> +  return (mConnectionState == BTHF_CONNECTION_STATE_SLC_CONNECTED);

Please note that the logic here may be slightly different from the original IsConnected(). Here we check if mConnectionState is BTHF_CONNECTION_STATE_SLC_CONNECTED. As for current implementation, we check the connection state of mSocket, which means IsConnected() returns true if RFCOMM session is established. Seems like we should check both BTHF_CONNECTION_STATE_CONNECTED and BTHF_CONNECTION_STATE_SLC_CONNECTED here.

::: dom/bluetooth/bluedroid/BluetoothHfpManager.h
@@ +111,5 @@
> +  void ProcessAtCnum();
> +  void ProcessAtCind();
> +  void ProcessAtCops();
> +  void ProcessAtClcc();
> +  void ProcessUnknownAt(char *at_string);

nit: parameter name usually starts with an 'a' and uses camel case.

@@ +178,5 @@
>    nsString mOperatorName;
>  
>    nsTArray<Call> mCurrentCallArray;
>    nsAutoPtr<BluetoothRilListener> mListener;
> +  BluetoothProfileController* mController;

nsRefPtr<BluetoothProfileController>

Comment 6

[Ben Tian (inactive)]

> ok, almost there. Most of my comments are nits. Please fix them and I'll
> review again.
> 
> By the way, in current BluetoothHfpManager.cpp (for BlueZ), there are many
> segments wrapped by build flag MOZ_B2G_RIL. I didn't see any in your
> BluetoothHfpManager.cpp (for Bluedroid), and it could be a problem. Please
> be careful.
I'll open a follow-up bug for it.

> > +/**
> > + * Process bluedroid callbacks with correspondent handlers.
> 
> super-nit: should be "corresponding"
Correspondent and corresponding are synonyms. Anyway I modify for clarity.

> @@ +1125,5 @@
> >  
> >    mCdmaSecondCall.mNumber = aNumber;
> > +  if (aNumber[0] == '+') {
> > +    mCdmaSecondCall.mType = BTHF_CALL_ADDRTYPE_INTERNATIONAL;
> > +  }
> 
> Question: do we need to handle the case aNumber[0] != '+'? The value of
> mCdmaSecondCall.mType would be set to TOA_UNKNOWN if aNumber[0] != '+'.
NO, that's what we want. The same logic in HandleCallStateChanged():
  if (aNumber.Length() && aNumber[0] == '+') {
    mCurrentCallArray[aCallIndex].mType = BTHF_CALL_ADDRTYPE_INTERNATIONAL;
  }

> @@ +1209,5 @@
> > +
> > +bool
> > +BluetoothHfpManager::IsConnected()
> > +{
> > +  return (mConnectionState == BTHF_CONNECTION_STATE_SLC_CONNECTED);
> 
> Please note that the logic here may be slightly different from the original
> IsConnected(). Here we check if mConnectionState is
> BTHF_CONNECTION_STATE_SLC_CONNECTED. As for current implementation, we check
> the connection state of mSocket, which means IsConnected() returns true if
> RFCOMM session is established. Seems like we should check both
> BTHF_CONNECTION_STATE_CONNECTED and BTHF_CONNECTION_STATE_SLC_CONNECTED here.
What do you mean by 'check both states'? Since the state can only be one of them, do you mean to ensure state be either BTHF_CONNECTION_STATE_CONNECTED or BTHF_CONNECTION_STATE_SLC_CONNECTED?

Comment 7

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

> > @@ +1209,5 @@
> > > +
> > > +bool
> > > +BluetoothHfpManager::IsConnected()
> > > +{
> > > +  return (mConnectionState == BTHF_CONNECTION_STATE_SLC_CONNECTED);
> > 
> > Please note that the logic here may be slightly different from the original
> > IsConnected(). Here we check if mConnectionState is
> > BTHF_CONNECTION_STATE_SLC_CONNECTED. As for current implementation, we check
> > the connection state of mSocket, which means IsConnected() returns true if
> > RFCOMM session is established. Seems like we should check both
> > BTHF_CONNECTION_STATE_CONNECTED and BTHF_CONNECTION_STATE_SLC_CONNECTED here.
> What do you mean by 'check both states'? Since the state can only be one of
> them, do you mean to ensure state be either BTHF_CONNECTION_STATE_CONNECTED
> or BTHF_CONNECTION_STATE_SLC_CONNECTED?

What I meant was like 

bool
BluetoothHfpManager::IsConnected()
{
  return (mConnectionState == BTHF_CONNECTION_STATE_CONNECTED) ||
         (mConnectionState == BTHF_CONNECTION_STATE_SLC_CONNECTED);
}

It would be more similar to current implementation in this case.

Comment 8

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

By the way, I agree that SLC established is more meaningful to us. Maybe we could file a followup to revisit the whole flow and see if we could change the mechanism for both BlueZ and Bluedroid cases.

Comment 9

[Ben Tian (inactive)]

Attachment: Patch 1 (v3): Implement HFP manager for bluedroid

Changes:
- revise IsConnected() according to RFCOMM connection.
- move call enumeration into NotifyConnectionStateChanged function.
- fix nits.

Comment 10

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #6)
> > ok, almost there. Most of my comments are nits. Please fix them and I'll
> > review again.
> > 
> > By the way, in current BluetoothHfpManager.cpp (for BlueZ), there are many
> > segments wrapped by build flag MOZ_B2G_RIL. I didn't see any in your
> > BluetoothHfpManager.cpp (for Bluedroid), and it could be a problem. Please
> > be careful.
> I'll open a follow-up bug for it.
Track in bug 939672.

> > @@ +1209,5 @@
> > > +
> > > +bool
> > > +BluetoothHfpManager::IsConnected()
> > > +{
> > > +  return (mConnectionState == BTHF_CONNECTION_STATE_SLC_CONNECTED);
> > 
> > Please note that the logic here may be slightly different from the original
> > IsConnected(). Here we check if mConnectionState is
> > BTHF_CONNECTION_STATE_SLC_CONNECTED. As for current implementation, we check
> > the connection state of mSocket, which means IsConnected() returns true if
> > RFCOMM session is established. Seems like we should check both
> > BTHF_CONNECTION_STATE_CONNECTED and BTHF_CONNECTION_STATE_SLC_CONNECTED here.
> What do you mean by 'check both states'? Since the state can only be one of
> them, do you mean to ensure state be either BTHF_CONNECTION_STATE_CONNECTED
> or BTHF_CONNECTION_STATE_SLC_CONNECTED?
Track in bug 939673 to refine IsConnected() definition.

Comment 11

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

Comment on attachment 8333684 [details] [diff] [review]
Patch 1 (v3): Implement HFP manager for bluedroid

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

r=me with nits addressed. Thanks.

::: dom/bluetooth/bluedroid/BluetoothHfpManager.cpp
@@ +529,5 @@
> +  BT_LOGR("%s: state %d", __FUNCTION__, aState);
> +
> +  mAudioState = aState;
> +
> +  if (IsScoConnected() || aState == BTHF_AUDIO_STATE_DISCONNECTED) {

not-even-a-nit: When I want to see if this if-statement would be entered, I need to go check what's done in IsScoConnected(). So I would prefer using 

  if (aState == BTHF_AUDIO_STATE_CONNECTED || aState == BTHF_AUDIO_STATE_DISCONNECTED) {

instead. It could also avoid a potential function call. One advantage of the original design is the statement would be shorter. Up to you.

@@ +1166,5 @@
> +  NS_ENSURE_TRUE(!sInShutdown, false);
> +  NS_ENSURE_TRUE(IsConnected() && !IsScoConnected(), false);
> +  NS_ENSURE_TRUE(sBluetoothHfpInterface, false);
> +
> +  bt_bdaddr_t device_addr;

super-nit: please use camel case.

@@ +1180,5 @@
> +{
> +  NS_ENSURE_TRUE(IsScoConnected(), false);
> +  NS_ENSURE_TRUE(sBluetoothHfpInterface, false);
> +
> +  bt_bdaddr_t device_addr;

super-nit: please use camel case.

@@ +1273,5 @@
>  
>  void
>  BluetoothHfpManager::OnUpdateSdpRecords(const nsAString& aDeviceAddress)
>  {
> +  // Bluedroid handles this part

MOZ_ASSERT(false), please.

@@ +1281,5 @@
>  BluetoothHfpManager::OnGetServiceChannel(const nsAString& aDeviceAddress,
>                                           const nsAString& aServiceUuid,
>                                           int aChannel)
>  {
> +  // Bluedroid handles this part

Ditto.

Comment 12

[Ben Tian (inactive)]

Attachment: [final] Patch 1: Implement HFP manager for bluedroid, r=echou

Comment 13

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

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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