1136514 - [bluetooth2] Implement ReadRemoteRssi method in GATT client API

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

This bug is for implementing readRemoteRssi method[1] in BluetoothGatt interface[2].

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGatt#readRemoteRssi.28.29
[2] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGatt#BluetoothGatt

Comment 1

[Bruce Sun [:brsun]]

Attachment: bug1136514_gatt_client_read_remote_rssi.patch

Hi Jocelyn, may I have your feedback? :)

Comment 2

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

Comment on attachment 8572350 [details] [diff] [review]
bug1136514_gatt_client_read_remote_rssi.patch

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

Overall looks good to me.
f=me with comments addressed.

::: dom/bluetooth2/BluetoothGatt.cpp
@@ +187,5 @@
> +    if (v.type() != BluetoothValue::Tuint32_t) {
> +      BT_WARNING("Not a uint32_t!");
> +      SetError(NS_LITERAL_STRING("BluetoothReplyTypeError"));
> +      return false;
> +    }

Suggest using |NS_ENSURE_TRUE(v.type() == BluetoothValue::Tint32_t, false);| here

@@ +223,5 @@
> +  nsRefPtr<BluetoothReplyRunnable> result =
> +    new ReadRemoteRssiTask(nullptr /* DOMRequest */,
> +                           promise,
> +                           NS_LITERAL_STRING("ReadRemoteRssiGattClient"));
> +  bs->ReadRemoteRssiGattClientInternal(mClientIf, mDeviceAddr, result);

It seems unnecessary to add GattClient into our function name for this case.
It's needed for Connect/Disconnect case since they're conflict with existing API.
What do you think?

::: dom/bluetooth2/BluetoothService.h
@@ +331,5 @@
>    UnregisterGattClientInternal(int aClientIf,
>                                 BluetoothReplyRunnable* aRunnable) = 0;
>  
> +  /**
> +   * Request RSSI for a given remote device. (platform specific implementation)

nit: Suggest to revise to "given remote BLE device" or "given remote GATT server".

::: dom/bluetooth2/bluedroid/BluetoothGattManager.cpp
@@ +524,5 @@
> +    NS_ENSURE_TRUE_VOID(bs);
> +
> +    // Reject the read remote rssi request
> +    NS_NAMED_LITERAL_STRING(errorStr, "ReadRemoteRssi failed");
> +    DispatchBluetoothReply(mClient->mReadRemoteRssiRunnable,

Please rebase and use |DispatchReplyError| here.

ref: https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/bluedroid/BluetoothGattManager.cpp#381

@@ +550,5 @@
> +
> +  // Reject the unregister request if the client is not found
> +  if (index == sClients->NoIndex) {
> +    NS_NAMED_LITERAL_STRING(errorStr, "Read remote RSSI failed");
> +    DispatchBluetoothReply(aRunnable, BluetoothValue(), errorStr);

DITTO.

@@ +815,5 @@
>  void
>  BluetoothGattManager::ReadRemoteRssiNotification(int aClientIf,
>                                                   const nsAString& aBdAddr,
>                                                   int aRssi,
>                                                   int aStatus)

Please rebase to use BluetoothGattStatus in this function, thanks!

@@ +836,5 @@
> +
> +    // Reject the read remote rssi request
> +    if (client->mReadRemoteRssiRunnable) {
> +      NS_NAMED_LITERAL_STRING(errorStr, "ReadRemoteRssi failed");
> +      DispatchBluetoothReply(client->mReadRemoteRssiRunnable,

DITTO.

@@ +847,5 @@
> +  }
> +
> +  // Resolve the read remote rssi request
> +  if (client->mReadRemoteRssiRunnable) {
> +    DispatchBluetoothReply(client->mReadRemoteRssiRunnable,

DITTO. Use |DispatchReplySuccess| here.

::: dom/webidl/BluetoothGatt.webidl
@@ +27,5 @@
>    Promise<void>                                     connect();
>    [NewObject]
>    Promise<void>                                     disconnect();
> +  [NewObject]
> +  Promise<short>                                    readRemoteRssi();

Please add some comments for this method about its functionality and also specify that Promise will be rejected if state != connected here.

Comment 3

[Bruce Sun [:brsun]]

(In reply to jocelyn [:jocelyn] from comment #2)
> @@ +223,5 @@
> > +  nsRefPtr<BluetoothReplyRunnable> result =
> > +    new ReadRemoteRssiTask(nullptr /* DOMRequest */,
> > +                           promise,
> > +                           NS_LITERAL_STRING("ReadRemoteRssiGattClient"));
> > +  bs->ReadRemoteRssiGattClientInternal(mClientIf, mDeviceAddr, result);
> 
> It seems unnecessary to add GattClient into our function name for this case.
> It's needed for Connect/Disconnect case since they're conflict with existing
> API.
> What do you think?
> 

