Closed
Bug 1181483
Opened 9 years ago
Closed 9 years ago
Implement GATT Server characteristic notification
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
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)
26.78 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
GATT Server characteristic notification: - Characteristic changed
Assignee | ||
Comment 1•9 years ago
|
||
WIP patch.
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8651624 -
Attachment is obsolete: true
Attachment #8655372 -
Flags: review?(joliu)
Comment 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
Address comment 3.
Attachment #8655372 -
Attachment is obsolete: true
Attachment #8658021 -
Flags: review?(joliu)
Comment 5•9 years ago
|
||
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+
Assignee | ||
Comment 6•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8661705 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 8•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=fce934a5b78d
Assignee | ||
Comment 9•9 years ago
|
||
Original try results seem good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=81d653bae0dd
Comment 10•9 years ago
|
||
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
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S8 (02Oct)
You need to log in
before you can comment on or make changes to this bug.
Description
•