Closed Bug 915628 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.2 FC (16sep)

People

(Reporter: ben.tian, Assigned: ben.tian)

References

Details

Attachments

(1 file, 5 obsolete files)

Define new log levels and add logs of connect/disconnect/onconnect/ondisconnect.
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
Attachment #803639 - Flags: review?(echou)
Remove redundant file including.
Attachment #803639 - Attachment is obsolete: true
Attachment #803639 - Flags: review?(echou)
Attachment #803643 - Flags: review?(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.
Add GetName() funciton to each profile manager and fix nits.
Attachment #803643 - Attachment is obsolete: true
Attachment #803643 - Flags: review?(echou)
Attachment #804296 - Flags: review?(echou)
fix a typo.
Attachment #804296 - Attachment is obsolete: true
Attachment #804296 - Flags: review?(echou)
Attachment #804298 - Flags: review?(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?
Define macro BT_LOGR_PROFILE to get profile name and prints logs.
Attachment #804298 - Attachment is obsolete: true
Attachment #804298 - Flags: review?(echou)
Attachment #804729 - Flags: review?(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.
Attachment #804729 - Flags: review?(echou) → review+
https://hg.mozilla.org/mozilla-central/rev/e63b6d17fbfd
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2 FC (16sep)
You need to log in before you can comment on or make changes to this bug.