As discussed offline, "GattClient" would be the common prefix for GATT related APIs in Bluetooth IPDL. So I would rename this function as |GattClientReadRemoteRssiInternal|.

Comment 4

[Bruce Sun [:brsun]]

Attachment: bug1136514_gatt_client_read_remote_rssi.v2.patch

Comment 5

[Ben Tian (inactive)]

Comment on attachment 8575091 [details] [diff] [review]
bug1136514_gatt_client_read_remote_rssi.v2.patch

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

r=me with comment addressed. Please request a DOM peer review webidl change.

::: dom/bluetooth2/BluetoothGatt.cpp
@@ +169,5 @@
>  
> +class ReadRemoteRssiTask MOZ_FINAL : public BluetoothReplyRunnable
> +{
> +public:
> +  ReadRemoteRssiTask(nsIDOMDOMRequest* aReq,

No need to pass
1) |aReq| since the method uses promise and
2) |aName| since the method name is fixed.

Refer to https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/BluetoothAdapter.cpp#46

@@ +190,5 @@
> +    return true;
> +  }
> +
> +  virtual void
> +  ReleaseMembers() MOZ_OVERRIDE

No need to override this function if nothing additional to free.

::: dom/bluetooth2/bluedroid/BluetoothGattManager.cpp
@@ +527,5 @@
> +
> +  size_t index = sClients->IndexOf(aClientIf, 0 /* Start */,
> +                                   ClientIfComparator());
> +
> +  // Reject the unregister request if the client is not found

nit: 'read remote rssi' request

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.h
@@ +190,5 @@
>  
> +  virtual void
> +  GattClientReadRemoteRssiInternal(int aClientIf,
> +                                   const nsAString& aDeviceAddress,
> +                                   BluetoothReplyRunnable* aRunnable) MOZ_OVERRIDE;

nit: this line is > 80 char

::: dom/bluetooth2/bluez/BluetoothDBusService.h
@@ +200,5 @@
>  
> +  virtual void
> +  GattClientReadRemoteRssiInternal(int aClientIf,
> +                                   const nsAString& aDeviceAddress,
> +                                   BluetoothReplyRunnable* aRunnable) MOZ_OVERRIDE;

nit: this line is > 80 char.

::: dom/bluetooth2/ipc/BluetoothParent.cpp
@@ +731,5 @@
>    return true;
>  }
> +
> +bool
> +BluetoothRequestParent::DoRequest(const GattClientReadRemoteRssiRequest& aRequest)

nit: this line is > 80 char.

::: dom/webidl/BluetoothGatt.webidl
@@ +26,5 @@
>    [NewObject]
>    Promise<void>                                     connect();
>    [NewObject]
>    Promise<void>                                     disconnect();
> +  

nit: remove trailing space.

Comment 6

[Bruce Sun [:brsun]]

Attachment: bug1136514_gatt_client_read_remote_rssi.v3.patch

This patch addresses suggestions on comment 5.

Hi Blake,
I would like to make some API changes in Bluetooth WebIDL[1]. Would you mind helping review this patch regarding to webidl part? Documents for ReadRemoteRssi can be found on comment 0[2].

[1] dom/webidl/BluetoothGatt.webidl
[2] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGatt#readRemoteRssi.28.29

Comment 7

[Bruce Sun [:brsun]]

Try results seem good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5e4b0da70f3

Comment 8

[Bruce Sun [:brsun]]

Attachment: bug1136514_gatt_client_read_remote_rssi.commit.patch

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/58122581b91d