[Bluetooth] Bluedroid BluetoothHfpManager prototype

RESOLVED FIXED in 1.3 Sprint 5 - 11/22

Status

Firefox OS
Bluetooth
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: btian, Assigned: btian)

Tracking

unspecified
1.3 Sprint 5 - 11/22
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

4 years ago
Implement new BluetoothHfpManager for bluedroid.
(Assignee)

Updated

4 years ago
Depends on: 938521
(Assignee)

Comment 1

4 years ago
Created attachment 832129 [details] [diff] [review]
Patch 1 (v1): Implement HFP manager for bluedroid
Attachment #832129 - Flags: review?(echou)
(Assignee)

Comment 2

4 years ago
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
(Assignee)

Updated

4 years ago
Depends on: 936732
(Assignee)

Updated

4 years ago
No longer depends on: 936732
(Assignee)

Updated

4 years ago
Summary: [Bluetooth] Implement new BluetoothHfpManager for bluedroid → [Bluetooth] Bluedroid BluetoothHfpManager prototype
(Assignee)

Comment 3

4 years ago
Created attachment 832743 [details] [diff] [review]
Patch 1 (v2): Implement HFP manager for bluedroid

Solve call enumeration crash by dispatching it to main thread.
Attachment #832129 - Attachment is obsolete: true
Attachment #832129 - Flags: review?(echou)
Attachment #832743 - Flags: review?(echou)
(Assignee)

Comment 4

4 years ago
Created attachment 832750 [details] [diff] [review]
Patch to enable connect/disconnect

Patch to remove Connect/Disconnect error reply on current codebase for testing.
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>
(Assignee)

Comment 6

4 years ago
> 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?
> > @@ +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.
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.
(Assignee)

Comment 9

4 years ago
Created attachment 8333684 [details] [diff] [review]
Patch 1 (v3): Implement HFP manager for bluedroid

Changes:
- revise IsConnected() according to RFCOMM connection.
- move call enumeration into NotifyConnectionStateChanged function.
- fix nits.
Attachment #832743 - Attachment is obsolete: true
Attachment #832743 - Flags: review?(echou)
Attachment #8333684 - Flags: review?(echou)
(Assignee)

Comment 10

4 years ago
(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 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.
Attachment #8333684 - Flags: review?(echou) → review+
(Assignee)

Comment 12

4 years ago
Created attachment 8333722 [details] [diff] [review]
[final] Patch 1: Implement HFP manager for bluedroid, r=echou
Attachment #8333684 - Attachment is obsolete: true
https://hg.mozilla.org/integration/b2g-inbound/rev/b18a7571287c
https://hg.mozilla.org/mozilla-central/rev/b18a7571287c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 Sprint 5 - 11/22
Blocks: 943758
Blocks: 939672
You need to log in before you can comment on or make changes to this bug.