Closed Bug 1181483 Opened 9 years ago Closed 9 years ago

Implement GATT Server characteristic notification

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox44 fixed)

RESOLVED FIXED
FxOS-S8 (02Oct)
Tracking Status
firefox44 --- fixed

People

(Reporter: brsun, Assigned: brsun)

References

Details

Attachments

(1 file, 3 obsolete files)

GATT Server characteristic notification:
 - Characteristic changed
Attachment #8651624 - Attachment is obsolete: true
Attachment #8655372 - Flags: review?(joliu)
Comment on attachment 8655372 [details] [diff] [review]
bug1181483_bluetoothgattserver_characteristic_notification.v1.patch

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

Please see my comments below, thanks!

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +2254,5 @@
> +    BT_WARNING("BluetoothGattServerInterface::NotifyCharacteristicChanged"
> +               "failed: %d", (int)aStatus);
> +
> +    BluetoothService* bs = BluetoothService::Get();
> +    NS_ENSURE_TRUE_VOID(bs);

These two lines seems unnecessary.

@@ +2284,5 @@
> +
> +  ENSURE_GATT_INTF_IS_READY_VOID(aRunnable);
> +
> +  size_t index = sServers->IndexOf(aAppUuid, 0 /* Start */, UuidComparator());
> +  if (index == sServers->NoIndex) {

I think we should reject in this case, no?

@@ +2301,5 @@
> +    DispatchReplyError(aRunnable, STATUS_BUSY);
> +  }
> +
> +  int connId = 0;
> +  if (server->mConnectionMap.Get(aAddress, &connId)) {

This is wrong?
Shouldn't it be |!server->mConnectionMap.Get(aAddress, &connId)|?

@@ +2302,5 @@
> +  }
> +
> +  int connId = 0;
> +  if (server->mConnectionMap.Get(aAddress, &connId)) {
> +    MOZ_ASSERT(connId > 0);

no MOZ_ASSERT here, connId might be 0 if it's still connecting.

Just reply error also if it's not connected.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +730,5 @@
>    gatt->ServerStopService(aAppUuid, aServiceHandle, aRunnable);
>  }
>  
> +void
> +BluetoothServiceBluedroid::GattServerNotifyCharacteristicChangedInternal(

nit: How about we use SendIndicationInternal in gecko backend?

@@ +745,5 @@
> +
> +  BluetoothGattManager* gatt = BluetoothGattManager::Get();
> +  ENSURE_GATT_MGR_IS_READY_VOID(gatt, aRunnable);
> +
> +  gatt->ServerNotifyCharacteristicChanged(aAppUuid,

DITTO.

::: dom/bluetooth/common/webapi/BluetoothGattServer.cpp
@@ +538,5 @@
>  void
>  BluetoothGattServer::AddServiceTask::OnSuccessFired()
>  {
>    mService->AssignAppUuid(mServer->mAppUuid);
> +  mService->AssignServer(mServer);

I think it's unnecessary.

@@ +675,5 @@
> +  BT_ENSURE_TRUE_REJECT(bs, promise, NS_ERROR_NOT_AVAILABLE);
> +
> +  nsRefPtr<BluetoothGattService> service = characteristic.Service();
> +  BT_ENSURE_TRUE_REJECT(service, promise, NS_ERROR_NOT_AVAILABLE);
> +  BT_ENSURE_TRUE_REJECT(service->Server() == this,

There's only one GattServer in one content process.
We could skip this check and remove Server() and mServer in GattService.

::: dom/bluetooth/common/webapi/BluetoothGattService.cpp
@@ +113,5 @@
>    mAppUuid = aAppUuid;
>  }
>  
>  void
> +BluetoothGattService::AssignServer(BluetoothGattServer* aServer)

I think we could remove this.

::: dom/bluetooth/common/webapi/BluetoothGattService.h
@@ +10,5 @@
>  #include "mozilla/Attributes.h"
>  #include "mozilla/dom/BluetoothGattServiceBinding.h"
>  #include "mozilla/dom/bluetooth/BluetoothCommon.h"
>  #include "mozilla/dom/bluetooth/BluetoothGattCharacteristic.h"
> +#include "mozilla/dom/bluetooth/BluetoothGattServer.h"

I think we could remove all changes in this file.
Attachment #8655372 - Flags: review?(joliu)
Address comment 3.
Attachment #8655372 - Attachment is obsolete: true
Attachment #8658021 - Flags: review?(joliu)
Comment on attachment 8658021 [details] [diff] [review]
bug1181483_bluetoothgattserver_characteristic_notification.v2.patch

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

r=me with comments addressed, thanks. :)

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +2282,5 @@
> +
> +  size_t index = sServers->IndexOf(aAppUuid, 0 /* Start */, UuidComparator());
> +  // Reject the request if the server has not been registered yet.
> +  if (index == sServers->NoIndex) {
> +    DispatchReplyError(aRunnable, STATUS_NOT_READY);

return for the error case and so as below.

@@ +2301,5 @@
> +    DispatchReplyError(aRunnable, STATUS_PARM_INVALID);
> +  }
> +
> +  if (!connId) {
> +    DispatchReplyError(aRunnable, STATUS_UNHANDLED);

DITTO.

Maybe STATUS_NOT_READY or UNEXPECTED is more suitable here?
I saw a similar logic in bluedroid for STATUS_NOT_READY, http://androidxref.com/5.1.1_r6/xref/external/bluetooth/bluedroid/btif/src/btif_rc.c#68.

@@ +2314,5 @@
> +    aValue,
> +    aConfirm,
> +    new ServerSendIndicationResultHandler(server));
> +}
> +

nit: remove the extra new line.

::: dom/bluetooth/common/webapi/BluetoothGattServer.cpp
@@ +728,5 @@
> +
> +already_AddRefed<Promise>
> +BluetoothGattServer::NotifyCharacteristicChanged(
> +  const nsAString& aAddress,
> +  BluetoothGattCharacteristic& characteristic,

aCharacteristic

::: dom/bluetooth/common/webapi/BluetoothGattServer.h
@@ +59,5 @@
>    already_AddRefed<Promise> RemoveService(BluetoothGattService& aService,
>                                            ErrorResult& aRv);
> +  already_AddRefed<Promise> NotifyCharacteristicChanged(
> +    const nsAString& aAddress,
> +    BluetoothGattCharacteristic& characteristic,

aCharacteristic
Attachment #8658021 - Flags: review?(joliu) → review+
Address comment 5.

Hi Blake,

In order to provide the facilities to send indication/notification to the remote GATT client, one of Bluetooth related WebAPI would be added by this bug:
 - BluetoothGattServer.notifyCharacteristicChanged()[1]

Would you mind helping to review this patch regarding to WebAPI parts?

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothGattServer#notifyCharacteristicChanged.28DOMString_address.2C_BluetoothGattCharacteristic_characteristic.2C_bool_confirm.29
Attachment #8658021 - Attachment is obsolete: true
Attachment #8661705 - Flags: review?(mrbkap)
Attachment #8661705 - Flags: review?(mrbkap) → review+
Assign to Bruce since he's actively working on this bug.
Assignee: nobody → brsun
https://hg.mozilla.org/mozilla-central/rev/fce934a5b78d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S8 (02Oct)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: