1163499 - [Bluetooth2] Implement GATT client support in daemon backend

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Jocelyn Liu [:jocelyn] [:joliu]]

This bug is to trace the GATT support work in gecko using daemon backend.

Comment 1

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1163499 - Patch1: Revise BluetoothGattInterface and result handler for daemon support.

Comment 2

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1163499 - Patch2: Add helpers for Bluetooth daemon GATT support.

Comment 3

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1163499 - Patch3: Add GATT module for Bluetooth daemon.

Comment 4

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1163499 - Patch4: Add GATT interface for Bluetooth daemon.

Comment 5

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1163499 - Patch5: Support GATT profile when using Bluetooth daemon.

* Note: gatt server is not supported yet.

Comment 6

[Jocelyn Liu [:jocelyn] [:joliu]]

There is an ongoing server work in Bug 1161991 which added server related pieces into BluetoothInterface.h.
To avoid dependency, I would like to support gatt client only in this bug first.
I will create another bug to finish the server part after Bug 1161991.

(I'm experiencing slowness of bugzilla right now, will create it next week.)

Comment 7

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8609286 [details] [diff] [review]
Bug 1163499 - Patch1: Revise BluetoothGattInterface and result handler for daemon support.

* Note: I'll revise all the commit message in these patch sets for replacing gatt with gatt client in next version.

Comment 8

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1163499 - Patch1(v2): Revise BluetoothGattInterface and result handler for daemon support.

diff between v1 and v2:
  fix nexus5 build break and add test commmand functions.

Comment 9

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1163499 - Patch3(v2): Add GATT module for Bluetooth daemon.

Comment 10

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1163499 - Patch4(v2): Add GATT interface for Bluetooth daemon.

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8609918 [details] [diff] [review]
Bug 1163499 - Patch1(v2): Revise BluetoothGattInterface and result handler for daemon support.

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

The comments are mostly nits. I guess we won't make much use of the HAL backend anyway.

::: dom/bluetooth/BluetoothCommon.h
@@ +733,1 @@
>    uint8_t mStatus;

There will be 1-byte gap after |mStatus| because of the alignment requirements in C++. I suggest to move |mStatus| after |mValueLength|.

@@ +749,1 @@
>    bool mIsNotify;

I'd guess that 'bool' is 8 byte wide, so there will again be an 1-byte gap here.

::: dom/bluetooth/BluetoothInterface.h
@@ +815,5 @@
> +                          char* aManufacturerData,
> +                          uint16_t aServiceDataLen,
> +                          char* aServiceData,
> +                          uint16_t aServiceUUIDLen,
> +                          char* aServiceUUID,

Cleaning up the JS parameters from the interface! NICE!

I think there was an include statement for <js*> that can be removed as well.

::: dom/bluetooth/bluedroid/BluetoothHALHelpers.cpp
@@ +323,5 @@
> +
> +nsresult
> +Convert(const BluetoothGattTestParam& aIn, btgatt_test_params_t& aOut)
> +{
> +  nsresult rv;

Please declare |rv| where it's first used.

@@ +326,5 @@
> +{
> +  nsresult rv;
> +
> +  rv = Convert(aIn.mBdAddr, *aOut.bda1);
> +  NS_ENSURE_SUCCESS(rv, rv);

|NS_ENSURE_SUCCESS| is deprecated AFAIK. The replacement pattern is

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

@@ +331,5 @@
> +
> +  rv = Convert(aIn.mUuid, *aOut.uuid1);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  aOut.u1 = aIn.mU1;

You should rather call |Convert(AOut.u2, aOut.u1)| for consistency with everything else.

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8609292 [details] [diff] [review]
Bug 1163499 - Patch2: Add helpers for Bluetooth daemon GATT support.

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

Only nits.

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp
@@ +516,5 @@
> +    CONVERT(0x000f, GATT_STATUS_INSUFFICIENT_ENCRYPTION),
> +    CONVERT(0x0010, GATT_STATUS_UNSUPPORTED_GROUP_TYPE),
> +    CONVERT(0x0011, GATT_STATUS_INSUFFICIENT_RESOURCES)
> +  };
> +  if (static_cast<uint32_t>(aIn) >= MOZ_ARRAY_LENGTH(sGattStatus)) {

Please add |NS_WARN_IF| around this test, so that an error will be logged.

And a comment on cleanliness: Casting |aIn| with a negative value to an 'uint32_t' type, will result in a large integer. While the check technically works it seems safer and cleaner to cast the array length to 'ssize_t' instead. This signed type is guaranteed to be large enough to hold size values.

The overall pattern would look like

if (NS_WARN_IF(aIn < 0) || NS_WARN_IF(aIn >= array-length as ssize_t)) {
  ..
}

@@ +1411,5 @@
> +  nsresult rv = PackPDU(aIn.mUuid, aPDU);
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +

Maybe no empty line here (and in the functions below) to keep the code more compact and the file a bit shorter.

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8609919 [details] [diff] [review]
Bug 1163499 - Patch3(v2): Add GATT module for Bluetooth daemon.

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

Looks good.

::: dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp
@@ +29,5 @@
> +nsresult
> +BluetoothDaemonGattModule::Send(BluetoothDaemonPDU* aPDU,
> +                                BluetoothGattResultHandler* aRes)
> +{
> +  aRes->AddRef(); // Keep reference for response

Please add

if (aRes) {
}

around this line. Usually we use a result handler, but sometimes it might be omitted.

@@ +35,5 @@
> +}
> +
> +nsresult
> +BluetoothDaemonGattModule::Send(BluetoothDaemonPDU* aPDU,
> +                                BluetoothGattClientResultHandler* aRes)

I thought ClientResultHandler was removed in the first patch. No?

@@ +1054,5 @@
> +      pdu, UnpackConversion<BluetoothAddress, nsAString>(aArg1));
> +    if (NS_FAILED(rv)) {
> +      return rv;
> +    }
> +

Maybe remove these empty lines to keep the code more compact.

Comment 14

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8609920 [details] [diff] [review]
Bug 1163499 - Patch4(v2): Add GATT interface for Bluetooth daemon.

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

Looks good.

::: dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp
@@ +1452,5 @@
> +  void OnError(BluetoothStatus aStatus) override
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    BT_LOGR("%s:%d", __func__, __LINE__);

Please remove the debug logging. :)

@@ +1881,5 @@
> +  sBluetoothDaemonGattClientInterface =
> +    new BluetoothDaemonGattClientInterface(mModule);
> +#else
> +  sBluetoothDaemonGattClientInterface =
> +    new BluetoothDaemonGattClientInterface();

I think you want to set this to 'nullptr' instead.

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8609920 [details] [diff] [review]
Bug 1163499 - Patch4(v2): Add GATT interface for Bluetooth daemon.

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

Looks good.

::: dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp
@@ +1452,5 @@
> +  void OnError(BluetoothStatus aStatus) override
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    BT_LOGR("%s:%d", __func__, __LINE__);

Please remove the debug logging. :)

@@ +1868,5 @@
> +BluetoothGattClientInterface*
> +BluetoothDaemonGattInterface::GetBluetoothGattClientInterface()
> +{
> +  static BluetoothDaemonGattClientInterface*
> +    sBluetoothDaemonGattClientInterface;

Please don't use a static field here. The field should be non-static as the overall interface management is implemented in |BluetoothDaemonInterface| and the manager classes.

@@ +1876,5 @@
> +  }
> +
> +  MOZ_ASSERT(mModule);
> +
> +#if ANDROID_VERSION >= 19

You should not have to use 'ANDROID_VERSION' here. Just return an instance of this class.

If GATT is not supported, the call to |Init| will fail. We have to handle such failures anyway, and it's an elegant way of moving system dependencies from Gecko to bluetoothd.

@@ +1881,5 @@
> +  sBluetoothDaemonGattClientInterface =
> +    new BluetoothDaemonGattClientInterface(mModule);
> +#else
> +  sBluetoothDaemonGattClientInterface =
> +    new BluetoothDaemonGattClientInterface();

I think you want to set this to 'nullptr' instead.

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hey Jocelyn,

Good job! Before you land the patches, please build them on some combinations of Bluetooth v1/v2 and JB, KK and L.

I'll prepare the daemon patches for review and send them out later today. You already tested them a bit, are you OK with reviewing the code?

