1181480 - Implement GATT Server connection

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Bruce Sun [:brsun]]

GATT Server connection:
 - Connect
 - Disconnect
 - Connection state changed

Comment 1

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

Attachment: Bug 1181480 - Add and implement GATT server connection related Web APIs.

- add connect, disconnect, onconnectionstatechanged in BluetoothGattServer.webidl
- implement register/unregister server interface and connect/disconnect functionalities.

Comment 2

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

Attachment: Bug 1181480 - Add and implement GATT server connection related Web APIs.

- add some comments.

Comment 3

[Ben Tian (inactive)]

Comment on attachment 8647887 [details] [diff] [review]
Bug 1181480 - Add and implement GATT server connection related Web APIs.

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

Please see my comments and questions below.

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +257,5 @@
>                int aClientIf) const
>    {
>      return aClient->mClientIf == aClientIf;
>    }
> +  bool Equals(const nsRefPtr<BluetoothGattServer>& aServer,

nit: add newline before.

@@ +1477,5 @@
> +  if (index == sServers->NoIndex) {
> +    index = sServers->Length();
> +    sServers->AppendElement(new BluetoothGattServer(aAppUuid));
> +  }
> +  nsRefPtr<BluetoothGattServer> server = sServers->ElementAt(index);

Simplify to |sServers[index]|.

@@ +1485,5 @@
> +   * sending a request to bluetooth stack.
> +   *
> +   * case 1) Connecting/Disconnecting: If connect/disconnect peripheral
> +   *         runnable exists, reject the request since the local GATT server is
> +   *         busy with connecting or disconnecting to a device.

nit: remove redundant 'with'.

@@ +1488,5 @@
> +   *         runnable exists, reject the request since the local GATT server is
> +   *         busy with connecting or disconnecting to a device.
> +   * case 2) Connected: If there is an entry whose key is |aAddress| in the
> +   *         connection map, resolve the request. Since disconnected devices
> +   *         will not be in the map, any entry in the map are connected

nit: all entries in the map are connected

@@ +1496,5 @@
> +      server->mDisconnectPeripheralRunnable) {
> +    DispatchReplyError(aRunnable, STATUS_BUSY);
> +    return;
> +  }
> +  int connId = 0;

nit: newline before.

@@ +1568,5 @@
> +  if (NS_WARN_IF(index == sServers->NoIndex)) {
> +    DispatchReplyError(aRunnable, STATUS_PARM_INVALID);
> +    return;
> +  }
> +  nsRefPtr<BluetoothGattServer> server = sServers->ElementAt(index);

sServers[index]

@@ +1659,5 @@
> +    DispatchReplyError(aRunnable, STATUS_PARM_INVALID);
> +    return;
> +  }
> +
> +  nsRefPtr<BluetoothGattServer> server = sServers->ElementAt(index);

Remove this declaration and use sServers[index] below.

@@ +2435,5 @@
> +
> +  size_t index = sServers->IndexOf(uuid, 0 /* Start */, UuidComparator());
> +  NS_ENSURE_TRUE_VOID(index != sServers->NoIndex);
> +
> +  nsRefPtr<BluetoothGattServer> server = sServers->ElementAt(index);

sServers[index]

