915628 - [Bluetooth] Define new log levels and add logs in profile controller

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

Define new log levels and add logs of connect/disconnect/onconnect/ondisconnect.

Comment 1

[Ben Tian (inactive)]

Attachment: Patch 1 (v1): define bt log levels and add profile controller logs

Changes
- define 3 log levels
  1) BT_LOGD
       debug logs. output only when dev flag ON in debug build
  2) BT_WARNING
       NS_WARNING with arguments. output in both debug and release builds
  3) BT_LOGR
       release logs. output in release build only

- remember profile name in profile managers for logging
- fix compilation warnings

Comment 2

[Ben Tian (inactive)]

Attachment: Patch 1 (v2): define bt log levels and add profile controller logs

Remove redundant file including.

Comment 3

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

Comment on attachment 803643 [details] [diff] [review]
Patch 1 (v2): define bt log levels and add profile controller logs

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

The patch looks good, but it would be better if we can keep BluetoothProfileManagerBase.h as an interface, so please update and send review request again.

Thanks for doing this, it can definitely make us debug easier. :)

::: dom/bluetooth/BluetoothCommon.h
@@ +18,5 @@
>  
>  #undef BT_LOG
>  #if defined(MOZ_WIDGET_GONK)
>  #include <android/log.h>
> +#define BT_LOGD(args...)                                             \

nit: please add comment to explain what D and R represent for.

::: dom/bluetooth/BluetoothHidManager.h
@@ +36,5 @@
>                            MOZ_OVERRIDE;
>    virtual void OnConnect(const nsAString& aErrorStr) MOZ_OVERRIDE;
>    virtual void OnDisconnect(const nsAString& aErrorStr) MOZ_OVERRIDE;
>  
> +  // HID member functions

The comment seems to be a little weird, because functions listed above are also member functions. It's just they are inherited from the base class BluetoothProfileManagerBase. How about using "HID-specific functions"?

::: dom/bluetooth/BluetoothProfileManagerBase.h
@@ +66,5 @@
> +
> +  /**
> +   * Return string of profile name
> +   */
> +  nsCString GetName()

We could modify this into a pure virtual function to force each profiles implementing it. mName could be removed as well in this case. The main advantage of this solution is to remain this class (BluetoothProfileManagerBase) as an interface.

Comment 4

[Ben Tian (inactive)]

Attachment: Patch 1 (v3): define bt log levels and add profile controller logs

Add GetName() funciton to each profile manager and fix nits.

Comment 5

[Ben Tian (inactive)]

Attachment: Patch 1 (v4): define bt log levels and add profile controller logs

fix a typo.

Comment 6

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

Comment on attachment 804298 [details] [diff] [review]
Patch 1 (v4): define bt log levels and add profile controller logs

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

::: dom/bluetooth/BluetoothProfileManagerBase.h
@@ +66,5 @@
> +
> +  /**
> +   * Returns string of profile name
> +   */
> +  virtual nsCString GetName() = 0;

So, we usually pass nsString/nsCString references into the function as an out parameter instead of a return value. I was thinking that maybe we could just return const char* since this function is only for printing out logs.

Any idea?

Comment 7

[Ben Tian (inactive)]

Attachment: Patch 1 (v5): define bt log levels and add profile controller logs

Define macro BT_LOGR_PROFILE to get profile name and prints logs.

Comment 8

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

Comment on attachment 804729 [details] [diff] [review]
Patch 1 (v5): define bt log levels and add profile controller logs

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

r=me with nits picked. Thanks for being so patient. :)

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +1429,5 @@
>  
>    BluetoothService* bs = BluetoothService::Get();
>    NS_ENSURE_TRUE_VOID(bs);
>  
> +  BT_LOGR("ben %d", aChannel);

I believe this is written by Ben, and I also believe that the message is not as expected as you want or this is left here accidentally. ;)

::: dom/bluetooth/gonk/BluetoothGonkService.cpp
@@ +98,5 @@
>      if (sBluedroidFunctions.bt_is_enabled() < 0) {
>        // if isEnabled < 0, this means we brought up the firmware, but something
>        // went wrong with bluetoothd. Post a warning message, but try to proceed
>        // with firmware unloading if that was requested, so we can retry later.
> +      BT_WARNING("Bluetooth firmware up, but cannot connect to HCI socket! Check bluetoothd and try stopping/starting bluetooth again.");

nit: 80 characters per line.

Comment 9

[Ben Tian (inactive)]

Attachment: [final] Patch 1: define bt log levels and add profile controller logs, r=echou

Comment 10

[Ben Tian (inactive)]

try server: https://tbpl.mozilla.org/?tree=Try&rev=e46e818e62c3

Comment 11

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

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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