Comment 17

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #16)
> Hey Jocelyn,
> 
> Good job! Before you land the patches, please build them on some
> combinations of Bluetooth v1/v2 and JB, KK and L.
> 

Will do, thanks for the reminder.

> I'll prepare the daemon patches for review and send them out later today.
> You already tested them a bit, are you OK with reviewing the code?

Sure, but it might take more time consider the amounts of gatt functions and it's also my first time reviewing daemon code. ;)

Comment 18

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #13)
> Comment on attachment 8609919 [details] [diff] [review]
> Bug 1163499 - Patch3(v2): Add GATT module for Bluetooth daemon.
> 
> Review of attachment 8609919 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> @@ +35,5 @@
> > +}
> > +
> > +nsresult
> > +BluetoothDaemonGattModule::Send(BluetoothDaemonPDU* aPDU,
> > +                                BluetoothGattClientResultHandler* aRes)
> 
> I thought ClientResultHandler was removed in the first patch. No?
> 

No, it hasn't been removed yet, I only removed the |onError| function in GattClientResultHandler and use the one in GattResultHandler.

Comment 19

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Bug 1163499 - Patch1: Revise BluetoothGattInterface and result handler for daemon support. r=tzimmermann

Thanks for your time, Thomas.

Comment 20

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Bug 1163499 - Patch2: Add helpers for Bluetooth daemon GATT client support. r=tzimmermann

Comment 21

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Bug 1163499 - Patch3: Add GATT module for Bluetooth daemon. r=tzimmermann

Comment 22

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Bug 1163499 - Patch4: Add GATT and GATT client interfaces for Bluetooth daemon. r=tzimmermann

Comment 23

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Bug 1163499 - Patch5: Support GATT profile when using Bluetooth daemon. r=tzimmermann

Comment 24

[Jocelyn Liu [:jocelyn] [:joliu]]

try result:
v1 + JB/KK/L : https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddbff3bda0f3
v2 + JB/KK/L : https://treeherder.mozilla.org/#/jobs?repo=try&revision=d72e780e62f6

Comment 25

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Bug 1163499 - Patch1: Revise BluetoothGattInterface and result handler for daemon support. r=tzimmermann

- remove TypedArray.h including

Comment 26

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Bug 1163499 - Patch3: Add GATT module for Bluetooth daemon. r=tzimmermann

- be more consistent on the letter case in the comments

Comment 27

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Bug 1163499 - Patch4: Add GATT and GATT client interfaces for Bluetooth daemon. r=tzimmermann

- remove unused friend class declaration.

Comment 28

[Jocelyn Liu [:jocelyn] [:joliu]]

try result:
v1 + JB/KK/L : https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddbff3bda0f3
v2 + JB/KK/L : https://treeherder.mozilla.org/#/jobs?repo=try&revision=d72e780e62f6

Windows build on try is broken yesterday, and they're all green after retry.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/d7e3ea6a1c7c
https://hg.mozilla.org/integration/b2g-inbound/rev/062246f7d1d4
https://hg.mozilla.org/integration/b2g-inbound/rev/7bd37e369b3a
https://hg.mozilla.org/integration/b2g-inbound/rev/9a8bb7dbdcd3
https://hg.mozilla.org/integration/b2g-inbound/rev/a82cebbfbbf3

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d7e3ea6a1c7c
https://hg.mozilla.org/mozilla-central/rev/062246f7d1d4
https://hg.mozilla.org/mozilla-central/rev/7bd37e369b3a
https://hg.mozilla.org/mozilla-central/rev/9a8bb7dbdcd3
https://hg.mozilla.org/mozilla-central/rev/a82cebbfbbf3