@@ +2465,5 @@
> +    uuid, BluetoothValue(uint32_t(aServerIf)));
> +
> +  if (server->mConnectPeripheralRunnable) {
> +    // Only one entry exists in the map during first connect peripheral request
> +    nsString deviceAddr(server->mConnectionMap.Iter().Key());

What happens when a server connects to a peripheral after being connected to another one?

@@ +2490,5 @@
> +  size_t index = sServers->IndexOf(aServerIf, 0 /* Start */,
> +                                   InterfaceIdComparator());
> +  NS_ENSURE_TRUE_VOID(index != sServers->NoIndex);
> +
> +  nsRefPtr<BluetoothGattServer> server = sServers->ElementAt(index);

sServers[index]

@@ +2494,5 @@
> +  nsRefPtr<BluetoothGattServer> server = sServers->ElementAt(index);
> +
> +  // Update the connection map based on the connection status
> +  if (aConnected) {
> +    server->mConnectionMap.Put(aBdAddr, aConnId);

Should we remove the original <aBdAddr, 0> entry from map?

::: dom/bluetooth/bluetooth2/BluetoothGattServer.cpp
@@ +116,5 @@
> +
> +  if (mServerIf > 0) {
> +    nsRefPtr<BluetoothVoidReplyRunnable> result =
> +      new BluetoothVoidReplyRunnable(nullptr);
> +    bs->UnregisterGattServerInternal(mServerIf, result);

Pass nullptr directly if no reply is needed.

::: dom/webidl/BluetoothGattServer.webidl
@@ +12,5 @@
> +
> +  /**
> +   * Connect/Disconnect to the remote BLE device with the target address.
> +   *
> +   * Promise will be rejected if the local GATT server is busy with connecting

nit: remove redundant 'with'

Comment 4

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

(In reply to Ben Tian [:btian] from comment #3)
> Comment on attachment 8647887 [details] [diff] [review]
> Bug 1181480 - Add and implement GATT server connection related Web APIs.
> 
> Review of attachment 8647887 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my comments and questions below.
> 
> ::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
> 
> @@ +1477,5 @@
> > +  if (index == sServers->NoIndex) {
> > +    index = sServers->Length();
> > +    sServers->AppendElement(new BluetoothGattServer(aAppUuid));
> > +  }
> > +  nsRefPtr<BluetoothGattServer> server = sServers->ElementAt(index);
> 
> Simplify to |sServers[index]|.
> 
OK, will revise to |(*sServers)[index]|.

> @@ +1659,5 @@
> > +    DispatchReplyError(aRunnable, STATUS_PARM_INVALID);
> > +    return;
> > +  }
> > +
> > +  nsRefPtr<BluetoothGattServer> server = sServers->ElementAt(index);
> 
> Remove this declaration and use sServers[index] below.
> 

Could we keep it since there are two lines below will use it?
Or you would prefer to use (*sServers)[index] directly for both of them?

> @@ +2465,5 @@
> > +    uuid, BluetoothValue(uint32_t(aServerIf)));
> > +
> > +  if (server->mConnectPeripheralRunnable) {
> > +    // Only one entry exists in the map during first connect peripheral request
> > +    nsString deviceAddr(server->mConnectionMap.Iter().Key());
> 
> What happens when a server connects to a peripheral after being connected to
> another one?
> 
In this |connectPeripheral| request, the application hasn't register server interface for its |BluetoothGattServer| object yet.
Hence, we will first register the interface, then proceed to ask for bluetooth stack to connect the remote device in the |RegisterServerNotification|.
We get the address directly from the first entry of the map in this case since there will be only one entry in the map at this moment.

After we have a registered server interface, we could call connect directly, and handle the result in ConnectionNotification which could get the target entry from the map by using address key.

> @@ +2494,5 @@
> > +  nsRefPtr<BluetoothGattServer> server = sServers->ElementAt(index);
> > +
> > +  // Update the connection map based on the connection status
> > +  if (aConnected) {
> > +    server->mConnectionMap.Put(aBdAddr, aConnId);
> 
> Should we remove the original <aBdAddr, 0> entry from map?
> 
Not necessary, it will update the value of the existing entry.

Comment 5

[Ben Tian (inactive)]

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #4)
> Could we keep it since there are two lines below will use it?
> Or you would prefer to use (*sServers)[index] directly for both of them?

OK please keep it. I missed there are two lines using it.

> In this |connectPeripheral| request, the application hasn't register server
> interface for its |BluetoothGattServer| object yet.
> Hence, we will first register the interface, then proceed to ask for
> bluetooth stack to connect the remote device in the
> |RegisterServerNotification|.
> We get the address directly from the first entry of the map in this case
> since there will be only one entry in the map at this moment.
> 
> After we have a registered server interface, we could call connect directly,
> and handle the result in ConnectionNotification which could get the target
> entry from the map by using address key.

Got it. Thanks for explanation.

Comment 6

[Ben Tian (inactive)]

Comment on attachment 8647887 [details] [diff] [review]
Bug 1181480 - Add and implement GATT server connection related Web APIs.

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

r=me with nits in comment 3 addressed. Thanks for comment 4 explanation.

Comment 7

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

Attachment: Bug 1181480 - Add and implement GATT server connection related Web APIs. r=btian

Hi Blake,

This bug is to implement connect/disconnect functionalities and the connectionstatechanged event handler of BluetoothGattServer interface[1].
Could you help to review from a dom peer perspective?

Brief summary of revised source code files:
1) BluetoothGattServer.webidl:
   - add connect(), disconnect(), and onconnectionstatechanged.
2) BluetoothGattServer.cpp/h:
   - implement |Connect| and |Disconnect|
   - implement |Invalidate()| function for cleanup
   - handle some bluetooth signals in |Notify| including firing connectionstatechanged events
3) BluetoothGattManager.cpp/h:
   - register/unregister server interface from bluetooth stack
   - issue connect/disconnect peripherals to bluetooth stack
   - handle register, unregister, connection callbacks from bluetooth stack
4) others: mostly for ipc.

Thanks,
Jocelyn

[1] BluetoothGattServer: https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattServer#BluetoothGattServer

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8648615 [details] [diff] [review]
Bug 1181480 - Add and implement GATT server connection related Web APIs. r=btian

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

::: dom/bluetooth/bluetooth2/BluetoothGattServer.cpp
@@ +46,4 @@
>  
>  BluetoothGattServer::BluetoothGattServer(nsPIDOMWindow* aOwner)
>    : mOwner(aOwner)
> +  , mAppUuid(EmptyString())

Nit: no need to explicitly initialize strings to the empty string.

Comment 9

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

Attachment: [Final] Bug 1181480 - Add and implement GATT server connection related Web APIs. r=btian, r=mrbkap

Thanks for your time, Ben and Blake.

- remove explicitly string initialization in BluetoothGatt, BluetoothGattServer, BluetoothDiscoveryHandle constructors.

Comment 10

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

try looks ok: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cfafcc364af

Comment 11

[Pulsebot]

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

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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